Re: Using sparse to catch invalid RCU dereferences?

[Johannes Berg]

On Thu, 2008-04-10 at 15:32 -0700, Paul E. McKenney wrote:
> On Wed, Apr 09, 2008 at 10:09:46PM +0200, Johannes Berg wrote:
> > 
> > > It might be.  There are a number of places where it is legal to access
> > > RCU-protected pointers directly, and all of these would need to be
> > > changed.  For example, in the example above, one could do:
> > > 
> > > 	foo = NULL;
> > 
> > Ok, that I understand, but sparse always treats NULL specially anyway.
> 
> But "int foo = 0;" would need the memory barrier -- index 0 of some
> RCU-protected array.

Oh. Hmm, I guess that wouldn't really be possible to find at least not
with sparse right now. Though maybe we can add some sort of annotation
that that special type can't even take zero directly.

> You are right -- I was confused.  The case where you can omit the
> rcu_assign_pointer() would be when building a multiple-element data
> structure that is then published as a unit.  For example:
> 
> 	p = kmalloc(sizeof(*p), GFP_KERNEL);
> 	q = kmalloc(sizeof(*p), GFP_KERNEL);
> 	p->next = q; /* don't need rcu_assign_pointer() here. */
> 	q->next = NULL;  /* or here. */
> 	/* initialize other fields of p and q. */
> 	rcu_assign_pointer(global_pointer, p);
> 
> The assignment to p->next doesn't have to be rcu_assign_pointer() because
> other CPUs are unable to access the data structure -- only the final
> assignment that publishes the whole group need be rcu_assign_pointer().
> On the other hand, the cost of the extra memory barrier would be
> insignificant in most cases.

Ah. Yeah, but we probably need a "raw" accessor anyway if we're going to
go this route, e.g. for any deref within the update-locked section.

> > I've been playing a bit, see below for my play rcupdate.h and test.c
> > test program.
> > 
> > Unfortunately, sparse doesn't have the ability to declare
> > "ï__attribute__((force_bitwise)) typeof(p)" or even
> > "ï__attribute__((force)) typeof(p)" which makes this force more than
> > necessary and causes it to not catch when incompatible pointers are
> > used. gcc notices that because I only do a cast at all for sparse, but
> > that doesn't help, since e.g. list_for_each_entry_rcu() requires that
> > the correct type is returned. So without sparse supporting the latter
> > notation, we don't stand a chance.
> 
> "<feff>"???

Hmm?

> > Also, I wouldn't know how to declare that an array or so needs
> > rcu-access to the members.
> 
> Hmmm...  Can you apply the address-space attribute to the array itself?
> I suppose one could convert the array to a pointer, but yecch!

Not sure if applying that to an array would work, and I wouldn't want to
convert it to pointers either. But I suppose you could declare the array
like this:

static struct foo * __attribute__((bitwise or address_space)) array[7];

which should, as far as I understand, apply the attribute to the array
members instead of the array.

johannes

Attachment: signature.asc
Description: This is a digitally signed message part