netdev - Re: sunrpc: what prevents an xprt from being freed before task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100803191833.GB31579@fieldses.org>
Date:	Tue, 3 Aug 2010 15:18:33 -0400
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Trond Myklebust <trond.myklebust@....uio.no>
Cc:	Jeff Layton <jlayton@...hat.com>, linux-nfs@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: sunrpc: what prevents an xprt from being freed before
 task_cleanup runs?

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html