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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
