Re: [PATCH bpf-next v2 2/4] 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 2 potential issue(s) to consider:
- [High] Bypassing VLAN device MTU checks when BPF_FIB_LOOKUP_VLAN is used without tot_len creates a PMTU blackhole.
- [Low] Multi-line comments introduced by the patch violate the BPF subsystem comment style guideline.
--

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

bpf: Add BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup() helper

bpf_fib_lookup() returns the FIB-resolved egress ifindex straight
from the fib result. When the egress is a VLAN device, the returned
ifindex is the VLAN netdev's, which has no XDP xmit handler; XDP
programs that want to forward the frame (e.g. xdp-forward) must
instead target the underlying physical device and push the VLAN tag
themselves.

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

[ ... ]

> @@ -7388,7 +7414,10 @@ struct bpf_fib_lookup {
>  
>  	union {
>  		struct {
> -			/* output */
> +			/* output with BPF_FIB_LOOKUP_VLAN: set from the
> +			 * resolved egress VLAN device (see the flag); zeroed
> +			 * on other successful lookups.
> +			 */

[Severity: Low]
This isn't a bug, but does this violate the BPF subsystem multi-line comment
style? The guidelines prefer the opening /* to be on its own line:

/*
 * output with BPF_FIB_LOOKUP_VLAN: set from the
 * ...
 */

>  			__be16	h_vlan_proto;
>  			__be16	h_vlan_TCI;
>  		};

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6fa172cb13480..b37a12321fba3 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -6119,10 +6119,40 @@ 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 IS_ENABLED(CONFIG_VLAN_8021Q)
> +	/* vlan_dev_priv() is only defined when 8021q is built in or as a
> +	 * module; under !CONFIG_VLAN_8021Q is_vlan_dev() is constant false
> +	 * so this would be dead, but it still has to compile.
> +	 */

[Severity: Low]
This isn't a bug, but this multi-line comment also starts with text on the
same line as the opening /*.

> +	if ((flags & BPF_FIB_LOOKUP_VLAN) && is_vlan_dev(dev)) {
> +		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> +
> +		/* Resolve the immediate parent only. For a stacked VLAN
> +		 * (QinQ) the parent is itself a VLAN device, and a single
> +		 * h_vlan_proto/h_vlan_TCI pair cannot describe both tags;
> +		 * leave ifindex and the vlan fields untouched in that case
> +		 * rather than report the lower device with only one tag.
> +		 * The same applies when the parent lives in another netns
> +		 * (a VLAN device can be moved while its parent stays):
> +		 * its ifindex would be meaningless, or match an unrelated
> +		 * device, in the caller's namespace.
> +		 */

[Severity: Low]
This isn't a bug, but this multi-line comment also starts with text on the
same line as the opening /*.

> +		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;

[Severity: High]
Will this cause PMTU blackholes when bpf_skb_fib_lookup() is called without
tot_len?

When tot_len is not provided, the MTU check is deferred and done in
bpf_skb_fib_lookup() after bpf_fib_set_fwd_params() returns. Because
params->ifindex is swapped to the parent device here, the deferred MTU
check looks up the parent device:

net/core/filter.c:bpf_skb_fib_lookup() {
    ...
    dev = dev_get_by_index_rcu(net, params->ifindex);
    if (unlikely(!dev))
        return -ENODEV;
    if (!is_skb_forwardable(dev, skb))
        rc = BPF_FIB_LKUP_RET_FRAG_NEEDED;
    params->mtu_result = dev->mtu;
    ...
}

If the VLAN device has a smaller MTU than the parent, will this incorrectly
pass oversized packets against the parent's MTU, or return the parent's
larger MTU for ICMP Fragmentation Needed responses?

> +		}
> +	}
> +#endif

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

[ ... ]

> @@ -7388,7 +7414,10 @@ struct bpf_fib_lookup {
>  
>  	union {
>  		struct {
> -			/* output */
> +			/* output with BPF_FIB_LOOKUP_VLAN: set from the
> +			 * resolved egress VLAN device (see the flag); zeroed
> +			 * on other successful lookups.
> +			 */

[Severity: Low]
This isn't a bug, but this copied UAPI multi-line comment also starts with
text on the same line as the opening /*.

>  			__be16	h_vlan_proto;
>  			__be16	h_vlan_TCI;
>  		};

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