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 16:37:05 +0000
From:	David Howells <dhowells@...hat.com>
To:	Trond Myklebust <Trond.Myklebust@...app.com>
Cc:	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()

Trond Myklebust <Trond.Myklebust@...app.com> wrote:

> > +	rcu_read_lock();
> >  	cred = rcu_dereference(delegation->cred);
> >  	rcu_assign_pointer(delegation->cred, NULL);
> > +	rcu_read_unlock();
> >  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> >  	if (cred)
> >  		put_rpccred(cred);
> 
> That's bogus. We're in the process of freeing the delegation, so we
> don't need to rely on rcu to read delegation->cred.
> 
> Better to just convert that rcu_dereference() into an ordinary pointer
> dereference.

Now that I take a second look at this code, it looks wrong.  You shouldn't
really start dissassembling the delegation record until the RCU callback
triggers as it may be in use by someone up to that point.  If you know it
isn't in use at this point, why use call_rcu()?

> The spinlock already provides protection. Again we can just convert the
> rcu_dereference() into a pointer dereference.

That seems reasonable.

> We cannot hold the rcu read lock across the entire RPC call in
> nfs_do_return_delegation(). All we want to do above is to check that
> nfsi->delegation != NULL.

Good point.

However, that leads me to note that nfs_detach_delegation_locked() then must
be using rcu_dereference() unnecessarily.  Also nfs_do_return_delegation()
must be too.

I think if all you're doing is checking the state of the pointer, you don't
need the interpolated memory barrier, since you've no need to synchronise what
the pointer points to.

David
--
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/