Re: rcu locking issue in mpls output code?

[Lennert Buytenhek <buytenh@wantstofly.org>]

On Sun, Jun 19, 2016 at 08:19:20PM -0600, David Ahern wrote:

> > diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
> > index fb31aa8..802956b 100644
> > --- a/net/mpls/mpls_iptunnel.c
> > +++ b/net/mpls/mpls_iptunnel.c
> > @@ -105,12 +105,15 @@ static int mpls_output(struct net *net, struct sock *sk, struct sk_buff *skb)
> >  		bos = false;
> >  	}
> > 
> > +	rcu_read_lock_bh();
> >  	if (rt)
> >  		err = neigh_xmit(NEIGH_ARP_TABLE, out_dev, &rt->rt_gateway,
> >  				 skb);
> >  	else if (rt6)
> >  		err = neigh_xmit(NEIGH_ND_TABLE, out_dev, &rt6->rt6i_gateway,
> >  				 skb);
> > +	rcu_read_unlock_bh();
> > +
> >  	if (err)
> >  		net_dbg_ratelimited("%s: packet transmission failed: %d\n",
> >  				    __func__, err);
> > 
> 
> I think those need to be added to neigh_xmit in the
> 
> 	if (likely(index < NEIGH_NR_TABLES)) {
> 
> 	}

That'll force callers that don't need the extra protection (i.e.
mpls_forward(), since that always runs from softirq and it's enough
to protect the neigh state with rcu_read_lock() from softirq and we're
already running under rcu_read_lock() when we get to neigh_xmit()) to
eat the useless overhead of an extra rcu_read_{,un}lock_bh() pair, but
sure, functionally that's correct, I think, and in my workload I don't
care about MPLS forwarding performance anyway. ;-)

Want me to send a patch moving it to neigh_xmit() ?

Thank you for having a look!


Cheers,
Lennert