Re: [PATCH bpf-next v6 1/3] bpf: Add BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The new `BPF_FIB_LKUP_RET_VLAN_FAILURE` return path clobbers input parameters, rendering the documented 're-issue' behavior unsafe.
--

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

bpf: Add BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup() helper

This commit adds the BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup(), allowing
XDP programs to resolve VLAN egress natively. If the egress cannot be reduced
to a physical device, the helper returns BPF_FIB_LKUP_RET_VLAN_FAILURE.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 89b36de5fdbb6..e00f0392e728f 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
[ ... ]
> @@ -3532,6 +3532,29 @@ union bpf_attr {
>   *			Use the mark present in *params*->mark for the fib lookup.
>   *			This option should not be used with BPF_FIB_LOOKUP_DIRECT,
>   *			as it only has meaning for full lookups.
> + *		**BPF_FIB_LOOKUP_VLAN**
> + *			If the fib lookup resolves to a VLAN device whose
> + *			parent is a real (non-VLAN) device, set
> + *			*params*->h_vlan_proto and *params*->h_vlan_TCI from
> + *			the VLAN device and replace *params*->ifindex with the
> + *			parent's ifindex. *params*->h_vlan_TCI carries the VID
> + *			only, with PCP and DEI bits zero; a consumer wanting to
> + *			set egress priority writes PCP itself. *params*->smac is
> + *			the VLAN device's own address, which can differ from the
> + *			parent's. Only the immediate parent is resolved; if it
> + *			is itself a VLAN device (QinQ) or in another namespace,
> + *			the egress cannot be reduced to a physical device plus
> + *			one tag and the lookup returns
> + *			**BPF_FIB_LKUP_RET_VLAN_FAILURE** with *params*->ifindex
> + *			left at the input. Re-issue without
> + *			**BPF_FIB_LOOKUP_VLAN** to obtain the VLAN device's own
> + *			ifindex. The swap and the vlan fields

[Severity: High]
Does this documentation create a trap for users? The struct bpf_fib_lookup
uses unions for input and output fields, meaning several input parameters
are already clobbered by the time BPF_FIB_LKUP_RET_VLAN_FAILURE is returned.

If an XDP program follows this advice and re-issues the lookup without
BPF_FIB_LOOKUP_VLAN using the same params struct, will it execute with
garbage inputs (e.g., tbid=0, or an overwritten mark), potentially leading
to silent routing failures or policy bypass?

[ ... ]
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2e96b4b847ce1..b5a45485a54bd 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -6201,10 +6201,29 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
>  #endif
>  
>  #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
> -static int bpf_fib_set_fwd_params(struct bpf_fib_lookup *params, u32 mtu)
> +static int bpf_fib_set_fwd_params(struct net_device *dev,
> +				  struct bpf_fib_lookup *params,
> +				  u32 flags, u32 mtu, u32 in_ifindex)
>  {
>  	params->h_vlan_TCI = 0;
>  	params->h_vlan_proto = 0;

[Severity: High]
Does this unconditionally clobber the aliased tbid input before the failure
path? In struct bpf_fib_lookup, h_vlan_proto and h_vlan_TCI share a union
with tbid.

When returning BPF_FIB_LKUP_RET_VLAN_FAILURE below, the original tbid is lost,
guaranteeing the params struct is corrupted for a subsequent lookup.

> +
> +#if IS_ENABLED(CONFIG_VLAN_8021Q)
> +	if ((flags & BPF_FIB_LOOKUP_VLAN) && is_vlan_dev(dev)) {
> +		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> +
> +		if (!is_vlan_dev(real_dev) &&
> +		    net_eq(dev_net(real_dev), dev_net(dev))) {
> +			params->h_vlan_proto = vlan_dev_vlan_proto(dev);
> +			params->h_vlan_TCI = htons(vlan_dev_vlan_id(dev));
> +			params->ifindex = real_dev->ifindex;
> +		} else {
> +			params->ifindex = in_ifindex;
> +			return BPF_FIB_LKUP_RET_VLAN_FAILURE;
> +		}
> +	}
> +#endif
[ ... ]
> @@ -6347,7 +6367,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>  	memcpy(params->smac, dev->dev_addr, ETH_ALEN);

[Severity: High]
Does this memcpy() overwrite the aliased mark input? In struct bpf_fib_lookup,
smac shares a union with mark.

When bpf_fib_set_fwd_params() returns BPF_FIB_LKUP_RET_VLAN_FAILURE on the
failure path below, is the caller's original mark permanently overwritten by
the MAC address?

>  
>  set_fwd_params:
> -	return bpf_fib_set_fwd_params(params, mtu);
> +	return bpf_fib_set_fwd_params(dev, params, flags, mtu, in_ifindex);
>  }
>  #endif

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