Message-ID: <20241115191029.37456575@elisabeth>
Date: Fri, 15 Nov 2024 19:10:29 +0100
From: Stefano Brivio <sbrivio@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org, Mike Manning
 <mvrmanning@...il.com>, David Gibson <david@...son.dropbear.id.au>, Ed
 Santiago <santiago@...hat.com>, Paul Holzinger <pholzing@...hat.com>
Subject: Re: [PATCH RFC net 2/2] datagram, udp: Set local address and rehash
 socket atomically against lookup

On Fri, 15 Nov 2024 12:50:22 -0500
Willem de Bruijn <willemdebruijn.kernel@...il.com> wrote:

> Stefano Brivio wrote:
> > If a UDP socket changes its local address while it's receiving
> > datagrams, as a result of connect(), there is a period during which
> > a lookup operation might fail to find it, after the address is changed
> > but before the secondary hash (port and address) is updated.
> > 
> > Secondary hash chains were introduced by commit 30fff9231fad ("udp:
> > bind() optimisation") and, as a result, a rehash operation became
> > needed to make a bound socket reachable again after a connect().
> > 
> > This operation was introduced by commit 719f835853a9 ("udp: add
> > rehash on connect()") which isn't however a complete fix: the
> > socket will be found once the rehashing completes, but not while
> > it's pending.
> > 
> > This is noticeable with a socat(1) server in UDP4-LISTEN mode, and a
> > client sending datagrams to it. After the server receives the first
> > datagram (cf. _xioopen_ipdgram_listen()), it issues a connect() to
> > the address of the sender, in order to set up a directed flow.
> > 
> > Now, if the client, running on a different CPU thread, happens to
> > send a (subsequent) datagram while the server's socket changes its
> > address, but is not rehashed yet, this will result in a failed
> > lookup and a port unreachable error delivered to the client, as
> > apparent from the following reproducer:
> > 
> >   LEN=$(($(cat /proc/sys/net/core/wmem_default) / 4))
> >   dd if=/dev/urandom bs=1 count=${LEN} of=tmp.in
> > 
> >   while :; do
> >   	taskset -c 1 socat UDP4-LISTEN:1337,null-eof OPEN:tmp.out,create,trunc &
> >   	sleep 0.1 || sleep 1
> >   	taskset -c 2 socat OPEN:tmp.in UDP4:localhost:1337,shut-null
> >   	wait
> >   done
> > 
> > where the client will eventually get ECONNREFUSED on a write()
> > (typically the second or third one of a given iteration):
> > 
> >   2024/11/13 21:28:23 socat[46901] E write(6, 0x556db2e3c000, 8192): Connection refused
> > 
> > This issue was first observed as a seldom failure in Podman's tests
> > checking UDP functionality while using pasta(1) to connect the
> > container's network namespace, which leads us to a reproducer with
> > the lookup error resulting in an ICMP packet on a tap device:
> > 
> >   LOCAL_ADDR="$(ip -j -4 addr show|jq -rM '.[] | .addr_info[0] | select(.scope == "global").local')"
> > 
> >   while :; do
> >   	./pasta --config-net -p pasta.pcap -u 1337 socat UDP4-LISTEN:1337,null-eof OPEN:tmp.out,create,trunc &
> >   	sleep 0.2 || sleep 1
> >   	socat OPEN:tmp.in UDP4:${LOCAL_ADDR}:1337,shut-null
> >   	wait
> >   	cmp tmp.in tmp.out
> >   done
> > 
> > Once this fails:
> > 
> >   tmp.in tmp.out differ: char 8193, line 29
> > 
> > we can finally have a look at what's going on:
> > 
> >   $ tshark -r pasta.pcap
> >       1   0.000000           :: ? ff02::16     ICMPv6 110 Multicast Listener Report Message v2
> >       2   0.168690 88.198.0.161 ? 88.198.0.164 UDP 8234 60260 ? 1337 Len=8192
> >       3   0.168767 88.198.0.161 ? 88.198.0.164 UDP 8234 60260 ? 1337 Len=8192
> >       4   0.168806 88.198.0.161 ? 88.198.0.164 UDP 8234 60260 ? 1337 Len=8192
> >       5   0.168827 c6:47:05:8d:dc:04 ? Broadcast    ARP 42 Who has 88.198.0.161? Tell 88.198.0.164
> >       6   0.168851 9a:55:9a:55:9a:55 ? c6:47:05:8d:dc:04 ARP 42 88.198.0.161 is at 9a:55:9a:55:9a:55
> >       7   0.168875 88.198.0.161 ? 88.198.0.164 UDP 8234 60260 ? 1337 Len=8192
> >       8   0.168896 88.198.0.164 ? 88.198.0.161 ICMP 590 Destination unreachable (Port unreachable)
> >       9   0.168926 88.198.0.161 ? 88.198.0.164 UDP 8234 60260 ? 1337 Len=8192
> >      10   0.168959 88.198.0.161 ? 88.198.0.164 UDP 8234 60260 ? 1337 Len=8192
> >      11   0.168989 88.198.0.161 ? 88.198.0.164 UDP 4138 60260 ? 1337 Len=4096
> >      12   0.169010 88.198.0.161 ? 88.198.0.164 UDP 42 60260 ? 1337 Len=0
> > 
> > On the third datagram received, the network namespace of the container
> > initiates an ARP lookup to deliver the ICMP message.
> > 
> > In another variant of this reproducer, starting the client with:
> > 
> >   strace -f pasta --config-net -u 1337 socat UDP4-LISTEN:1337,null-eof OPEN:tmp.out,create,trunc 2>strace.log &
> > 
> > and connecting to the socat server using a loopback address:
> > 
> >   socat OPEN:tmp.in UDP4:localhost:1337,shut-null
> > 
> > we can more clearly observe a sendmmsg() call failing after the
> > first datagram is delivered:
> > 
> >   [pid 278012] connect(173, 0x7fff96c95fc0, 16) = 0
> >   [...]
> >   [pid 278012] recvmmsg(173, 0x7fff96c96020, 1024, MSG_DONTWAIT, NULL) = -1 EAGAIN (Resource temporarily unavailable)
> >   [pid 278012] sendmmsg(173, 0x561c5ad0a720, 1, MSG_NOSIGNAL) = 1
> >   [...]
> >   [pid 278012] sendmmsg(173, 0x561c5ad0a720, 1, MSG_NOSIGNAL) = -1 ECONNREFUSED (Connection refused)
> > 
> > and, somewhat confusingly, after a connect() on the same socket
> > succeeded.
> > 
> > To fix this, replace the rehash operation by a set_rcv_saddr()
> > callback holding the spinlock on the primary hash chain, just like
> > the rehash operation used to do, but also setting the address while
> > holding the spinlock.
> > 
> > To make this atomic against the lookup operation, also acquire the
> > spinlock on the primary chain there.
> > 
> > This results in some awkwardness at a caller site, specifically
> > sock_bindtoindex_locked(), where we really just need to rehash the
> > socket without changing its address. With the new operation, we now
> > need to forcibly set the current address again.
> > 
> > On the other hand, this appears more elegant than alternatives such
> > as fetching the spinlock reference in ip4_datagram_connect() and
> > ip6_datagram_conect(), and keeping the rehash operation around for
> > a single user also seems a tad overkill.
> > 
> > Reported-by: Ed Santiago <santiago@...hat.com>
> > Link: https://github.com/containers/podman/issues/24147
> > Analysed-by: David Gibson <david@...son.dropbear.id.au>
> > Fixes: 30fff9231fad ("udp: bind() optimisation")
> > Signed-off-by: Stefano Brivio <sbrivio@...hat.com>  
> 
> Thanks for the detailed well written explanation of the
> condition, and the repro.
> 
> The current patch is quite complex, making it very hard to backport to
> stable kernels.

It shouldn't be backported to stable, the issue has been there since
2009, so I don't think anybody will be offended if we don't.

> Let's investigate if this issue can be mitigated with a much smaller
> patch.

One thing I tried was to open-code the current rehash operation in the
datagram connect stuff. It works and I can submit that version, but if
you have one copy for IPv4 and one for IPv6 the change gets actually
bigger.

I would rather submit the whole thing for net-next instead. Does that
make sense?

-- 
Stefano

Powered by blists - more mailing lists