Re: [PATCH bpf-next v7 5/6] bpf: clear decap state on skb_adjust_room shrink path

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf_skb_net_shrink() leaves SKB_GSO_PARTIAL and SKB_GSO_TUNNEL_REMCSUM dangling in shinfo->gso_type after removing a tunnel.
--

--- Patch [5]: [PATCH bpf-next v7 5/6] bpf: clear decap state on skb_adjust_room shrink path ---
commit 672aa1a359079a58d5e4d80fc47569e00a437a80
Author: Nick Hudson <[email protected]>
bpf: clear decap state on skb_adjust_room shrink path

This commit updates bpf_skb_adjust_room() on the shrink path to conditionally
clear decapsulation state updates based on BPF_F_ADJ_ROOM_DECAP_* flags. For
GSO skbs, it selectively clears tunnel gso_type bits and encapsulation, while
for non-GSO skbs it directly clears the encapsulation flag.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 981b75941e7f7..89effff3bc525 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3754,9 +3754,48 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff,
>  		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
>  			skb_increase_gso_size(shinfo, len_diff);
>  
> +		/* Selective GSO flag clearing based on decap type.
> +		 * Only clear the flags for the tunnel layer being removed.
> +		 */
> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_L4_UDP) &&
> +		    (shinfo->gso_type & (SKB_GSO_UDP_TUNNEL |
> +					 SKB_GSO_UDP_TUNNEL_CSUM)))
> +			shinfo->gso_type &= ~(SKB_GSO_UDP_TUNNEL |
> +					      SKB_GSO_UDP_TUNNEL_CSUM);

[Severity: High]
Does this leave SKB_GSO_PARTIAL and SKB_GSO_TUNNEL_REMCSUM dangling in
shinfo->gso_type after removing a tunnel?

If an SKB acquires SKB_GSO_PARTIAL or SKB_GSO_TUNNEL_REMCSUM (for example,
via software partial segmentation across a veth pair or GRO) and a BPF program
uses bpf_skb_adjust_room() to shrink the SKB, it seems the base tunnel flags
are cleared but the modifier flags remain.

Hardware drivers like ixgbe or fbnic use SKB_GSO_PARTIAL to identify packets
requiring partial GSO offload or special tunnel logic. Could leaving this flag
on a decapsulated, non-tunnel SKB cause drivers to misinterpret the packet
structure and corrupt the packet on transmit?

[ ... ]

> +		/* Clear encapsulation flag only when no tunnel GSO flags remain */
> +		if (flags & (BPF_F_ADJ_ROOM_DECAP_L4_MASK |
> +			     BPF_F_ADJ_ROOM_DECAP_IPXIP_MASK)) {
> +			if (!(shinfo->gso_type & (SKB_GSO_UDP_TUNNEL |
> +						  SKB_GSO_UDP_TUNNEL_CSUM |
> +						  SKB_GSO_GRE |
> +						  SKB_GSO_GRE_CSUM |
> +						  SKB_GSO_IPXIP4 |
> +						  SKB_GSO_IPXIP6 |
> +						  SKB_GSO_ESP)))
> +				if (skb->encapsulation)
> +					skb->encapsulation = 0;
> +		}

[Severity: High]
If all base tunnel flags are cleared and encapsulation is set to 0, which
confirms the packet is no longer a tunnel, is it expected that SKB_GSO_PARTIAL
and SKB_GSO_TUNNEL_REMCSUM might still be set in gso_type?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5