On 6/20/16 12:30 AM, Lennert Buytenhek wrote:
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. ;-)
__neigh_lookup_noref expects bh level protection. Since the if block in neigh_xmit requires the locking seems like this the appropriate place for it.
Want me to send a patch moving it to neigh_xmit() ?
Roopa/Robert: agree?
