Re: [PATCH net v3 2/2] selftests/net: psp: test bidirectional data transfer

[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] _recv_careful() in tools/testing/selftests/drivers/net/psp.py,
  added by this patch, does not check for end-of-stream.

--

selftests/net: psp: test bidirectional data transfer

This patch makes the PSP selftests check data in both directions. The
test socket now asks psp_responder for a "data echo" and checks that the
PSP-encrypted reply arrives. The check is added to _data_basic_send() and
_data_basic_send_netkit_psp_assoc().

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index 473500901879b..c114d2245135b 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py
> @@ -120,6 +120,26 @@ def _check_data_rx(cfg, exp_len):
>      ksft_eq(read_len, exp_len)
>  
>  
> +def _recv_careful(s, target, timeout=2):
> +    """Read exactly target bytes, tolerating short reads"""
> +    data = b''
> +    end = time.monotonic() + timeout
> +    while time.monotonic() < end:
> +        try:
> +            data += s.recv(target - len(data), socket.MSG_DONTWAIT)
> +            if len(data) == target:
> +                return data
> +        except BlockingIOError:
> +            time.sleep(0.001)
> +    raise KsftFailEx(f"short read, got {len(data)} of {target} bytes")

[Severity: Low]
What happens in _recv_careful() if the peer has already sent a FIN on the
data socket?

In that case s.recv(..., socket.MSG_DONTWAIT) returns b'' and does not
raise. data stays the same, the len(data) == target check stays false, and
the loop never reaches time.sleep(0.001), because the sleep is only in the
BlockingIOError branch.

So the loop calls recv() with no delay for the full 2 second timeout. It
then reports a generic "short read, got N of 5 bytes" instead of saying the
peer closed the connection.

This looks reachable. run_session() in psp_responder.c acks "data echo"
even when it has no data socket:

    if (cmd("data echo")) {
        if (data_sock >= 0)
            send(data_sock, "echo", 5, MSG_WAITALL);
        else
            fprintf(stderr, "WARN: echo but no data sock\n");
        send_ack(comm_sock);
    }

The responder can also close an established data socket on its "new data
sock but old one still here" accept path, which sends a FIN to the test
socket.

Would it make sense to check the chunk returned by recv() and raise
KsftFailEx right away, with a message like "peer closed data socket", when
it is empty?

> +
> +
> +def _req_echo(cfg, s):
> +    """Ask the peer to echo, and check the reply arrives intact"""
> +    _send_with_ack(cfg, b'data echo\0')
> +    ksft_eq(_recv_careful(s, 5), b'echo\0')

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106182007.374660-1-weiwan%40google.com