[PATCH 0/7] Intro: convert lockd to kthread and fix use-after-free (try #2)

[PATCH 0/7] Intro: convert lockd to kthread and fix use-after-free (try #2)

[Jeff Layton]

This is the second patchset to fix the use-after-free problem in lockd,
and to convert lockd to use the kthread API instead of kernel_thread.
The main change from the last patchset is that svc_prepare_thread is
now exported and svc_create_kthread no longer calls it. The caller of
svc_create_kthread must now call it (or do the equivalent to set up
the svc_rqst.

Tested against the reproducer I have for the main issue (detailed in
patch #7). As always, comments and suggestions are appreciated.

Signed-off-by: Jeff Layton <jlayton@redhat.com>

[PATCH 1/7] SUNRPC: Allow svc_pool_map_set_cpumask to work with any task

[Jeff Layton]

svc_pool_map_set_cpumask will only affect "current" as of now. Add a
new arg so that it can change the cpumask on any given task. Also if
we're not changing "current" we don't care what the oldmask was, so
allow it to be a NULL pointer.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/svc.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index a4a6bf7..9696ae7 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -297,7 +297,8 @@ svc_pool_map_put(void)
  * Returns 1 and fills in oldmask iff a cpumask was applied.
  */
 static inline int
-svc_pool_map_set_cpumask(unsigned int pidx, cpumask_t *oldmask)
+svc_pool_map_set_cpumask(struct task_struct *task, unsigned int pidx,
+			 cpumask_t *oldmask)
 {
 	struct svc_pool_map *m = &svc_pool_map;
 	unsigned int node; /* or cpu */
@@ -314,13 +315,15 @@ svc_pool_map_set_cpumask(unsigned int pidx, cpumask_t *oldmask)
 		return 0;
 	case SVC_POOL_PERCPU:
 		node = m->pool_to[pidx];
-		*oldmask = current->cpus_allowed;
-		set_cpus_allowed(current, cpumask_of_cpu(node));
+		if (oldmask != NULL)
+			*oldmask = task->cpus_allowed;
+		set_cpus_allowed(task, cpumask_of_cpu(node));
 		return 1;
 	case SVC_POOL_PERNODE:
 		node = m->pool_to[pidx];
-		*oldmask = current->cpus_allowed;
-		set_cpus_allowed(current, node_to_cpumask(node));
+		if (oldmask != NULL)
+			*oldmask = task->cpus_allowed;
+		set_cpus_allowed(task, node_to_cpumask(node));
 		return 1;
 	}
 }
@@ -569,7 +572,8 @@ __svc_create_thread(svc_thread_fn func, struct svc_serv *serv,
 	rqstp->rq_pool = pool;
 
 	if (serv->sv_nrpools > 1)
-		have_oldmask = svc_pool_map_set_cpumask(pool->sp_id, &oldmask);
+		have_oldmask = svc_pool_map_set_cpumask(current, pool->sp_id,
+							&oldmask);
 
 	error = kernel_thread((int (*)(void *)) func, rqstp, 0);
 
-- 
1.5.3.3

[PATCH 2/7] SUNRPC: spin svc_rqst initialization to its own function

[Jeff Layton]

Move the initialzation in __svc_create_thread that happens prior to
thread creation to a new function. Export the function so that when
we replace __svc_create_thread with a kthread version, callers will
have complete control over this initialization.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/sunrpc/svc.h |    2 ++
 net/sunrpc/sunrpc_syms.c   |    1 +
 net/sunrpc/svc.c           |   42 ++++++++++++++++++++++++++++++------------
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 8531a70..19483fe 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -382,6 +382,8 @@ struct svc_procedure {
  */
 struct svc_serv *  svc_create(struct svc_program *, unsigned int,
 			      void (*shutdown)(struct svc_serv*));
+struct svc_rqst *  svc_prepare_thread(struct svc_serv *serv,
+					struct svc_pool *pool);
 int		   svc_create_thread(svc_thread_fn, struct svc_serv *);
 void		   svc_exit_thread(struct svc_rqst *);
 struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 33d89e8..27bb185 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -63,6 +63,7 @@ EXPORT_SYMBOL(put_rpccred);
 
 /* RPC server stuff */
 EXPORT_SYMBOL(svc_create);
+EXPORT_SYMBOL(svc_prepare_thread);
 EXPORT_SYMBOL(svc_create_thread);
 EXPORT_SYMBOL(svc_create_pooled);
 EXPORT_SYMBOL(svc_set_num_threads);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 9696ae7..854e8ec 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -538,23 +538,14 @@ svc_release_buffer(struct svc_rqst *rqstp)
 			put_page(rqstp->rq_pages[i]);
 }
 
-/*
- * Create a thread in the given pool.  Caller must hold BKL.
- * On a NUMA or SMP machine, with a multi-pool serv, the thread
- * will be restricted to run on the cpus belonging to the pool.
- */
-static int
-__svc_create_thread(svc_thread_fn func, struct svc_serv *serv,
-		    struct svc_pool *pool)
+struct svc_rqst *
+svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool)
 {
 	struct svc_rqst	*rqstp;
-	int		error = -ENOMEM;
-	int		have_oldmask = 0;
-	cpumask_t	oldmask;
 
 	rqstp = kzalloc(sizeof(*rqstp), GFP_KERNEL);
 	if (!rqstp)
-		goto out;
+		goto out_enomem;
 
 	init_waitqueue_head(&rqstp->rq_wait);
 
@@ -570,6 +561,33 @@ __svc_create_thread(svc_thread_fn func, struct svc_serv *serv,
 	spin_unlock_bh(&pool->sp_lock);
 	rqstp->rq_server = serv;
 	rqstp->rq_pool = pool;
+	return rqstp;
+
+out_thread:
+	svc_exit_thread(rqstp);
+out_enomem:
+	return ERR_PTR(-ENOMEM);
+}
+
+/*
+ * Create a thread in the given pool.  Caller must hold BKL.
+ * On a NUMA or SMP machine, with a multi-pool serv, the thread
+ * will be restricted to run on the cpus belonging to the pool.
+ */
+static int
+__svc_create_thread(svc_thread_fn func, struct svc_serv *serv,
+		    struct svc_pool *pool)
+{
+	struct svc_rqst	*rqstp;
+	int		error = -ENOMEM;
+	int		have_oldmask = 0;
+	cpumask_t	oldmask;
+
+	rqstp = svc_prepare_thread(serv, pool);
+	if (IS_ERR(rqstp)) {
+		error = PTR_ERR(rqstp);
+		goto out;
+	}
 
 	if (serv->sv_nrpools > 1)
 		have_oldmask = svc_pool_map_set_cpumask(current, pool->sp_id,
-- 
1.5.3.3

[PATCH 3/7] SUNRPC: make new svc_create_kthread function

[Jeff Layton]

Add a new function svc_create_kthread that spawns svc threads using the
kthread API.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/sunrpc/svc.h |    2 ++
 net/sunrpc/sunrpc_syms.c   |    1 +
 net/sunrpc/svc.c           |   19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 19483fe..5296b52 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -385,6 +385,8 @@ struct svc_serv *  svc_create(struct svc_program *, unsigned int,
 struct svc_rqst *  svc_prepare_thread(struct svc_serv *serv,
 					struct svc_pool *pool);
 int		   svc_create_thread(svc_thread_fn, struct svc_serv *);
+int		   svc_create_kthread(svc_thread_fn func,
+					struct svc_rqst *rqstp);
 void		   svc_exit_thread(struct svc_rqst *);
 struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
 			void (*shutdown)(struct svc_serv*),
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 27bb185..757e730 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -65,6 +65,7 @@ EXPORT_SYMBOL(put_rpccred);
 EXPORT_SYMBOL(svc_create);
 EXPORT_SYMBOL(svc_prepare_thread);
 EXPORT_SYMBOL(svc_create_thread);
+EXPORT_SYMBOL(svc_create_kthread);
 EXPORT_SYMBOL(svc_create_pooled);
 EXPORT_SYMBOL(svc_set_num_threads);
 EXPORT_SYMBOL(svc_exit_thread);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 854e8ec..0c8c327 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -18,6 +18,7 @@
 #include <linux/mm.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/kthread.h>
 
 #include <linux/sunrpc/types.h>
 #include <linux/sunrpc/xdr.h>
@@ -619,6 +620,24 @@ svc_create_thread(svc_thread_fn func, struct svc_serv *serv)
 	return __svc_create_thread(func, serv, &serv->sv_pools[0]);
 }
 
+int
+svc_create_kthread(svc_thread_fn func, struct svc_rqst *rqstp)
+{
+	struct svc_serv *serv = rqstp->rq_server;
+	struct task_struct *task;
+
+	task = kthread_create((int (*)(void *)) func, rqstp, serv->sv_name);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+
+	if (serv->sv_nrpools > 1)
+		svc_pool_map_set_cpumask(task, rqstp->rq_pool->sp_id, NULL);
+
+	svc_sock_update_bufs(serv);
+	wake_up_process(task);
+	return 0;
+}
+
 /*
  * Choose a pool in which to create a new thread, for svc_set_num_threads
  */
-- 
1.5.3.3

[PATCH 4/7] NLM: Initialize completion variable in lockd_up

[Jeff Layton]

lockd_start_done is a global var that can be reused if lockd is
restarted, but it's never reinitialized. On all but the first use,
wait_for_completion isn't actually waiting on it since it has
already completed once.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/lockd/svc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 82e2192..0f4148a 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -300,6 +300,7 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 	/*
 	 * Create the kernel thread and wait for it to start.
 	 */
+	init_completion(&lockd_start_done);
 	error = svc_create_thread(lockd, serv);
 	if (error) {
 		printk(KERN_WARNING
-- 
1.5.3.3

[PATCH 5/7] NLM: Have lockd call try_to_freeze

[Jeff Layton]

lockd makes itself freezable, but never calls try_to_freeze(). Have it
call try_to_freeze() within the main loop.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/lockd/svc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 0f4148a..03a83a0 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -155,6 +155,9 @@ lockd(struct svc_rqst *rqstp)
 		long timeout = MAX_SCHEDULE_TIMEOUT;
 		char buf[RPC_MAX_ADDRBUFLEN];
 
+		if (try_to_freeze())
+			continue;
+
 		if (signalled()) {
 			flush_signals(current);
 			if (nlmsvc_ops) {
-- 
1.5.3.3

[PATCH 6/7] NLM: Convert lockd to use kthreads

[Jeff Layton]

Have lockd_up start lockd using svc_create_kthread. With this change,
lockd_down now blocks until lockd actually exits, so there's no longer
need for the waitqueue code at the end of lockd_down. This also means
that only one lockd can be running at a time which simplifies the code
within lockd's main loop a bit.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/lockd/svc.c |   64 +++++++++++++++++++++++--------------------------------
 1 files changed, 27 insertions(+), 37 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 03a83a0..28c290c 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -25,6 +25,7 @@
 #include <linux/smp.h>
 #include <linux/smp_lock.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 #include <linux/freezer.h>
 
 #include <linux/sunrpc/types.h>
@@ -48,13 +49,12 @@ EXPORT_SYMBOL(nlmsvc_ops);
 
 static DEFINE_MUTEX(nlmsvc_mutex);
 static unsigned int		nlmsvc_users;
-static pid_t			nlmsvc_pid;
-static struct svc_serv		*nlmsvc_serv;
+static struct task_struct *	nlmsvc_task;
+static struct svc_serv *	nlmsvc_serv;
 int				nlmsvc_grace_period;
 unsigned long			nlmsvc_timeout;
 
 static DECLARE_COMPLETION(lockd_start_done);
-static DECLARE_WAIT_QUEUE_HEAD(lockd_exit);
 
 /*
  * These can be set at insmod time (useful for NFS as root filesystem),
@@ -128,11 +128,10 @@ lockd(struct svc_rqst *rqstp)
 	/*
 	 * Let our maker know we're running.
 	 */
-	nlmsvc_pid = current->pid;
+	nlmsvc_task = current;
 	nlmsvc_serv = rqstp->rq_server;
 	complete(&lockd_start_done);
 
-	daemonize("lockd");
 	set_freezable();
 
 	/* Process request with signals blocked, but allow SIGKILL.  */
@@ -151,7 +150,7 @@ lockd(struct svc_rqst *rqstp)
 	 * NFS mount or NFS daemon has gone away, and we've been sent a
 	 * signal, or else another process has taken over our job.
 	 */
-	while ((nlmsvc_users || !signalled()) && nlmsvc_pid == current->pid) {
+	while (!kthread_should_stop()) {
 		long timeout = MAX_SCHEDULE_TIMEOUT;
 		char buf[RPC_MAX_ADDRBUFLEN];
 
@@ -203,23 +202,18 @@ lockd(struct svc_rqst *rqstp)
 	 * Check whether there's a new lockd process before
 	 * shutting down the hosts and clearing the slot.
 	 */
-	if (!nlmsvc_pid || current->pid == nlmsvc_pid) {
-		if (nlmsvc_ops)
-			nlmsvc_invalidate_all();
-		nlm_shutdown_hosts();
-		nlmsvc_pid = 0;
-		nlmsvc_serv = NULL;
-	} else
-		printk(KERN_DEBUG
-			"lockd: new process, skipping host shutdown\n");
-	wake_up(&lockd_exit);
+	if (nlmsvc_ops)
+		nlmsvc_invalidate_all();
+	nlm_shutdown_hosts();
+	nlmsvc_task = NULL;
+	nlmsvc_serv = NULL;
 
 	/* Exit the RPC thread */
 	svc_exit_thread(rqstp);
 
 	/* Release module */
 	unlock_kernel();
-	module_put_and_exit(0);
+	module_put(THIS_MODULE);
 }
 
 
@@ -270,13 +264,14 @@ 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_pid) {
+	if (nlmsvc_task) {
 		if (proto)
 			error = make_socks(nlmsvc_serv, proto);
 		goto out;
@@ -303,11 +298,21 @@ 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);
+		printk(KERN_WARNING
+			"lockd_up: svc_rqst allocation failed, error=%d\n",
+			error);
+		goto destroy_and_out;
+	}
+
 	init_completion(&lockd_start_done);
-	error = svc_create_thread(lockd, serv);
+	error = svc_create_kthread(lockd, rqstp);
 	if (error) {
 		printk(KERN_WARNING
 			"lockd_up: create thread failed, error=%d\n", error);
+		svc_exit_thread(rqstp);
 		goto destroy_and_out;
 	}
 	wait_for_completion(&lockd_start_done);
@@ -339,30 +344,15 @@ lockd_down(void)
 		if (--nlmsvc_users)
 			goto out;
 	} else
-		printk(KERN_WARNING "lockd_down: no users! pid=%d\n", nlmsvc_pid);
+		printk(KERN_WARNING "lockd_down: no users! task=%p\n", nlmsvc_task);
 
-	if (!nlmsvc_pid) {
+	if (!nlmsvc_task) {
 		if (warned++ == 0)
 			printk(KERN_WARNING "lockd_down: no lockd running.\n"); 
 		goto out;
 	}
 	warned = 0;
-
-	kill_proc(nlmsvc_pid, SIGKILL, 1);
-	/*
-	 * Wait for the lockd process to exit, but since we're holding
-	 * the lockd semaphore, we can't wait around forever ...
-	 */
-	clear_thread_flag(TIF_SIGPENDING);
-	interruptible_sleep_on_timeout(&lockd_exit, HZ);
-	if (nlmsvc_pid) {
-		printk(KERN_WARNING 
-			"lockd_down: lockd failed to exit, clearing pid\n");
-		nlmsvc_pid = 0;
-	}
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	kthread_stop(nlmsvc_task);
 out:
 	mutex_unlock(&nlmsvc_mutex);
 }
-- 
1.5.3.3

[PATCH 7/7] NLM: Add reference counting to lockd

[Jeff Layton]

...and only have lockd exit when the last reference is dropped.

The problem is this:

When a lock that a client is blocking on comes free, lockd does this in
nlmsvc_grant_blocked():

    nlm_async_call(block->b_call, NLMPROC_GRANTED_MSG, &nlmsvc_grant_ops);

the callback from this call is nlmsvc_grant_callback(). That function
does this at the end to wake up lockd:

    svc_wake_up(block->b_daemon);

However there is no guarantee that lockd will be up when this happens.
If someone shuts down or restarts lockd before the async call completes,
then the b_daemon pointer will point to freed memory and the kernel may
oops.

I first noticed this on older kernels and had mistakenly thought that
newer kernels weren't susceptible, but that's not correct. There's a bit
of a race to make sure that the nlm_host is bound when the async call is
done, but I can now reproduce this at will on current kernels.

This patch is based on Trond's suggestion to add a new reference counter
to lockd, and only allows lockd to go down when it reaches 0. With this
change we can't use kthread_stop here. nlmsvc_unlink_block is called by
lockd and a kthread can't call kthread_stop on itself. So the patch
changes lockd to check the refcount itself and to return if it goes to
0. We do the checking and exit while holding the nlmsvc_mutex to make
sure that a new lockd is not started until the old one is down.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/lockd/svc.c              |   51 ++++++++++++++++++++++++++++++++----------
 fs/lockd/svclock.c          |    5 ++++
 include/linux/lockd/lockd.h |    1 +
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 28c290c..3c56827 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -51,6 +51,7 @@ static DEFINE_MUTEX(nlmsvc_mutex);
 static unsigned int		nlmsvc_users;
 static struct task_struct *	nlmsvc_task;
 static struct svc_serv *	nlmsvc_serv;
+atomic_t			nlmsvc_ref = ATOMIC_INIT(0);
 int				nlmsvc_grace_period;
 unsigned long			nlmsvc_timeout;
 
@@ -134,7 +135,10 @@ lockd(struct svc_rqst *rqstp)
 
 	set_freezable();
 
-	/* Process request with signals blocked, but allow SIGKILL.  */
+	/*
+	 * Process request with signals blocked, but allow SIGKILL which
+	 * signifies that lockd should drop all of its locks.
+	 */
 	allow_signal(SIGKILL);
 
 	dprintk("NFS locking service started (ver " LOCKD_VERSION ").\n");
@@ -147,15 +151,19 @@ lockd(struct svc_rqst *rqstp)
 
 	/*
 	 * The main request loop. We don't terminate until the last
-	 * NFS mount or NFS daemon has gone away, and we've been sent a
-	 * signal, or else another process has taken over our job.
+	 * NFS mount or NFS daemon has gone away, and the nlm_blocked
+	 * list is empty. The nlmsvc_mutex ensures that we prevent a
+	 * new lockd from being started before the old one is down.
 	 */
-	while (!kthread_should_stop()) {
+	mutex_lock(&nlmsvc_mutex);
+	while (atomic_read(&nlmsvc_ref) != 0) {
 		long timeout = MAX_SCHEDULE_TIMEOUT;
 		char buf[RPC_MAX_ADDRBUFLEN];
 
+		mutex_unlock(&nlmsvc_mutex);
+
 		if (try_to_freeze())
-			continue;
+			goto again;
 
 		if (signalled()) {
 			flush_signals(current);
@@ -182,11 +190,12 @@ lockd(struct svc_rqst *rqstp)
 		 */
 		err = svc_recv(rqstp, timeout);
 		if (err == -EAGAIN || err == -EINTR)
-			continue;
+			goto again;
 		if (err < 0) {
 			printk(KERN_WARNING
 			       "lockd: terminating on error %d\n",
 			       -err);
+			mutex_lock(&nlmsvc_mutex);
 			break;
 		}
 
@@ -194,19 +203,22 @@ lockd(struct svc_rqst *rqstp)
 				svc_print_addr(rqstp, buf, sizeof(buf)));
 
 		svc_process(rqstp);
+again:
+		mutex_lock(&nlmsvc_mutex);
 	}
 
-	flush_signals(current);
-
 	/*
-	 * Check whether there's a new lockd process before
-	 * shutting down the hosts and clearing the slot.
-	 */
+	 * at this point lockd is committed to going down. We hold the
+	 * nlmsvc_mutex until just before exit to prevent a new one
+	 * from starting before it's down.
+ 	 */
+	flush_signals(current);
 	if (nlmsvc_ops)
 		nlmsvc_invalidate_all();
 	nlm_shutdown_hosts();
 	nlmsvc_task = NULL;
 	nlmsvc_serv = NULL;
+	mutex_unlock(&nlmsvc_mutex);
 
 	/* Exit the RPC thread */
 	svc_exit_thread(rqstp);
@@ -268,6 +280,10 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 	int			error = 0;
 
 	mutex_lock(&nlmsvc_mutex);
+
+	if (!nlmsvc_users)
+		atomic_inc(&nlmsvc_ref);
+
 	/*
 	 * Check whether we're already up and running.
 	 */
@@ -324,6 +340,8 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 destroy_and_out:
 	svc_destroy(serv);
 out:
+	if (!nlmsvc_users && error)
+		atomic_dec(&nlmsvc_ref);
 	if (!error)
 		nlmsvc_users++;
 	mutex_unlock(&nlmsvc_mutex);
@@ -352,7 +370,16 @@ lockd_down(void)
 		goto out;
 	}
 	warned = 0;
-	kthread_stop(nlmsvc_task);
+	atomic_dec(&nlmsvc_ref);
+
+	/*
+	 * Sending a signal is necessary here. If we get to this point and
+	 * nlm_blocked isn't empty then lockd may be held hostage by clients
+	 * that are still blocking. Sending the signal makes sure that lockd
+	 * invalidates all of its locks so that it's just waiting on RPC
+	 * callbacks to complete
+	 */
+	kill_proc(nlmsvc_task->pid, SIGKILL, 1);
 out:
 	mutex_unlock(&nlmsvc_mutex);
 }
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index d120ec3..b8fbda3 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -61,6 +61,9 @@ nlmsvc_insert_block(struct nlm_block *block, unsigned long when)
 	struct list_head *pos;
 
 	dprintk("lockd: nlmsvc_insert_block(%p, %ld)\n", block, when);
+	if (list_empty(&nlm_blocked))
+		atomic_inc(&nlmsvc_ref);
+
 	if (list_empty(&block->b_list)) {
 		kref_get(&block->b_count);
 	} else {
@@ -239,6 +242,8 @@ static int nlmsvc_unlink_block(struct nlm_block *block)
 	/* Remove block from list */
 	status = posix_unblock_lock(block->b_file->f_file, &block->b_call->a_args.lock.fl);
 	nlmsvc_remove_block(block);
+	if (list_empty(&nlm_blocked))
+		atomic_dec(&nlmsvc_ref);
 	return status;
 }
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index e2d1ce3..7389553 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -154,6 +154,7 @@ extern struct svc_procedure	nlmsvc_procedures4[];
 extern int			nlmsvc_grace_period;
 extern unsigned long		nlmsvc_timeout;
 extern int			nsm_use_hostnames;
+extern atomic_t			nlmsvc_ref;
 
 /*
  * Lockd client functions
-- 
1.5.3.3

Re: [PATCH 3/7] SUNRPC: make new svc_create_kthread function

[Christoph Hellwig]

> +int
> +svc_create_kthread(svc_thread_fn func, struct svc_rqst *rqstp)
> +{
> +	struct svc_serv *serv = rqstp->rq_server;
> +	struct task_struct *task;
> +
> +	task = kthread_create((int (*)(void *)) func, rqstp, serv->sv_name);
> +	if (IS_ERR(task))
> +		return PTR_ERR(task);
> +
> +	if (serv->sv_nrpools > 1)
> +		svc_pool_map_set_cpumask(task, rqstp->rq_pool->sp_id, NULL);
> +
> +	svc_sock_update_bufs(serv);
> +	wake_up_process(task);
> +	return 0;

I don't think we'll need this helper at all.  For the normal case of
not using a pool the caller can just call svc_sock_update_bufs even
before creating the thread, and the just use kthread_run.  That'll also
get rid of all this horrible casting things around for the thread entry
point function pointer.

[PATCH 0/7] Intro: convert lockd to kthread and fix use-after-free (try #3)

[Jeff Layton]

This is the third patchset to fix the use-after-free problem in lockd,
and to convert lockd to use the kthread API instead of kernel_thread.
The main change from the last patchset is the elimination of the
svc_create_kthread helper function, and having lockd_up call kthread_run
directly. I've also made some changes to the function prototype for
the lockd function to eliminate the need for casting the function
pointer.

Tested against the reproducer I have for the main issue (detailed in
patch #7). As always, comments and suggestions are appreciated.

Signed-off-by: Jeff Layton <jlayton@redhat.com>

[PATCH 1/7] SUNRPC: Allow svc_pool_map_set_cpumask to work with any task

[Jeff Layton]

svc_pool_map_set_cpumask will only affect "current" as of now. Add a
new arg so that it can change the cpumask on any given task. Also if
we're not changing "current" we don't care what the oldmask was, so
allow it to be a NULL pointer.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/svc.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index a4a6bf7..9696ae7 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -297,7 +297,8 @@ svc_pool_map_put(void)
  * Returns 1 and fills in oldmask iff a cpumask was applied.
  */
 static inline int
-svc_pool_map_set_cpumask(unsigned int pidx, cpumask_t *oldmask)
+svc_pool_map_set_cpumask(struct task_struct *task, unsigned int pidx,
+			 cpumask_t *oldmask)
 {
 	struct svc_pool_map *m = &svc_pool_map;
 	unsigned int node; /* or cpu */
@@ -314,13 +315,15 @@ svc_pool_map_set_cpumask(unsigned int pidx, cpumask_t *oldmask)
 		return 0;
 	case SVC_POOL_PERCPU:
 		node = m->pool_to[pidx];
-		*oldmask = current->cpus_allowed;
-		set_cpus_allowed(current, cpumask_of_cpu(node));
+		if (oldmask != NULL)
+			*oldmask = task->cpus_allowed;
+		set_cpus_allowed(task, cpumask_of_cpu(node));
 		return 1;
 	case SVC_POOL_PERNODE:
 		node = m->pool_to[pidx];
-		*oldmask = current->cpus_allowed;
-		set_cpus_allowed(current, node_to_cpumask(node));
+		if (oldmask != NULL)
+			*oldmask = task->cpus_allowed;
+		set_cpus_allowed(task, node_to_cpumask(node));
 		return 1;
 	}
 }
@@ -569,7 +572,8 @@ __svc_create_thread(svc_thread_fn func, struct svc_serv *serv,
 	rqstp->rq_pool = pool;
 
 	if (serv->sv_nrpools > 1)
-		have_oldmask = svc_pool_map_set_cpumask(pool->sp_id, &oldmask);
+		have_oldmask = svc_pool_map_set_cpumask(current, pool->sp_id,
+							&oldmask);
 
 	error = kernel_thread((int (*)(void *)) func, rqstp, 0);
 
-- 
1.5.3.3