Re: Another possible race in put_rpccred / rpcauth_unhash_cred

[Trond Myklebust <trond.myklebust@fys.uio.no>]

On Wed, 2009-11-18 at 11:56 +1100, Neil Brown wrote: 
> Hi again Trond,
> 
>  I've been looking at a customer report of an oops in
>  rpcauth_lookup_credcache.
>    https://bugzilla.novell.com/show_bug.cgi?id=547137
>  
>  It appears that one of the hash chains in the cache has become
>  corrupted.
> 
>  In trying to guess how this might happen, the one possibility I have
>  found is that it might be possible for hlist_del_rcu to be called on
>  an rpc_cred that is not hashed.  This certainly could cause list
>  corruption.
> 
>  put_rpccred checks if the cred is hashed (RPCAUTH_CRED_HASHED) and if
>  not, it takes a lock and then - if the cred it not uptodate -
>  calls rpcauth_unhash_cred which will call hlist_del_rcu.
>  If some other thread unhashed the cred while this thread was waiting
>  for rpc_credcache_lock, we could get list corruption.
> 
>  The kernel in question did not have commit 5f707eb429 which claims to
>  fix a race in much the same place, however I don't think that patch
>  would fix this problem.
>  
>  I think the simplest fix would be to use hlist_del_init_rcu in place
>  of hlist_del_rcu as shown below.  That version protects again
>  deleting the same element multiple times.
> 
>  Do you agree with this fix?
> 
> Thanks,
> NeilBrown
> 
> 
> 
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 54a4e04..323eff6 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -118,7 +118,7 @@ static DEFINE_SPINLOCK(rpc_credcache_lock);
>  static void
>  rpcauth_unhash_cred_locked(struct rpc_cred *cred)
>  {
> -	hlist_del_rcu(&cred->cr_hash);
> +	hlist_del_init_rcu(&cred->cr_hash);
>  	smp_mb__before_clear_bit();
>  	clear_bit(RPCAUTH_CRED_HASHED, &cred->cr_flags);
>  }

Hmm... Why not simply convert that 'clear_bit()' into a
'test_and_clear_bit()' and have it protect the hlist_del_rcu?

I can't see anything that would break if we do this. There doesn't seem
to be anything that explicitly depends on the ordering of those two
operations...

Cheers
  Trond