Re: [PATCH bpf-next v5 3/3] selftests/bpf: Add bpf_fib_lookup() VLAN flag tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Multi-line comment does not follow the modern BPF kernel style.
- [Low] Dead write to `skb.ifindex` before running the XDP test in `test_fib_lookup_vlan_netns`.
--

commit 8850d806ed94f65fe16bb923b209b37fa59c65ae
Author: Avinash Duduskar <avinash.duduskar@gmail.com>

selftests/bpf: Add bpf_fib_lookup() VLAN flag tests

This commit introduces selftests for the bpf_fib_lookup() VLAN flags. It covers
both BPF_FIB_LOOKUP_VLAN for egress and BPF_FIB_LOOKUP_VLAN_INPUT for scoping
the lookup to a VLAN subinterface.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> index bd76589580041..8caed9d43b981 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
[ ... ]
> @@ -343,6 +730,15 @@ void test_fib_lookup(void)
>  		if (!ASSERT_OK(err, "bpf_prog_test_run_opts"))
>  			continue;
>  
> +		/* BPF_FIB_LOOKUP_VLAN is XDP-only; the tc helper rejects it.
> +		 * These cases are exercised on the XDP path below.
> +		 */

[Severity: Low]
This isn't a bug, but this multi-line comment does not follow the preferred
kernel comment style for the BPF subsystem, which places the opening /* on
its own line:

/*
 * BPF_FIB_LOOKUP_VLAN is XDP-only; the tc helper rejects it.
 * These cases are exercised on the XDP path below.
 */

> +		if (tests[i].lookup_flags & BPF_FIB_LOOKUP_VLAN) {
> +			ASSERT_EQ(skel->bss->fib_lookup_ret, -EINVAL,
> +				  "tc rejects BPF_FIB_LOOKUP_VLAN");
> +			continue;
> +		}
[ ... ]
> +	/*
> +	 * egress: the fib result is the VLAN device here, but its parent
> +	 * is in the other netns, so the swap must not happen
> +	 */
> +	SYS(fail, "ip link set dev veth7.66 up");
> +	SYS(fail, "ip addr add 10.66.0.1/24 dev veth7.66");
> +	err = write_sysctl("/proc/sys/net/ipv4/conf/veth7.66/forwarding", "1");
> +	if (!ASSERT_OK(err, "write_sysctl(forwarding)"))
> +		goto fail;
> +
> +	vlan_idx = if_nametoindex("veth7.66");
> +	if (!ASSERT_NEQ(vlan_idx, 0, "if_nametoindex(veth7.66)"))
> +		goto fail;
> +
> +	skb.ifindex = vlan_idx;

[Severity: Low]
Is this assignment to skb.ifindex necessary? The subsequent test run uses
xdp_opts, which doesn't specify a context (unlike run_opts which passes skb as
ctx_in), so it appears the skb is ignored here.

> +	memset(fib_params, 0, sizeof(*fib_params));
> +	fib_params->family = AF_INET;
> +	fib_params->l4_protocol = IPPROTO_TCP;
> +	fib_params->ifindex = vlan_idx;
> +	if (!ASSERT_EQ(inet_pton(AF_INET, "10.66.0.2", &fib_params->ipv4_dst),
> +		       1, "inet_pton(dst)") ||
> +	    !ASSERT_EQ(inet_pton(AF_INET, "10.66.0.1", &fib_params->ipv4_src),
> +		       1, "inet_pton(src)"))
> +		goto fail;
> +
> +	skel->bss->fib_lookup_ret = -1;
> +	skel->bss->lookup_flags = BPF_FIB_LOOKUP_VLAN |
> +				  BPF_FIB_LOOKUP_SKIP_NEIGH;
> +	err = bpf_prog_test_run_opts(xdp_fd, &xdp_opts);
> +	if (!ASSERT_OK(err, "test_run(egress)"))
> +		goto fail;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624030530.3342884-1-avinash.duduskar@gmail.com?part=3