Re: [PATCH] svcrpc: modifying positive sunrpc cache entries is racy

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Dec 30, 2010 at 12:19:40PM +1100, Neil Brown wrote:
> On Wed, 29 Dec 2010 15:59:42 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> wrote:
> 
> > On Wed, Dec 29, 2010 at 03:47:52PM -0500, bfields wrote:
> > > From: J. Bruce Fields <bfields@redhat.com>
> > > 
> > > Once a sunrpc cache entry is non-NEGATIVE, we should be replacing it
> > > (and allowing any concurrent users to destroy it on last put) instead of
> > > trying to update it in place.
> > > 
> > > Otherwise someone referencing the ip_map we're modifying here could try
> > > to use the m_client just as we're putting the last reference.
> > > 
> > > The bug should only be seen by users of the legacy nfsd interfaces.
> > > 
> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > ---
> > >  net/sunrpc/svcauth_unix.c |   18 ++++++++++++++++--
> > >  1 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > Intended to apply for 2.6.38 if this looks right....
> > 
> > Also noticed while trying to track down an rhel5 oops in
> > svcauth_unix_set_client():
> > 
> > 	- cache_check() can set an entry negative in place, which if
> > 	  nothing else must cause a leak in some cases.  (Because when
> > 	  the entry is eventually destroyed, it will be assumed to not
> > 	  have any contents.)  I suppose the fix is again to try to
> > 	  adding a new negative entry instead.
> 
> cache_check should only set an entry 'negative' if it is not already valid
> (rv == -EAGAIN) and there is no up-call pending.

I don't think anything keeps VALID from being set after the
cache_is_valid check but before the code that does the
set_bit(CACHE_NEGATIVE).

> Maybe we should check CACHE_VALID again after the test_and_set of
> CACHE_PENDING, but is a very unlikely race (if it is actually a race at all)
> 
> > 
> > 	- since cache_check() doesn't use any locking, I can't see what
> > 	  guarantees that when it sees the CACHE_VALID bit set and
> > 	  CACHE_NEGATIVE cleared, it must necessarily see the new
> > 	  contents.   I think that'd be fixed by a wmb() before setting
> > 	  those bits and a rmb() after checking them.  I don't know if
> > 	  it's actually possible to hit that bug....
> 
> Yes, we probably want a set_bit_lock in cache_fresh_locked() though I don't
> think that exists, so we could use test_and_set_bit_locked() instead.
> 
> But it does feel like maybe we should add some locking to cache_check.
> Take the lock at the the start, and release it after the
> test_and_set_bit(CACHE_PENDING) or once we have decided not to do that ???

Maybe so.

--b.

> 
> I think when I wrote this I might have thought that bit ops implied memory
> ordering ... or maybe I just didn't think through the issues properly at all.
> 
> Thanks,
> NeilBrown
> 
> 
> > 
> > --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
>