Date:	Fri, 21 Sep 2007 17:32:08 -0400 (EDT)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
cc:	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH] Add CRC checksum for RCU lists



--
On Fri, 21 Sep 2007, Paul E. McKenney wrote:

> On Thu, Sep 20, 2007 at 02:34:11PM -0400, Steven Rostedt wrote:
> > In recent development of the RT kernel, some of our experimental code
> > corrupted the rcu header. But the side effect (crashing) didn't rear its
> > ugly head until way after the fact. Discussing this with Paul, he
> > suggested that RCU should have a "self checking" mechanism to detect
> > these kind of issues. He also suggested putting in a CRC into the
> > rcu_head structure.
> >
> > This patch does so.
>
> Very cool!!!

Thanks :-)



>
> > This patch also takes care to update the crc when rcu_head items are
> > moved from list to list and whenever the next pointer is modified due to
> > addition.
>
> We can only be thankful that it is not possible to cancel outstanding
> RCU callbacks...

true

> Looks good -- a few suggestions for better fault coverage interspersed
> below.  But would be useful as is.  (And it would be good to apply after
> the preempt/boost patches, which are currently undergoing integration
> with the CPU hotplug patches -- the good news is that this integration
> eliminates the need for patch #4!)
>
> Acked-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>

Thanks, but this is still going to go through changes, from your comments
as well as your latest patches.

>
> > Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> >
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index fe17d7d..baca7e6 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -50,13 +50,81 @@
> >  struct rcu_head {
> >  	struct rcu_head *next;
> >  	void (*func)(struct rcu_head *head);
> > +#ifdef CONFIG_RCU_CRC_HEADER_CHECK
> > +	/*
> > +	 * Checks are made in C files knowing that "next" is
> > +	 * the first element. So keep it the first element.
> > +	 */
> > +	unsigned long crc;
> > +	void *caller;
> > +#endif
> >  };
>
> Looks good, but one question -- why not include the caller in the CRC?
> Not a big deal either way, but would catch a few more cases of corruption.
> Also, as things stand, the caller pointer can be silently corrupted,
> which might causes confusion if someone had to examine the RCU callback
> lists from a crash dump.

One reason was that the caller was an addition. I originally didn't have
it, but during my tests, the output was basically useless. It didn't give
any hint to where things went wrong, so I added it. The CRC is to check
the rcu is working, not really the checker itself.

Note, it helped us out lately with Peter's latest file_table patches in
-rt. With this patch applied, it triggered corruption. That was due to the
union in the fs.h between the llist and rcu_head there was a dependency
in the llist where the rcu_head would not grow. The llist can still do a
spin lock on the lock _after_ the item was added to the call_rcu. Because
of that union, the locking of the lock corrupted the crc. Set it to one.

You'll see patches from Peter soon to get rid of that dependency.

>
> Interchanging the order of "crc" and "caller" would change the strategy,
> if I understand correctly.

yep.

>
> > -#define RCU_HEAD_INIT 	{ .next = NULL, .func = NULL }
> > +#ifdef CONFIG_RCU_CRC_HEADER_CHECK
> > +
> > +#define RCU_CRC_MAGIC 0xC4809168UL
>
> Very magic indeed -- Google doesn't find it, other than in your
> patch.  ;-)

Paul, I'm disappointed in you. That number doesn't ring a bell at all??

(hint, ignore the 'C' that was added by me).

>
> > +static inline unsigned long rcu_crc_calc(struct rcu_head *head)
> > +{
> > +	unsigned int *p = (unsigned int*)head; /* 32 bit */
> > +	unsigned long crc = RCU_CRC_MAGIC;
> > +
> > +	for (p=(void *)head; (void*)p < (void*)&head->crc; p++)
> > +		crc ^= *p;
> > +	return crc;
> > +}
>
> Why initialize "p" twice?  (Once in the declaration, and once in the
> "for" loop, but with different casts.)

Why? probably because I was half asleep when writing it ;-)
Will fix.

>
> > +static inline void rcu_crc_check(struct rcu_head *head)
> > +{
> > +	static int once;
> > +	if (unlikely(head->crc != rcu_crc_calc(head)) && !once) {
> > +		once++;
>
> Do we want exactly one (give or take concurrent checks), or do we want
> the first ten or so?  Sometimes having a modest sample rather than
> exactly one is helpful.

I added that because during testing, it would flood the serial console. I
can modify this to whatever deems fit.  Perhaps more hits will give a
better clue to what went wrong. I could also just add a print_ratelimit to
it too.


>
> And I know that it doesn't matter in this case, but I cannot stop myself
> from pointing out the possibility of making "once" be an atomic_t
> that is initialized to (say) -10, then making the !once check be an
> atomic_add_return().  (Whew!  Good to get that off my chest!!!)

Would you prefer the above or the print_ratelimit? Maybe 10 would be best.
I really don't have a preference.

>
> Now back to real problems.  ;-)
>
> (Note to self...)  The way this is coded could possibly result in false
> positives.  Suppose that the last element in a given callback list has
> its CRC correctly calculated.  Now suppose that a new callback is being
> added to the end of the list.  This addition is non-atomic, so the of
> the old last element will be momentarily incorrect.  So, need to check
> that all list checks are protected by some lock.  (And all the cases
> I saw below are in fact OK.)

I believe that I tried to keep all the checks done in the same locations
that could possible modify the lists. So they should be protect by the
same mechanism.  On the rcupreempt side, I made the checks within the
holding of the data lock (for when you see that patch ;-)

>
> > +		printk("BUG: RCU check failed!");
> > +		if (head->caller)
> > +			printk(" (caller=%p)",
> > +			       head->caller);
> > +		printk("\n");
> > +		printk("  CRC was %08lx, expected %08lx\n",
> > +		       head->crc, rcu_crc_calc(head));
>
> I suggest also printing head->crc^rcu_crc_calc(head) to make cases
> where a single bit is being corrupted more obvious.

OK, will do.

>
> > +		printk("  %p %p\n",
> > +		       head->next, head->func);
> > +		dump_stack();
> > +	}
> > +}
> > +
> > +static inline void rcu_assign_crc(struct rcu_head *head)
> > +{
> > +	head->crc = rcu_crc_calc(head);
> > +	head->caller = __builtin_return_address(0);
> > +}
>
> If you do decide to move the caller into the CRC calculation, it
> will be necessary to reverse the above pair of lines.

Noted.

>
> > +static inline void rcu_check_list(struct rcu_head *head)
> > +{
> > +	int loop;
> > +	for (loop = 0;
> > +	     head != NULL && loop < 100;
> > +	     head=head->next, loop++)
> > +		rcu_crc_check(head);
> > +}
> > +
> > +# define RCU_CRC_INIT , .crc = RCU_CRC_MAGIC
>
> Would need to initialize caller here if you add it to the CRC.
>
> > +# define RCU_CRC_SET(ptr) (ptr)->crc = RCU_CRC_MAGIC
>
> And here as well.
>
> But this has the effect of causing the CRC to be born correct.  Do we
> really want that?  Suppose someone incorrectly re-initialized a callback
> that was still on a list.  Wouldn't it be better to get a CRC warning than
> a NULL pointer exception?  So suggest something like RCU_CRC_MAGIC+1 --
> perhaps with a RCU_CRC_BAD_MAGIC symbol.

Yeah, I can scrap those initialization macros. That came about my first
attempt where I forgot to add the assignment to the call_rcu and it
obviously failed.  So I added these macros, and it stilled failed. Then I
saw the mistake I made, fixed it, and it worked. But I never removed these
macros.  I think we can just keep the crc as zero. That would also fail
the test. Hmm, maybe we should add a BAD_CRC number so that it will give
us a hint that something was initiazed incorrectly.

>
> > +#else
> > +# define rcu_crc_calc(head) 0
> > +# define rcu_crc_check(head) do { } while(0)
> > +# define rcu_assign_crc(head) do { } while(0)
> > +# define rcu_check_list(head) do { } while(0)
> > +# define RCU_CRC_INIT
> > +# define RCU_CRC_SET(ptr) do { } while(0)
> > +#endif /* CONFIG_RCU_CRC_HEADER_CHECK */
> > +
> > +#define RCU_HEAD_INIT 	{ .next = NULL, .func = NULL RCU_CRC_INIT }
> >  #define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
> > -#define INIT_RCU_HEAD(ptr) do { \
> > -       (ptr)->next = NULL; (ptr)->func = NULL; \
> > -} while (0)
> > +#define INIT_RCU_HEAD(ptr) do {				\
> > +		(ptr)->next = NULL; (ptr)->func = NULL; \
> > +		RCU_CRC_SET(ptr);			\
> > +	} while (0)
> >
> >
> >
> > diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> > index 2c2dd84..4c3cc9c 100644
> > --- a/kernel/rcupdate.c
> > +++ b/kernel/rcupdate.c
> > @@ -76,6 +76,23 @@ static atomic_t rcu_barrier_cpu_count;
> >  static DEFINE_MUTEX(rcu_barrier_mutex);
> >  static struct completion rcu_barrier_completion;
> >
> > +#ifdef CONFIG_RCU_CRC_HEADER_CHECK
> > +#define rcu_check_rdp(rdp)			\
> > +	do {					\
> > +		rcu_check_list(rdp->nxtlist);	\
> > +		rcu_check_list(rdp->curlist);	\
> > +		rcu_check_list(rdp->donelist);	\
> > +	} while(0)
> > +#define rcu_crc_update_tail(rdp, tail, list)				\
> > +	do {								\
> > +		if ((rdp)->tail != &(rdp)->list)			\
> > +			rcu_assign_crc((struct rcu_head*)(rdp)->tail);	\
> > +	} while(0)
> > +#else
> > +# define rcu_check_rdp(rdp) do { } while(0)
> > +# define rcu_crc_update_tail(rdp, tail, list) do { } while(0)
> > +#endif
> > +
> >  #ifdef CONFIG_SMP
> >  static void force_quiescent_state(struct rcu_data *rdp,
> >  			struct rcu_ctrlblk *rcp)
> > @@ -122,14 +139,19 @@ void fastcall call_rcu(struct rcu_head *head,
> >
> >  	head->func = func;
> >  	head->next = NULL;
> > +	rcu_assign_crc(head);
> >  	local_irq_save(flags);
> >  	rdp = &__get_cpu_var(rcu_data);
> >  	*rdp->nxttail = head;
>
> The CRC of the tail element is incorrect at this point, but that is OK
> because we have interrupts disabled and no other CPU can access our list
> in the meantime.

Yep, that was planned.

>
> > +	rcu_crc_update_tail(rdp, nxttail, nxtlist);
> >  	rdp->nxttail = &head->next;
> >  	if (unlikely(++rdp->qlen > qhimark)) {
> >  		rdp->blimit = INT_MAX;
> >  		force_quiescent_state(rdp, &rcu_ctrlblk);
> >  	}
> > +
> > +	rcu_check_rdp(rdp);
>
> This check is OK -- no other CPU should be able to manipulate our
> rdp, and we have interrupts disabled.  Same situation for call_rcu_bh()
> below.
>
> >  	local_irq_restore(flags);
> >  }
> >
> > @@ -157,9 +179,11 @@ void fastcall call_rcu_bh(struct rcu_head *head,
> >
> >  	head->func = func;
> >  	head->next = NULL;
> > +	rcu_assign_crc(head);
> >  	local_irq_save(flags);
> >  	rdp = &__get_cpu_var(rcu_bh_data);
> >  	*rdp->nxttail = head;
> > +	rcu_crc_update_tail(rdp, nxttail, nxtlist);
> >  	rdp->nxttail = &head->next;
> >
> >  	if (unlikely(++rdp->qlen > qhimark)) {
> > @@ -167,6 +191,8 @@ void fastcall call_rcu_bh(struct rcu_head *head,
> >  		force_quiescent_state(rdp, &rcu_bh_ctrlblk);
> >  	}
> >
> > +	rcu_check_rdp(rdp);
> > +
> >  	local_irq_restore(flags);
> >  }
> >
> > @@ -233,6 +259,8 @@ static void rcu_do_batch(struct rcu_data *rdp)
> >  	struct rcu_head *next, *list;
> >  	int count = 0;
> >
> > +	rcu_check_rdp(rdp);
>
> OK, interrupts disabled here.
>
> >  	list = rdp->donelist;
> >  	while (list) {
> >  		next = list->next;
>
> Why not invalidate the CRC of the element that we just removed?  This
> would catch some cases of list mangling.

Good idea, will add.

>
> > @@ -373,6 +401,7 @@ static void rcu_move_batch(struct rcu_data *this_rdp, struct rcu_head *list,
> >  {
> >  	local_irq_disable();
> >  	*this_rdp->nxttail = list;
>
> Momentarily wrong CRC OK, interrupts disabled here.  Ditto for
> __rcu_process_callbacks() below.

yep

>
> > +	rcu_crc_update_tail(this_rdp, nxttail, nxtlist);
> >  	if (list)
> >  		this_rdp->nxttail = tail;
> >  	local_irq_enable();
> > @@ -424,6 +453,7 @@ static void __rcu_process_callbacks(struct rcu_ctrlblk *rcp,
> >  {
> >  	if (rdp->curlist && !rcu_batch_before(rcp->completed, rdp->batch)) {
> >  		*rdp->donetail = rdp->curlist;
> > +		rcu_crc_update_tail(rdp, donetail, donelist);
> >  		rdp->donetail = rdp->curtail;
> >  		rdp->curlist = NULL;
> >  		rdp->curtail = &rdp->curlist;
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 50a94ee..981fc93 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -424,6 +424,17 @@ config RCU_TORTURE_TEST
> >  	  Say M if you want the RCU torture tests to build as a module.
> >  	  Say N if you are unsure.
> >
> > +config RCU_CRC_HEADER_CHECK
> > +	bool "RCU header self check"
> > +	depends on DEBUG_KERNEL
> > +	help
> > +	  This option enables CRC verification of RCU lists to catch
> > +	  possible corruption to the RCU list by improper application
> > +	  of RCU callbacks. This adds overhead to the running system
> > +	  so only enable it if you suspect RCU corruption is occurring.
> > +
> > +	  If unsure, say N.
> > +
> >  config LKDTM
> >  	tristate "Linux Kernel Dump Test Tool Module"
> >  	depends on DEBUG_KERNEL
> >
> >
>

Paul, thanks for all the comments. I'll put out a new round of patches
after yours becomes offical (no "not for inclussion" statements).

-- Steve

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

Powered by blists - more mailing lists