Re: [PATCH bpf-next] 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] Incorrect traversal of stacked VLANs leads to dropped intermediate VLAN tags due to the use of `vlan_dev_real_dev()`.
--

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

bpf: Add BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup() helper

This commit introduces the BPF_FIB_LOOKUP_VLAN flag for the bpf_fib_lookup()
helper. When set, if the egress resolves to a VLAN device, the helper returns
the VLAN tag and the underlying device's ifindex to allow XDP programs to push
the tag and forward the frame directly.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714f..782fa86df95a3 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -6119,10 +6119,28 @@ 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)
>  {
>  	params->h_vlan_TCI = 0;
>  	params->h_vlan_proto = 0;
> +
> +	if ((flags & BPF_FIB_LOOKUP_VLAN) && is_vlan_dev(dev)) {
> +		struct net_device *real_dev = vlan_dev_real_dev(dev);

[Severity: High]
Does this correctly handle stacked VLANs (QinQ)? Both the commit message and
the inline comment below mention that stacked VLANs are not walked and only
the immediate parent is resolved.

However, looking at the implementation of vlan_dev_real_dev() in
net/8021q/vlan_core.c:

struct net_device *vlan_dev_real_dev(const struct net_device *dev)
{
        struct net_device *ret = vlan_dev_priv(dev)->real_dev;

        while (is_vlan_dev(ret))
                ret = vlan_dev_priv(ret)->real_dev;

        return ret;
}

It appears vlan_dev_real_dev() loops through all stacked VLANs to return the
ultimate non-VLAN underlying device. If the configuration is QinQ, wouldn't
this set params->ifindex to the base physical device while only returning the
topmost VLAN tag in params->h_vlan_TCI?

This could cause an XDP program to push only a single tag and drop the
intermediate VLAN tags, bypassing the outer VLAN entirely.

Should this use vlan_dev_priv(dev)->real_dev instead to only get the
immediate parent as intended?

> +
> +		/* Only the immediate parent is resolved; stacked VLANs
> +		 * (QinQ) are not walked, and a NULL real_dev (which
> +		 * is_vlan_dev() rules out in practice) keeps the
> +		 * original ifindex.
> +		 */
> +		if (real_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;
> +		}
> +	}

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