lkml.org 
[lkml]   [2005]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
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 06:24    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site