On Fri, 2006-07-28 at 00:58 +0200, Patrick McHardy wrote:
> > +int fib_rules_lookup(struct fib_rules_ops *ops, struct flowi *fl,
> > + int flags, struct fib_lookup_arg *arg)
> > +{
> > + struct fib_rule *rule;
> > + int err;
> > +
> > + rcu_read_lock();
> > +
> > + list_for_each_entry(rule, ops->rules_list, list) {
>
> Shouldn't that be list_for_each_entry_rcu?
Yes that's correct, it should.
> > + if (rule->ifname[0] && (rule->ifindex != fl->iif))
> > + continue;
> > +
> > + if (!ops->match(rule, fl, flags))
> > + continue;
> > +
> > + rcu_read_unlock();
> > +
> > + err = ops->action(rule, fl, flags, arg);
> > + if (err != -EAGAIN) {
> > + fib_rule_get(rule);
> > + arg->rule = rule;
> > + goto out;
> > + }
> > + }
> > +
> > + err = -ENETUNREACH;
> > +out:
> > + rcu_read_unlock();
>
> rcu_read_unlock might get called multiple times in the list iteration
> and once again here.
Yes, the rcu_read_unlock() in the list iteration is misplaced, it
shouldn't be there. Besides the unbalanced lock/unlocks it suffers from
the general issue described below
As a somewhat related note, I've just digged a bit through RCU land,
talked to dipankar and mckenney, and discovered that rcu_read_lock() /
rcu_read_unlock() aren't strictly needed in softirqs since preempt is
already disabled in softirqs. This means that you can use the result of
the rcu read-side critical outside of the rcu_read_lock() /
rcu_read_unlock() section. BUT this changes with the -rt kernel where
softirqs are preemptable and where rcu_read_lock() / rcu_read_unlock()
doesn't disable/enable preempt anymore, which means the rcu read-side
critical section is also preemptable. This means that we can get
preempted in the read-side critical section but the resulting grace
period won't occur until rcu_read_unlock() is called, which means that
using results of an read-side critical section outside of the critical
section is just not going to work in softirqs in -rt kernels.
I'm sure Ingo has reviewed the RCU usage in softirqs but I don't know if
there's been any changes in this area after his review.
--
/Martin
signature.asc
Description: This is a digitally signed message part
