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

[bot+bpf-ci]

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 11dd610fa5fa..aa7fe378a35d 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714..782fa86df95a 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);

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:

const struct net_device *vlan_dev_real_dev(const struct net_device *dev)
{
    while (is_vlan_dev(dev))
        dev = vlan_dev_priv(dev)->real_dev;
    return dev;
}

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

This isn't a bug, but the comment style here differs from BPF subsystem
guidelines. The opening /* has comment text on the same line, but the BPF
comment style prefers the opening /* on its own line with text starting
on the next line.

> +		 * (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;
> +		}
> +	}
> +
>  	if (mtu)
>  		params->mtu_result = mtu; /* union with tot_len */
>
> @@ -6265,7 +6283,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>  	memcpy(params->smac, dev->dev_addr, ETH_ALEN);
>
>  set_fwd_params:
> -	return bpf_fib_set_fwd_params(params, mtu);
> +	return bpf_fib_set_fwd_params(dev, params, flags, mtu);
>  }
>  #endif

[ ... ]

> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 11dd610fa5fa..aa7fe378a35d 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27223878961