linux-kernel - Re: RCU condition checks

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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 7 Apr 2010 10:10:48 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	David Howells <dhowells@...hat.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>, Trond.Myklebust@...app.com,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: RCU condition checks

On Wed, Apr 07, 2010 at 05:35:30PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@...ux.vnet.ibm.com> wrote:
> 
> > > Why is there a need for 'c'?
> > 
> > An example use is where rcu_access_pointer() is legal because we are
> > either initializing or cleaning up, so that no other CPU has access
> > to the pointer.  In these cases, you might do something like:
> > 
> > 	q = rcu_access_pointer(p->a, p->refcnt == 0);
> 
> I think the main problem I have with this is that the fact that p->refcnt
> should be 0 here is unrelated to the fact that we're wanting to look at the
> value of p->a.  I'd say that this should be two separate statements, for
> example:
> 
> 	ASSERT(p->refcnt == 0);
> 	q = rcu_access_pointer(p->a);
> 
> I could see using a lockdep-managed ASSERT here would work, though.
> 
> The other problem I have with this is that I'm assuming rcu_access_pointer()
> is simply for looking at the value of the pointer without dereferencing it -
> in which case, is there any need for the lock-describing condition?

I agree that in many cases there won't be a reasonable condition.
In which case, using "1" and an explanatory comment makes sense.
In other cases, the fact that the value is zero can mean that no one
else can possibly have a reference.

All that aside, I fully expect that uses of rcu_access_pointer() will
require more than the usual code-review effort, as these sorts of
unprotected accesses are notoriously error-prone.

> I agree, though, that:
> 
> 	q = rcu_dereference_check(p->a,
> 				  rcu_read_lock_held() || (
> 				   lockdep_is_held(p->lock) &&
> 				   lockdep_is_held(q->lock)));
> 
> is a reasonable way of keeping the dereference and the lock checks together,
> though that could equally well be written, say:
> 
> 	LOCKDEP_ASSERT(rcu_read_lock_held() || (
> 		        lockdep_is_held(p->lock) &&
> 			lockdep_is_held(q->lock)));
> 	q = rcu_dereference_protected(p->a);
> 
> but combining those makes it easier to ensure people to write lock checking.

Glad you like it!

							Thanx, Paul
--
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/