Re: [PATCH] Markers : fix check format with rcu callback race

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > The fix "markers: fix unchecked format" introduced an RCU callback race. This
> > patch takes care of calling any pending RCU callback before set_format is
> > called.
> > 
> 
> marker_set_format() has this statement:
> 
> 	if ((*entry)->rcu_pending)
> 		rcu_barrier_sched();
> 

True, therefore my fix is not needed.

Mathieu

> > 
> > * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> >> bit-field is not thread-safe nor smp-safe.
> >>
> >> struct marker_entry.rcu_pending is not protected by any lock
> >> in rcu-callback free_old_closure().
> >> so we must turn it into a safe type.
> >>
> > 
> > All struct marker_entry.rcu_pending accesses are done with the
> > markers_mutex held, except the one done in free_old_closure(). Normally,
> > there should be a
> >         if (entry->rcu_pending)
> >            rcu_barrier_sched();
> > 
> > At the beginning of each markers_mutex section (just after get_marker())
> > to make sure any pending callback is executed at that point before any
> > of rcu_pending or ptype are touched. 
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> > CC: Ingo Molnar <mingo@elte.hu>
> > CC: Lai Jiangshan <laijs@cn.fujitsu.com>
> > ---
> >  kernel/marker.c |   24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > Index: linux-2.6-lttng/kernel/marker.c
> > ===================================================================
> > --- linux-2.6-lttng.orig/kernel/marker.c	2008-10-10 01:35:32.000000000 -0400
> > +++ linux-2.6-lttng/kernel/marker.c	2008-10-10 01:35:32.000000000 -0400
> > @@ -657,21 +657,23 @@ int marker_probe_register(const char *na
> >  		entry = add_marker(name, format);
> >  		if (IS_ERR(entry))
> >  			ret = PTR_ERR(entry);
> > -	} else if (format) {
> > -		if (!entry->format)
> > -			ret = marker_set_format(&entry, format);
> > -		else if (strcmp(entry->format, format))
> > -			ret = -EPERM;
> > +	} else {
> > +		/*
> > +		 * If we detect that a call_rcu is pending for this marker,
> > +		 * make sure it's executed now.
> > +		 */
> > +		if (entry->rcu_pending)
> > +			rcu_barrier_sched();
> > +		if (format) {
> > +			if (!entry->format)
> > +				ret = marker_set_format(&entry, format);
> > +			else if (strcmp(entry->format, format))
> > +				ret = -EPERM;
> > +		}
> >  	}
> >  	if (ret)
> >  		goto end;
> >  
> > -	/*
> > -	 * If we detect that a call_rcu is pending for this marker,
> > -	 * make sure it's executed now.
> > -	 */
> > -	if (entry->rcu_pending)
> > -		rcu_barrier_sched();
> >  	old = marker_entry_add_probe(entry, probe, probe_private);
> >  	if (IS_ERR(old)) {
> >  		ret = PTR_ERR(old);
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68