Re: [PATCH bpf-next v7 4/6] bpf: allow new DECAP flags and add guard rails

[bot+bpf-ci]

> commit e891bb72fdfdae2b71119ffb8be655bb3a4b26a7
> Author: Nick Hudson <nhudson@xxxxxxxxxx>
> Date:   Wed Aug 12 09:31:13 2026 +0100
>
>     bpf: allow new DECAP flags and add guard rails
>
>     Add checks to require shrink-only decap, reject conflicting decap flag
>     combinations, and verify removed length is sufficient for claimed header
>     decapsulation.
>
>     Co-developed-by: Max Tottenham <mtottenh@xxxxxxxxxx>
>     Signed-off-by: Max Tottenham <mtottenh@xxxxxxxxxx>
>     Co-developed-by: Anna Glasgall <aglasgal@xxxxxxxxxx>
>     Signed-off-by: Anna Glasgall <aglasgal@xxxxxxxxxx>
>     Signed-off-by: Nick Hudson <nhudson@xxxxxxxxxx>
>     Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>

The changelog states this patch "Add checks to require shrink-only decap",
but that check already exists in the parent commit. From e891bb72^:

    if (flags & BPF_F_ADJ_ROOM_DECAP_MASK) {
            if (!shrink)
                    return -EINVAL;

The diff confirms both the shrink check and the L3 pair check are context
lines, not additions. What this patch actually does is extend the coverage
of those pre-existing checks by widening BPF_F_ADJ_ROOM_DECAP_MASK to
include the new L4 and IPXIP flags.

Could the changelog be reworded to clarify this extends existing checks to
the new flags, rather than implying it adds the shrink-only requirement?

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 89e516049c852..981b75941e7f7 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -3827,6 +3836,8 @@ BPF_CALL_4(bpf_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
>  	}
>
>  	if (flags & BPF_F_ADJ_ROOM_DECAP_MASK) {
> +		u32 len_decap_min = 0;
> +
>  		if (!shrink)
>  			return -EINVAL;
>
> @@ -3835,6 +3846,37 @@ BPF_CALL_4(bpf_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
>  		    BPF_F_ADJ_ROOM_DECAP_L3_MASK)
>  			return -EINVAL;
>
> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_L4_MASK) ==
> +		    BPF_F_ADJ_ROOM_DECAP_L4_MASK)
> +			return -EINVAL;
> +
> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_IPXIP_MASK) ==
> +		    BPF_F_ADJ_ROOM_DECAP_IPXIP_MASK)
> +			return -EINVAL;
> +
> +		/* Reject mutually exclusive decap tunnel type flags. */
> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_L4_MASK) &&
> +		    (flags & BPF_F_ADJ_ROOM_DECAP_IPXIP_MASK))
> +			return -EINVAL;
> +
> +		if (flags & BPF_F_ADJ_ROOM_DECAP_L4_MASK)
> +			len_decap_min += bpf_skb_net_base_len(skb);
> +
> +		if (flags & BPF_F_ADJ_ROOM_DECAP_L4_UDP)
> +			len_decap_min += sizeof(struct udphdr);
> +
> +		if (flags & BPF_F_ADJ_ROOM_DECAP_L4_GRE)
> +			len_decap_min += sizeof(struct gre_base_hdr);
> +
> +		if (flags & BPF_F_ADJ_ROOM_DECAP_IPXIP4)
> +			len_decap_min += sizeof(struct iphdr);
> +
> +		if (flags & BPF_F_ADJ_ROOM_DECAP_IPXIP6)
> +			len_decap_min += sizeof(struct ipv6hdr);
> +
> +		if (len_diff_abs < len_decap_min)
> +			return -EINVAL;
> +

Does this minimum-length check account for the mode parameter? The
calculation assumes mode == BPF_ADJ_ROOM_MAC, but nothing constrains mode
when DECAP flags are set.

Earlier in this function, off is computed:

    off = skb_mac_header_len(skb);
    switch (mode) {
    case BPF_ADJ_ROOM_NET:
            off += bpf_skb_net_base_len(skb);
            break;
    case BPF_ADJ_ROOM_MAC:
            break;

In BPF_ADJ_ROOM_NET mode, the removal window starts after the outer L3
header, so that header is not part of the len_diff_abs bytes being removed.

Yet when an L4 decap flag is present, the check adds the outer L3 length
unconditionally:

    if (flags & BPF_F_ADJ_ROOM_DECAP_L4_MASK)
            len_decap_min += bpf_skb_net_base_len(skb);

Consider a tc program on an IPv4 outer packet calling:

    bpf_skb_adjust_room(skb, -8, BPF_ADJ_ROOM_NET,
                        BPF_F_ADJ_ROOM_DECAP_L4_UDP)

to strip an 8-byte FOU/UDP tunnel header. This would compute len_decap_min
= 20 + 8 = 28, so len_diff_abs (8) < len_decap_min (28) causes -EINVAL even
though the request removes exactly the claimed header.

The same mismatch applies to the IPXIP terms: in NET mode the outer IP
header is outside the removal window entirely, yet
BPF_F_ADJ_ROOM_DECAP_IPXIP4/6 requires 20/40 bytes to be removed.

Should this either reject mode != BPF_ADJ_ROOM_MAC when
BPF_F_ADJ_ROOM_DECAP_MASK is set, or make the bpf_skb_net_base_len() term
conditional on mode == BPF_ADJ_ROOM_MAC?

>  		if (flags & BPF_F_ADJ_ROOM_DECAP_L3_IPV4)
>  			len_min = sizeof(struct iphdr);


---
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/31584739682