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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The input netns boundary check in `test_fib_lookup_vlan_netns` is a tautology and cannot detect regressions.
--

--- Patch [3]: [PATCH bpf-next v7 3/3] selftests/bpf: Add bpf_fib_lookup() VLAN flag tests ---
commit dd8cb85eafac54bc022600352c949874324b276d
Author: Avinash Duduskar <avinash.duduskar@gmail.com>

selftests/bpf: Add bpf_fib_lookup() VLAN flag tests

Cover both new VLAN flags in the fib_lookup test. BPF_FIB_LOOKUP_VLAN
reduces a VLAN egress to its physical parent plus the tag, and
BPF_FIB_LOOKUP_VLAN_INPUT scopes 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..f7361f9a34592 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
[ ... ]
> @@ -851,0 +852,136 @@ void test_fib_lookup(void)
> +void test_fib_lookup_vlan_netns(void)
> +{
[ ... ]
> +	/*
> +	 * input: the moved device is still in veth7's VLAN group, but it
> +	 * lives in another netns, so the lookup must fail closed
> +	 */
> +	skb.ifindex = parent_idx;
> +	memset(fib_params, 0, sizeof(*fib_params));
> +	fib_params->family = AF_INET;
> +	fib_params->l4_protocol = IPPROTO_TCP;
> +	fib_params->ifindex = parent_idx;
> +	fib_params->h_vlan_proto = htons(ETH_P_8021Q);
> +	fib_params->h_vlan_TCI = htons(66);
> +	if (!ASSERT_EQ(inet_pton(AF_INET, IPV4_VLAN_NETNS_DST, &fib_params->ipv4_dst),
> +		       1, "inet_pton(dst)"))
> +		goto fail;
> +
> +	skel->bss->fib_lookup_ret = -1;
> +	skel->bss->lookup_flags = BPF_FIB_LOOKUP_VLAN_INPUT |
> +				  BPF_FIB_LOOKUP_SKIP_NEIGH;
> +	err = bpf_prog_test_run_opts(prog_fd, &run_opts);
> +	if (!ASSERT_OK(err, "test_run(input)"))
> +		goto fail;
> +	ASSERT_EQ(skel->bss->fib_lookup_ret, BPF_FIB_LKUP_RET_NOT_FWDED,
> +		  "input across netns fails closed");

[Severity: Medium]
Is this test effectively a tautology?

The IP and routing configuration for the target namespace NS_VLAN_B isn't
set up until after this input test runs. 

If a kernel bug completely skipped the netns boundary check, wouldn't the
route lookup fail anyway and return BPF_FIB_LKUP_RET_NOT_FWDED, causing this
assertion to falsely pass?

> +	ASSERT_EQ(fib_params->ifindex, parent_idx, "ifindex untouched");
> +	ASSERT_EQ(fib_params->h_vlan_TCI, htons(66), "tag untouched");
> +
> +	close_netns(nstoken);
> +	nstoken = open_netns(NS_VLAN_B);
> +	if (!ASSERT_OK_PTR(nstoken, "open_netns(b)"))
> +		goto fail;
> +
> +	/*
> +	 * 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 addr add %s/24 dev veth7.66", IPV4_VLAN_NETNS_ADDR);
> +	err = write_sysctl("/proc/sys/net/ipv4/conf/veth7.66/forwarding", "1");

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