Re: [PATCH net v5 6/8] selftests: drv-net: psp: test ipv6 tcp socket with extension headers

[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 data_send_ipv6_exthdr()
  (tools/testing/selftests/drivers/net/psp.py), the finally block runs…
- [Low] Concern: the commit message overstates coverage ('a variant for
  each extension header a socket can set') because there is no…

--

selftests: drv-net: psp: test ipv6 tcp socket with extension headers

This adds a PSP selftest that sends data on an IPv6 PSP TCP socket in
three phases: with no extension header, with one set (IPV6_DSTOPTS,
IPV6_HOPOPTS or IPV6_RTHDR), and after the option is removed. After each
phase it checks the device's tx-packets and a tc flower UDP counter on
the remote. It also adds a helper that waits for stats to quiesce, and
adds CONFIG_NET_ACT_GACT to the config.

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index 473500901879b..7e251439a2fd9 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py

[ ... ]

> @@ -658,6 +710,95 @@ def removal_device_bi(cfg):

[ ... ]

> +@ksft_variants(_ipv6_exthdr_variants())
> +def data_send_ipv6_exthdr(cfg, opt, val):
> +    """ Test that PSP drops IPv6 packets with an extension header """

[ ... ]

> +        # 2. With the option: dropped, not counted, and nothing reaches the
> +        # peer. The data stays queued.
> +        s.setsockopt(socket.IPPROTO_IPV6, opt, val(peer))

[ ... ]

> +    finally:
> +        # With the option, the queued data, the FIN and the ACKs would be
> +        # dropped, leaving an orphan holding the association
> +        s.setsockopt(socket.IPPROTO_IPV6, opt, b'')
> +        _close_psp_conn(cfg, s)

[Severity: Low]
What happens to this cleanup in data_send_ipv6_exthdr() when the test
runs without CAP_NET_RAW, for the dstopts and hopopts variants?

ipv6_set_opt_hdr() checks the capability before it reaches the
zero-length removal path. That means clearing the option with b'' can
also fail with EPERM:

net/ipv6/ipv6_sockglue.c:ipv6_set_opt_hdr() {
    ...
	/* hop-by-hop / destination options are privileged option */
	if (optname != IPV6_RTHDR && !sockopt_ns_capable(net->user_ns, CAP_NET_RAW))
		return -EPERM;

	/* remove any sticky options header with a zero option
	 * length, per RFC3542.
	 */
	if (optlen > 0) {
    ...
}

In that case the phase 2 setsockopt raises PermissionError first. Then
the setsockopt in the finally block raises a second PermissionError, so
_close_psp_conn() never runs.

When that happens, psp_responder never gets the "data close" command and
keeps its data socket open. The local PSP socket isn't closed explicitly
either, and the original exception ends up chained under the second one.
The following test cases share cfg.comm_sock and the responder state, so
they could be affected.

The rthdr variant isn't affected because IPV6_RTHDR skips the capability
check.

Could the option removal be guarded separately, for example with a
nested try/finally, so that _close_psp_conn() always runs?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111001831.3710881-1-toddfreed%40meta.com