Re: [PATCH bpf v5 2/2] selftests/bpf: Cover generic devmap egress last-dst rewrite

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The sentinel value `sentinel_mac` is identically initialized to the same value as `last_mac`, masking the bug the test is intended to detect and resulting in unreachably broken assertions.
- [Low] Redundant manual file descriptor validation on skeleton-generated maps violates the BPF subsystem Skeleton vs Manual Lookup guidelines.

Pre-existing issues:
- [High] Uninitialized stack variable `net_config` is passed to `cleanup_network` on error paths in `xdp_veth_egress`, leading to undefined behavior and potential command injection.
--

commit a20e1dfd3338912dfdbb631c91d105ada7591c25
Author: Sun Jian <sun.jian.kdev@gmail.com>

selftests/bpf: Cover generic devmap egress last-dst rewrite

This commit strengthens xdp_veth_egress to check that each destination
observes the MAC selected for its own egress ifindex, and adds a generic
XDP last-destination test.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> index 3e98a16659362..1675b32753a8a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> @@ -531,15 +541,162 @@ static void xdp_veth_egress(u32 flags)

[ ... ]

> +destroy_xdp_redirect_map:
> +	close_netns(nstoken);
> +	xdp_redirect_map__destroy(xdp_redirect_map);
> +destroy_xdp_redirect_multi_kern:
> +	xdp_redirect_multi_kern__destroy(xdp_redirect_multi_kern);
> +destroy_xdp_dummy:
> +	xdp_dummy__destroy(xdp_dummy);
> +
> +	cleanup_network(&net_config);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to pass the
uninitialized net_config struct to cleanup_network() on error paths?

In xdp_veth_egress(), we declare net_config without initialization:

	struct net_configuration net_config;

If skeleton loading fails (e.g., xdp_redirect_multi_kern__open_and_load()),
the code jumps to destroy_xdp_dummy and calls cleanup_network(&net_config).
Inside cleanup_network(), uninitialized stack data like net_config.ns0_name
is passed to system() commands via SYS_NOFAIL(), which might trigger arbitrary
shell commands since selftests run as root.

> +}
> +
> +static void xdp_veth_egress_last_dst(u32 flags)
> +{

[ ... ]

> +	if (!ASSERT_OK(create_network(&net_config), "create network"))
> +		goto destroy_xdp_redirect_map;
> +
> +	mac_map = bpf_map__fd(xdp_redirect_multi_kern->maps.mac_map);
> +	if (!ASSERT_OK_FD(mac_map, "open mac_map"))
> +		goto destroy_xdp_redirect_map;
> +
> +	egress_map = bpf_map__fd(xdp_redirect_multi_kern->maps.map_egress);
> +	if (!ASSERT_OK_FD(egress_map, "open map_egress"))
> +		goto destroy_xdp_redirect_map;

[Severity: Low]
Are these FD validation checks needed?

After a successful skeleton __open_and_load(), the skeleton guarantees that
all maps and programs are loaded with valid FDs. We don't need additional
ASSERT_OK_FD() validation when accessing fields like
xdp_redirect_multi_kern->maps.mac_map.

[ ... ]

> +	res_map = bpf_map__fd(xdp_redirect_map->maps.rx_mac);
> +	if (!ASSERT_OK_FD(res_map, "open rx_map"))
> +		goto destroy_xdp_redirect_map;
> +
> +	memcpy(&sentinel_mac, egress_macs[VETH_PAIRS_COUNT - 1], ETH_ALEN);
> +	memcpy(&last_mac, egress_macs[VETH_PAIRS_COUNT - 1], ETH_ALEN);

[Severity: Medium]
Does initializing sentinel_mac and last_mac to the same value bypass the
intended checks?

Since both are initialized to egress_macs[VETH_PAIRS_COUNT - 1], if the
res equals last_mac, the test will fail at the first assertion:

	if (!ASSERT_NEQ(res, sentinel_mac, "rx_mac overwritten by store_mac_1"))

with a misleading error message. The second assertion checking against
last_mac then becomes unreachable:

	if (!ASSERT_NEQ(res, last_mac, "earlier dst not rewritten by last dst"))

> +
> +	key = 0;
> +	err = bpf_map_update_elem(res_map, &key, &sentinel_mac, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612114032.244616-1-sun.jian.kdev@gmail.com?part=2