Re: [PATCH v3 net-next 12/15] selftests: xsk: pass non-test traffic to the stack in hardware mode

[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] In hardware mode, adjust_tail_grow_skip()
  (tools/testing/selftests/net/lib/xsk/test_xsk.c) can make the two…
- [Low] This feature patch also carries the only fix for a real userspace
  stack buffer overflow, without a Fixes: tag.

--

selftests: xsk: pass non-test traffic to the stack in hardware mode

In hardware mode, the xskxceiver XDP programs now redirect only the
UDP/IPv4 packets sent to the test port and pass everything else to the
stack. xsk_xdp_adjust_tail() now accounts for the IPv4 and UDP headers,
so the XDP_ADJUST_TAIL_GROW cases can run with --hw. is_adjust_tail_supported()
now reads adjust_value through the skeleton instead of copying all of
.bss into an int.

> diff --git a/tools/testing/selftests/net/lib/xsk/test_xsk.c b/tools/testing/selftests/net/lib/xsk/test_xsk.c
> index a280956d53f4..b70dbefe9729 100644
> --- a/tools/testing/selftests/net/lib/xsk/test_xsk.c
> +++ b/tools/testing/selftests/net/lib/xsk/test_xsk.c
> @@ -944,31 +944,12 @@ static bool is_metadata_correct(struct pkt *pkt, void *buffer, u64 addr)
>  	return true;
>  }
>  
> -static int is_adjust_tail_supported(struct xsk_xdp_progs *skel_rx, bool *supported)
> +static bool is_adjust_tail_supported(struct xsk_xdp_progs *skel_rx)
>  {
> -	struct bpf_map *data_map;
> -	int adjust_value = 0;
> -	int key = 0;
> -	int ret;
> -
> -	data_map = bpf_object__find_map_by_name(skel_rx->obj, "xsk_xdp_.bss");
> -	if (!data_map || !bpf_map__is_internal(data_map)) {
> -		ksft_print_msg("Error: could not find bss section of XDP program\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = bpf_map_lookup_elem(bpf_map__fd(data_map), &key, &adjust_value);

[Severity: Low]
The replacement below looks correct. Should this part go out as its own
fix with a Fixes: tag, though?

The removed bpf_map_lookup_elem() copies the whole value_size of the
internal "xsk_xdp_.bss" map into the 4-byte stack int adjust_value.
Before hw_port was added, .bss already held idx, adjust_value, count and
drop_idx. So the copy writes at least 12 bytes and overwrites nearby
stack slots such as key, ret and data_map.

The commit message mentions the overrun. However, the fix is bundled
with the hardware-mode feature and depends on the move to selftests/net.
The same code is still in tools/testing/selftests/bpf/prog_tests/test_xsk.c,
which test_progs runs in trees without this series.

A standalone patch carrying:

Fixes: 4b302092553c ("selftests/xsk: Add tail adjustment tests and support check")

would let the fix be backported to net and stable.

> -	if (ret) {

[ ... ]

> +	return skel_rx->bss->adjust_value != -EOPNOTSUPP;
>  }

[ ... ]

> @@ -2774,10 +2754,20 @@ int testapp_adjust_tail_shrink_mb(struct test_spec *test)
>  	return testapp_adjust_tail(test, -XSK_UMEM__MAX_FRAME_SIZE, XSK_UMEM__LARGE_FRAME_SIZE * 2);
>  }
>  
> +/* Generic XDP leaves no room to grow the tail. Only the RX endpoint runs the
> + * program on the test flow, and a hardware TX endpoint, which does not know
> + * the mode of its peer, leaves that skip to the RX endpoint.
> + */
> +static bool adjust_tail_grow_skip(struct test_spec *test)
> +{
> +	if (hw_test && !ifobj_is_local(test->ifobj_rx))
> +		return false;

[Severity: Low]
Can the two hardware endpoints disagree on the skip here?

With --hw and a local RX endpoint in SKB mode, testapp_adjust_tail_grow()
and testapp_adjust_tail_grow_mb() return TEST_SKIP before
sync_endpoints() is reached. The RX side then closes the control
connection.

On the TX side, ifobj_rx is the non-local shadow. adjust_tail_grow_skip()
therefore always returns false, and TX continues:

testapp_adjust_tail()->worker_testapp_validate_tx()->sync_endpoints()
  ->xsk_peer_ready()

worker_testapp_validate_tx() {
    ...
	err = sync_endpoints(test);
	if (err)
		return err;
    ...
}

xsk_peer_ready() in xsk_peer.c {
    ...
	err = peer_send(peer, XSK_PEER_READY, 0);
	while (!err && !peer->ready_seen)
		err = peer_recv_one(peer);
    ...
}

There is no "peer skipped" message, so TX gets EPIPE or ECONNRESET.
The run ends with TX failing and RX skipping.

The old check (test->mode == TEST_MODE_SKB || hw_test) skipped on both
sides. That matches the existing convention in testapp_stats_rx_dropped():

	/* Hardware endpoints run different modes, so ZC would skip on one only. */
	if (hw_test)
		return TEST_SKIP;

Later in the series, "selftests: drv-net: test AF_XDP zero-copy with an
SKB peer" adds drivers/net/hw/xsk.py. Its _test() reports the case as a
skip when either side returns SKIP_CODE:

    if SKIP_CODE in (dut_ret, remote_ret):
        raise KsftSkipEx("xskxceiver skipped the case")

Also, "selftests: xsk: list the hardware test cases" puts the grow cases
only in hw_rx_tests, where the SKB remote is the TX side.

At this commit, no in-tree runner drives --hw. Is the one-sided failure
the intended result for manual xskxceiver --hw runs?

> +	return test->mode == TEST_MODE_SKB;
> +}
> +
>  int testapp_adjust_tail_grow(struct test_spec *test)
>  {
> -	/* The XDP program numbers the grown tail after a non-UDP header. */
> -	if (test->mode == TEST_MODE_SKB || hw_test)
> +	if (adjust_tail_grow_skip(test))
>  		return TEST_SKIP;

[ ... ]

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