[tip/core/rcu,09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The rcu_print_detail_task_stall_rnp() function invokes
rcu_preempt_blocked_readers_cgp() to verify that there are some preempted
RCU readers blocking the current grace period outside of the protection
of the rcu_node structure's ->lock.  This means that the last blocked
reader might exit its RCU read-side critical section and remove itself
from the ->blkd_tasks list before the ->lock is acquired, resulting in
a segmentation fault when the subsequent code attempts to dereference
the now-NULL gp_tasks pointer.

This commit therefore moves the test under the lock.  This will not
have measurable effect on lock contention because this code is invoked
only when printing RCU CPU stall warnings, in other words, in the common
case, never.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree_plugin.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

On Thu, Aug 30, 2012 at 11:56:22AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> The rcu_print_detail_task_stall_rnp() function invokes
> rcu_preempt_blocked_readers_cgp() to verify that there are some preempted
> RCU readers blocking the current grace period outside of the protection
> of the rcu_node structure's ->lock.  This means that the last blocked
> reader might exit its RCU read-side critical section and remove itself
> from the ->blkd_tasks list before the ->lock is acquired, resulting in
> a segmentation fault when the subsequent code attempts to dereference
> the now-NULL gp_tasks pointer.
> 
> This commit therefore moves the test under the lock.  This will not
> have measurable effect on lock contention because this code is invoked
> only when printing RCU CPU stall warnings, in other words, in the common
> case, never.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree_plugin.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index 139a803..c02dc1d 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
>  	unsigned long flags;
>  	struct task_struct *t;
>  
> -	if (!rcu_preempt_blocked_readers_cgp(rnp))
> -		return;
>  	raw_spin_lock_irqsave(&rnp->lock, flags);
> +	if (!rcu_preempt_blocked_readers_cgp(rnp)) {
> +		raw_spin_unlock_irqrestore(&rnp->lock, flags);
> +		return;
> +	}
>  	t = list_entry(rnp->gp_tasks,
>  		       struct task_struct, rcu_node_entry);
>  	list_for_each_entry_continue(t, &rnp->blkd_tasks, rcu_node_entry)

Given the small number of lines of code inside the critical section
here, I think this would look clearer without the early return and
duplicate lock release:

	raw_spin_lock_irqsave(&rnp->lock, flags);
	if (rcu_preempt_blocked_readers_cgp(rnp)) {
		...
	}
	raw_spin_unlock_irqrestore(&rnp->lock, flags);

- Josh Triplett

On Fri, Aug 31, 2012 at 11:19:17AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:22AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > The rcu_print_detail_task_stall_rnp() function invokes
> > rcu_preempt_blocked_readers_cgp() to verify that there are some preempted
> > RCU readers blocking the current grace period outside of the protection
> > of the rcu_node structure's ->lock.  This means that the last blocked
> > reader might exit its RCU read-side critical section and remove itself
> > from the ->blkd_tasks list before the ->lock is acquired, resulting in
> > a segmentation fault when the subsequent code attempts to dereference
> > the now-NULL gp_tasks pointer.
> > 
> > This commit therefore moves the test under the lock.  This will not
> > have measurable effect on lock contention because this code is invoked
> > only when printing RCU CPU stall warnings, in other words, in the common
> > case, never.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutree_plugin.h |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > index 139a803..c02dc1d 100644
> > --- a/kernel/rcutree_plugin.h
> > +++ b/kernel/rcutree_plugin.h
> > @@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
> >  	unsigned long flags;
> >  	struct task_struct *t;
> >  
> > -	if (!rcu_preempt_blocked_readers_cgp(rnp))
> > -		return;
> >  	raw_spin_lock_irqsave(&rnp->lock, flags);
> > +	if (!rcu_preempt_blocked_readers_cgp(rnp)) {
> > +		raw_spin_unlock_irqrestore(&rnp->lock, flags);
> > +		return;
> > +	}
> >  	t = list_entry(rnp->gp_tasks,
> >  		       struct task_struct, rcu_node_entry);
> >  	list_for_each_entry_continue(t, &rnp->blkd_tasks, rcu_node_entry)
> 
> Given the small number of lines of code inside the critical section
> here, I think this would look clearer without the early return and
> duplicate lock release:
> 
> 	raw_spin_lock_irqsave(&rnp->lock, flags);
> 	if (rcu_preempt_blocked_readers_cgp(rnp)) {
> 		...
> 	}
> 	raw_spin_unlock_irqrestore(&rnp->lock, flags);

You might well be right, but doing that gets me another line longer
than 80 characters.

Hey, I have an excuse -- I actually spent a significant fraction of
my career using punched cards.  ;-)

							Thanx, Paul

On Tue, Sep 04, 2012 at 03:46:59PM -0700, Paul E. McKenney wrote:
> On Fri, Aug 31, 2012 at 11:19:17AM -0700, Josh Triplett wrote:
> > On Thu, Aug 30, 2012 at 11:56:22AM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > 
> > > The rcu_print_detail_task_stall_rnp() function invokes
> > > rcu_preempt_blocked_readers_cgp() to verify that there are some preempted
> > > RCU readers blocking the current grace period outside of the protection
> > > of the rcu_node structure's ->lock.  This means that the last blocked
> > > reader might exit its RCU read-side critical section and remove itself
> > > from the ->blkd_tasks list before the ->lock is acquired, resulting in
> > > a segmentation fault when the subsequent code attempts to dereference
> > > the now-NULL gp_tasks pointer.
> > > 
> > > This commit therefore moves the test under the lock.  This will not
> > > have measurable effect on lock contention because this code is invoked
> > > only when printing RCU CPU stall warnings, in other words, in the common
> > > case, never.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> > >  kernel/rcutree_plugin.h |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > > index 139a803..c02dc1d 100644
> > > --- a/kernel/rcutree_plugin.h
> > > +++ b/kernel/rcutree_plugin.h
> > > @@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
> > >  	unsigned long flags;
> > >  	struct task_struct *t;
> > >  
> > > -	if (!rcu_preempt_blocked_readers_cgp(rnp))
> > > -		return;
> > >  	raw_spin_lock_irqsave(&rnp->lock, flags);
> > > +	if (!rcu_preempt_blocked_readers_cgp(rnp)) {
> > > +		raw_spin_unlock_irqrestore(&rnp->lock, flags);
> > > +		return;
> > > +	}
> > >  	t = list_entry(rnp->gp_tasks,
> > >  		       struct task_struct, rcu_node_entry);
> > >  	list_for_each_entry_continue(t, &rnp->blkd_tasks, rcu_node_entry)
> > 
> > Given the small number of lines of code inside the critical section
> > here, I think this would look clearer without the early return and
> > duplicate lock release:
> > 
> > 	raw_spin_lock_irqsave(&rnp->lock, flags);
> > 	if (rcu_preempt_blocked_readers_cgp(rnp)) {
> > 		...
> > 	}
> > 	raw_spin_unlock_irqrestore(&rnp->lock, flags);
> 
> You might well be right, but doing that gets me another line longer
> than 80 characters.

Even with that line broken in an appropriate place, the result still
seems clearer.

> Hey, I have an excuse -- I actually spent a significant fraction of
> my career using punched cards.  ;-)

:)

- Josh Triplett