Re: [PATCH net-next 3/3] selftests: net: getsockopt_iter: cover tcp

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In tools/testing/selftests/net/getsockopt_iter.c, the test
  tcp/nodelay_undersize_clamped is commented "Unlike IPv4, TCP clamps…
- [Low] In tools/testing/selftests/net/getsockopt_iter.c, the test
  tcp/zerocopy_receive_oversize_clamped is commented "A larger struct is…

--

selftests: net: getsockopt_iter: cover tcp

This commit adds a tcp fixture to the getsockopt_iter selftest. The fixture
sets up a loopback connection with TCP_SAVE_SYN on the listener. The new
tests cover do_tcp_getsockopt() clamping, zero-length replies, the
TCP_SAVED_SYN length-then-EINVAL path, and the TCP_ZEROCOPY_RECEIVE struct
size handling.

> diff --git a/tools/testing/selftests/net/getsockopt_iter.c b/tools/testing/selftests/net/getsockopt_iter.c
> index 2367fce226d4c..4e51c8aa6a855 100644
> --- a/tools/testing/selftests/net/getsockopt_iter.c
> +++ b/tools/testing/selftests/net/getsockopt_iter.c

[ ... ]

> +/* Unlike IPv4, TCP clamps the reply rather than shrinking it to a byte. */
> +TEST_F(tcp, nodelay_undersize_clamped)
> +{
> +	socklen_t optlen = 1;
> +	int val = 0;
> +
> +	ASSERT_EQ(0, getsockopt(self->fd, IPPROTO_TCP, TCP_NODELAY, &val,
> +				&optlen));
> +	ASSERT_EQ(1, optlen);
> +}

[Severity: Low]
Does this test actually show the difference the comment describes?

With optlen = 1, both paths report the same length. do_tcp_getsockopt()
clamps:

net/ipv4/tcp.c:do_tcp_getsockopt() {
    ...
	len = min_t(unsigned int, len, sizeof(int));
    ...
}

and do_ip_getsockopt() also ends up with optlen = 1:

net/ipv4/ip_sockglue.c:do_ip_getsockopt() {
    ...
	if (len < sizeof(int) && len > 0 && val >= 0 && val <= 255) {
		unsigned char ucval = (unsigned char)val;

		sopt->optlen = 1;
    ...
}

So if TCP started shrinking sub-int replies to a single byte the way SOL_IP
does, this test would still pass.

The ip/ttl_single_byte and ipv6/hops_sub_int tests in the same file use a
3-byte buffer, which does tell the two apart. Would optlen = 2 or 3, with a
check that optlen is unchanged, match the intent of the comment better?

[ ... ]

> +/* A larger struct is cut down to the kernel's, if its tail is zero. */
> +TEST_F(tcp, zerocopy_receive_oversize_clamped)
> +{
> +	char buf[256] = {};
> +	socklen_t optlen = sizeof(buf);
> +
> +	ASSERT_EQ(0, getsockopt(self->fd, IPPROTO_TCP, TCP_ZEROCOPY_RECEIVE,
> +				buf, &optlen));
> +	ASSERT_GE(optlen, sizeof(struct tcp_zerocopy_receive));
> +	ASSERT_LT(optlen, sizeof(buf));
> +}

[Severity: Low]
Should this assert the exact clamped length? When the tail is zero, the
kernel sets optlen to the size of its own struct:

net/ipv4/tcp.c:do_tcp_getsockopt() {
    ...
	if (unlikely(len > sizeof(zc))) {
		...
		len = sizeof(zc);
		sopt->optlen = len;
    ...
}

That is the uapi struct tcp_zerocopy_receive from include/uapi/linux/tcp.h,
which is 64 bytes.

The test includes netinet/tcp.h, so sizeof(struct tcp_zerocopy_receive) here
is the 16-byte glibc struct the commit message mentions. That means any
optlen from 16 to 255 passes. A clamp to the wrong size, such as one of the
older 16 or 40 byte layouts, would not be caught.

raw/icmpfilter_oversize_clamped in the same file uses ASSERT_EQ against the
exact struct size. Could this test do the same with the kernel struct size
(64)?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-getsockopt_phase7-v1-0-1eb759dd0929%40debian.org