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

[Paolo Abeni <pabeni@redhat.com>]

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?

Thanks!

Paolo