On Wed, Sep 15, 2010 at 02:28:32PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 September 2010, Paul E. McKenney wrote:
> > The current version of the __rcu_access_pointer(),
> > __rcu_dereference_check(), and __rcu_dereference_protected() macros
> > evaluate their "p" argument three times, not counting typeof()s. This is
> > bad news if that argument contains a side effect. This commit therefore
> > evaluates this argument only once in normal kernel builds. However, the
> > straightforward approach defeats sparse's RCU-pointer checking, so this
> > commit also adds a KBUILD_CHECKSRC symbol defined when running a checker.
> > Therefore, when this new KBUILD_CHECKSRC symbol is defined, the additional
> > pair of evaluations of the "p" argument are performed in order to permit
> > sparse to detect misuse of RCU-protected pointers.
>
> In general, I don't like the idea much because that means we're passing
> semantically different code into sparse and gcc. Of course if my other
> patch doesn't work, we might need to do it after all.
Agreed in principle, but please see below.
> > diff --git a/Makefile b/Makefile
> > index f3bdff8..1c4984d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -330,7 +330,7 @@ PERL = perl
> > CHECK = sparse
> >
> > CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
> > - -Wbitwise -Wno-return-void $(CF)
> > + -Wbitwise -Wno-return-void -DKBUILD_CHECKSRC $(CF)
> > CFLAGS_MODULE =
> > AFLAGS_MODULE =
> > LDFLAGS_MODULE =
>
> sparse already define __CHECKER__ itself, no need to define another symbol.
Good point, will fix if we are in fact sticking with this solution.
> > +#ifdef KBUILD_CHECKSRC
> > +#define rcu_dereference_sparse(p, space) \
> > + ((void)(((typeof(*p) space *)p) == p))
> > +#else /* #ifdef KBUILD_CHECKSRC */
> > +#define rcu_dereference_sparse(p, space)
> > +#endif /* #else #ifdef KBUILD_CHECKSRC */
>
> Did you see a problem with my macro?
>
> #define rcu_dereference_sparse(p, space) \
> ((void)(((typeof(*p) space *)NULL) == ((typeof(p))NULL)))
I don't see a specific problem with it. However, I am not sure that
it really does what we want, and you indicated some doubts when you
posted it. So I opted for something that very obviously will work.
If you can assure me that sparse will interpret the typeof()s and
space casts properly, I have no problem going with your version.
> I think this should warn in all the cases we want it to, but have no side-effects.
I still note a tone of uncertainty in the above sentence. ;-)
> > #define __rcu_access_pointer(p, space) \
> > ({ \
> > typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> > - (void) (((typeof (*p) space *)p) == p); \
> > + rcu_dereference_sparse(p, space); \
> > ((typeof(*p) __force __kernel *)(_________p1)); \
> > })
> > #define __rcu_dereference_check(p, c, space) \
> > ({ \
> > typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> > rcu_lockdep_assert(c); \
> > - (void) (((typeof (*p) space *)p) == p); \
> > + rcu_dereference_sparse(p, space); \
> > smp_read_barrier_depends(); \
> > ((typeof(*p) __force __kernel *)(_________p1)); \
> > })
> > #define __rcu_dereference_protected(p, c, space) \
> > ({ \
> > rcu_lockdep_assert(c); \
> > - (void) (((typeof (*p) space *)p) == p); \
> > + rcu_dereference_sparse(p, space); \
> > ((typeof(*p) __force __kernel *)(p)); \
> > })
> >
>
> This part might be useful in any case, to better document what the cast and
> compare does, and to prevent the three users from diverging.
And it would probably make sense to pull the rcu_dereference_sparse()
into the macro, for that matter.
> >diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> >index 439ddab..adb09cb 100644
> >--- a/kernel/rcutorture.c
> >+++ b/kernel/rcutorture.c
>
> This didn't seem to belong here.
Yep, I really should put this in a separate commit.
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
