From 0e24d849c4ea777c59955b241fd3af14a1b84af5 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Fri, 28 Sep 2012 12:03:09 -0400 Subject: [PATCH 1/3] NFSv4: Remove BUG_ON() and ACCESS_ONCE() calls in the idmapper The use of ACCESS_ONCE() is wrong, since the various routines that set/clear idmap->idmap_key_cons should be strictly ordered w.r.t. each other, and the idmap->idmap_mutex ensures that only one thread at a time may be in an upcall situation. Also replace the BUG_ON()s with WARN_ON_ONCE() where appropriate. Signed-off-by: Trond Myklebust --- fs/nfs/idmap.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c index a850079467d8..79f6424aa081 100644 --- a/fs/nfs/idmap.c +++ b/fs/nfs/idmap.c @@ -465,8 +465,6 @@ nfs_idmap_new(struct nfs_client *clp) struct rpc_pipe *pipe; int error; - BUG_ON(clp->cl_idmap != NULL); - idmap = kzalloc(sizeof(*idmap), GFP_KERNEL); if (idmap == NULL) return -ENOMEM; @@ -510,7 +508,6 @@ static int __rpc_pipefs_event(struct nfs_client *clp, unsigned long event, switch (event) { case RPC_PIPEFS_MOUNT: - BUG_ON(clp->cl_rpcclient->cl_dentry == NULL); err = __nfs_idmap_register(clp->cl_rpcclient->cl_dentry, clp->cl_idmap, clp->cl_idmap->idmap_pipe); @@ -689,7 +686,11 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, if (ret < 0) goto out2; - BUG_ON(idmap->idmap_key_cons != NULL); + if (idmap->idmap_key_cons != NULL) { + WARN_ON_ONCE(1); + ret = -EAGAIN; + goto out2; + } idmap->idmap_key_cons = cons; ret = rpc_queue_upcall(idmap->idmap_pipe, msg); @@ -746,7 +747,7 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen) * will have been woken up and someone else may now have used * idmap_key_cons - so after this point we may no longer touch it. */ - cons = ACCESS_ONCE(idmap->idmap_key_cons); + cons = idmap->idmap_key_cons; idmap->idmap_key_cons = NULL; if (mlen != sizeof(im)) { @@ -790,7 +791,7 @@ idmap_pipe_destroy_msg(struct rpc_pipe_msg *msg) struct idmap *idmap = data->idmap; struct key_construction *cons; if (msg->errno) { - cons = ACCESS_ONCE(idmap->idmap_key_cons); + cons = idmap->idmap_key_cons; idmap->idmap_key_cons = NULL; complete_request_key(cons, msg->errno); } From e9ab41b620e4b679ed069ab05cb85e67870b7c87 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Thu, 27 Sep 2012 15:44:19 -0400 Subject: [PATCH 2/3] NFSv4: Clean up the legacy idmapper upcall Replace the BUG_ON(idmap->idmap_key_cons != NULL) with a WARN_ON_ONCE(). Then get rid of the ACCESS_ONCE(idmap->idmap_key_cons). Then add helper functions for starting, finishing and aborting the legacy upcall. Signed-off-by: Trond Myklebust Cc: Bryan Schumaker --- fs/nfs/idmap.c | 65 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c index 79f6424aa081..8222ad861456 100644 --- a/fs/nfs/idmap.c +++ b/fs/nfs/idmap.c @@ -330,7 +330,6 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen, ret = nfs_idmap_request_key(&key_type_id_resolver_legacy, name, namelen, type, data, data_size, idmap); - idmap->idmap_key_cons = NULL; mutex_unlock(&idmap->idmap_mutex); } return ret; @@ -662,6 +661,34 @@ static int nfs_idmap_prepare_message(char *desc, struct idmap *idmap, return ret; } +static bool +nfs_idmap_prepare_pipe_upcall(struct idmap *idmap, + struct key_construction *cons) +{ + if (idmap->idmap_key_cons != NULL) { + WARN_ON_ONCE(1); + return false; + } + idmap->idmap_key_cons = cons; + return true; +} + +static void +nfs_idmap_complete_pipe_upcall_locked(struct idmap *idmap, int ret) +{ + struct key_construction *cons = idmap->idmap_key_cons; + + idmap->idmap_key_cons = NULL; + complete_request_key(cons, ret); +} + +static void +nfs_idmap_abort_pipe_upcall(struct idmap *idmap, int ret) +{ + if (idmap->idmap_key_cons != NULL) + nfs_idmap_complete_pipe_upcall_locked(idmap, ret); +} + static int nfs_idmap_legacy_upcall(struct key_construction *cons, const char *op, void *aux) @@ -686,21 +713,17 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, if (ret < 0) goto out2; - if (idmap->idmap_key_cons != NULL) { - WARN_ON_ONCE(1); - ret = -EAGAIN; + ret = -EAGAIN; + if (!nfs_idmap_prepare_pipe_upcall(idmap, cons)) goto out2; - } - idmap->idmap_key_cons = cons; ret = rpc_queue_upcall(idmap->idmap_pipe, msg); - if (ret < 0) - goto out3; + if (ret < 0) { + nfs_idmap_abort_pipe_upcall(idmap, ret); + kfree(data); + } return ret; - -out3: - idmap->idmap_key_cons = NULL; out2: kfree(data); out1: @@ -741,14 +764,15 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen) struct key_construction *cons; struct idmap_msg im; size_t namelen_in; - int ret; + int ret = -ENOKEY; /* If instantiation is successful, anyone waiting for key construction * will have been woken up and someone else may now have used * idmap_key_cons - so after this point we may no longer touch it. */ cons = idmap->idmap_key_cons; - idmap->idmap_key_cons = NULL; + if (cons == NULL) + goto out_noupcall; if (mlen != sizeof(im)) { ret = -ENOSPC; @@ -778,7 +802,8 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen) } out: - complete_request_key(cons, ret); + nfs_idmap_complete_pipe_upcall_locked(idmap, ret); +out_noupcall: return ret; } @@ -789,12 +814,9 @@ idmap_pipe_destroy_msg(struct rpc_pipe_msg *msg) struct idmap_legacy_upcalldata, pipe_msg); struct idmap *idmap = data->idmap; - struct key_construction *cons; - if (msg->errno) { - cons = idmap->idmap_key_cons; - idmap->idmap_key_cons = NULL; - complete_request_key(cons, msg->errno); - } + + if (msg->errno) + nfs_idmap_abort_pipe_upcall(idmap, msg->errno); /* Free memory allocated in nfs_idmap_legacy_upcall() */ kfree(data); } @@ -804,7 +826,8 @@ idmap_release_pipe(struct inode *inode) { struct rpc_inode *rpci = RPC_I(inode); struct idmap *idmap = (struct idmap *)rpci->private; - idmap->idmap_key_cons = NULL; + + nfs_idmap_abort_pipe_upcall(idmap, -EPIPE); } int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid) From 0cac120233305b614cfe3ad419f3655876066017 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Thu, 27 Sep 2012 16:15:00 -0400 Subject: [PATCH 3/3] NFSv4: Ensure that idmap_pipe_downcall sanity-checks the downcall data Use the idmapper upcall data to verify that the legacy idmapper daemon is indeed responding to an upcall that we sent. Signed-off-by: Trond Myklebust Cc: Bryan Schumaker --- fs/nfs/idmap.c | 62 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c index 8222ad861456..7ac93e0dd4cf 100644 --- a/fs/nfs/idmap.c +++ b/fs/nfs/idmap.c @@ -55,18 +55,19 @@ static const struct cred *id_resolver_cache; static struct key_type key_type_id_resolver_legacy; -struct idmap { - struct rpc_pipe *idmap_pipe; - struct key_construction *idmap_key_cons; - struct mutex idmap_mutex; -}; - struct idmap_legacy_upcalldata { struct rpc_pipe_msg pipe_msg; struct idmap_msg idmap_msg; + struct key_construction *key_cons; struct idmap *idmap; }; +struct idmap { + struct rpc_pipe *idmap_pipe; + struct idmap_legacy_upcalldata *idmap_upcall_data; + struct mutex idmap_mutex; +}; + /** * nfs_fattr_init_names - initialise the nfs_fattr owner_name/group_name fields * @fattr: fully initialised struct nfs_fattr @@ -663,29 +664,30 @@ static int nfs_idmap_prepare_message(char *desc, struct idmap *idmap, static bool nfs_idmap_prepare_pipe_upcall(struct idmap *idmap, - struct key_construction *cons) + struct idmap_legacy_upcalldata *data) { - if (idmap->idmap_key_cons != NULL) { + if (idmap->idmap_upcall_data != NULL) { WARN_ON_ONCE(1); return false; } - idmap->idmap_key_cons = cons; + idmap->idmap_upcall_data = data; return true; } static void nfs_idmap_complete_pipe_upcall_locked(struct idmap *idmap, int ret) { - struct key_construction *cons = idmap->idmap_key_cons; + struct key_construction *cons = idmap->idmap_upcall_data->key_cons; - idmap->idmap_key_cons = NULL; + kfree(idmap->idmap_upcall_data); + idmap->idmap_upcall_data = NULL; complete_request_key(cons, ret); } static void nfs_idmap_abort_pipe_upcall(struct idmap *idmap, int ret) { - if (idmap->idmap_key_cons != NULL) + if (idmap->idmap_upcall_data != NULL) nfs_idmap_complete_pipe_upcall_locked(idmap, ret); } @@ -714,14 +716,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, goto out2; ret = -EAGAIN; - if (!nfs_idmap_prepare_pipe_upcall(idmap, cons)) + if (!nfs_idmap_prepare_pipe_upcall(idmap, data)) goto out2; ret = rpc_queue_upcall(idmap->idmap_pipe, msg); - if (ret < 0) { + if (ret < 0) nfs_idmap_abort_pipe_upcall(idmap, ret); - kfree(data); - } return ret; out2: @@ -738,21 +738,32 @@ static int nfs_idmap_instantiate(struct key *key, struct key *authkey, char *dat authkey); } -static int nfs_idmap_read_message(struct idmap_msg *im, struct key *key, struct key *authkey) +static int nfs_idmap_read_and_verify_message(struct idmap_msg *im, + struct idmap_msg *upcall, + struct key *key, struct key *authkey) { char id_str[NFS_UINT_MAXLEN]; - int ret = -EINVAL; + int ret = -ENOKEY; + /* ret = -ENOKEY */ + if (upcall->im_type != im->im_type || upcall->im_conv != im->im_conv) + goto out; switch (im->im_conv) { case IDMAP_CONV_NAMETOID: + if (strcmp(upcall->im_name, im->im_name) != 0) + break; sprintf(id_str, "%d", im->im_id); ret = nfs_idmap_instantiate(key, authkey, id_str); break; case IDMAP_CONV_IDTONAME: + if (upcall->im_id != im->im_id) + break; ret = nfs_idmap_instantiate(key, authkey, im->im_name); break; + default: + ret = -EINVAL; } - +out: return ret; } @@ -770,10 +781,11 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen) * will have been woken up and someone else may now have used * idmap_key_cons - so after this point we may no longer touch it. */ - cons = idmap->idmap_key_cons; - if (cons == NULL) + if (idmap->idmap_upcall_data == NULL) goto out_noupcall; + cons = idmap->idmap_upcall_data->key_cons; + if (mlen != sizeof(im)) { ret = -ENOSPC; goto out; @@ -793,9 +805,11 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen) if (namelen_in == 0 || namelen_in == IDMAP_NAMESZ) { ret = -EINVAL; goto out; - } +} - ret = nfs_idmap_read_message(&im, cons->key, cons->authkey); + ret = nfs_idmap_read_and_verify_message(&im, + &idmap->idmap_upcall_data->idmap_msg, + cons->key, cons->authkey); if (ret >= 0) { key_set_timeout(cons->key, nfs_idmap_cache_timeout); ret = mlen; @@ -817,8 +831,6 @@ idmap_pipe_destroy_msg(struct rpc_pipe_msg *msg) if (msg->errno) nfs_idmap_abort_pipe_upcall(idmap, msg->errno); - /* Free memory allocated in nfs_idmap_legacy_upcall() */ - kfree(data); } static void