On Fri, Oct 30, 2015, at 11:36, Florian Westphal wrote:
> Eric Dumazet <eric.duma...@gmail.com> wrote:
> > > Signed-off-by: Ani Sinha <a...@arista.com>
> > > ---
> > > net/ipv4/ipmr.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> > > index 866ee89..48df3cc 100644
> > > --- a/net/ipv4/ipmr.c
> > > +++ b/net/ipv4/ipmr.c
> > > @@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net,
> > > struct mr_table *mrt,
> > >
> > > rtnl_unicast(skb, net, NETLINK_CB(skb).portid);
> > > } else {
> > > + preempt_disable();
> > > ip_mr_forward(net, mrt, skb, c, 0);
> > > + preempt_enable();
> > > }
> > > }
> > > }
> >
> > I do not believe this fix is correct.
>
> Yes, sorry. I should have suggested local_bh_disable instead.
>
> > Better replace the
> > IP_INC_STATS_BH() by IP_INC_STATS()
> >
> > and IP_ADD_STATS_BH() by IP_ADD_STATS()
>
> Hmm, whats the rationale for this?
>
> Note that IP_ADD_STATS_BH in question is unconditional (not in
> error path). It seems that its virtually always called from softirq
> except in the setsockopt case.
The naming of the functions is bad if you compare them to e.g.
spin_lock_bh.
STATS_BH can only be used from bottom half and the normal ones (without
_BH) can be called from everywhere. It is a common pattern in the
kernel.
Eric's proposal is correct.
Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
