Re: [PATCH v4 net-next 11/15] selftests: xsk: send UDP over IPv4 or IPv6 between the interface MACs

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] The new XDP filter makes the INVALID_DESC tests less able to
  catch a kernel that transmits a descriptor it should reject.
- [Medium] (Baseline: not a defect, handled later in the series.) The
  concern: on a multi-queue physical NIC, nothing steers the new UDP…
- [Low] This feature commit, which changes the wire format, the XDP
  programs and the CLI, also carries three fixes for bugs that predate…
- [Low] (Baseline: intentional design, the launchers validate.) The
  concern: parse_command_line() stores `strtoul(optarg, NULL, 10)` in…

--

selftests: xsk: send UDP over IPv4 or IPv6 between the interface MACs

This patch changes the xskxceiver test traffic from loopback-ethertype
frames between made-up MACs to UDP over IPv4 or IPv6 between the real
interface MACs. The XDP programs now redirect only UDP to the test port
and pass everything else to the stack. The patch also fixes several
helpers in test_xsk.c that could overrun buffers.

> diff --git a/tools/testing/selftests/net/lib/xsk/test_xsk.c b/tools/testing/selftests/net/lib/xsk/test_xsk.c
> index ce88218fef00b..9c401a43da435 100644
> --- a/tools/testing/selftests/net/lib/xsk/test_xsk.c
> +++ b/tools/testing/selftests/net/lib/xsk/test_xsk.c

[ ... ]

> @@ -238,10 +364,10 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  			else
>  				xsk->pkt_stream = test->rx_pkt_stream_default;
>  
> -			memcpy(xsk->src_mac, g_mac, ETH_ALEN);
> -			memcpy(xsk->dst_mac, g_mac, ETH_ALEN);
> -			xsk->src_mac[5] += ((j * 2) + 0);
> -			xsk->dst_mac[5] += ((j * 2) + 1);
> +			memcpy(xsk->src_mac, ifobj->caps.mac, ETH_ALEN);
> +			memcpy(xsk->dst_mac, peer->caps.mac, ETH_ALEN);
> +			/* xsk_xdp_shared_umem() picks the socket by it. */
> +			xsk->src_port = udp_port + j;
>  		}

[Severity: Medium]
On a multi-queue physical NIC, what steers this flow to the queue the
AF_XDP socket is bound to?

If RSS hashes L4 ports, the flow could land on another queue, and
xsk_rcv_check() would reject the redirect there. With XDP_SHARED_UMEM,
socket 1 sends from udp_port + 1. Its flow could therefore hash to a
different queue even when socket 0's flow lands on the right one.

At this patch the only in-tree runner is veth, so nothing breaks yet. Two
later patches in the series cover this:

"selftests: drv-net: test AF_XDP zero-copy with an SKB peer" adds
drivers/net/hw/xsk.py. Its _reserve_queue() takes the bound queue out of
RSS with "ethtool -X <if> equal <queue>". Its _steer_udp() installs a
dst-port ntuple rule, which matches both sockets' flows.

"selftests: xsk: document generic and hardware endpoint runs" documents
the ntuple and RSS requirement in README.rst.

This commit message is aimed at physical links. Should it, or the header
comment in xskxceiver.c, mention the steering requirement too?

[ ... ]

> @@ -792,31 +933,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);
> -	if (ret) {
> -		ksft_print_msg("Error: bpf_map_lookup_elem failed with error %d\n", ret);
> -		return ret;
> -	}
> -
>  	/* Set the 'adjust_value' variable to -EOPNOTSUPP in the XDP program if the adjust_tail
>  	 * helper is not supported. Skip the adjust_tail test case in this scenario.
>  	 */
> -	*supported = adjust_value != -EOPNOTSUPP;
> -
> -	return 0;
> +	return skel_rx->bss->adjust_value != -EOPNOTSUPP;
>  }

[Severity: Low]
This isn't a bug, but this feature patch also carries three fixes for bugs
that existed before the series:

1) is_adjust_tail_supported() used bpf_map_lookup_elem() to copy the whole
xsk_xdp_.bss map into one stack int. At the baseline, .bss held idx,
adjust_value, count and drop_idx, 16 bytes in all. That overwrote 12
bytes of stack past adjust_value, and the int got the first word of .bss,
which is not necessarily adjust_value.

2) is_frag_valid() did len -= PKT_HDR_SIZE on a first fragment shorter
than the header. len wrapped, and the later reads went out of bounds.

3) is_pkt_valid() passed the total packet length to pkt_dump(). On a
multi-buffer packet, pkt_dump() read past the end of the first fragment.

The commit message describes all three. But because they ship with the
wire format, XDP program and CLI changes, they can't be backported or
reverted on their own. Older trees still have the original code in
tools/testing/selftests/bpf/prog_tests/test_xsk.c.

Could these go into a separate patch at the start of the series, with
Fixes: tags?

[ ... ]

> diff --git a/tools/testing/selftests/net/lib/xsk/xsk_xdp_progs.bpf.c b/tools/testing/selftests/net/lib/xsk/xsk_xdp_progs.bpf.c
> index 023d8befd4cab..2b55dbaf13b24 100644
> --- a/tools/testing/selftests/net/lib/xsk/xsk_xdp_progs.bpf.c
> +++ b/tools/testing/selftests/net/lib/xsk/xsk_xdp_progs.bpf.c

[ ... ]

> @@ -15,12 +23,64 @@ struct {

[ ... ]

> +	} else {
> +		return NULL;
> +	}
> +
> +	if ((void *)(udp + 1) > data_end || udp->dest != bpf_htons(test_port))
> +		return NULL;
> +	*hdr_len += sizeof(*udp) + PKT_HDR_PAD;
> +	return udp;
> +}

[ ... ]

>  SEC("xdp.frags") int xsk_def_prog(struct xdp_md *xdp)
>  {
> +	if (!is_test_packet(xdp))
> +		return XDP_PASS;
>  	return bpf_redirect_map(&xsk, 0, XDP_DROP);
>  }

[Severity: Medium]
Could this filter hide a kernel that wrongly transmits a descriptor that
the INVALID_DESC cases expect it to reject?

Before this patch, xsk_def_prog() redirected every frame to the XSKMAP.
An unexpected frame on RX then failed is_frag_valid() on the pkt_nb or
seqnum check. Now any frame that test_udp_hdr() does not see as UDP to
test_port gets XDP_PASS and never reaches __receive_pkts().

__send_pkts() writes a header only for valid entries:

    if (pkt->valid)
        pkt_generate(xsk, umem, tx_desc->addr, tx_desc->len, total_len,
                     pkt->pkt_nb, bytes_written);

pkt_get_addr() also gives invalid entries their raw pkt->offset. So the
bytes behind a negative descriptor are usually not a test header. For
example, this entry in testapp_invalid_desc():

    /* Straddle a 4K boundary */
    {0x1000 - MIN_PKT_SIZE / 2, MIN_PKT_SIZE, 0, false},

starts in the zero-filled tail of UMEM frame 0, so h_proto is 0.

The "Straddle the end of umem" entry starts inside the payload of the
packet at umem_sz - MIN_PKT_SIZE. The invalid-options and chunk-crossing
entries in testapp_invalid_desc_mb() look the same, for example:

    {0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XSK_DESC__INVALID_OPTION},

If the kernel transmitted any of these, the RX program would pass them to
the stack, and the case would still pass.

The TX side would not catch it either. __send_pkts() adds every frag to
outstanding_tx (valid_frags += nb_frags), and these cases don't check the
tx_invalid_descs statistic.

Could these cases check tx_invalid_descs? Alternatively, could the RX
program redirect or count non-test frames while they run?

[ ... ]

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

[ ... ]

> @@ -293,6 +314,20 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj

[ ... ]

> +		case OPT_UDP_PORT:
> +			opt_udp_port = strtoul(optarg, NULL, 10);
> +			break;

[Severity: Low]
Should this check errno, the end pointer and the range? The strtoul()
result goes straight into a u16, so:

    65536 becomes 0, which is rejected
    70000 becomes 4464
    "-1" becomes 65535
    "123junk" becomes 123

Only values that truncate to 0 are rejected, here by !opt_udp_port and
in xsk_set_flow().

With --udp-port 65535, socket 1's source port in __test_spec_init() wraps
to 0:

    xsk->src_port = udp_port + j;

Both endpoints truncate the same way, so the flow stays consistent.
xsk_xdp_shared_umem() still gets idx 1 for source port 0 through the
__u16 wrap-around.

Later commit messages in the series say the launchers own the range
checks. drivers/net/hw/xsk.py calls _check_opt("--udp-port",
cfg.udp_port, 1, 0xffff), and test_xsk.py takes the port from
rand_port(). Within xskxceiver, only --max-frags is range-checked, since
it sizes an array. A hand-run xskxceiver could still quietly use a
different port than the one requested.

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