[PATCH] NFS: hold BKL when clearing nfs_callback_info.task

[PATCH] NFS: hold BKL when clearing nfs_callback_info.task

[Jeff Layton]

The global task pointers for the nfs4 callback thread is normally
protected by the nfs_callback_mutex. The exception is when the thread
exits abnormally. When this occurs, this variable is cleared without any
locking.

Make sure that this variable is cleared while still holding the BKL.

Also, there's no reason that nfs_callback_up and nfs_callback_down need
to hold the BKL while trying to lock nfs_callback_mutex. Reverse the
lock order as a micro-optimization.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/callback.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 2e5de77..2646724 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -84,8 +84,8 @@ nfs_callback_svc(void *vrqstp)
 		}
 		svc_process(rqstp);
 	}
-	unlock_kernel();
 	nfs_callback_info.task = NULL;
+	unlock_kernel();
 	svc_exit_thread(rqstp);
 	return 0;
 }
@@ -99,8 +99,8 @@ int nfs_callback_up(void)
 	struct svc_rqst *rqstp;
 	int ret = 0;
 
-	lock_kernel();
 	mutex_lock(&nfs_callback_mutex);
+	lock_kernel();
 	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
 		goto out;
 	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
@@ -142,8 +142,8 @@ out:
 	 */
 	if (serv)
 		svc_destroy(serv);
-	mutex_unlock(&nfs_callback_mutex);
 	unlock_kernel();
+	mutex_unlock(&nfs_callback_mutex);
 	return ret;
 out_err:
 	dprintk("Couldn't create callback socket or server thread; err = %d\n",
@@ -157,13 +157,13 @@ out_err:
  */
 void nfs_callback_down(void)
 {
-	lock_kernel();
 	mutex_lock(&nfs_callback_mutex);
+	lock_kernel();
 	nfs_callback_info.users--;
 	if (nfs_callback_info.users == 0 && nfs_callback_info.task != NULL)
 		kthread_stop(nfs_callback_info.task);
-	mutex_unlock(&nfs_callback_mutex);
 	unlock_kernel();
+	mutex_unlock(&nfs_callback_mutex);
 }
 
 static int nfs_callback_authenticate(struct svc_rqst *rqstp)
-- 
1.5.4.1

[PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

The global task and serv pointers for lockd are normally protected by
the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
this occurs, these variables are cleared without any locking.

Make sure that these variables are cleared while still holding the BKL,
and have lockd_up and lockd_down take the BKL.

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

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 66b5c98..efba919 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -191,12 +191,11 @@ lockd(void *vrqstp)
 	if (nlmsvc_ops)
 		nlmsvc_invalidate_all();
 	nlm_shutdown_hosts();
-
-	unlock_kernel();
-
 	nlmsvc_task = NULL;
 	nlmsvc_serv = NULL;
 
+	unlock_kernel();
+
 	/* Exit the RPC thread */
 	svc_exit_thread(rqstp);
 
@@ -250,6 +249,7 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 	int		error = 0;
 
 	mutex_lock(&nlmsvc_mutex);
+	lock_kernel();
 	/*
 	 * Check whether we're already up and running.
 	 */
@@ -312,6 +312,7 @@ destroy_and_out:
 out:
 	if (!error)
 		nlmsvc_users++;
+	unlock_kernel();
 	mutex_unlock(&nlmsvc_mutex);
 	return error;
 }
@@ -324,6 +325,7 @@ void
 lockd_down(void)
 {
 	mutex_lock(&nlmsvc_mutex);
+	lock_kernel();
 	if (nlmsvc_users) {
 		if (--nlmsvc_users)
 			goto out;
@@ -339,6 +341,7 @@ lockd_down(void)
 	}
 	kthread_stop(nlmsvc_task);
 out:
+	unlock_kernel();
 	mutex_unlock(&nlmsvc_mutex);
 }
 EXPORT_SYMBOL(lockd_down);
-- 
1.5.4.1

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Christoph Hellwig]

On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> The global task and serv pointers for lockd are normally protected by
> the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> this occurs, these variables are cleared without any locking.

Shouldn't we get rid of the case where it exits abnormally instead?

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

On Mon, 7 Apr 2008 12:45:01 -0400
Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > The global task and serv pointers for lockd are normally protected by
> > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > this occurs, these variables are cleared without any locking.
> 
> Shouldn't we get rid of the case where it exits abnormally instead?
> 

Not a bad idea. After chatting with Christoph a bit on IRC, I suppose
we have 2 options if we want to pursue this. When we get an unexpected
error from svc_recv(), we could:

1) sleep for a bit and then retry

2) call schedule() and sleep until kthread_stop shuts down the thread

I think #1 is probably the best option. It's certainly the more fault
tolerant. That also fixes another potential problem -- right now if the
thread exits and the nlmsvc_users count isn't 0, then we can
potentially BUG() on the next lockd_up/lockd_down.

Any thoughts on what an appropriate sleep timeout should be when this
happens? I was thinking 1s or so...

Trond, Bruce, any thoughts?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[J. Bruce Fields]

On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > The global task and serv pointers for lockd are normally protected by
> > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > this occurs, these variables are cleared without any locking.
> 
> Shouldn't we get rid of the case where it exits abnormally instead?

I tried to figure out when this could actually occur (when can
svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
in sock_recvmsg():

	- svc_recv() itself returns only -EAGAIN or the return from
	  ->xpo_recvfrom().
	- the only xpo_recvfrom() that's interesting is
	  svc_tcp_recvfrom(), which can return the error it gets from
	  svc_recvfrom(), which can return the error from
	  kernel_recvmsg(), which gets its return from sock_recvmsg().

Since __sock_recvmsg() has a security hook, it looks like we can end up
with an -EACCES from selinux?

So one case would be selinux deciding we weren't allowed to receive
packets from this socket.  Huh.

--b.

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Tom Tucker]

On Mon, 2008-04-07 at 13:56 -0400, J. Bruce Fields wrote:
> On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > The global task and serv pointers for lockd are normally protected by
> > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > this occurs, these variables are cleared without any locking.
> > 
> > Shouldn't we get rid of the case where it exits abnormally instead?
> 
> I tried to figure out when this could actually occur (when can
> svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> in sock_recvmsg():
> 
> 	- svc_recv() itself returns only -EAGAIN or the return from
> 	  ->xpo_recvfrom().
> 	- the only xpo_recvfrom() that's interesting is
> 	  svc_tcp_recvfrom(), which can return the error it gets from
> 	  svc_recvfrom(), which can return the error from
> 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> 
> Since __sock_recvmsg() has a security hook, it looks like we can end up
> with an -EACCES from selinux?

FWIW: I believe that if svc_recv returns anything other then -EINTR or
-EAGAIN, the service thread exits. I believe that the current design
(could be broken) is that if the transport finds an error, the action is
to set the XPT_CLOSE bit, enqueue the transport and return -EAGAIN. This
will cause the service thread to call svc_recv again and close
processing to occur.

> 
> So one case would be selinux deciding we weren't allowed to receive
> packets from this socket.  Huh.
> 
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

On Mon, 7 Apr 2008 13:56:15 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > The global task and serv pointers for lockd are normally protected by
> > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > this occurs, these variables are cleared without any locking.
> > 
> > Shouldn't we get rid of the case where it exits abnormally instead?
> 
> I tried to figure out when this could actually occur (when can
> svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> in sock_recvmsg():
> 
> 	- svc_recv() itself returns only -EAGAIN or the return from
> 	  ->xpo_recvfrom().
> 	- the only xpo_recvfrom() that's interesting is
> 	  svc_tcp_recvfrom(), which can return the error it gets from
> 	  svc_recvfrom(), which can return the error from
> 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> 
> Since __sock_recvmsg() has a security hook, it looks like we can end up
> with an -EACCES from selinux?
> 
> So one case would be selinux deciding we weren't allowed to receive
> packets from this socket.  Huh.

I got lost there too, but I would suspect that there are other errors
that can bubble up from the lower networking layers as well. Even if
there aren't currently, it's probably still prudent to assume that it's
a possibility and code for it.

I tend to think the safest thing is probably to do a long sleep (1s or
so and retry when we get an error (maybe also a ratelimited printk?).
It's unlikely to do any harm (other than a few wasted CPU cycles), and
if the error is transient then we have the possibility to recover and
continue normal operation.

The current situation is rather bad. If lockd exits abnormally,
then we'll BUG if someone happens to do a lockd_down().

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[J. Bruce Fields]

On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
> On Mon, 7 Apr 2008 13:56:15 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > > The global task and serv pointers for lockd are normally protected by
> > > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > > this occurs, these variables are cleared without any locking.
> > > 
> > > Shouldn't we get rid of the case where it exits abnormally instead?
> > 
> > I tried to figure out when this could actually occur (when can
> > svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> > in sock_recvmsg():
> > 
> > 	- svc_recv() itself returns only -EAGAIN or the return from
> > 	  ->xpo_recvfrom().
> > 	- the only xpo_recvfrom() that's interesting is
> > 	  svc_tcp_recvfrom(), which can return the error it gets from
> > 	  svc_recvfrom(), which can return the error from
> > 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> > 
> > Since __sock_recvmsg() has a security hook, it looks like we can end up
> > with an -EACCES from selinux?
> > 
> > So one case would be selinux deciding we weren't allowed to receive
> > packets from this socket.  Huh.
> 
> I got lost there too, but I would suspect that there are other errors
> that can bubble up from the lower networking layers as well. Even if
> there aren't currently, it's probably still prudent to assume that it's
> a possibility and code for it.
> 
> I tend to think the safest thing is probably to do a long sleep (1s or
> so and retry when we get an error (maybe also a ratelimited printk?).

Yeah, I guess I can't think of anything better.

--b.

> It's unlikely to do any harm (other than a few wasted CPU cycles), and
> if the error is transient then we have the possibility to recover and
> continue normal operation.

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

On Mon, 7 Apr 2008 16:50:27 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
> > On Mon, 7 Apr 2008 13:56:15 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > > > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > > > The global task and serv pointers for lockd are normally protected by
> > > > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > > > this occurs, these variables are cleared without any locking.
> > > > 
> > > > Shouldn't we get rid of the case where it exits abnormally instead?
> > > 
> > > I tried to figure out when this could actually occur (when can
> > > svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> > > in sock_recvmsg():
> > > 
> > > 	- svc_recv() itself returns only -EAGAIN or the return from
> > > 	  ->xpo_recvfrom().
> > > 	- the only xpo_recvfrom() that's interesting is
> > > 	  svc_tcp_recvfrom(), which can return the error it gets from
> > > 	  svc_recvfrom(), which can return the error from
> > > 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> > > 
> > > Since __sock_recvmsg() has a security hook, it looks like we can end up
> > > with an -EACCES from selinux?
> > > 
> > > So one case would be selinux deciding we weren't allowed to receive
> > > packets from this socket.  Huh.
> > 
> > I got lost there too, but I would suspect that there are other errors
> > that can bubble up from the lower networking layers as well. Even if
> > there aren't currently, it's probably still prudent to assume that it's
> > a possibility and code for it.
> > 
> > I tend to think the safest thing is probably to do a long sleep (1s or
> > so and retry when we get an error (maybe also a ratelimited printk?).
> 
> Yeah, I guess I can't think of anything better.
> 

Ok, I went ahead and did patches for this and gave them a quick test
this morning. Obviously, these are hard to fully unit test since this
seems to be a very uncommon occurrence.

Any thoughts?
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[J. Bruce Fields]

On Tue, Apr 08, 2008 at 09:21:02AM -0400, Jeff Layton wrote:
> On Mon, 7 Apr 2008 16:50:27 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
> > > On Mon, 7 Apr 2008 13:56:15 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > > > > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > > > > The global task and serv pointers for lockd are normally protected by
> > > > > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > > > > this occurs, these variables are cleared without any locking.
> > > > > 
> > > > > Shouldn't we get rid of the case where it exits abnormally instead?
> > > > 
> > > > I tried to figure out when this could actually occur (when can
> > > > svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> > > > in sock_recvmsg():
> > > > 
> > > > 	- svc_recv() itself returns only -EAGAIN or the return from
> > > > 	  ->xpo_recvfrom().
> > > > 	- the only xpo_recvfrom() that's interesting is
> > > > 	  svc_tcp_recvfrom(), which can return the error it gets from
> > > > 	  svc_recvfrom(), which can return the error from
> > > > 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> > > > 
> > > > Since __sock_recvmsg() has a security hook, it looks like we can end up
> > > > with an -EACCES from selinux?
> > > > 
> > > > So one case would be selinux deciding we weren't allowed to receive
> > > > packets from this socket.  Huh.
> > > 
> > > I got lost there too, but I would suspect that there are other errors
> > > that can bubble up from the lower networking layers as well. Even if
> > > there aren't currently, it's probably still prudent to assume that it's
> > > a possibility and code for it.
> > > 
> > > I tend to think the safest thing is probably to do a long sleep (1s or
> > > so and retry when we get an error (maybe also a ratelimited printk?).
> > 
> > Yeah, I guess I can't think of anything better.
> > 
> 
> Ok, I went ahead and did patches for this and gave them a quick test
> this morning. Obviously, these are hard to fully unit test since this
> seems to be a very uncommon occurrence.

I suppose this could probably be reproduced with some selinux magic.

> Any thoughts?

If anyone does ever hit this and it doesn't go away, the printk (even
with the ratelimiting) could be pretty annoying, so it might be worth
arranging to print this just once.  But perhaps we can wait and see if
that actually happens.

Given what appears to be a very unusual crash, and what I'm assuming is
an impending release, I suppose we should wait for the merge window (but
possibly also submit to 2.6.25.x).

--b.

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

On Tue, 8 Apr 2008 12:28:21 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Apr 08, 2008 at 09:21:02AM -0400, Jeff Layton wrote:
> > On Mon, 7 Apr 2008 16:50:27 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
> > > > On Mon, 7 Apr 2008 13:56:15 -0400
> > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > 
> > > > > On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > > > > > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > > > > > The global task and serv pointers for lockd are normally protected by
> > > > > > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > > > > > this occurs, these variables are cleared without any locking.
> > > > > > 
> > > > > > Shouldn't we get rid of the case where it exits abnormally instead?
> > > > > 
> > > > > I tried to figure out when this could actually occur (when can
> > > > > svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> > > > > in sock_recvmsg():
> > > > > 
> > > > > 	- svc_recv() itself returns only -EAGAIN or the return from
> > > > > 	  ->xpo_recvfrom().
> > > > > 	- the only xpo_recvfrom() that's interesting is
> > > > > 	  svc_tcp_recvfrom(), which can return the error it gets from
> > > > > 	  svc_recvfrom(), which can return the error from
> > > > > 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> > > > > 
> > > > > Since __sock_recvmsg() has a security hook, it looks like we can end up
> > > > > with an -EACCES from selinux?
> > > > > 
> > > > > So one case would be selinux deciding we weren't allowed to receive
> > > > > packets from this socket.  Huh.
> > > > 
> > > > I got lost there too, but I would suspect that there are other errors
> > > > that can bubble up from the lower networking layers as well. Even if
> > > > there aren't currently, it's probably still prudent to assume that it's
> > > > a possibility and code for it.
> > > > 
> > > > I tend to think the safest thing is probably to do a long sleep (1s or
> > > > so and retry when we get an error (maybe also a ratelimited printk?).
> > > 
> > > Yeah, I guess I can't think of anything better.
> > > 
> > 
> > Ok, I went ahead and did patches for this and gave them a quick test
> > this morning. Obviously, these are hard to fully unit test since this
> > seems to be a very uncommon occurrence.
> 
> I suppose this could probably be reproduced with some selinux magic.
> 

Good idea. I'll chat with our SELinux guys and see if they can point me
in the right direction on this.

> > Any thoughts?
> 
> If anyone does ever hit this and it doesn't go away, the printk (even
> with the ratelimiting) could be pretty annoying, so it might be worth
> arranging to print this just once.  But perhaps we can wait and see if
> that actually happens.
> 

I tend to think that this is most likely to happen when there's a
significant problem in lower level networking (or maybe a VM problem?).
SELinux is also a possibility, as you pointed out, but I'm not sure how
much kernel threads are affected by SELinux...

In any case, if this happens, then we're probably going to have bigger
problems than a printk every second. My suggestion would be that we keep
it like this for now, and reconsider it if it turns out to be a
problem...

> Given what appears to be a very unusual crash, and what I'm assuming is
> an impending release, I suppose we should wait for the merge window (but
> possibly also submit to 2.6.25.x).
> 

Yes. I was thinking that all of this should probably be 2.6.26
material (along with all of the kthread conversion patches). Aside from
this possibly happening in Michael's crash, I've never seen lockd exit
abnormally like this.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

On Tue, 8 Apr 2008 12:28:21 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Apr 08, 2008 at 09:21:02AM -0400, Jeff Layton wrote:
> > On Mon, 7 Apr 2008 16:50:27 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
> > > > On Mon, 7 Apr 2008 13:56:15 -0400
> > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > 
> > > > > On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> > > > > > On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> > > > > > > The global task and serv pointers for lockd are normally protected by
> > > > > > > the nlmsvc_mutex. The exception is when the lockd exits abnormally. When
> > > > > > > this occurs, these variables are cleared without any locking.
> > > > > > 
> > > > > > Shouldn't we get rid of the case where it exits abnormally instead?
> > > > > 
> > > > > I tried to figure out when this could actually occur (when can
> > > > > svc_recv() return an error other than -EINTR or -EAGAIN?), and got lost
> > > > > in sock_recvmsg():
> > > > > 
> > > > > 	- svc_recv() itself returns only -EAGAIN or the return from
> > > > > 	  ->xpo_recvfrom().
> > > > > 	- the only xpo_recvfrom() that's interesting is
> > > > > 	  svc_tcp_recvfrom(), which can return the error it gets from
> > > > > 	  svc_recvfrom(), which can return the error from
> > > > > 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> > > > > 
> > > > > Since __sock_recvmsg() has a security hook, it looks like we can end up
> > > > > with an -EACCES from selinux?
> > > > > 
> > > > > So one case would be selinux deciding we weren't allowed to receive
> > > > > packets from this socket.  Huh.
> > > > 
> > > > I got lost there too, but I would suspect that there are other errors
> > > > that can bubble up from the lower networking layers as well. Even if
> > > > there aren't currently, it's probably still prudent to assume that it's
> > > > a possibility and code for it.
> > > > 
> > > > I tend to think the safest thing is probably to do a long sleep (1s or
> > > > so and retry when we get an error (maybe also a ratelimited printk?).
> > > 
> > > Yeah, I guess I can't think of anything better.
> > > 
> > 
> > Ok, I went ahead and did patches for this and gave them a quick test
> > this morning. Obviously, these are hard to fully unit test since this
> > seems to be a very uncommon occurrence.
> 
> I suppose this could probably be reproduced with some selinux magic.
> 

This turns out to be rather difficult. SELinux apparently doesn't have
much support for restricting kernel threads. I ended up hacking
together the following fault-injection patch to unit test this:

--------[snip]--------
diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
index 10709cb..3e86cba 100644
--- a/include/linux/sunrpc/debug.h
+++ b/include/linux/sunrpc/debug.h
@@ -24,6 +24,7 @@
 #define RPCDBG_SVCDSP		0x0200
 #define RPCDBG_MISC		0x0400
 #define RPCDBG_CACHE		0x0800
+#define RPCDBG_BREAKME		0x1000
 #define RPCDBG_ALL		0x7fff
 
 #ifdef __KERNEL__
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index d8e8d79..0333c64 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -569,6 +569,9 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 	struct xdr_buf		*arg;
 	DECLARE_WAITQUEUE(wait, current);
 
+	if (rpc_debug & RPCDBG_BREAKME)
+		return -EACCES;
+
 	dprintk("svc: server %p waiting for data (to = %ld)\n",
 		rqstp, timeout);
--------[snip]--------


...with that, I can see the new code working as expected, but I think
you have a point that those printk's could get to be rather annoying.
I've got a new set of patches that I'll send out that has it only print
the warning on the first unexpected error, or if the error changes.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Chuck Lever]

On Apr 8, 2008, at 12:28 PM, J. Bruce Fields wrote:
> On Tue, Apr 08, 2008 at 09:21:02AM -0400, Jeff Layton wrote:
>> On Mon, 7 Apr 2008 16:50:27 -0400
>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>
>>> On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
>>>> On Mon, 7 Apr 2008 13:56:15 -0400
>>>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>>>
>>>>> On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
>>>>>> On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
>>>>>>> The global task and serv pointers for lockd are normally  
>>>>>>> protected by
>>>>>>> the nlmsvc_mutex. The exception is when the lockd exits  
>>>>>>> abnormally. When
>>>>>>> this occurs, these variables are cleared without any locking.
>>>>>>
>>>>>> Shouldn't we get rid of the case where it exits abnormally  
>>>>>> instead?
>>>>>
>>>>> I tried to figure out when this could actually occur (when can
>>>>> svc_recv() return an error other than -EINTR or -EAGAIN?), and  
>>>>> got lost
>>>>> in sock_recvmsg():
>>>>>
>>>>> 	- svc_recv() itself returns only -EAGAIN or the return from
>>>>> 	  ->xpo_recvfrom().
>>>>> 	- the only xpo_recvfrom() that's interesting is
>>>>> 	  svc_tcp_recvfrom(), which can return the error it gets from
>>>>> 	  svc_recvfrom(), which can return the error from
>>>>> 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
>>>>>
>>>>> Since __sock_recvmsg() has a security hook, it looks like we  
>>>>> can end up
>>>>> with an -EACCES from selinux?
>>>>>
>>>>> So one case would be selinux deciding we weren't allowed to  
>>>>> receive
>>>>> packets from this socket.  Huh.
>>>>
>>>> I got lost there too, but I would suspect that there are other  
>>>> errors
>>>> that can bubble up from the lower networking layers as well.  
>>>> Even if
>>>> there aren't currently, it's probably still prudent to assume  
>>>> that it's
>>>> a possibility and code for it.
>>>>
>>>> I tend to think the safest thing is probably to do a long sleep  
>>>> (1s or
>>>> so and retry when we get an error (maybe also a ratelimited  
>>>> printk?).
>>>
>>> Yeah, I guess I can't think of anything better.
>>>
>>
>> Ok, I went ahead and did patches for this and gave them a quick test
>> this morning. Obviously, these are hard to fully unit test since this
>> seems to be a very uncommon occurrence.
>
> I suppose this could probably be reproduced with some selinux magic.
>
>> Any thoughts?
>
> If anyone does ever hit this and it doesn't go away, the printk (even
> with the ratelimiting) could be pretty annoying, so it might be worth
> arranging to print this just once.  But perhaps we can wait and see if
> that actually happens.


Coding by contract would be useful here.

If svc_recv() returns only specific error codes (and never anything  
else) then its callers don't need any special logic for  
"unrecognized" return codes.

Thus, ensuring that svc_recv() returns only EINTR, EACCES, or EAGAIN  
would limit the complexity and failure modes of its callers.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] NLM: hold BKL when clearing global lockd task and serv vars

[Jeff Layton]

On Tue, 8 Apr 2008 16:08:40 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On Apr 8, 2008, at 12:28 PM, J. Bruce Fields wrote:
> > On Tue, Apr 08, 2008 at 09:21:02AM -0400, Jeff Layton wrote:
> >> On Mon, 7 Apr 2008 16:50:27 -0400
> >> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >>
> >>> On Mon, Apr 07, 2008 at 04:22:41PM -0400, Jeff Layton wrote:
> >>>> On Mon, 7 Apr 2008 13:56:15 -0400
> >>>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >>>>
> >>>>> On Mon, Apr 07, 2008 at 12:45:01PM -0400, Christoph Hellwig wrote:
> >>>>>> On Mon, Apr 07, 2008 at 09:38:34AM -0400, Jeff Layton wrote:
> >>>>>>> The global task and serv pointers for lockd are normally  
> >>>>>>> protected by
> >>>>>>> the nlmsvc_mutex. The exception is when the lockd exits  
> >>>>>>> abnormally. When
> >>>>>>> this occurs, these variables are cleared without any locking.
> >>>>>>
> >>>>>> Shouldn't we get rid of the case where it exits abnormally  
> >>>>>> instead?
> >>>>>
> >>>>> I tried to figure out when this could actually occur (when can
> >>>>> svc_recv() return an error other than -EINTR or -EAGAIN?), and  
> >>>>> got lost
> >>>>> in sock_recvmsg():
> >>>>>
> >>>>> 	- svc_recv() itself returns only -EAGAIN or the return from
> >>>>> 	  ->xpo_recvfrom().
> >>>>> 	- the only xpo_recvfrom() that's interesting is
> >>>>> 	  svc_tcp_recvfrom(), which can return the error it gets from
> >>>>> 	  svc_recvfrom(), which can return the error from
> >>>>> 	  kernel_recvmsg(), which gets its return from sock_recvmsg().
> >>>>>
> >>>>> Since __sock_recvmsg() has a security hook, it looks like we  
> >>>>> can end up
> >>>>> with an -EACCES from selinux?
> >>>>>
> >>>>> So one case would be selinux deciding we weren't allowed to  
> >>>>> receive
> >>>>> packets from this socket.  Huh.
> >>>>
> >>>> I got lost there too, but I would suspect that there are other  
> >>>> errors
> >>>> that can bubble up from the lower networking layers as well.  
> >>>> Even if
> >>>> there aren't currently, it's probably still prudent to assume  
> >>>> that it's
> >>>> a possibility and code for it.
> >>>>
> >>>> I tend to think the safest thing is probably to do a long sleep  
> >>>> (1s or
> >>>> so and retry when we get an error (maybe also a ratelimited  
> >>>> printk?).
> >>>
> >>> Yeah, I guess I can't think of anything better.
> >>>
> >>
> >> Ok, I went ahead and did patches for this and gave them a quick test
> >> this morning. Obviously, these are hard to fully unit test since this
> >> seems to be a very uncommon occurrence.
> >
> > I suppose this could probably be reproduced with some selinux magic.
> >
> >> Any thoughts?
> >
> > If anyone does ever hit this and it doesn't go away, the printk (even
> > with the ratelimiting) could be pretty annoying, so it might be worth
> > arranging to print this just once.  But perhaps we can wait and see if
> > that actually happens.
> 
> 
> Coding by contract would be useful here.
> 
> If svc_recv() returns only specific error codes (and never anything  
> else) then its callers don't need any special logic for  
> "unrecognized" return codes.
> 
> Thus, ensuring that svc_recv() returns only EINTR, EACCES, or EAGAIN  
> would limit the complexity and failure modes of its callers.
> 

I don't think that gains us much. The errors of interest are basically
EINTR, EAGAIN and "everything else". If we have to deal with EACCES
anyway, then it's not really too much more complexity to just deal with
"everything else" at this level.

-- 
Jeff Layton <jlayton@redhat.com>