[PATCH] Switch nfs/callback.c to using struct pid, not pid_t

[PATCH] Switch nfs/callback.c to using struct pid, not pid_t

[Pavel Emelyanov]

Pid namespaces make it dangerous to use pid and tgid values
when run in some namespace. The struct pid itself is going
to be the only way for working with task pids, so make the
nfs callback thread use it.

Since nfs_callback_info.pid is set to current's one and reset
on the thread exit, it is safe not to get the struct pid. 

Since this pid is used later under lock_kernel() w/o sleeping 
operations, checking for i to be not NULL and killing the 
thread with kill_pid() is safe.

Signed-off-by: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>

---

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index a796be5..5b8e5fc 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -27,7 +27,7 @@
 struct nfs_callback_data {
 	unsigned int users;
 	struct svc_serv *serv;
-	pid_t pid;
+	struct pid *pid;
 	struct completion started;
 	struct completion stopped;
 };
@@ -64,7 +64,7 @@ static void nfs_callback_svc(struct svc_
 	__module_get(THIS_MODULE);
 	lock_kernel();
 
-	nfs_callback_info.pid = current->pid;
+	nfs_callback_info.pid = task_pid(current);
 	daemonize("nfsv4-svc");
 	/* Process request with signals blocked, but allow SIGKILL.  */
 	allow_signal(SIGKILL);
@@ -98,7 +98,7 @@ static void nfs_callback_svc(struct svc_
 	}
 
 	svc_exit_thread(rqstp);
-	nfs_callback_info.pid = 0;
+	nfs_callback_info.pid = NULL;
 	complete(&nfs_callback_info.stopped);
 	unlock_kernel();
 	module_put_and_exit(0);
@@ -114,7 +114,7 @@ int nfs_callback_up(void)
 
 	lock_kernel();
 	mutex_lock(&nfs_callback_mutex);
-	if (nfs_callback_info.users++ || nfs_callback_info.pid != 0)
+	if (nfs_callback_info.users++ || nfs_callback_info.pid != NULL)
 		goto out;
 	init_completion(&nfs_callback_info.started);
 	init_completion(&nfs_callback_info.stopped);
@@ -157,9 +157,9 @@ void nfs_callback_down(void)
 	mutex_lock(&nfs_callback_mutex);
 	nfs_callback_info.users--;
 	do {
-		if (nfs_callback_info.users != 0 || nfs_callback_info.pid == 0)
+		if (nfs_callback_info.users != 0 || nfs_callback_info.pid == NULL)
 			break;
-		if (kill_proc(nfs_callback_info.pid, SIGKILL, 1) < 0)
+		if (kill_pid(nfs_callback_info.pid, SIGKILL, 1) < 0)
 			break;
 	} while (wait_for_completion_timeout(&nfs_callback_info.stopped, 5*HZ) == 0);
 	mutex_unlock(&nfs_callback_mutex);

[PATCH] Switch nfs/callback.c to using struct pid, not pid_t

[Pavel Emelyanov]

Pid namespaces make it dangerous to use pid and tgid values
when run in some namespace. The struct pid itself is going
to be the only way for working with task pids, so make the
nfs callback thread use it.

Since nfs_callback_info.pid is set to current's one and reset
on the thread exit, it is safe not to get the struct pid. 

Since this pid is used later under lock_kernel() w/o sleeping 
operations, checking for i to be not NULL and killing the 
thread with kill_pid() is safe.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index a796be5..5b8e5fc 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -27,7 +27,7 @@
 struct nfs_callback_data {
 	unsigned int users;
 	struct svc_serv *serv;
-	pid_t pid;
+	struct pid *pid;
 	struct completion started;
 	struct completion stopped;
 };
@@ -64,7 +64,7 @@ static void nfs_callback_svc(struct svc_
 	__module_get(THIS_MODULE);
 	lock_kernel();
 
-	nfs_callback_info.pid = current->pid;
+	nfs_callback_info.pid = task_pid(current);
 	daemonize("nfsv4-svc");
 	/* Process request with signals blocked, but allow SIGKILL.  */
 	allow_signal(SIGKILL);
@@ -98,7 +98,7 @@ static void nfs_callback_svc(struct svc_
 	}
 
 	svc_exit_thread(rqstp);
-	nfs_callback_info.pid = 0;
+	nfs_callback_info.pid = NULL;
 	complete(&nfs_callback_info.stopped);
 	unlock_kernel();
 	module_put_and_exit(0);
@@ -114,7 +114,7 @@ int nfs_callback_up(void)
 
 	lock_kernel();
 	mutex_lock(&nfs_callback_mutex);
-	if (nfs_callback_info.users++ || nfs_callback_info.pid != 0)
+	if (nfs_callback_info.users++ || nfs_callback_info.pid != NULL)
 		goto out;
 	init_completion(&nfs_callback_info.started);
 	init_completion(&nfs_callback_info.stopped);
@@ -157,9 +157,9 @@ void nfs_callback_down(void)
 	mutex_lock(&nfs_callback_mutex);
 	nfs_callback_info.users--;
 	do {
-		if (nfs_callback_info.users != 0 || nfs_callback_info.pid == 0)
+		if (nfs_callback_info.users != 0 || nfs_callback_info.pid == NULL)
 			break;
-		if (kill_proc(nfs_callback_info.pid, SIGKILL, 1) < 0)
+		if (kill_pid(nfs_callback_info.pid, SIGKILL, 1) < 0)
 			break;
 	} while (wait_for_completion_timeout(&nfs_callback_info.stopped, 5*HZ) == 0);
 	mutex_unlock(&nfs_callback_mutex);

Re: [PATCH] Switch nfs/callback.c to using struct pid, not pid_t

[Christoph Hellwig]

On Wed, Aug 29, 2007 at 05:36:24PM +0400, Pavel Emelyanov wrote:
> Pid namespaces make it dangerous to use pid and tgid values
> when run in some namespace. The struct pid itself is going
> to be the only way for working with task pids, so make the
> nfs callback thread use it.
> 
> Since nfs_callback_info.pid is set to current's one and reset
> on the thread exit, it is safe not to get the struct pid. 
> 
> Since this pid is used later under lock_kernel() w/o sleeping 
> operations, checking for i to be not NULL and killing the 
> thread with kill_pid() is safe.

NACK.  This just makes the code even more obscure.  Please get rid
of the pid references entirely and convert the code to the kthread
API.

Re: [PATCH] Switch nfs/callback.c to using struct pid, not pid_t

[Trond Myklebust]

On Wed, 2007-08-29 at 14:52 +0100, Christoph Hellwig wrote:
> On Wed, Aug 29, 2007 at 05:36:24PM +0400, Pavel Emelyanov wrote:
> > Pid namespaces make it dangerous to use pid and tgid values
> > when run in some namespace. The struct pid itself is going
> > to be the only way for working with task pids, so make the
> > nfs callback thread use it.
> > 
> > Since nfs_callback_info.pid is set to current's one and reset
> > on the thread exit, it is safe not to get the struct pid. 
> > 
> > Since this pid is used later under lock_kernel() w/o sleeping 
> > operations, checking for i to be not NULL and killing the 
> > thread with kill_pid() is safe.
> 
> NACK.  This just makes the code even more obscure.  Please get rid
> of the pid references entirely and convert the code to the kthread
> API.

That would require converting the full sunrpc server code to use
kthreads, which again means changing nfsd, and lockd too.

I'm not saying that is a bad thing, but it is nontrivial to do. In
particular, kthread's abominable shutdown mechanism simply does not work
or scale when the thread is listening for new requests in svc_recv().

Cheers
  Trond

Re: [PATCH] Switch nfs/callback.c to using struct pid, not pid_t

[Christoph Hellwig]

On Wed, Aug 29, 2007 at 10:10:41AM -0400, Trond Myklebust wrote:
> On Wed, 2007-08-29 at 14:52 +0100, Christoph Hellwig wrote:
> > On Wed, Aug 29, 2007 at 05:36:24PM +0400, Pavel Emelyanov wrote:
> > > Pid namespaces make it dangerous to use pid and tgid values
> > > when run in some namespace. The struct pid itself is going
> > > to be the only way for working with task pids, so make the
> > > nfs callback thread use it.
> > > 
> > > Since nfs_callback_info.pid is set to current's one and reset
> > > on the thread exit, it is safe not to get the struct pid. 
> > > 
> > > Since this pid is used later under lock_kernel() w/o sleeping 
> > > operations, checking for i to be not NULL and killing the 
> > > thread with kill_pid() is safe.
> > 
> > NACK.  This just makes the code even more obscure.  Please get rid
> > of the pid references entirely and convert the code to the kthread
> > API.
> 
> That would require converting the full sunrpc server code to use
> kthreads, which again means changing nfsd, and lockd too.
> 
> I'm not saying that is a bad thing, but it is nontrivial to do. In
> particular, kthread's abominable shutdown mechanism simply does not work
> or scale when the thread is listening for new requests in svc_recv().

Actually converting callback.c is not that bad, it just means splitting
up svc_create_thread.  Only problem is whether we want to allow SIGKILL
from serspace to terminate the thread aswell, in which case we still
need changes to the core kthread code which I'm waiting for someone
for the containers crowd to finally implement as it's needed in
various other places.

I'll try to send out my svc_create_thread splitup soon because it's
actually a nice cleanup all by itself.