Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Paul E. McKenney]

On Thu, Sep 09, 2010 at 08:46:03PM -0700, Paul E. McKenney wrote:
> On Thu, Sep 09, 2010 at 05:14:43PM +0200, Arnd Bergmann wrote:
> > On Tuesday 07 September 2010, Paul E. McKenney wrote:
> > > On Tue, Sep 07, 2010 at 10:00:58PM +0200, Arnd Bergmann wrote:
> > > > On Tuesday 07 September 2010 21:21:55 Kulikov Vasiliy wrote:
> > > > > #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); \
> > > > >                                       ^     ^
> > > > >                 smp_read_barrier_depends(); \
> > > > >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> > > > >         })
> > > > > 
> > > > > If I understand this, it is evaluated three times, right?
> > > > 
> > > > Yes, that looks like my own fault, I added that :(
> > > > 
> > > > This patch seems to fix it, but I need to think about it some more
> > > > to make sure it still does everything we need.
> > > 
> > > Let me know when you are satisfied with it, and then I will pick it up.
> > 
> > I guess it would be good to put it in now. I haven't had the time
> > to try out all cases, but the current code in -next is definitely
> > broken, so please put the fix in now.
> 
> Hmmm...  One approach would be have a secondary macro that was:
> 
> 	#define __rcu_dereference_check_sparse(p, space) \
> 		(void) (((typeof (*p) space *)p) == p);
> 
> when running sparse and:
> 
> 	#define __rcu_dereference_check_sparse(p, space)
> 
> otherwise.
> 
> Would that do the trick?

Here is a patch that more fully describes what I had in mind.  I have
done light compile/sparse testing, but this should be considered to be
full of bugs.

Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

commit 493a0a1001137f7058da0e01c3d05b0fcb92841d
Author: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
Date:   Mon Sep 13 17:24:21 2010 -0700

    rcu: only one evaluation of arg in rcu_dereference_check() unless sparse
    
    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.
    
    This commit also fixes some sparse complaints in rcutorture.c.
    
    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
    Cc: Arnd Bergmann <arnd@xxxxxxxx>

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  =
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 682bf4c..9fda1e6 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -309,24 +309,32 @@ extern int rcu_my_thread_group_empty(void);
  * (e.g., __rcu_bh, * __rcu_sched, and __srcu), should this make sense in
  * the future.
  */
+
+#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 */
+
 #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)); \
 	})
 
diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index 439ddab..adb09cb 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -131,7 +131,7 @@ struct rcu_torture {
 };
 
 static LIST_HEAD(rcu_torture_freelist);
-static struct rcu_torture *rcu_torture_current;
+static struct rcu_torture __rcu *rcu_torture_current;
 static long rcu_torture_current_version;
 static struct rcu_torture rcu_tortures[10 * RCU_TORTURE_PIPE_LEN];
 static DEFINE_SPINLOCK(rcu_torture_lock);
@@ -171,7 +171,7 @@ int rcutorture_runnable = RCUTORTURE_RUNNABLE_INIT;
 #endif /* #else #ifdef CONFIG_BOOST_RCU */
 
 static unsigned long boost_starttime;	/* jiffies of next boost test start. */
-DEFINE_MUTEX(boost_mutex);		/* protect setting boost_starttime */
+static DEFINE_MUTEX(boost_mutex);	/* protect setting boost_starttime */
 					/*  and boost task create/destroy. */
 
 /* Mediate rmmod and system shutdown.  Concurrent rmmod & shutdown illegal! */
@@ -180,7 +180,8 @@ DEFINE_MUTEX(boost_mutex);		/* protect setting boost_starttime */
 #define FULLSTOP_SHUTDOWN 1	/* System shutdown with rcutorture running. */
 #define FULLSTOP_RMMOD    2	/* Normal rmmod of rcutorture. */
 static int fullstop = FULLSTOP_RMMOD;
-DEFINE_MUTEX(fullstop_mutex);	/* Protect fullstop transitions and spawning */
+static DEFINE_MUTEX(fullstop_mutex);
+				/* Protect fullstop transitions and spawning */
 				/*  of kthreads. */
 
 /*
@@ -873,7 +874,8 @@ rcu_torture_writer(void *arg)
 			continue;
 		rp->rtort_pipe_count = 0;
 		udelay(rcu_random(&rand) & 0x3ff);
-		old_rp = rcu_torture_current;
+		old_rp = rcu_dereference_check(rcu_torture_current,
+					       current == writer_task);
 		rp->rtort_mbtest = 1;
 		rcu_assign_pointer(rcu_torture_current, rp);
 		smp_wmb(); /* Mods to old_rp must follow rcu_assign_pointer() */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/