Re: [PATCH v2 net-next 11/14] selftests: xsk: pass non-test traffic to the stack in hardware mode

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] The comment this patch adds in xsk_load_xdp_programs() ("The
  shadow's copy too, swapped directions attach its programs.") is…
- [Low] In hardware (UDP) mode, xsk_xdp_adjust_tail() in
  xsk_xdp_progs.bpf.c writes the wrong sequence number at the end of a…
- [Low] is_test_packet() in xsk_xdp_progs.bpf.c does not check
  ip->frag_off before reading the bytes after the IPv4 header as a…

Pre-existing issues:
- [Medium] is_adjust_tail_supported() in
  tools/testing/selftests/net/lib/xsk/test_xsk.c overflows a stack…

--

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

In hardware mode, this commit makes the xskxceiver XDP programs redirect
only UDP/IPv4 packets sent to the test port (--udp-port). Everything else
is passed to the stack. A new hw_port .bss variable is set from
xsk_load_xdp_programs(), and an is_test_packet() helper gates each
program. When hw_port is 0, every packet is still redirected.

[Severity: Low]
This comment looks right, but it contradicts the comment in setup_peer()
in xskxceiver.c, which was added earlier in this series:

tools/testing/selftests/net/lib/xsk/xskxceiver.c:setup_peer() {
	/* Unattached skeleton copy; the engine only attaches the local side. */
	if (xsk_load_xdp_programs(shadow))
		return TEST_FAILURE;
    ...
}

test_spec_reset() binds test->xdp_prog_tx and test->xskmap_tx to
ifobj_tx->xdp_progs. That is the shadow when the local endpoint is RX.

swap_directions() only swaps the ifobject pointers. After the swap,
xsk_attach_xdp_progs() does:

	if (xdp_prog_changed_tx(test))
		err = xsk_reattach_xdp(ifobj_tx, test->xdp_prog_tx, test->xskmap_tx, test->mode);

This attaches the shadow's program and XSKMAP to the real NIC, for
example in testapp_bidirectional().

Should the setup_peer() comment be updated too? A reader who trusts it
could drop the hw_port write for the shadow. That would bring back the
redirect-everything behaviour after a direction swap.

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but adding hw_port to .bss makes it
somewhat worse. is_adjust_tail_supported() in test_xsk.c reads the whole
.bss map value into a 4-byte local:

tools/testing/selftests/net/lib/xsk/test_xsk.c:is_adjust_tail_supported() {
	int adjust_value = 0;
	...
	ret = bpf_map_lookup_elem(bpf_map__fd(data_map), &key, &adjust_value);
	...
}

bpf_map_lookup_elem() copies value_size bytes, which here is the full
.bss section. Before this patch, .bss already held idx, adjust_value,
count and drop_idx, so about 16 bytes were written into a 4-byte int.
With hw_port added, it is now about 18-20 bytes.

Can this overwrite other stack variables, or trip the stack protector?
This happens when testapp_validate_rx_endpoint() calls
is_adjust_tail_supported() after an adjust_tail test fails.

This patch already sets hw_port through the skeleton. Could
is_adjust_tail_supported() read skel_rx->bss->adjust_value the same way?

[Severity: Low]
Should is_test_packet() also check ip->frag_off? A non-initial IPv4
fragment has no UDP header after the IP header. If it has ihl == 5 and
protocol == IPPROTO_UDP, its payload bytes 2-3 are read here as
udp->dest.

If those bytes happen to equal htons(hw_port), is_test_packet() returns
true. xsk_def_prog() and the other programs then redirect, drop or
modify an unrelated fragment instead of returning XDP_PASS.

That datagram can then not be reassembled. The test socket may also
receive a frame that fails validation.

The commit message says only UDP/IPv4 packets sent to the test port are
redirected. Other parsers in the tree, such as xdp_flowtable.c and
xdp_synproxy_kern.c, reject fragments by checking
frag_off & bpf_htons(IP_MF | IP_OFFSET).

[ ... ]

[Severity: Low]
This patch gates xsk_xdp_adjust_tail() for hardware mode. Does its grow
path still write the wrong end sequence number for UDP frames?

tools/testing/selftests/net/lib/xsk/xsk_xdp_progs.bpf.c:xsk_xdp_adjust_tail() {
	...
	if (curr_buff_len > buff_len) {
		...
		len = curr_buff_len - PKT_HDR_ALIGN;
		words_to_end = len / sizeof(*pkt_data) - 1;
		...
}

PKT_HDR_ALIGN is sizeof(struct ethhdr) + 2, which is 16. In hardware
mode, xsk_set_udp_packet_format() sets pkt_hdr_size to UDP_PKT_HDR_SIZE
(44), and is_frag_valid() uses that value:

	pkt_data += pkt_hdr_size / sizeof(*pkt_data);

Take a 132-byte grown frame. The program stores (132 - 16) / 4 - 1 = 28,
but is_frag_valid() expects (132 - 44) / 4 - 1 = 21. The check then
prints "expected seqnum at end [21], got seqnum [28]". The multi-buffer
variant is off by the same 7 words.

testapp_adjust_tail_grow() and testapp_adjust_tail_grow_mb() only skip
TEST_MODE_SKB. testapp_xdp_shared_umem(), by contrast, returns TEST_SKIP
under hw_test.

xsk.py does not schedule these cases on hardware, because their hardware
directions are 0 in test_xsk_case_defs.h. Running xskxceiver --hw
directly with these cases would fail rather than skip.

The mismatch comes from the earlier patch "selftests: xsk: add a hardware
mode to xskxceiver", and this patch leaves it in place. Should these
cases skip under hw_test, or should the program use the UDP header size
in hardware mode?

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