Re: [PATCH net v2 2/2] selftests: drv-net: psp: test PSP and TCP ULP mutual exclusion

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Wed Sep 16, 2026 at 7:13 PM EDT, netdev-bot+sashiko wrote:
> 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]
>

I understand the point, but I'm not sure this really worth the change. I
considered checking for specific error codes here, but decided against
it because maybe that would be end up being brittle if someone in the
future changed or added error codes. Aside from ENOENT when the feature
is not available, I think the only other error this could throw is
ENOMEM.

> 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 -

The code in question can't take this branch as it is now, and if it did,
a skip seems arguably more appropriate to me, because otherwise how
would we even interpret the result of the test.

> 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()
>
> [ ... ]