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@xxxxxxxxxxxx> > wrote: > > > On Wed, Dec 29, 2010 at 03:47:52PM -0500, bfields wrote: > > > From: J. Bruce Fields <bfields@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
