Re: [PATCH v4 net-next 12/15] selftests: xsk: prepare xskxceiver for endpoints on two hosts

[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] parse_command_line() does not fully validate the new numeric
  options.
- [Low] In the adjust-tail cases, RX can decide to skip only after the
  traffic has run: testapp_validate_rx_endpoint() clears…

--

selftests: xsk: prepare xskxceiver for endpoints on two hosts

This prepares xskxceiver to run a case with its two endpoints on two
hosts. It adds --queue, --max-frags and --timeout options, a SKIP message
on the control channel, and a wait for the link to come up once the
endpoints are in sync. It also changes the default case MTU to
ETH_DATA_LEN and restores the original MTU in cleanup_iface().

> diff --git a/tools/testing/selftests/net/lib/xsk/xsk_peer.c b/tools/testing/selftests/net/lib/xsk/xsk_peer.c
> index e3e353d6abd95..256e63196f923 100644
> --- a/tools/testing/selftests/net/lib/xsk/xsk_peer.c
> +++ b/tools/testing/selftests/net/lib/xsk/xsk_peer.c

[ ... ]

> @@ -162,6 +179,30 @@ void xsk_peer_abort(struct xsk_peer *peer)
>  		peer_send(peer, XSK_PEER_ABORT, 0);
>  }
>  
> +/*
> + * Tell the peer that this endpoint skips the case, so that it skips too.
> + * Closing with unread data, or getting data once closed, resets the
> + * connection and drops the SKIP if the peer has yet to acknowledge it, as
> + * when a NIC reset holds the link down. So read until the peer closes.
> + */
> +void xsk_peer_skip(struct xsk_peer *peer, int timeout_ms)

[Severity: Low]
Is "so that it skips too" accurate for the adjust-tail cases?

In those cases the RX endpoint decides to skip only after the traffic has
run. testapp_validate_rx_endpoint() clears the support flag:

    if (!is_adjust_tail_supported(ifobject->xdp_progs)) {
        test->adjust_tail_support = false;
        return TEST_PASS;
    }

and then testapp_adjust_tail() returns TEST_SKIP:

    if (!test->adjust_tail_support) {
        ...
        return TEST_SKIP;
    }

By that point the TX endpoint has usually finished with TEST_PASS. It
never sees the SKIP, so it does not skip.

The final verdict still comes out right, because xsk.py later in the
series reports a pass plus a skip as a skipped case. Could the comment
say that the peer either skips or passes?

[ ... ]

> diff --git a/tools/testing/selftests/net/lib/xsk/xskxceiver.c b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> index 9704702f60fc9..56ea5ac6a3c63 100644
> --- a/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> +++ b/tools/testing/selftests/net/lib/xsk/xskxceiver.c

[ ... ]

> @@ -328,6 +302,18 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
>  		case OPT_UDP_PORT:
>  			opt_udp_port = strtoul(optarg, NULL, 10);
>  			break;
> +		case OPT_QUEUE:
> +			opt_queue = strtoul(optarg, NULL, 10);
> +			break;
> +		case OPT_MAX_FRAGS:
> +			/* It sizes the packet array of TOO_MANY_FRAGS. */
> +			opt_max_frags = strtoul(optarg, NULL, 10);
> +			if (opt_max_frags > 0xffff)
> +				print_usage(argv);
> +			break;
> +		case OPT_TIMEOUT:
> +			opt_timeout = strtoul(optarg, NULL, 10);
> +			break;

[Severity: Low]
Should --timeout and --max-frags get a full range check here?

strtoul() returns an unsigned long, and the result is truncated to u32
before the > 0xffff check. A --max-frags value of 2^32 or more can
therefore pass the check.

--timeout accepts any u32, including 0. Later the value is multiplied by
1000 in u32 and passed on as an int:

    wait_link_up()
        err = xsk_wait_link_up(ifobj->ifname, thread_tmout * 1000);

    main()
        xsk_peer_skip(peer, opt_timeout * 1000);

A timeout of 2147484 seconds or more, or a negative string (which
strtoul() wraps), gives a negative timeout_ms. With a negative
timeout_ms, the poll() in xsk_peer_skip() blocks with no limit.

In this series, xsk.py checks --max-frags and --timeout against 1-0xffff
before passing them on. So this only happens when xskxceiver is run by
hand.

Could xskxceiver reject these out-of-range values itself, as it already
does for the --max-frags upper bound?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016174338.787135-1-maciej.fijalkowski%40intel.com