Re: [PATCH net v2 2/2] selftests: drv-net: psp: test ipv6 tcp socket with extension headers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In tools/testing/selftests/drivers/net/psp.py,
  data_send_ipv6_exthdr() is supposed to skip when the remote UDP filter…
- [Low] In tools/testing/selftests/drivers/net/psp.py, the docstring of
  _remote_udp_from() describes a counter only: "Count what reaches the…

--

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

This adds a PSP selftest that sends on an IPv6 TCP socket in three
phases: no options, then destination options, hop-by-hop options and a
segment routing header, then with each option removed. After each phase
it checks the device's tx-packets and the data the peer received. It
also installs a tc flower filter on the peer to confirm no plain UDP
arrives, and skips the test if that filter can't be installed.

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index a5b1e14f120f0..47e8cb19f3763 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py

[ ... ]

> +def _remote_udp_from(cfg, src, dst):
> +    """
> +    Count what reaches the remote from @src to @dst as UDP, with a software
> +    filter on its ingress (offloaded filter counters are refreshed
> +    periodically). Returns a function reading the count, or None if the
> +    filter can't be installed.
> +    """
> +    flt = (f"dev {cfg.remote_ifname} ingress protocol ipv6 pref 1 "
> +           f"handle 0x5053 flower")
> +    try:
> +        qdiscs = tc(f"qdisc show dev {cfg.remote_ifname}", json=True,
> +                    host=cfg.remote)
> +        if not any(q['kind'] == 'clsact' for q in qdiscs):
> +            tc(f"qdisc add dev {cfg.remote_ifname} clsact", host=cfg.remote)
> +            defer(tc, f"qdisc del dev {cfg.remote_ifname} clsact",
> +                  host=cfg.remote)
> +        tc(f"filter add {flt} skip_hw src_ip {src} dst_ip {dst} "
> +           f"ip_proto udp action drop", host=cfg.remote)

[Severity: Low]
This isn't a bug, but the docstring above says the helper counts what
reaches the remote, and the commit message says:

  "a software filter on the peer also counts udp packets from the
  socket's address to the peer's"

The filter is installed with "action drop", so matching UDP packets are
dropped at the peer's clsact ingress for as long as the test runs. That
is also why CONFIG_NET_ACT_GACT was added to the selftest config.

Should the docstring mention the drop? Or would "action pass" be enough
if counting is all that's needed?

[ ... ]

> +def data_send_ipv6_exthdr(cfg):
> +    """ Test PSP on IPv6 packets with extension headers """
> +    cfg.require_ipver("6")
> +    _init_psp_dev(cfg)
> +
> +    s = _make_psp_conn(cfg, 0, "6")
> +    try:
> +        # In happy path, PSP traffic arrives at the remote decrypted as TCP
> +        remote_udp = _remote_udp_from(cfg, s.getsockname()[0],
> +                                      s.getpeername()[0])
> +        if not remote_udp:
> +            raise KsftSkipEx("can't count UDP on the remote")

[Severity: Medium]
Does this skip path really get reported as a skip?

At this point _make_psp_conn() has opened the data connection, but
_spi_xchg() hasn't sent the client's key yet. On the responder,
run_session() calls conn_setup_psp() synchronously for the new PSP data
socket, and that function blocks waiting for the key:

tools/testing/selftests/drivers/net/psp_responder.c:conn_setup_psp() {
    ...
	send(data_sock, info, sz, MSG_WAITALL);
	psp_rx_assoc_rsp_free(rsp);

	sz = recv(data_sock, info, sz, MSG_WAITALL);
    ...
}

After KsftSkipEx is raised, the finally block at the end of
data_send_ipv6_exthdr() runs:

data_send_ipv6_exthdr()
  _close_psp_conn()
    _close_conn()
      _send_with_ack(cfg, b'data close\0')
        cfg.comm_sock.recv(4)

The responder is single-threaded and still blocked in recv() on the data
socket, so it can't handle 'data close'. cfg.comm_sock has a 1 second
timeout, so recv(4) raises TimeoutError. That exception replaces the
KsftSkipEx, and ksft_run() counts the case as a failure instead of a
skip.

The s.close() in _close_conn() is also skipped. When the responder
later sees EOF, it handles the queued 'data close' with no data socket
and sends an unsolicited 'err\0' on the control channel.

This path is hit whenever any of the remote tc commands fail. For
example, cls_flower or act_gact may be missing on the remote, an
ingress qdisc may already be present, or the pref/handle may already be
in use.

Could the filter be installed before _make_psp_conn()? Or could the skip
path close the data socket without going through _close_psp_conn()?

[ ... ]

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