Hi!
While trying to chase down a memory corruption issue that only occurs
when originating large amounts of MPLS tagged IP traffic, I came across
something in the MPLS output code for which I'm not entirely sure that
it's correct.
Specifically, there is the code path dst_output() -> lwtunnel_output()
-> mpls_output() -> neigh_xmit() -> ___neigh_lookup_noref(), where the
latter accesses a RCU-bh protected struct neigh_table pointer, but there
is no RCU-bh protection being arranged anywhere in this call chain.
Since this is locally generated IP traffic, we're running in process
context, and while lwtunnel_output() holds rcu_read_lock() across its
call to lwtunnel_encap_ops::output() (which is mpls_output() here),
nothing in the chain disables BHs, and in RCU-bh, the completion of a
softirq signals the end of any pending read-side critical sections,
and BHs can preempt this call chain at any time because it runs with
hardirqs and softirqs both enabled, so that would mean that neighbour
table entries can be zapped at any time even while we hold
rcu_read_lock(). I think.
The mpls_forward() path doesn't seem susceptible to the same issue,
as it runs from softirq, where rcu_read_lock() suffices, so I figured
that mpls_output() would be a good place to deal with this and that
something like the patch below would do the trick. I can't say yet if
this makes my memory corruption issues go away, as they don't reproduce
that easily, but I'll keep testing. Any thoughts so far?
Thanks,
Lennert
@@ -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);elseif(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
@@ -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);elseif(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() ?
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.
quoted
Want me to send a patch moving it to neigh_xmit() ?
Roopa/Robert: agree?
yes, seems like an appropriate place for it. provided it does not add
unnecessary overhead for others.
But then neigh_xmit seems to be only called from mpls_output and mpls_forward.
thanks!
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.
quoted
Want me to send a patch moving it to neigh_xmit() ?
Roopa/Robert: agree?
yes, seems like an appropriate place for it. provided it does not add
unnecessary overhead for others.
But then neigh_xmit seems to be only called from mpls_output and mpls_forward.
On Mon, Jun 20, 2016 at 10:38:39AM -0600, David Ahern wrote:
quoted
OK, patch coming up. Thanks!
can you build a kernel with rcu debugging enabled as well and run
it through your tests?
git HEAD with CONFIG_DEBUG_RT_MUTEXES=y CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y CONFIG_PROVE_RCU=y gives me a lockdep splat on the
machine under my desk when I cause mpls_output() to be called.
The script I use for that is this one -- it creates a namespace that
accepts MPLS tagged packets for one of its local IPs and then sends
an MPLS tagged packet into that namespace. If you run the script on
an unpatched kernel with lock debugging enabled, you should be able
to see the issue as well, the lockdep splat happens on the very first
packet.
=====
#!/bin/sh
ip link add tons type veth peer name tempitf
ifconfig tons 172.16.20.20 netmask 255.255.255.0
ip netns add ns1
ip netns exec ns1 ifconfig lo 127.0.0.1 up
ip link set tempitf netns ns1
ip netns exec ns1 ip link set tempitf name eth0
ip netns exec ns1 ifconfig eth0 172.16.20.21 netmask 255.255.255.0
modprobe mpls_iptunnel
ip route add 10.10.10.10/32 encap mpls 100 via inet 172.16.20.21
ip netns exec ns1 sysctl -w net.ipv4.conf.all.rp_filter=0
ip netns exec ns1 sysctl -w net.ipv4.conf.lo.rp_filter=0
ip netns exec ns1 sysctl -w net.mpls.conf.eth0.input=1
ip netns exec ns1 sysctl -w net.mpls.platform_labels=1000
ip netns exec ns1 ip addr add 10.10.10.10/32 dev lo
ip netns exec ns1 ip -f mpls route add 100 dev lo
ping -c 1 10.10.10.10
=====
The patch below (which I'll submit shortly with a proper commit
message) makes this lockdep splat go away.
Enabling lock/rcu debugging gives you a lockdep splat on the first
packet going out through mpls_output(), but then makes the packet
loss / memory corruption issue stop appearing, both on my local
space heater and on much more serious hardware, probably due to
timing differences.
But, with lock/rcu debugging disabled and the patch below included,
I don't see packet loss anymore in a production environment during a
test that would fairly reliably show it before.