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

[Neil Brown <neilb@suse.de>]

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.

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 ???

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