From abd1ec4efd82ca06127bce833ad8a4bbec8a0dcb Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Wed, 11 Jun 2008 10:03:12 -0400 Subject: lockd: close potential race with rapid lockd_up/lockd_down cycle If lockd_down is called very rapidly after lockd_up returns, then there is a slim chance that lockd() will never be called. kthread() will return before calling the function, so we'll end up never actually calling the cleanup functions for the thread. Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- fs/lockd/svc.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c index 2169af4d5455..5bd9bf0fa9df 100644 --- a/fs/lockd/svc.c +++ b/fs/lockd/svc.c @@ -50,7 +50,7 @@ EXPORT_SYMBOL(nlmsvc_ops); static DEFINE_MUTEX(nlmsvc_mutex); static unsigned int nlmsvc_users; static struct task_struct *nlmsvc_task; -static struct svc_serv *nlmsvc_serv; +static struct svc_rqst *nlmsvc_rqst; int nlmsvc_grace_period; unsigned long nlmsvc_timeout; @@ -194,20 +194,11 @@ lockd(void *vrqstp) svc_process(rqstp); } - flush_signals(current); if (nlmsvc_ops) nlmsvc_invalidate_all(); nlm_shutdown_hosts(); - unlock_kernel(); - - nlmsvc_task = NULL; - nlmsvc_serv = NULL; - - /* Exit the RPC thread */ - svc_exit_thread(rqstp); - return 0; } @@ -254,16 +245,15 @@ int lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */ { struct svc_serv *serv; - struct svc_rqst *rqstp; int error = 0; mutex_lock(&nlmsvc_mutex); /* * Check whether we're already up and running. */ - if (nlmsvc_serv) { + if (nlmsvc_rqst) { if (proto) - error = make_socks(nlmsvc_serv, proto); + error = make_socks(nlmsvc_rqst->rq_server, proto); goto out; } @@ -288,9 +278,10 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */ /* * Create the kernel thread and wait for it to start. */ - rqstp = svc_prepare_thread(serv, &serv->sv_pools[0]); - if (IS_ERR(rqstp)) { - error = PTR_ERR(rqstp); + nlmsvc_rqst = svc_prepare_thread(serv, &serv->sv_pools[0]); + if (IS_ERR(nlmsvc_rqst)) { + error = PTR_ERR(nlmsvc_rqst); + nlmsvc_rqst = NULL; printk(KERN_WARNING "lockd_up: svc_rqst allocation failed, error=%d\n", error); @@ -298,16 +289,15 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */ } svc_sock_update_bufs(serv); - nlmsvc_serv = rqstp->rq_server; - nlmsvc_task = kthread_run(lockd, rqstp, serv->sv_name); + nlmsvc_task = kthread_run(lockd, nlmsvc_rqst, serv->sv_name); if (IS_ERR(nlmsvc_task)) { error = PTR_ERR(nlmsvc_task); + svc_exit_thread(nlmsvc_rqst); nlmsvc_task = NULL; - nlmsvc_serv = NULL; + nlmsvc_rqst = NULL; printk(KERN_WARNING "lockd_up: kthread_run failed, error=%d\n", error); - svc_exit_thread(rqstp); goto destroy_and_out; } @@ -346,6 +336,9 @@ lockd_down(void) BUG(); } kthread_stop(nlmsvc_task); + svc_exit_thread(nlmsvc_rqst); + nlmsvc_task = NULL; + nlmsvc_rqst = NULL; out: mutex_unlock(&nlmsvc_mutex); } -- cgit v1.2.2 From b0e92aae157cbf78ede7b7dd03967b49fcb2102e Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 15 Jul 2008 12:35:20 -0400 Subject: lockd: nlm_release_host() checks for NULL, caller needn't No need to check for a NULL argument twice. Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- fs/lockd/svc4proc.c | 3 +-- fs/lockd/svcproc.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c index 385437e3387d..006a832d46f2 100644 --- a/fs/lockd/svc4proc.c +++ b/fs/lockd/svc4proc.c @@ -58,8 +58,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp, return 0; no_locks: - if (host) - nlm_release_host(host); + nlm_release_host(host); if (error) return error; return nlm_lck_denied_nolocks; diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c index 88379cc6e0b1..fce3d7039625 100644 --- a/fs/lockd/svcproc.c +++ b/fs/lockd/svcproc.c @@ -87,8 +87,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp, return 0; no_locks: - if (host) - nlm_release_host(host); + nlm_release_host(host); if (error) return error; return nlm_lck_denied_nolocks; -- cgit v1.2.2 From 8f920d5e29f86d3425a68e1c3bc264d1f6f55112 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 15 Jul 2008 14:06:48 -0400 Subject: lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_testlock nlmsvc_testlock calls nlmsvc_lookup_host to find a nlm_host struct. The callers of this functions, however, call nlmsvc_retrieve_args or nlm4svc_retrieve_args, which also return a nlm_host struct. Change nlmsvc_testlock to take a host arg instead of calling nlmsvc_lookup_host itself and change the callers to pass a pointer to the nlm_host they've already found. We take a reference to host in the place where nlmsvc_testlock() previous did a new lookup, so the reference counting is unchanged from before. Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- fs/lockd/svc4proc.c | 2 +- fs/lockd/svclock.c | 12 +++--------- fs/lockd/svcproc.c | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c index 006a832d46f2..8cfb9daa7c77 100644 --- a/fs/lockd/svc4proc.c +++ b/fs/lockd/svc4proc.c @@ -99,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp, return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success; /* Now check for conflicting locks */ - resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie); + resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie); if (resp->status == nlm_drop_reply) rc = rpc_drop_reply; else diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index 81aca859bfde..f40afb3a0e69 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -460,8 +460,8 @@ out: */ __be32 nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, - struct nlm_lock *lock, struct nlm_lock *conflock, - struct nlm_cookie *cookie) + struct nlm_host *host, struct nlm_lock *lock, + struct nlm_lock *conflock, struct nlm_cookie *cookie) { struct nlm_block *block = NULL; int error; @@ -479,16 +479,10 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, if (block == NULL) { struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL); - struct nlm_host *host; if (conf == NULL) return nlm_granted; - /* Create host handle for callback */ - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len); - if (host == NULL) { - kfree(conf); - return nlm_lck_denied_nolocks; - } + nlm_get_host(host); block = nlmsvc_create_block(rqstp, host, file, lock, cookie); if (block == NULL) { kfree(conf); diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c index fce3d7039625..e099f589b61b 100644 --- a/fs/lockd/svcproc.c +++ b/fs/lockd/svcproc.c @@ -128,7 +128,7 @@ nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp, return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success; /* Now check for conflicting locks */ - resp->status = cast_status(nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie)); + resp->status = cast_status(nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie)); if (resp->status == nlm_drop_reply) rc = rpc_drop_reply; else -- cgit v1.2.2 From 6cde4de80773497d8333985b135f472eda870904 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 15 Jul 2008 14:26:17 -0400 Subject: lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_lock nlmsvc_lock calls nlmsvc_lookup_host to find a nlm_host struct. The callers of this function, however, call nlmsvc_retrieve_args or nlm4svc_retrieve_args, which also return a nlm_host struct. Change nlmsvc_lock to take a host arg instead of calling nlmsvc_lookup_host itself and change the callers to pass a pointer to the nlm_host they've already found. Since nlmsvc_testlock() now just uses the caller's reference, we no longer need to get or release it. Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- fs/lockd/svc4proc.c | 2 +- fs/lockd/svclock.c | 10 ++-------- fs/lockd/svcproc.c | 2 +- 3 files changed, 4 insertions(+), 10 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c index 8cfb9daa7c77..189b2ce01da6 100644 --- a/fs/lockd/svc4proc.c +++ b/fs/lockd/svc4proc.c @@ -145,7 +145,7 @@ nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp, #endif /* Now try to lock the file */ - resp->status = nlmsvc_lock(rqstp, file, &argp->lock, + resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock, argp->block, &argp->cookie); if (resp->status == nlm_drop_reply) rc = rpc_drop_reply; diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index f40afb3a0e69..bcf73f6e8226 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -358,10 +358,10 @@ nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block) */ __be32 nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, - struct nlm_lock *lock, int wait, struct nlm_cookie *cookie) + struct nlm_host *host, struct nlm_lock *lock, int wait, + struct nlm_cookie *cookie) { struct nlm_block *block = NULL; - struct nlm_host *host; int error; __be32 ret; @@ -373,11 +373,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, (long long)lock->fl.fl_end, wait); - /* Create host handle for callback */ - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len); - if (host == NULL) - return nlm_lck_denied_nolocks; - /* Lock file against concurrent access */ mutex_lock(&file->f_mutex); /* Get existing block (in case client is busy-waiting) @@ -450,7 +445,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, out: mutex_unlock(&file->f_mutex); nlmsvc_release_block(block); - nlm_release_host(host); dprintk("lockd: nlmsvc_lock returned %u\n", ret); return ret; } diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c index e099f589b61b..82dc9083ba67 100644 --- a/fs/lockd/svcproc.c +++ b/fs/lockd/svcproc.c @@ -175,7 +175,7 @@ nlmsvc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp, #endif /* Now try to lock the file */ - resp->status = cast_status(nlmsvc_lock(rqstp, file, &argp->lock, + resp->status = cast_status(nlmsvc_lock(rqstp, file, host, &argp->lock, argp->block, &argp->cookie)); if (resp->status == nlm_drop_reply) rc = rpc_drop_reply; -- cgit v1.2.2 From 6d7bbbbacc5202eaabbc232681cc325b22a73eeb Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Tue, 15 Jul 2008 14:38:32 -0400 Subject: lockd: minor svclock.c style fixes Signed-off-by: J. Bruce Fields --- fs/lockd/svclock.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index bcf73f6e8226..51a0dea0ae8a 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -129,9 +129,9 @@ nlmsvc_lookup_block(struct nlm_file *file, struct nlm_lock *lock) static inline int nlm_cookie_match(struct nlm_cookie *a, struct nlm_cookie *b) { - if(a->len != b->len) + if (a->len != b->len) return 0; - if(memcmp(a->data,b->data,a->len)) + if (memcmp(a->data, b->data, a->len)) return 0; return 1; } @@ -381,7 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, block = nlmsvc_lookup_block(file, lock); if (block == NULL) { block = nlmsvc_create_block(rqstp, nlm_get_host(host), file, - lock, cookie); + lock, cookie); ret = nlm_lck_denied_nolocks; if (block == NULL) goto out; @@ -412,7 +412,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, lock->fl.fl_flags &= ~FL_SLEEP; dprintk("lockd: vfs_lock_file returned %d\n", error); - switch(error) { + switch (error) { case 0: ret = nlm_granted; goto out; @@ -880,7 +880,7 @@ nlmsvc_retry_blocked(void) if (block->b_when == NLM_NEVER) break; - if (time_after(block->b_when,jiffies)) { + if (time_after(block->b_when, jiffies)) { timeout = block->b_when - jiffies; break; } -- cgit v1.2.2 From 560de0e65904db392e1c443c4bf5ee750573336b Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Tue, 15 Jul 2008 15:05:45 -0400 Subject: lockd: get host reference in nlmsvc_create_block() instead of callers It may not be obvious (till you look at the definition of nlm_alloc_call()) that a function like nlmsvc_create_block() should consume a reference on success or failure, so I find it clearer if it takes the reference it needs itself. And both callers already do this immediately before the call anyway. Signed-off-by: J. Bruce Fields --- fs/lockd/svclock.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index 51a0dea0ae8a..b8f86b73a85d 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -180,6 +180,7 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host, struct nlm_block *block; struct nlm_rqst *call = NULL; + nlm_get_host(host); call = nlm_alloc_call(host); if (call == NULL) return NULL; @@ -380,8 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, */ block = nlmsvc_lookup_block(file, lock); if (block == NULL) { - block = nlmsvc_create_block(rqstp, nlm_get_host(host), file, - lock, cookie); + block = nlmsvc_create_block(rqstp, host, file, lock, cookie); ret = nlm_lck_denied_nolocks; if (block == NULL) goto out; @@ -476,7 +476,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, if (conf == NULL) return nlm_granted; - nlm_get_host(host); block = nlmsvc_create_block(rqstp, host, file, lock, cookie); if (block == NULL) { kfree(conf); -- cgit v1.2.2 From 367c8c7bd9a2882daad6c9cb607e1db8ef781ad4 Mon Sep 17 00:00:00 2001 From: Chuck Lever Date: Mon, 30 Jun 2008 18:58:14 -0400 Subject: lockd: Pass "struct sockaddr *" to new failover-by-IP function Pass a more generic socket address type to nlmsvc_unlock_all_by_ip() to allow for future support of IPv6. Also provide additional sanity checking in failover_unlock_ip() when constructing the server's IP address. As an added bonus, provide clean kerneldoc comments on related NLM interfaces which were recently added. Signed-off-by: Chuck Lever Signed-off-by: J. Bruce Fields --- fs/lockd/svcsubs.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) (limited to 'fs/lockd') diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c index d1c48b539df8..198b4e55b373 100644 --- a/fs/lockd/svcsubs.c +++ b/fs/lockd/svcsubs.c @@ -373,13 +373,16 @@ nlmsvc_free_host_resources(struct nlm_host *host) } } -/* - * Remove all locks held for clients +/** + * nlmsvc_invalidate_all - remove all locks held for clients + * + * Release all locks held by NFS clients. + * */ void nlmsvc_invalidate_all(void) { - /* Release all locks held by NFS clients. + /* * Previously, the code would call * nlmsvc_free_host_resources for each client in * turn, which is about as inefficient as it gets. @@ -396,6 +399,12 @@ nlmsvc_match_sb(void *datap, struct nlm_file *file) return sb == file->f_file->f_path.mnt->mnt_sb; } +/** + * nlmsvc_unlock_all_by_sb - release locks held on this file system + * @sb: super block + * + * Release all locks held by clients accessing this file system. + */ int nlmsvc_unlock_all_by_sb(struct super_block *sb) { @@ -409,17 +418,22 @@ EXPORT_SYMBOL_GPL(nlmsvc_unlock_all_by_sb); static int nlmsvc_match_ip(void *datap, struct nlm_host *host) { - __be32 *server_addr = datap; - - return host->h_saddr.sin_addr.s_addr == *server_addr; + return nlm_cmp_addr(&host->h_saddr, datap); } +/** + * nlmsvc_unlock_all_by_ip - release local locks by IP address + * @server_addr: server's IP address as seen by clients + * + * Release all locks held by clients accessing this host + * via the passed in IP address. + */ int -nlmsvc_unlock_all_by_ip(__be32 server_addr) +nlmsvc_unlock_all_by_ip(struct sockaddr *server_addr) { int ret; - ret = nlm_traverse_files(&server_addr, nlmsvc_match_ip, NULL); - return ret ? -EIO : 0; + ret = nlm_traverse_files(server_addr, nlmsvc_match_ip, NULL); + return ret ? -EIO : 0; } EXPORT_SYMBOL_GPL(nlmsvc_unlock_all_by_ip); -- cgit v1.2.2