Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0
- [Medium] In tools/testing/selftests/drivers/net/psp.py, the new helper
`_require_tls_ulp()` (lines 338-344) wraps…
--
selftests: drv-net: psp: test PSP and TCP ULP mutual exclusion
This adds two psp.py cases that check PSP associations and TCP ULPs are
mutually exclusive in both orders, plus a _require_tls_ulp() helper that
skips when kTLS is unavailable, and enables CONFIG_TLS=y in the
drivers/net selftest config.
> diff --git a/tools/testing/selftests/drivers/net/psp.py
> b/tools/testing/selftests/drivers/net/psp.py
> index 315648a770d00..a5b1e14f120f0 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py
[ ... ]
> @@ -333,6 +335,50 @@ def assoc_version_mismatch(cfg):
> ksft_eq(the_exception.nl_msg.error, -errno.EINVAL)
>
>
> +def _require_tls_ulp():
> + with socket.create_server(("localhost", 0)) as srv, \
> + socket.create_connection(srv.getsockname()) as s:
> + try:
> + s.setsockopt(socket.SOL_TCP, TCP_ULP, b"tls")
> + except OSError as exc:
> + raise KsftSkipEx("kTLS not available") from exc
[Severity: Medium]
Should this handler be narrowed to the errnos that actually mean "no
kTLS"? As written, every OSError from the TCP_ULP setsockopt turns into
a skip, including -EINVAL, which is exactly the errno the kernel side of
this series returns:
net/ipv4/tcp_ulp.c:__tcp_set_ulp() {
...
err = -EINVAL;
if (sk_has_decrypt_user(sk))
goto out_err;
...
}
The probe socket here is a plain loopback TCP connection with no PSP
association, so the only legitimate reasons to skip are the "ULP unknown
/ TLS not built" errnos, ENOENT from __tcp_ulp_find_autoload() and
EOPNOTSUPP.
If sk_has_decrypt_user() ever returns true for an ordinary TCP socket -
and the parent commit anticipates growth there ("As other users of
skb->decrypted come along, they can be added to sk_has_decrypt_user()") -
the probe fails with EINVAL and the helper reports a skip.
Would restricting the skip to exc.errno in (errno.ENOENT,
errno.EOPNOTSUPP) and letting other errnos propagate keep the assertions
below load-bearing?
> +
> +
> +def assoc_psp_ulp_exclusive(cfg):
> + """ Test that a TCP ULP cannot be attached to a PSP socket """
> + _init_psp_dev(cfg)
> + _require_tls_ulp()
[Severity: Medium]
Since _require_tls_ulp() is the first gate in both assoc_psp_ulp_exclusive()
and assoc_ulp_psp_exclusive(), does an over-broad kernel-side
sk_has_decrypt_user() end up disabling both directions of the
mutual-exclusion check at once? The result would be two skips in CI rather
than a failure for the case these tests were added to catch.
> +
> + with _make_clr_conn(cfg) as s:
> + try:
> + cfg.pspnl.rx_assoc({"version": 0,
> + "dev-id": cfg.psp_dev_id,
> + "sock-fd": s.fileno()})
> + with ksft_raises(OSError) as cm:
> + s.setsockopt(socket.SOL_TCP, TCP_ULP, b"tls")
> + ksft_eq(cm.exception.errno, errno.EINVAL)
> + finally:
> + _close_conn(cfg, s)
> +
> +
> +def assoc_ulp_psp_exclusive(cfg):
> + """ Test that a PSP assoc cannot be added to a socket with a TCP ULP """
> + _init_psp_dev(cfg)
> + _require_tls_ulp()
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-psp-ktls-fix-v2-0-0eedc3b148ec%40gmail.com
