Re: [PATCH] tcp: probe for SO_PEEK_OFF both in tcpv4 and tcp6

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 20 Jul 2024 09:54:53 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> The recently added socket option SO_PEEK_OFF is not supported for
> TCP/IPv6 sockets. Until we get that support into the kernel we need to
> test for support in both protocols to set the global 'peek_offset_cap´
> to true.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  tcp.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index c5431f1..32026ca 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2717,6 +2717,28 @@ static void tcp_sock_refill_init(const struct ctx *c)
>  	}
>  }
>  
> +/**
> + * tcp_probe_peek_offset_cap() - Check if SO_PEEK_OFF is supported by kernel
> + * @af:		Address family, IPv4 or IPv6
> + *
> + * Return: true if supported, false otherwise
> + */
> +bool tcp_probe_peek_offset_cap(int af)
> +{
> +	bool ret = false;
> +	int s, optv = 0;
> +
> +	s = socket(af, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
> +	if (s < 0) {
> +		warn_perror("Temporary TCP socket creation failed");
> +	} else {
> +		if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
> +			ret = true;
> +		close(s);
> +	}
> +	return ret;
> +}
> +
>  /**
>   * tcp_init() - Get initial sequence, hash secret, initialise per-socket data
>   * @c:		Execution context
> @@ -2725,8 +2747,7 @@ static void tcp_sock_refill_init(const struct ctx *c)
>   */
>  int tcp_init(struct ctx *c)
>  {
> -	unsigned int b, optv = 0;
> -	int s;
> +	unsigned int b;
>  
>  	ASSERT(!c->no_tcp);
>  
> @@ -2752,15 +2773,8 @@ int tcp_init(struct ctx *c)
>  		NS_CALL(tcp_ns_socks_init, c);
>  	}
>  
> -	/* Probe for SO_PEEK_OFF support */
> -	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
> -	if (s < 0) {
> -		warn_perror("Temporary TCP socket creation failed");
> -	} else {
> -		if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
> -			peek_offset_cap = true;
> -		close(s);
> -	}
> +	peek_offset_cap = tcp_probe_peek_offset_cap(AF_INET) &&
> +		tcp_probe_peek_offset_cap(AF_INET6);

I think we shouldn't probe for IPv4 SO_PEEK_OFF support if we're not
interested in it, and the same applies for IPv6: those two checks
should depend on whether c->ifi4 and c->ifi6 are set (following the
same logic as tcp_sock_init()).

In practice, since you just submitted the fix to have SO_PEEK_OFF
support also for IPv6 sockets, it doesn't matter so much, but it might
still be relevant for users who will stick to 6.9 and 6.10 kernel
versions for a while, for whatever reason.

Eventually, we might want to support IPv6 operation on IPv4-only hosts,
maybe even with default options, making this even less relevant.

But the day we get to it, it should be simpler to just replace all the
checks of c->ifi4 / c->ifi6 used to represent IPv4 / IPv6 support,
rather than replacing slightly different bits of logic.

-- 
Stefano