| 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
| ||
|
Message-ID: <CANn89i+BKDL-BHqHyev9PAzbHqp8xhkC=4kZTB7vydcBVkc0Nw@mail.gmail.com>
Date: Thu, 18 Apr 2024 12:22:16 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: David Ahern <dsahern@...nel.org>, "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 12:15 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Thu, Apr 18, 2024 at 11:58 AM Paolo Abeni <pabeni@...hat.com> wrote:
> >
> > On Thu, 2024-04-18 at 11:26 +0200, Eric Dumazet wrote:
> > > On Thu, Apr 18, 2024 at 10:03 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > > >
> > > > 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 !
> > >
> > > 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
> > }
>
> Thanks Paolo
>
> I found that the following patch is fixing the issue for me.
>
> diff --git a/tools/testing/selftests/net/nettest.c
> b/tools/testing/selftests/net/nettest.c
> index cd8a580974480212b45d86f35293b77f3d033473..ff25e53024ef6d4101f251c8a8a5e936e44e280f
> 100644
> --- a/tools/testing/selftests/net/nettest.c
> +++ b/tools/testing/selftests/net/nettest.c
> @@ -1744,6 +1744,7 @@ static int connectsock(void *addr, socklen_t
> alen, struct sock_args *args)
> if (args->bind_test_only)
> goto out;
>
> + set_recv_attr(sd, args->version);
> if (connect(sd, addr, alen) < 0) {
> if (errno != EINPROGRESS) {
> log_err_errno("Failed to connect to remote host");
When tracing nettest we now have EHOSTUNREACH
3343 setsockopt(3, SOL_SOCKET, SO_REUSEPORT, [1], 4) = 0 <0.000210>
3343 setsockopt(3, SOL_SOCKET, SO_BINDTODEVICE, "eth1\0", 5) = 0 <0.000170>
3343 setsockopt(3, SOL_IP, IP_PKTINFO, [1], 4) = 0 <0.000161>
3343 setsockopt(3, SOL_IP, IP_RECVERR, [1], 4) = 0 <0.000181>
3343 connect(3, {sa_family=AF_INET, sin_port=htons(12345),
sin_addr=inet_addr("172.16.2.1")}, 16) = -1 EINPROGRESS (Operation now
in progress) <0.000874>
3343 pselect6(1024, NULL, [3], NULL, {tv_sec=5, tv_nsec=0}, NULL) = 1
(out [3], left {tv_sec=1, tv_nsec=930762080}) <3.069673>
3343 getsockopt(3, SOL_SOCKET, SO_ERROR, [EHOSTUNREACH], [4]) = 0 <0.000270>
As mentioned in net/ipv4/icmp.c :
RFC 1122: 3.2.2.1 States that NET_UNREACH, HOST_UNREACH and SR_FAILED
MUST be considered 'transient errs'.
Maybe another way to fix nettest would be to change wait_for_connect()
to pass a non NULL fdset in 4th argument of select()
select(FD_SETSIZE, NULL, &wfd, NULL /* here */, tv);
Powered by blists - more mailing lists