Re: [PATCH net-next v3 2/2] selftests/net/packetdrill: cover SO_RCVBUF vs the route window clamp

[netdev-bot+sashiko]

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] The new tests only cover the active-open side.
- [Low] Two of the new scripts use raw socket option numbers instead of
  their names.

--

selftests/net/packetdrill: cover SO_RCVBUF vs the route window clamp

This adds three packetdrill scripts. They check the window and window scale
sent on an active-open SYN when SO_RCVBUF, TCP_WINDOW_CLAMP and the route
RTAX_WINDOW metric interact. One script covers the deferred SYN that
TCP_FASTOPEN_CONNECT sends.

> diff --git a/tools/testing/selftests/net/packetdrill/tcp_fastopen_connect_rcvbuf_window.pkt b/tools/testing/selftests/net/packetdrill/tcp_fastopen_connect_rcvbuf_window.pkt
> new file mode 100644
> index 0000000000000..7892ac566e189
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp_fastopen_connect_rcvbuf_window.pkt
> @@ -0,0 +1,22 @@

[ ... ]

> +// optname 30 = TCP_FASTOPEN_CONNECT, 34 = TCP_FASTOPEN_NO_COOKIE on SOL_TCP.

[ ... ]

> +    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +   +0 setsockopt(3, SOL_TCP, 30, [1], 4) = 0
> +   +0 setsockopt(3, SOL_TCP, 34, [1], 4) = 0

[Severity: Low]
This isn't a bug, but could these lines use the symbolic names instead of
the raw optnames 30 and 34?

tcp_syscall_bad_arg_fastopen-invalid-buf-ptr.pkt in the same directory
already does this:

    +0 setsockopt(3, SOL_TCP, TCP_FASTOPEN_CONNECT, [1], 4) = 0

So the packetdrill these selftests run against knows at least that name.

The raw 10 in tcp_window_clamp_vs_rcvbuf.pkt has this comment:

    // TCP_WINDOW_CLAMP is optname 10 on SOL_TCP (this packetdrill build has no
    // symbol for it).

That comment describes a local packetdrill build, not something the
upstream selftest depends on. If the packetdrill used here really has no
TCP_WINDOW_CLAMP symbol, could the comment say that without referring to a
particular build?

The numeric values themselves are correct.

> +   +0 connect(3, ..., ...) = 0

[ ... ]

> diff --git a/tools/testing/selftests/net/packetdrill/tcp_window_clamp_vs_rcvbuf.pkt b/tools/testing/selftests/net/packetdrill/tcp_window_clamp_vs_rcvbuf.pkt
> new file mode 100644
> index 0000000000000..3d73178df6433
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp_window_clamp_vs_rcvbuf.pkt
> @@ -0,0 +1,24 @@

[ ... ]

> +    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +   +0 setsockopt(3, SOL_TCP, 10, [16384], 4) = 0
> +   +0 setsockopt(3, SOL_SOCKET, SO_RCVBUF, [1048576], 4) = 0
> +   +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> +   +0 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)

[Severity: Low]
All three new scripts call connect() and check only the outgoing SYN. Is
the passive side of the fix tested anywhere?

The fix earlier in this series ("tcp: keep the route window metric when
SO_RCVBUF is set before connect") also changes tcp_openreq_init_rwin() in
net/ipv4/tcp_minisocks.c. That function now picks the clamp based on the
listener's user-clamp bit:

    req->rsk_window_clamp = tp->window_clamp_user ? window_clamp :
                            dst_metric(dst, RTAX_WINDOW);

The fix's commit message says the change applies "on a listener" and to
the "SYN / SYN-ACK". It also says "a passive socket inherits the decision
from its listener".

No script in tools/testing/selftests/net/packetdrill/ sets up a listener
behind a route with a window metric and then checks the SYN-ACK.

Would it make sense to add listener versions of two cases: SO_RCVBUF
alone, and TCP_WINDOW_CLAMP followed by SO_RCVBUF?

The new reset rules also look untested. In do_tcp_setsockopt(),
TCP_WINDOW_CLAMP=0 now clears the bit:

    tp->window_clamp_user = !!val;

tcp_disconnect() clears it as well.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012132622.48550-1-jain.sm%40gmail.com