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

[sashiko-bot@kernel.org]

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

New issues:
- [High] bpf_map_lookup_elem on an ARRAY map never returns -ENOENT, masking dropped packets and causing a false positive test pass.
- [Low] The test performs manual FD validation (`ASSERT_OK_FD`) on map file descriptors immediately after successfully loading the skeleton, violating the explicit BPF selftest guidelines.

Pre-existing issues:
- [Critical] Uninitialized stack structure `net_config` is passed to cleanup functions on early error paths, causing arbitrary shell command execution via stack garbage.
- [High] bpf_map_update_elem reads 8 bytes from a 6-byte user-space array, causing an out-of-bounds stack read.
--

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

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

Strengthen xdp_veth_egress to check that each destination observes the
MAC selected for its own egress ifindex, instead of only checking that
the observed MAC differs from a single magic value.

Add a generic XDP last-destination test where earlier destinations do
not have a devmap egress program while the final destination does.

> 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..0fa2cf835c629 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> @@ -456,7 +456,11 @@ static void xdp_veth_egress(u32 flags)
>  			.remote_flags = flags,
>  		}
>  	};
> -	const char magic_mac[6] = { 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF};
> +	const unsigned char egress_macs[VETH_PAIRS_COUNT][ETH_ALEN] = {
> +		{ 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0x01 },
> +		{ 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0x02 },
> +		{ 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0x03 },
> +	};

[Severity: Critical]
This is a pre-existing issue, but does the error handling in
xdp_veth_egress() execute arbitrary shell commands?

In xdp_veth_egress(), net_config is declared on the stack without
initialization. If an early failure occurs (such as
xdp_dummy__open_and_load() failing), the error path calls
cleanup_network(&net_config).

This cleanup function invokes SYS_NOFAIL("ip netns del %s", 
net_config.ns0_name), which will execute a command using whatever stack
garbage is in the uninitialized ns0_name field.

The new xdp_veth_egress_last_dst() function added in this patch correctly
uses net_config = {} to avoid this problem.

[ ... ]
> @@ -512,7 +516,7 @@ static void xdp_veth_egress(u32 flags)
>  						 &net_config, prog_cfg, i))
>  			goto destroy_xdp_redirect_map;
>  
> -		err = bpf_map_update_elem(mac_map, &ifindex, magic_mac, 0);
> +		err = bpf_map_update_elem(mac_map, &ifindex, egress_macs[i], 0);

[Severity: High]
This is a pre-existing issue duplicated by this patch, but does this
bpf_map_update_elem() call read out of bounds?

The mac_map is defined in xdp_redirect_multi_kern.c with a value_size of
8 bytes (__be64), but egress_macs elements are only 6 bytes (ETH_ALEN).
For the final element in the array, this operation will read 2 bytes past
the end of the egress_macs stack array into stack garbage.

[ ... ]
> +static void xdp_veth_egress_last_dst(u32 flags)
> +{
[ ... ]
> +	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]
Is this FD validation necessary?

According to BPF selftest guidelines, after a successful
xdp_redirect_multi_kern__open_and_load(), all maps are guaranteed to be
created with valid FDs. Manual FD validation like this is not needed when
using skeleton-generated fields.

[ ... ]
> +		err = bpf_map_update_elem(mac_map, &ifindex, egress_macs[i], 0);
> +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
> +			goto destroy_xdp_redirect_map;

[Severity: High]
Does this bpf_map_update_elem() call also read out of bounds, similar to
the pre-existing issue above?

The map expects an 8-byte value (__be64), but the egress_macs array element
provides only 6 bytes.

[ ... ]
> +	for (i = 0; i < VETH_PAIRS_COUNT - 1; i++) {
>  		u32 key = i;
>  		u64 res;
>  
>  		err = bpf_map_lookup_elem(res_map, &key, &res);
> +		if (err == -ENOENT)
> +			continue;
>  		if (!ASSERT_OK(err, "get MAC res"))
>  			goto destroy_xdp_redirect_map;
>  
> -		ASSERT_STRNEQ((const char *)&res, magic_mac, ETH_ALEN, "compare mac");
> +		found = true;
> +		ASSERT_NEQ(res, last_mac, "compare last dst mac");
>  	}

[Severity: High]
Can bpf_map_lookup_elem() actually return -ENOENT for this map type?

Looking at xdp_redirect_map.c, rx_mac is a BPF_MAP_TYPE_ARRAY. Since array
maps pre-allocate their elements and zero-initialize them, lookups for valid
indices will always succeed (returning 0) and yield the current element
value (0 if unwritten), rather than returning -ENOENT.

If a packet is dropped and never updates the map, the value will remain 0.
The bpf_map_lookup_elem() call will succeed (err == 0), and the test will
trivially pass because res (0) is not equal to last_mac. Does this mask
datapath failures and cause a false positive test pass?

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