On Tue, Jan 04, 2011 at 03:51:07PM +1100, NeilBrown wrote:
> On Mon, 3 Jan 2011 22:08:05 -0500 "J. Bruce Fields" <bfields@xxxxxxxxxxxx>
> wrote:
>
> > On Mon, Jan 03, 2011 at 05:26:05PM -0500, J. Bruce Fields 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.
> > >
> > > Or the following seems simpler.
> > >
> > > (And I was thinking it was necessary to ensure that the right thing
> > > happened to the cached xprt->xpt_auth_cache entry--though on a second
> > > look I see that sunrpc_cache_update also expires the replaced entry
> > > immediately. Still, this seems simpler if it also works.)
> >
> > Eh, on third thoughts: we probably do want a real negative entry created
> > in the cache, so I think the original patch was correct!
>
> I like your second thought better than your third.
> I don't see any reason to push this item out of the cache extra quickly.
> In fact I think it would still be correct to just remove those two lines and
> not set expiry_time to 0. auth_unix_lookup will never find that IP address,
> and so it doesn't matter if it remains in the cache or not.
> I guess there could result in the cache appearing to contain different data
> depending on whether you look at it the 'old' way or the 'new' way, but I
> don't that is a real problem, and setting expiry_time to 0 overcomes that.
>
> What was the substance of your third thought?
I had some idea that we could end up with a cache entry stuck in the
cache forever.
OK, actually I think late last night I reverted into some sort of "maybe
if I type the first thing that comes to my mind, somebody else will
think this through for me" state. Apologies. Um, did I win?
> BTW, you could use sunrpc_invalidate rather than just setting expiry_time to
> zero, which would hurry it out of the cache a bit faster.
Yep, I like that, and I'd forgotten about sunrpc_invalidate, thanks!
Done.... (As below).
> And this all made me realise that there is more code that can be placed under
> CONFIG_NFSD_DEPRECTATED.
Yep, applied.
(When do we get to remove all this? Taking a stab at the 2.6.40 merge
window.... OK, party at my place in May!)
--b.
commit ab5c05c579b0b37b9bf2c79c9c8f0ef6045ee41d
Author: J. Bruce Fields <bfields@xxxxxxxxxx>
Date: Fri Dec 24 14:03:38 2010 -0500
svcrpc: modifying valid sunrpc cache entries is racy
Once a sunrpc cache entry is VALID, 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>
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index a04ac91..59a7c52 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -401,8 +401,7 @@ struct auth_domain *auth_unix_lookup(struct net *net, struct in6_addr *addr)
return NULL;
if ((ipm->m_client->addr_changes - ipm->m_add_change) >0) {
- if (test_and_set_bit(CACHE_NEGATIVE, &ipm->h.flags) == 0)
- auth_domain_put(&ipm->m_client->h);
+ sunrpc_invalidate(&ipm->h, sn->ip_map_cache);
rv = NULL;
} else {
rv = &ipm->m_client->h;
--
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
