nfs: fix NFSv4.x client name generation problems
Subsystems (2)
v3 cover nfs: fix NFSv4.x client name generation problems
Hi Trond,
This patchset is essentially the same as the v2 set, but prepends one patch that just increases the size of the on-stack buffer for EXCHANGE_ID, and marks that patch for stable.That should be enough for a minimal fix for stable.
These patches and all of the others that I have queued for v4.2 are in my nfs-4.2 branch here (which may be easier for cherry-picking):
https://git.samba.org/?p=jlayton/linux.git;a=shortlog;h=refs/heads/nfs-4.2
v3:
- add preliminary patch that just increases the buffer in struct nfs41_exchange_id_args, and mark it for stable.
- no need to use GFP_NOFS to allocate cl_owner_id. That will now always be successfully allocated at mount time, and there is no potential for reclaim deadlock. Also add comments to explicitly spell this out.
v2:
- improve patch description on fourth patch
- fix potential memory leak in uniform name string generation
- remove unneeded NULL cl_owner_id string check
- account for name blob length word in encode_setclientid_maxsz and encode_exchange_id_maxsz
Original patch description follows:
Michael Skralivetsky reported some problems recently that turned out to be client name string collisions. He has a test environment where the hostnames are quite long, and only differ at the very end.
The v4.1 name string buffer is currently limited to 48 bytes, which is very small. When we go to build the name string, the end can be truncated and if the hostnames are long enough, this can allow the client identifiers to be duplicates of one another.
This patchset should fix the problem by allowing the client name string to be as long as the spec allows (1k). It also gets rid of the large-ish on-stack buffers that are currently used to build these strings, and fixes some potential memory reclaim recursion problems that could occur when these strings are generated.
Jeff Layton (5):
nfs: increase size of EXCHANGE_ID name string buffer
nfs: convert setclientid and exchange_id encoders to use
clp->cl_owner_id
nfs: update maxsz values for SETCLIENTID and EXCHANGE_ID
nfs: make nfs4_init_nonuniform_client_string use a dynamically
allocated buffer
nfs: make nfs4_init_uniform_client_string use a dynamically allocated
buffer
fs/nfs/nfs4proc.c | 171 +++++++++++++++++++++++++++++++++++------------- fs/nfs/nfs4xdr.c | 14 ++-- include/linux/nfs_xdr.h | 8 +-- 3 files changed, 136 insertions(+), 57 deletions(-)
Signature · 1 line
2.4.2
PATCH v3 1/5 nfs: increase size of EXCHANGE_ID name string buffer
The current buffer is much too small if you have a relatively long hostname. Bring it up to the size of the one that SETCLIENTID has.
--- include/linux/nfs_xdr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/nfs_xdr.h +1 -1
@@ -1142,7 +1142,7 @@ struct nfs41_state_protection {struct nfs4_op_map allow; }; -#define NFS4_EXCHANGE_ID_LEN (48) +#define NFS4_EXCHANGE_ID_LEN (127)struct nfs41_exchange_id_args {struct nfs_client *client;nfs4_verifier *verifier;
Signature · 1 line
2.4.2
PATCH v3 2/5 nfs: convert setclientid and exchange_id encoders to use clp->cl_owner_id
In reply to Jeff Layton...instead of buffers that are part of their arg structs. We already hold a reference to the client, so we might as well use the allocated buffer. In the event that we can't allocate the clp->cl_owner_id, then just return -ENOMEM.
Note too that we switch from a GFP_KERNEL allocation here to GFP_NOFS. It's possible we could end up trying to do a SETCLIENTID or EXCHANGE_ID in order to reclaim some memory, and the GFP_KERNEL allocations in the existing code could cause recursion back into NFS reclaim.
--- fs/nfs/nfs4proc.c | 21 ++++++++++++++++----- fs/nfs/nfs4xdr.c | 8 +++++--- include/linux/nfs_xdr.h | 2 +- 3 files changed, 22 insertions(+), 9 deletions(-)
fs/nfs/nfs4proc.c +16 -5
@@ -5044,7 +5044,7 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,struct nfs4_setclientid setclientid = { .sc_verifier = &sc_verifier, .sc_prog = program, - .sc_cb_ident = clp->cl_cb_ident, + .sc_clnt = clp, };struct rpc_message msg = { .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETCLIENTID ],
@@ -5074,6 +5074,12 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,nfs4_init_nonuniform_client_string (clp, setclientid.sc_name,sizeof (setclientid.sc_name)); + +if (!clp->cl_owner_id) { + status = -ENOMEM ; + goto out; + } +/* cb_client4 */ setclientid.sc_netid_len = nfs4_init_callback_netid(clp,
@@ -5083,9 +5089,9 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,sizeof (setclientid.sc_uaddr),"%s.%u.%u" , clp->cl_ipaddr, port >> 8, port & 255); -dprintk ("NFS call setclientid auth=%s, '%.*s'\n" , +dprintk ("NFS call setclientid auth=%s, '%s'\n" , clp->cl_rpcclient->cl_auth->au_ops->au_name, - setclientid.sc_name_len, setclientid.sc_name); + clp->cl_owner_id); task =rpc_run_task (&task_setup_data);if (IS_ERR (task)) { status =PTR_ERR (task);
@@ -6848,9 +6854,14 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,nfs4_init_boot_verifier (clp, &verifier); args.id_len =nfs4_init_uniform_client_string (clp, args.id,sizeof (args.id)); -dprintk ("NFS call exchange_id auth=%s, '%.*s'\n" , +if (!clp->cl_owner_id) { + status = -ENOMEM ; + goto out; + } + +dprintk ("NFS call exchange_id auth=%s, '%s'\n" , clp->cl_rpcclient->cl_auth->au_ops->au_name, - args.id_len, args.id); + clp->cl_owner_id); res.server_owner =kzalloc (sizeof (struct nfs41_server_owner ),GFP_NOFS );
fs/nfs/nfs4xdr.c +5 -3
@@ -1667,13 +1667,14 @@ static void encode_setclientid(struct xdr_stream *xdr, const struct nfs4_setclieencode_op_hdr (xdr,OP_SETCLIENTID , decode_setclientid_maxsz, hdr);encode_nfs4_verifier (xdr, setclientid->sc_verifier); -encode_string (xdr, setclientid->sc_name_len, setclientid->sc_name); +encode_string (xdr,strlen (setclientid->sc_clnt->cl_owner_id), + setclientid->sc_clnt->cl_owner_id); p =reserve_space (xdr,4 ); *p =cpu_to_be32 (setclientid->sc_prog);encode_string (xdr, setclientid->sc_netid_len, setclientid->sc_netid);encode_string (xdr, setclientid->sc_uaddr_len, setclientid->sc_uaddr); p =reserve_space (xdr,4 ); - *p =cpu_to_be32 (setclientid->sc_cb_ident); + *p =cpu_to_be32 (setclientid->sc_clnt->cl_cb_ident); }static void encode_setclientid_confirm (struct xdr_stream *xdr,const struct nfs4_setclientid_res *arg,struct compound_hdr *hdr)
@@ -1747,7 +1748,8 @@ static void encode_exchange_id(struct xdr_stream *xdr,encode_op_hdr (xdr,OP_EXCHANGE_ID , decode_exchange_id_maxsz, hdr);encode_nfs4_verifier (xdr, args->verifier); -encode_string (xdr, args->id_len, args->id); +encode_string (xdr,strlen (args->client->cl_owner_id), + args->client->cl_owner_id);encode_uint32 (xdr, args->flags);encode_uint32 (xdr, args->state_protect.how);
include/linux/nfs_xdr.h +1 -1
@@ -994,7 +994,7 @@ struct nfs4_setclientid {char sc_netid[RPCBIND_MAXNETIDLEN +1 ];unsigned int sc_uaddr_len;char sc_uaddr[RPCBIND_MAXUADDRLEN +1 ]; -u32 sc_cb_ident; +struct nfs_client *sc_clnt;struct rpc_cred *sc_cred; };
Signature · 1 line
2.4.2
PATCH v3 3/5 nfs: update maxsz values for SETCLIENTID and EXCHANGE_ID
In reply to Jeff LaytonThe spec allows for up to NFS4_OPAQUE_LIMIT (1k). While we'll almost certainly never use that much, these ops are generally the only ones in the compound so we might as well allow for them to be that large.
Also, the existing code didn't add in a word for the opaque length field for either name string. Fix that while we're in there.
--- fs/nfs/nfs4xdr.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
fs/nfs/nfs4xdr.c +4 -2
@@ -139,7 +139,8 @@ static int nfs4_stat_to_errno(int);#define encode_setclientid_maxsz \ (op_encode_hdr_maxsz + \ XDR_QUADLEN(NFS4_VERIFIER_SIZE) + \ - XDR_QUADLEN(NFS4_SETCLIENTID_NAMELEN) + \ +/* client name */ \ +1 +XDR_QUADLEN (NFS4_OPAQUE_LIMIT ) + \1 /* sc_prog */ + \1 +XDR_QUADLEN (RPCBIND_MAXNETIDLEN ) + \ 1 + XDR_QUADLEN(RPCBIND_MAXUADDRLEN) + \
@@ -288,7 +289,8 @@ static int nfs4_stat_to_errno(int);#define encode_exchange_id_maxsz (op_encode_hdr_maxsz + \ encode_verifier_maxsz + \ 1/* co_ownerid.len */ + \ -XDR_QUADLEN (NFS4_EXCHANGE_ID_LEN ) + \ +/* eia_clientowner */ \ +1 +XDR_QUADLEN (NFS4_OPAQUE_LIMIT ) + \1 /* flags */ + \1 /* spa_how */ + \ /* max is SP4_MACH_CRED (for now) */ + \
Signature · 1 line
2.4.2
PATCH v3 4/5 nfs: make nfs4_init_nonuniform_client_string use a dynamically allocated buffer
In reply to Jeff LaytonThe way the *_client_string functions work is a little goofy. They build the string in an on-stack buffer and then use kstrdup to copy it. This is not only stack-heavy but artificially limits the size of the client name string. Change it so that we determine the length of the string, allocate it and then scnprintf into it.
Since the contents of the nonuniform string depend on rcu-managed data structures, it's possible that they'll change between when we allocate the string and when we go to fill it. If that happens, free the string, recalculate the length and try again. If it the mismatch isn't resolved on the second try then just give up and return -EINVAL.
--- fs/nfs/nfs4proc.c | 74 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 23 deletions(-)
fs/nfs/nfs4proc.c +51 -23
@@ -4955,25 +4955,54 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,memcpy (bootverf->data, verf,sizeof (bootverf->data)); } -static unsigned int -nfs4_init_nonuniform_client_string (struct nfs_client *clp, -char *buf,size_t len) +static int +nfs4_init_nonuniform_client_string (struct nfs_client *clp) { -unsigned int result; +int result; +size_t len; +char *str; +bool retried = false;if (clp->cl_owner_id !=NULL ) -return strlcpy (buf, clp->cl_owner_id, len); +return 0 ; +retry: +rcu_read_lock (); + len =10 +strlen (clp->cl_ipaddr) +1 + +strlen (rpc_peeraddr2str (clp->cl_rpcclient,RPC_DISPLAY_ADDR )) + +1 + +strlen (rpc_peeraddr2str (clp->cl_rpcclient,RPC_DISPLAY_PROTO )) + +1 ; +rcu_read_unlock (); + +if (len >NFS4_OPAQUE_LIMIT +1 ) +return -EINVAL ; + +/* +* Since this string is allocated at mount time, and held until the +* nfs_client is destroyed, we can use GFP_KERNEL here w/o worrying +* about a memory-reclaim deadlock. +*/ + str =kmalloc (len,GFP_KERNEL ); +if (!str) +return -ENOMEM ;rcu_read_lock (); - result =scnprintf (buf, len,"Linux NFSv4.0 %s/%s %s" , - clp->cl_ipaddr, -rpc_peeraddr2str (clp->cl_rpcclient, -RPC_DISPLAY_ADDR ), -rpc_peeraddr2str (clp->cl_rpcclient, -RPC_DISPLAY_PROTO )); + result =scnprintf (str, len,"Linux NFSv4.0 %s/%s %s" , + clp->cl_ipaddr, +rpc_peeraddr2str (clp->cl_rpcclient,RPC_DISPLAY_ADDR ), +rpc_peeraddr2str (clp->cl_rpcclient,RPC_DISPLAY_PROTO ));rcu_read_unlock (); - clp->cl_owner_id =kstrdup (buf,GFP_KERNEL ); -return result; + +/* Did something change? */ +if (result >= len) { +kfree (str); +if (retried) +return -EINVAL ; + retried = true; + goto retry; + } + clp->cl_owner_id = str; +return 0 ; } static unsigned int
@@ -5064,20 +5093,19 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,/* nfs_client_id4 */ nfs4_init_boot_verifier (clp, &sc_verifier); -if (test_bit (NFS_CS_MIGRATION , &clp->cl_flags)) +if (test_bit (NFS_CS_MIGRATION , &clp->cl_flags)) { setclientid.sc_name_len =nfs4_init_uniform_client_string (clp, setclientid.sc_name,sizeof (setclientid.sc_name)); -else - setclientid.sc_name_len = -nfs4_init_nonuniform_client_string (clp, - setclientid.sc_name, -sizeof (setclientid.sc_name)); - -if (!clp->cl_owner_id) { - status = -ENOMEM ; - goto out; +if (!clp->cl_owner_id) { + status = -ENOMEM ; + goto out; + } + }else { + status =nfs4_init_nonuniform_client_string (clp); +if (status) + goto out; }/* cb_client4 */
Signature · 1 line
2.4.2
PATCH v3 5/5 nfs: make nfs4_init_uniform_client_string use a dynamically allocated buffer
In reply to Jeff LaytonChange the uniform client string generator to dynamically allocate the NFSv4 client name string buffer. With this patch, we can eliminate the buffers that are embedded within the "args" structs and simply use the name string that is hanging off the client.
This uniform string case is a little simpler than the nonuniform since we don't need to deal with RCU, but we do have two different cases, depending on whether there is a uniquifier or not.
--- fs/nfs/nfs4proc.c | 112 +++++++++++++++++++++++++++++++++--------------- include/linux/nfs_xdr.h | 6 --- 2 files changed, 77 insertions(+), 41 deletions(-)
fs/nfs/nfs4proc.c +77 -35
@@ -5005,28 +5005,78 @@ retry:return 0 ; } -static unsigned int -nfs4_init_uniform_client_string (struct nfs_client *clp, -char *buf,size_t len) +static int +nfs4_init_uniquifier_client_string (struct nfs_client *clp) { -const char *nodename = clp->cl_rpcclient->cl_nodename; -unsigned int result; +int result; +size_t len; +char *str; + + len =10 +10 +1 +10 +1 + +strlen (nfs4_client_id_uniquifier) +1 + +strlen (clp->cl_rpcclient->cl_nodename) +1 ; + +if (len >NFS4_OPAQUE_LIMIT +1 ) +return -EINVAL ; + +/* +* Since this string is allocated at mount time, and held until the +* nfs_client is destroyed, we can use GFP_KERNEL here w/o worrying +* about a memory-reclaim deadlock. +*/ + str =kmalloc (len,GFP_KERNEL ); +if (!str) +return -ENOMEM ; + + result =scnprintf (str, len,"Linux NFSv%u.%u %s/%s" , + clp->rpc_ops->version, clp->cl_minorversion, + nfs4_client_id_uniquifier, + clp->cl_rpcclient->cl_nodename); +if (result >= len) { +kfree (str); +return -EINVAL ; + } + clp->cl_owner_id = str; +return 0 ; +} + +static int +nfs4_init_uniform_client_string (struct nfs_client *clp) +{ +int result; +size_t len; +char *str;if (clp->cl_owner_id !=NULL ) -return strlcpy (buf, clp->cl_owner_id, len); +return 0 ;if (nfs4_client_id_uniquifier[0 ] !='\0' ) - result =scnprintf (buf, len,"Linux NFSv%u.%u %s/%s" , - clp->rpc_ops->version, - clp->cl_minorversion, - nfs4_client_id_uniquifier, - nodename); -else - result =scnprintf (buf, len,"Linux NFSv%u.%u %s" , - clp->rpc_ops->version, clp->cl_minorversion, - nodename); - clp->cl_owner_id =kstrdup (buf,GFP_KERNEL ); -return result; +return nfs4_init_uniquifier_client_string (clp); + + len =10 +10 +1 +10 +1 + +strlen (clp->cl_rpcclient->cl_nodename) +1 ; + +if (len >NFS4_OPAQUE_LIMIT +1 ) +return -EINVAL ; + +/* +* Since this string is allocated at mount time, and held until the +* nfs_client is destroyed, we can use GFP_KERNEL here w/o worrying +* about a memory-reclaim deadlock. +*/ + str =kmalloc (len,GFP_KERNEL ); +if (!str) +return -ENOMEM ; + + result =scnprintf (str, len,"Linux NFSv%u.%u %s" , + clp->rpc_ops->version, clp->cl_minorversion, + clp->cl_rpcclient->cl_nodename); +if (result >= len) { +kfree (str); +return -EINVAL ; + } + clp->cl_owner_id = str; +return 0 ; }/*
@@ -5093,20 +5143,14 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,/* nfs_client_id4 */ nfs4_init_boot_verifier (clp, &sc_verifier); -if (test_bit (NFS_CS_MIGRATION , &clp->cl_flags)) { - setclientid.sc_name_len = -nfs4_init_uniform_client_string (clp, - setclientid.sc_name, -sizeof (setclientid.sc_name)); -if (!clp->cl_owner_id) { - status = -ENOMEM ; - goto out; - } - }else { + +if (test_bit (NFS_CS_MIGRATION , &clp->cl_flags)) + status =nfs4_init_uniform_client_string (clp); +else status =nfs4_init_nonuniform_client_string (clp); -if (status) - goto out; - } + +if (status) + goto out;/* cb_client4 */ setclientid.sc_netid_len =
@@ -6880,12 +6924,10 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, };nfs4_init_boot_verifier (clp, &verifier); - args.id_len =nfs4_init_uniform_client_string (clp, args.id, -sizeof (args.id)); -if (!clp->cl_owner_id) { - status = -ENOMEM ; + + status =nfs4_init_uniform_client_string (clp); +if (status) goto out; - } dprintk("NFS call exchange_id auth=%s, '%s'\n", clp->cl_rpcclient->cl_auth->au_ops->au_name,
include/linux/nfs_xdr.h +0 -6
@@ -984,11 +984,8 @@ struct nfs4_readlink_res {struct nfs4_sequence_res seq_res; }; -#define NFS4_SETCLIENTID_NAMELEN (127)struct nfs4_setclientid {const nfs4_verifier * sc_verifier; -unsigned int sc_name_len; -char sc_name[NFS4_SETCLIENTID_NAMELEN +1 ];u32 sc_prog;unsigned int sc_netid_len;char sc_netid[RPCBIND_MAXNETIDLEN +1 ];
@@ -1142,12 +1139,9 @@ struct nfs41_state_protection {struct nfs4_op_map allow; }; -#define NFS4_EXCHANGE_ID_LEN (127)struct nfs41_exchange_id_args {struct nfs_client *client;nfs4_verifier *verifier; -unsigned int id_len; -char id[NFS4_EXCHANGE_ID_LEN ];u32 flags;struct nfs41_state_protection state_protect; };
Signature · 1 line
2.4.2