Re: Using sparse to catch invalid RCU dereferences?

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]


> 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.

> I recently tried to modify rcu_assign_pointer() to issue the memory
> memory barrier only when the pointer was non-NULL, but this ended badly.

Hm? I thought that's in the current tree.

> Probably because I am not the greatest gcc expert around...  We ended
> up having to define an rcu_assign_index() to handle the possibility of
> assigning a zero-value array index, but my attempts to do type-checking
> backfired, and I eventually gave it up.  Again, someone a bit more clued
> in to gcc than I am could probably pull it off.

Ah, ok.

> In addition, it is legal to omit rcu_dereference() and rcu_assign_pointer()
> when holding the update-side lock.

That I don't understand. Well, I do understand that omitting
rcu_dereference() is ok, but it seems to me that the memory and compiler
barrier in rcu_assign_pointer() is actually needed.

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.

Also, I wouldn't know how to declare that an array or so needs
rcu-access to the members.

johannes


rcupdate.h:

#define USE_BITWISE

#ifdef __CHECKER__
#ifdef USE_BITWISE
#define __rcu __attribute__((bitwise))
#define __force_rcu_cast(p) (*((__attribute__((force)) void **)&(p)))
// would like instead:
//#define __force_rcu_cast(p) ((__attribute__((force_bitwise)) typeof(p)) (p))
#else /* not bitwise */
#define __rcu __attribute__((address_space(3)))
#define __force_rcu_cast(p) (*((__attribute__((force)) void **)&(p)))
// would like instead:
//#define __force_rcu_cast(p) ((__attribute__((force_address_space)) typeof(p)) (p))
#endif

#else /* not checker */
#define __rcu
#define __force_rcu_cast(p) (p)
#endif

#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))

#define rcu_dereference(p)     ({ \
				typeof(p) _________p1 = ACCESS_ONCE(p); \
				smp_read_barrier_depends(); \
				__force_rcu_cast(_________p1); \
				})

/**
 * rcu_fetch - fetch an RCU-protected pointer in the update-locked
 * critical section.
 *
 * This macro exists for documentation and code checking purposes.
 */
#define rcu_fetch(p)     __force_rcu_cast(p);

#define rcu_assign_pointer(p, v) \
	({ \
		if (!__builtin_constant_p(v) || \
		    ((v) != NULL)) \
			smp_wmb(); \
		__force_rcu_cast(p) = (v); \
	})


test.c:

#include <stdlib.h>
#include "rcupdate.h"

/* my rcu protected variables */
static unsigned int __rcu *prot;
static unsigned int __rcu *prot_same;
static unsigned char __rcu *prot2;

// dummies
static smp_read_barrier_depends(void) {}
static smp_wmb(void) {}

int main(void)
{
	unsigned int *tmp;

	// no warnings from sparse due to forced cast
	rcu_assign_pointer(prot, tmp);
	// but gcc warns
	rcu_assign_pointer(prot2, tmp);

	// no warnings
	rcu_assign_pointer(prot, NULL);
	rcu_assign_pointer(prot2, NULL);

	// no warnings
	prot = NULL;
	prot2 = NULL;

	// no warnings from sparse due to forced cast
	tmp = rcu_dereference(prot);
	// but gcc warns
	tmp = rcu_dereference(prot2);

	/* now within locked section rcu_dereference isn't required */

	// no warnings from sparse due to forced cast
	tmp = rcu_fetch(prot);
	// but gcc warns
	tmp = rcu_fetch(prot2);

	/* not caught with address_space, but is caught with bitwise */
	prot = prot_same;
}


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]