We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
The problem appears to be that a task_cleanup workqueue job ran and got
passed a pointer to an xprt that had been freed. The bug is here in
case anyone is interested in the details:
https://bugzilla.redhat.com/show_bug.cgi?id=611938
The situation seems to be pretty difficult to reproduce, but I don't
see anything that's intended to ensure that this doesn't occur in RHEL5
or mainline. The task_cleanup workqueue job doesn't hold a reference to
the xprt, and the job isn't canceled when the xprt is torn down.
Bruce had a look and suggested that we may need something like the
patch below (pasted in, so it probably won't apply correctly). I've
tested a backported version of it on RHEL5 and it seems to work fine.
Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
am I missing something that should prevent this situation in mainline
(and perhaps isn't in RHEL5's kernel).
Any help is appreciated...
-----------------------------[snip]---------------------------------
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index dcd0132..2a1f664 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
rpc_destroy_wait_queue(&xprt->sending);
rpc_destroy_wait_queue(&xprt->resend);
rpc_destroy_wait_queue(&xprt->backlog);
+ cancel_work_sync(&xprt->task_cleanup);
/*
* Tear down transport state and free the rpc_xprt
*/
-----------------------------[snip]---------------------------------
Thanks,
--
Jeff Layton <[email protected]>
From: J. Bruce Fields <[email protected]>
We saw a report of a NULL dereference in xprt_autoclose:
https://bugzilla.redhat.com/show_bug.cgi?id=611938
This appears to be the result of an xprt's task_cleanup running after
the xprt is destroyed. Nothing in the current code appears to prevent
that.
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/xprt.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
On Tue, Aug 03, 2010 at 02:49:23PM -0400, Trond Myklebust wrote:
> Hmm... On the other hand, since xprt_destroy() may get called from
> inside the rpciod_workqueue, the wait_on_bit_lock() may end up
> deadlocking.
>
> Nevermind, then. Let's go with the cancel_work_sync().
Here you are--only if you don't have something equivalent already queued
up.--b.
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index dcd0132..2a1f664 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
rpc_destroy_wait_queue(&xprt->sending);
rpc_destroy_wait_queue(&xprt->resend);
rpc_destroy_wait_queue(&xprt->backlog);
+ cancel_work_sync(&xprt->task_cleanup);
/*
* Tear down transport state and free the rpc_xprt
*/
--
1.7.0.4
On Tue, 2010-08-03 at 14:48 -0400, J. Bruce Fields wrote:
> On Tue, Aug 03, 2010 at 02:40:15PM -0400, Trond Myklebust wrote:
> > On Tue, 2010-08-03 at 13:24 -0400, Jeff Layton wrote:
> > > We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
> > > The problem appears to be that a task_cleanup workqueue job ran and got
> > > passed a pointer to an xprt that had been freed. The bug is here in
> > > case anyone is interested in the details:
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=611938
> > >
> > > The situation seems to be pretty difficult to reproduce, but I don't
> > > see anything that's intended to ensure that this doesn't occur in RHEL5
> > > or mainline. The task_cleanup workqueue job doesn't hold a reference to
> > > the xprt, and the job isn't canceled when the xprt is torn down.
> > >
> > > Bruce had a look and suggested that we may need something like the
> > > patch below (pasted in, so it probably won't apply correctly). I've
> > > tested a backported version of it on RHEL5 and it seems to work fine.
> > >
> > > Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
> > > am I missing something that should prevent this situation in mainline
> > > (and perhaps isn't in RHEL5's kernel).
> > >
> > > Any help is appreciated...
> > >
> > > -----------------------------[snip]---------------------------------
> > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > > index dcd0132..2a1f664 100644
> > > --- a/net/sunrpc/xprt.c
> > > +++ b/net/sunrpc/xprt.c
> > > @@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
> > > rpc_destroy_wait_queue(&xprt->sending);
> > > rpc_destroy_wait_queue(&xprt->resend);
> > > rpc_destroy_wait_queue(&xprt->backlog);
> > > + cancel_work_sync(&xprt->task_cleanup);
> > > /*
> > > * Tear down transport state and free the rpc_xprt
> > > */
> > > -----------------------------[snip]---------------------------------
> > >
> > > Thanks,
> >
> > How about doing a wait_on_bit_lock(&xprt->state, XPRT_LOCKED,...)
> > instead?
>
> I'll believe you, but I don't get it: what guarantees that XPRT_LOCKED
> isn't cleared while xprt_autoclose autoclose is still running?
Look more closely: the callers that enqueue task_cleanup on the
workqueue all ensure that XPRT_LOCKED is held, and it isn't released
until the call to xprt_release_write() at the very end of
xprt_autoclose().
The rpciod_workqueue deadlocking is an issue, though, so unfortunately,
we won't be able to make use of the above.
On Tue, Aug 03, 2010 at 02:40:15PM -0400, Trond Myklebust wrote:
> On Tue, 2010-08-03 at 13:24 -0400, Jeff Layton wrote:
> > We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
> > The problem appears to be that a task_cleanup workqueue job ran and got
> > passed a pointer to an xprt that had been freed. The bug is here in
> > case anyone is interested in the details:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=611938
> >
> > The situation seems to be pretty difficult to reproduce, but I don't
> > see anything that's intended to ensure that this doesn't occur in RHEL5
> > or mainline. The task_cleanup workqueue job doesn't hold a reference to
> > the xprt, and the job isn't canceled when the xprt is torn down.
> >
> > Bruce had a look and suggested that we may need something like the
> > patch below (pasted in, so it probably won't apply correctly). I've
> > tested a backported version of it on RHEL5 and it seems to work fine.
> >
> > Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
> > am I missing something that should prevent this situation in mainline
> > (and perhaps isn't in RHEL5's kernel).
> >
> > Any help is appreciated...
> >
> > -----------------------------[snip]---------------------------------
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index dcd0132..2a1f664 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
> > rpc_destroy_wait_queue(&xprt->sending);
> > rpc_destroy_wait_queue(&xprt->resend);
> > rpc_destroy_wait_queue(&xprt->backlog);
> > + cancel_work_sync(&xprt->task_cleanup);
> > /*
> > * Tear down transport state and free the rpc_xprt
> > */
> > -----------------------------[snip]---------------------------------
> >
> > Thanks,
>
> How about doing a wait_on_bit_lock(&xprt->state, XPRT_LOCKED,...)
> instead?
I'll believe you, but I don't get it: what guarantees that XPRT_LOCKED
isn't cleared while xprt_autoclose autoclose is still running?
--b.
On Tue, 2010-08-03 at 14:40 -0400, Trond Myklebust wrote:
> On Tue, 2010-08-03 at 13:24 -0400, Jeff Layton wrote:
> > We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
> > The problem appears to be that a task_cleanup workqueue job ran and got
> > passed a pointer to an xprt that had been freed. The bug is here in
> > case anyone is interested in the details:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=611938
> >
> > The situation seems to be pretty difficult to reproduce, but I don't
> > see anything that's intended to ensure that this doesn't occur in RHEL5
> > or mainline. The task_cleanup workqueue job doesn't hold a reference to
> > the xprt, and the job isn't canceled when the xprt is torn down.
> >
> > Bruce had a look and suggested that we may need something like the
> > patch below (pasted in, so it probably won't apply correctly). I've
> > tested a backported version of it on RHEL5 and it seems to work fine.
> >
> > Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
> > am I missing something that should prevent this situation in mainline
> > (and perhaps isn't in RHEL5's kernel).
> >
> > Any help is appreciated...
> >
> > -----------------------------[snip]---------------------------------
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index dcd0132..2a1f664 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
> > rpc_destroy_wait_queue(&xprt->sending);
> > rpc_destroy_wait_queue(&xprt->resend);
> > rpc_destroy_wait_queue(&xprt->backlog);
> > + cancel_work_sync(&xprt->task_cleanup);
> > /*
> > * Tear down transport state and free the rpc_xprt
> > */
> > -----------------------------[snip]---------------------------------
> >
> > Thanks,
>
> How about doing a wait_on_bit_lock(&xprt->state, XPRT_LOCKED,...)
> instead?
Hmm... On the other hand, since xprt_destroy() may get called from
inside the rpciod_workqueue, the wait_on_bit_lock() may end up
deadlocking.
Nevermind, then. Let's go with the cancel_work_sync().
On Tue, 2010-08-03 at 13:24 -0400, Jeff Layton wrote:
> We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
> The problem appears to be that a task_cleanup workqueue job ran and got
> passed a pointer to an xprt that had been freed. The bug is here in
> case anyone is interested in the details:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=611938
>
> The situation seems to be pretty difficult to reproduce, but I don't
> see anything that's intended to ensure that this doesn't occur in RHEL5
> or mainline. The task_cleanup workqueue job doesn't hold a reference to
> the xprt, and the job isn't canceled when the xprt is torn down.
>
> Bruce had a look and suggested that we may need something like the
> patch below (pasted in, so it probably won't apply correctly). I've
> tested a backported version of it on RHEL5 and it seems to work fine.
>
> Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
> am I missing something that should prevent this situation in mainline
> (and perhaps isn't in RHEL5's kernel).
>
> Any help is appreciated...
>
> -----------------------------[snip]---------------------------------
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index dcd0132..2a1f664 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
> rpc_destroy_wait_queue(&xprt->sending);
> rpc_destroy_wait_queue(&xprt->resend);
> rpc_destroy_wait_queue(&xprt->backlog);
> + cancel_work_sync(&xprt->task_cleanup);
> /*
> * Tear down transport state and free the rpc_xprt
> */
> -----------------------------[snip]---------------------------------
>
> Thanks,
How about doing a wait_on_bit_lock(&xprt->state, XPRT_LOCKED,...)
instead?
Cheers
Trond
On Tue, 03 Aug 2010 14:40:15 -0400
Trond Myklebust <[email protected]> wrote:
> On Tue, 2010-08-03 at 13:24 -0400, Jeff Layton wrote:
> > We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
> > The problem appears to be that a task_cleanup workqueue job ran and got
> > passed a pointer to an xprt that had been freed. The bug is here in
> > case anyone is interested in the details:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=611938
> >
> > The situation seems to be pretty difficult to reproduce, but I don't
> > see anything that's intended to ensure that this doesn't occur in RHEL5
> > or mainline. The task_cleanup workqueue job doesn't hold a reference to
> > the xprt, and the job isn't canceled when the xprt is torn down.
> >
> > Bruce had a look and suggested that we may need something like the
> > patch below (pasted in, so it probably won't apply correctly). I've
> > tested a backported version of it on RHEL5 and it seems to work fine.
> >
> > Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
> > am I missing something that should prevent this situation in mainline
> > (and perhaps isn't in RHEL5's kernel).
> >
> > Any help is appreciated...
> >
> > -----------------------------[snip]---------------------------------
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index dcd0132..2a1f664 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
> > rpc_destroy_wait_queue(&xprt->sending);
> > rpc_destroy_wait_queue(&xprt->resend);
> > rpc_destroy_wait_queue(&xprt->backlog);
> > + cancel_work_sync(&xprt->task_cleanup);
> > /*
> > * Tear down transport state and free the rpc_xprt
> > */
> > -----------------------------[snip]---------------------------------
> >
> > Thanks,
>
> How about doing a wait_on_bit_lock(&xprt->state, XPRT_LOCKED,...)
> instead?
>
I haven't quite grokked what XPRT_LOCKED is supposed to signify. What
would be the advantage of doing that over just killing off the
task_cleanup job? It seems like if we're tearing down the xprt then
waiting for task_cleanup to run is sort of pointless -- xs_destroy will
take care of closing the socket anyway.
Also, if we do go that route, do we need a wake_up_bit call in
xprt_clear_locked?
Thanks,
--
Jeff Layton <[email protected]>
On Tue, Aug 03, 2010 at 02:56:15PM -0400, Trond Myklebust wrote:
> On Tue, 2010-08-03 at 14:48 -0400, J. Bruce Fields wrote:
> > On Tue, Aug 03, 2010 at 02:40:15PM -0400, Trond Myklebust wrote:
> > > On Tue, 2010-08-03 at 13:24 -0400, Jeff Layton wrote:
> > > > We got a report recently about a panic in RHEL5 (2.6.18 based kernel).
> > > > The problem appears to be that a task_cleanup workqueue job ran and got
> > > > passed a pointer to an xprt that had been freed. The bug is here in
> > > > case anyone is interested in the details:
> > > >
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=611938
> > > >
> > > > The situation seems to be pretty difficult to reproduce, but I don't
> > > > see anything that's intended to ensure that this doesn't occur in RHEL5
> > > > or mainline. The task_cleanup workqueue job doesn't hold a reference to
> > > > the xprt, and the job isn't canceled when the xprt is torn down.
> > > >
> > > > Bruce had a look and suggested that we may need something like the
> > > > patch below (pasted in, so it probably won't apply correctly). I've
> > > > tested a backported version of it on RHEL5 and it seems to work fine.
> > > >
> > > > Is it reasonable to cancel task_cleanup when destroying the xprt? Or,
> > > > am I missing something that should prevent this situation in mainline
> > > > (and perhaps isn't in RHEL5's kernel).
> > > >
> > > > Any help is appreciated...
> > > >
> > > > -----------------------------[snip]---------------------------------
> > > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > > > index dcd0132..2a1f664 100644
> > > > --- a/net/sunrpc/xprt.c
> > > > +++ b/net/sunrpc/xprt.c
> > > > @@ -1129,6 +1129,7 @@ static void xprt_destroy(struct kref *kref)
> > > > rpc_destroy_wait_queue(&xprt->sending);
> > > > rpc_destroy_wait_queue(&xprt->resend);
> > > > rpc_destroy_wait_queue(&xprt->backlog);
> > > > + cancel_work_sync(&xprt->task_cleanup);
> > > > /*
> > > > * Tear down transport state and free the rpc_xprt
> > > > */
> > > > -----------------------------[snip]---------------------------------
> > > >
> > > > Thanks,
> > >
> > > How about doing a wait_on_bit_lock(&xprt->state, XPRT_LOCKED,...)
> > > instead?
> >
> > I'll believe you, but I don't get it: what guarantees that XPRT_LOCKED
> > isn't cleared while xprt_autoclose autoclose is still running?
>
> Look more closely: the callers that enqueue task_cleanup on the
> workqueue all ensure that XPRT_LOCKED is held, and it isn't released
> until the call to xprt_release_write() at the very end of
> xprt_autoclose().
I see that (assuming snd_task is always NULL--but, OK, I think I see why
that's true), but we still dereference the xprt after returning from
xprt_release_write(). Only to drop the transport_lock, but I don't see
why there isn't a race.
> The rpciod_workqueue deadlocking is an issue, though, so unfortunately,
> we won't be able to make use of the above.
Yeah, OK, it was just bugging me that I didn't feel like I understood
how XPRT_LOCKED is used.
--b.
On Tue, 2010-08-03 at 17:22 -0400, J. Bruce Fields wrote:
> From: J. Bruce Fields <[email protected]>
>
> We saw a report of a NULL dereference in xprt_autoclose:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=611938
>
> This appears to be the result of an xprt's task_cleanup running after
> the xprt is destroyed. Nothing in the current code appears to prevent
> that.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
Thanks. Applied to nfs-for-2.6.36...
Trond