diff mbox

net: hang in ip_finish_output

Message ID 1453134112.1223.221.camel@edumazet-glaptop2.roam.corp.google.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Jan. 18, 2016, 4:21 p.m. UTC 
On Sun, 2016-01-17 at 19:12 -0800, Eric Dumazet wrote:
> On Fri, 2016-01-15 at 23:29 -0800, Eric Dumazet wrote:
> > On Fri, 2016-01-15 at 19:20 -0500, Craig Gallek wrote:
> > 
> > > I wasn't able to reproduce this exact stack trace, but I was able to
> > > cause soft lockup messages with a fork bomb of your test program.  It
> > > is certainly related to my recent SO_REUSEPORT change (reverting it
> > > seems to fix the problem).  I haven't completely figured out the exact
> > > cause yet, though.  Could you please post your configuration and
> > > exactly how you are running this 'parallel loop'?
> > 
> > There is a problem in the lookup functions (udp4_lib_lookup2() &
> > __udp4_lib_lookup())
> > 
> > Because of RCU SLAB_DESTROY_BY_RCU semantics (check
> > Documentation/RCU/rculist_nulls.txt for some details), you should not
> > call reuseport_select_sock(sk, ...) without taking a stable reference on
> > the sk socket. (and checking the lookup keys again)
> > 
> > This is because sk could be freed, re-used by a totally different UDP
> > socket on a different port, and the incoming frame(s) could be delivered
> > on the wrong socket/channel/application :(
> > 
> > Note that we discussed some time ago to remove SLAB_DESTROY_BY_RCU for
> > UDP sockets (and freeing them after rcu grace period instead), so make
> > UDP rx path faster, as we would no longer need to increment/decrement
> > the socket refcount. This also would remove the added false sharing on
> > sk_refcnt for the case the UDP socket serves as a tunnel (up->encap_rcv
> > being non NULL)
> 
> Hmm... not it looks you do the lookup , refcnt change, re-lookup just
> fine.
> 
> The problem here is that UDP connected sockets update the
> sk->sk_incoming_cpu from __udp_queue_rcv_skb()
> 
> This means that we can find the first socket in hash table with a
> matching incoming cpu, and badness == high_score + 1
> 
> Then, the reuseport_select_sock() can selects another socket from the
> array (using bpf or the hash )
> 
> We do the atomic_inc_not_zero_hint() to update sk_refcnt on the new
> socket, then compute_score2() returns high_score (< badness)
> 
> So we loop back to the beginning of udp4_lib_lookup2(), and we loop
> forever (as long as the first socket in hash table has still this match
> about incoming cpu)
> 
> In short, the recent SO_REUSE_PORT changes are not compatible with the
> SO_INCOMING_CPU ones, if connected UDP sockets are used.
> 
> A fix could be to not check sk_incoming_cpu on connected sockets (this
> makes really little sense, as this option was meant to spread traffic on
> UDP _servers_ ). Also it collides with SO_REUSEPORT notion of a group of
> sockets having the same score.
> 
> Dmitry, could you test it ? I could not get the trace you reported.

BTW, it could be the bug is hard to trigger because of IP early demux :

When connected UDP sockets are used, __udp4_lib_demux_lookup() returns
first socket found in the hash chain, so all incoming messages should be
delivered on this socket. (The normal reuseport hash/bpf spread does not
happen)

So to trigger the bug more easily we can disable early demux :

echo 0 >/proc/sys/net/ipv4/ip_early_demux

We also should disallow ip early demux on SO_REUSEPORT UDP sockets.
diff mbox

Patch

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index dc45b538e237..55954094ab17 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2026,7 +2026,8 @@  static struct sock *__udp4_lib_demux_lookup(struct net *net,
 	result = NULL;
 	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
 		if (INET_MATCH(sk, net, acookie,
-			       rmt_addr, loc_addr, ports, dif))
+			       rmt_addr, loc_addr, ports, dif) &&
+		    !sk->sk_reuseport)
 			result = sk;
 		/* Only check first socket in chain */
 		break;