Message ID | 20100803132453.4fa18444@tlielax.poochiereds.net |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 03 Aug 2010 14:40:15 -0400 Trond Myklebust <trond.myklebust@fys.uio.no> 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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */