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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+-cjHze1yiFZKr-cCGG7Fh4gb9NZnS1u4u_77bG2Mf6Q@mail.gmail.com>
Date: Thu, 18 Apr 2024 10:03:51 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org, 
	Neal Cardwell <ncardwell@...gle.com>, Dragos Tatulea <dtatulea@...dia.com>, eric.dumazet@...il.com, 
	Maciej Żenczykowski <maze@...gle.com>, 
	Willem de Bruijn <willemb@...gle.com>, Shachar Kagan <skagan@...dia.com>
Subject: Re: [PATCH net-next 1/2] tcp: conditionally call ip_icmp_error() from tcp_v4_err()

On Thu, Apr 18, 2024 at 10:02 AM Paolo Abeni <pabeni@...hat.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@...gle.com>
> > Tested-by: Dragos Tatulea <dtatulea@...dia.com>
> > Cc: Dragos Tatulea <dtatulea@...dia.com>
> > Cc: Maciej Żenczykowski <maze@...gle.com>
> > Cc: Willem de Bruijn <willemb@...gle.com>
> > Cc: Neal Cardwell <ncardwell@...gle.com>
> > Cc: Shachar Kagan <skagan@...dia.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 !