lkml.org 
[lkml]   [2005]   [Nov]   [12]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateFri, 11 Nov 2005 21:22:13 -0800
From"Paul E. McKenney" <>
SubjectRe: [Lse-tech] Subject: [RFC][PATCH] Fix for unsafe notifier chain mechanism
On Fri, Nov 11, 2005 at 09:36:40PM -0500, Alan Stern wrote:
> On Fri, 11 Nov 2005, Paul E. McKenney wrote:> > > On Fri, Nov 11, 2005 at 03:43:39PM -0800, Chandra Seetharaman wrote:
> > > Hello,> > > > > > In 2.6.14, the notifier chains are unsafe. notifier_call_chain() walks through
> > > the list of a call chain without any protection.> > > Looks pretty good!  Some RCU-related review comments interspersed below.
> > > > > Alan and I did think about changing the data structure to use list_head, but 
> > > deferred it (as a cleanup) as it is not directly tied with what Alan was
> > > trying to fix.> > > > It would simplify the code...> > It would.  It would also mean auditing every place in the kernel where a
> notifier_block structure is defined.  There are a _lot_ of them, and many
> don't use C99 initializers or do initialize the link pointer.  Chandra and
> I decided it was best to leave this as a subsequent cleanup job, maybe
> something suitable for kernel-janitors.

Fair enough by me!

> > > +	down_write(&nh->rwsem);> > > +	nl = &nh->head;> > > +	while ((*nl) != NULL) {> > > +		if (n->priority > (*nl)->priority)
> > > +			break;
> > > +		nl = &((*nl)->next);
> > > +	}
> > > +	rcu_assign_pointer(n->next, *nl);> > > > The above can simply be "n->next = *nl;".  The reason is that this change
> > of state is not visible to RCU readers until after the following statement,
> > and it therefore need not be an RCU-reader-safe assignment.  You only need
> > to use rcu_assign_pointer() when the results of the assignment are
> > immediately visible to RCU readers.> > Correct, the rcu call isn't really needed.  It doesn't hurt perceptibly,
> though, and part of the RCU documentation states:> >  * ...  More importantly, this
>  * call documents which pointers will be dereferenced by RCU read-side
>  * code.
> 
> For that reason, I felt it was worth putting it in.

But the following statement does a much better job of documenting the
pointer that is to be RCU-dereferenced.  Duplicate documentation can
be just as confusing as no documentation.

> > > +	rcu_assign_pointer(*nl, n);> > > +	up_write(&nh->rwsem);> > > +	if (nh->type == ATOMIC_NOTIFIER)> > > +		synchronize_rcu();> > > > This "if" statement and the "synchronize_rcu()" are not needed.  Nothing
> > has been removed from the list, so nothing will be freed, so no need to
> > wait for readers to get done.> > You're right.  In an earlier form of the patch this call was left out, but
> then it crept back in later.  We can remove it.

Sounds good!

> > In contrast, the synchronize_rcu() in notifier_chain_unregister() -is-
> > needed, since we need to free the removed element.> > > > +	if (!nh->head)> > > +		return ret;> > > +	if (nh->type == ATOMIC_NOTIFIER)> > > +		rcu_read_lock();> > > +	else> > > +		down_read(&nh->rwsem);> > > > Is it possible for the value of nh->type to change?  If so, there needs
> > to be some additional mechanism to guard against such a change.  However,
> > if this field is constant, this code is just fine as is.> > nh->type is never supposed to change.

OK, then the code is fine as it is.

							Thanx, Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-11-12 05:24    [W:0.452 / U:0.450 seconds]
©2003-2008 Jasper Spaans