linux-kernel - Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 16 Mar 2010 13:35:54 -0400
From:	Trond Myklebust <Trond.Myklebust@...app.com>
To:	paulmck@...ux.vnet.ibm.com
Cc:	David Howells <dhowells@...hat.com>, linux-nfs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] NFS: Fix RCU warnings in
 nfs_inode_return_delegation_noreclaim()

On Tue, 2010-03-16 at 10:17 -0700, Paul E. McKenney wrote: 
> On Tue, Mar 16, 2010 at 11:51:30AM +0000, David Howells wrote:
> > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > index 2563beb..a77c735 100644
> > --- a/fs/nfs/delegation.c
> > +++ b/fs/nfs/delegation.c
> > @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
> >  {
> >  	struct rpc_cred *cred;
> > 
> > +	rcu_read_lock();
> >  	cred = rcu_dereference(delegation->cred);
> >  	rcu_assign_pointer(delegation->cred, NULL);
> 
> The lock is probably held here, in which case something like the
> following would work well without needing the artificial rcu_read_lock()
> and rcu_read_unlock():

No. The lock is not held here. At this point, the delegation has been
detached from the inode that pointed to it, and so we can free up its
contents.

We still need the call_rcu() to free up the allocated memory in order to
ensure that some process doing lockless traversal of the
clp->cl_delegations list doesn't crash.

> 	cred = rcu_dereference_check(delegation->cred,
> 				     lockdep_is_held(&delegation->lock));
> 
> > +	rcu_read_unlock();
> >  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> >  	if (cred)
> >  		put_rpccred(cred);
> > @@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> >  	spin_lock_init(&delegation->lock);
> > 
> >  	spin_lock(&clp->cl_lock);
> > +	rcu_read_lock();
> >  	if (rcu_dereference(nfsi->delegation) != NULL) {
> 
> Same here, though I am not sure whether clp->cl_lock or something in
> nfs_inode should be used.

Yes. As I indicated to David in another email, the clp->cl_lock protects
us here, so it looks as if your suggestion above would be perfect.

> >  		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
> >  					sizeof(delegation->stateid)) == 0 &&
> >  				delegation->type == nfsi->delegation->type) {
> > +			rcu_read_unlock();
> >  			goto out;
> >  		}
> >  		/*
> > @@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> >  		if (delegation->type <= nfsi->delegation->type) {
> >  			freeme = delegation;
> >  			delegation = NULL;
> > +			rcu_read_lock();
> >  			goto out;
> >  		}
> >  		freeme = nfs_detach_delegation_locked(nfsi, NULL);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/