Re: [PATCH net-next 1/2] tcp: conditionally call ip_icmp_error() from tcp_v4_err()

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2024-04-18 at 11:26 +0200, Eric Dumazet wrote:
> On Thu, Apr 18, 2024 at 10:03 AM Eric Dumazet <edumazet@google.com> wrote:
> > 
> > On Thu, Apr 18, 2024 at 10:02 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > 
> > > Hi,
> > > 
> > > On Wed, 2024-04-17 at 16:57 +0000, Eric Dumazet wrote:
> > > > Blamed commit claimed in its changelog that the new functionality
> > > > was guarded by IP_RECVERR/IPV6_RECVERR :
> > > > 
> > > >     Note that applications need to set IP_RECVERR/IPV6_RECVERR option to
> > > >     enable this feature, and that the error message is only queued
> > > >     while in SYN_SNT state.
> > > > 
> > > > This was true only for IPv6, because ipv6_icmp_error() has
> > > > the following check:
> > > > 
> > > > if (!inet6_test_bit(RECVERR6, sk))
> > > >     return;
> > > > 
> > > > Other callers check IP_RECVERR by themselves, it is unclear
> > > > if we could factorize these checks in ip_icmp_error()
> > > > 
> > > > For stable backports, I chose to add the missing check in tcp_v4_err()
> > > > 
> > > > We think this missing check was the root cause for commit
> > > > 0a8de364ff7a ("tcp: no longer abort SYN_SENT when receiving
> > > > some ICMP") breakage, leading to a revert.
> > > > 
> > > > Many thanks to Dragos Tatulea for conducting the investigations.
> > > > 
> > > > As Jakub said :
> > > > 
> > > >     The suspicion is that SSH sees the ICMP report on the socket error queue
> > > >     and tries to connect() again, but due to the patch the socket isn't
> > > >     disconnected, so it gets EALREADY, and throws its hands up...
> > > > 
> > > >     The error bubbles up to Vagrant which also becomes unhappy.
> > > > 
> > > >     Can we skip the call to ip_icmp_error() for non-fatal ICMP errors?
> > > > 
> > > > Fixes: 45af29ca761c ("tcp: allow traceroute -Mtcp for unpriv users")
> > > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > > Tested-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > Cc: Dragos Tatulea <dtatulea@nvidia.com>
> > > > Cc: Maciej Żenczykowski <maze@google.com>
> > > > Cc: Willem de Bruijn <willemb@google.com>
> > > > Cc: Neal Cardwell <ncardwell@google.com>
> > > > Cc: Shachar Kagan <skagan@nvidia.com>
> > > > ---
> > > >  net/ipv4/tcp_ipv4.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > > > index 88c83ac4212957f19efad0f967952d2502bdbc7f..a717db99972d977a64178d7ed1109325d64a6d51 100644
> > > > --- a/net/ipv4/tcp_ipv4.c
> > > > +++ b/net/ipv4/tcp_ipv4.c
> > > > @@ -602,7 +602,8 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
> > > >               if (fastopen && !fastopen->sk)
> > > >                       break;
> > > > 
> > > > -             ip_icmp_error(sk, skb, err, th->dest, info, (u8 *)th);
> > > > +             if (inet_test_bit(RECVERR, sk))
> > > > +                     ip_icmp_error(sk, skb, err, th->dest, info, (u8 *)th);
> > > > 
> > > >               if (!sock_owned_by_user(sk)) {
> > > >                       WRITE_ONCE(sk->sk_err, err);
> > > 
> > > We have a fcnal-test.sh self-test failure:
> > > 
> > > https://netdev.bots.linux.dev/contest.html?branch=net-next-2024-04-18--06-00&test=fcnal-test-sh
> > > 
> > > that I suspect are related to this patch (or the following one): the
> > > test case creates a TCP connection on loopback and this is the only
> > > patchseries touching the related code, included in the relevant patch
> > > burst.
> > > 
> > > Could you please have a look?
> > 
> > Sure, thanks Paolo !
> 
> First patch is fine, I see no failure from fcnal-test.sh (as I would expect)
> 
> For the second one, I am not familiar enough with this very slow test
> suite (all these "sleep 1" ... oh well)

@David, some of them could be replaced with loopy_wait calls

> I guess "failing tests" depended on TCP connect() to immediately abort
> on one ICMP message,
> depending on old kernel behavior.
> 
> I do not know how to launch a subset of the tests, and trace these.
> 
> "./fcnal-test.sh -t ipv4_tcp" alone takes more than 9 minutes [1] in a
> VM running a non debug kernel :/
> 
> David, do you have an idea how to proceed ?

One very dumb thing I do in that cases is commenting out the other
tests, something alike (completely untested!):
---
diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
index 386ebd829df5..494932aa99b2 100755
--- a/tools/testing/selftests/net/fcnal-test.sh
+++ b/tools/testing/selftests/net/fcnal-test.sh
@@ -1186,6 +1186,7 @@ ipv4_tcp_novrf()
 {
 	local a
 
+if false; then
 	#
 	# server tests
 	#
@@ -1271,6 +1272,7 @@ ipv4_tcp_novrf()
 		log_test_addr ${a} $? 1 "Device server, unbound client, local connection"
 	done
 
+fi
 	a=${NSA_IP}
 	log_start
 	run_cmd nettest -s &
@@ -1487,12 +1489,14 @@ ipv4_tcp()
 	set_sysctl net.ipv4.tcp_l3mdev_accept=0
 	ipv4_tcp_novrf
 	log_subsection "tcp_l3mdev_accept enabled"
+if false; then
 	set_sysctl net.ipv4.tcp_l3mdev_accept=1
 	ipv4_tcp_novrf
 
 	log_subsection "With VRF"
 	setup "yes"
 	ipv4_tcp_vrf
+fi
 }
 
 ################################################################################