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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch fails to clear SKB_GSO_TUNNEL_REMCSUM when decapsulating UDP or GRE tunnels, leaving the SKB in a corrupted GSO state.
- [High] Unconditionally clearing `skb->encapsulation` for non-GSO packets breaks hardware ESP offloads for non-GSO ESP-in-UDP packets.
--

commit 531781236d1767d9010b79132e818c1dd8e04a3e
Author: Nick Hudson <nhudson@akamai.com>

bpf: clear decap state on skb_adjust_room shrink path

This commit updates bpf_skb_adjust_room() to selectively clear decapsulation
state when shrinking the skb. For GSO packets, it removes only the gso_type
bits corresponding to the requested decap layer and clears skb->encapsulation
if no tunnel bits remain. For non-GSO packets, it unconditionally clears
skb->encapsulation.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 04871a57f085..ff1c936bb46d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -3666,9 +3666,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);
> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_L4_GRE) &&
> +		    (shinfo->gso_type & (SKB_GSO_GRE | SKB_GSO_GRE_CSUM)))
> +			shinfo->gso_type &= ~(SKB_GSO_GRE |
> +					      SKB_GSO_GRE_CSUM);

When decapsulating a tunnel that uses Remote Checksum Offload (like FOU or
VXLAN), the SKB has both the tunnel GSO flag and SKB_GSO_TUNNEL_REMCSUM set
in shinfo->gso_type.

Does this code leave SKB_GSO_TUNNEL_REMCSUM set when decapsulating these UDP
or GRE tunnels?

> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_IPXIP4) &&
> +		    (shinfo->gso_type & SKB_GSO_IPXIP4))
> +			shinfo->gso_type &= ~SKB_GSO_IPXIP4;
> +		if ((flags & BPF_F_ADJ_ROOM_DECAP_IPXIP6) &&
> +		    (shinfo->gso_type & SKB_GSO_IPXIP6))
> +			shinfo->gso_type &= ~SKB_GSO_IPXIP6;
> +
> +		/* 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;

Because the exclusionary mask above doesn't include SKB_GSO_TUNNEL_REMCSUM (or
SKB_GSO_PARTIAL), will this check evaluate to true and incorrectly execute
skb->encapsulation = 0 if a remote checksum flag is the only one remaining?

Could this leave the SKB in a corrupted GSO state with a tunnel remote
checksum flag active but skb->encapsulation == 0, potentially leading to
incorrect checksum calculations, out-of-bounds inner header accesses, or
kernel panics during GSO segmentation?

> +		}
> +
>  		/* Header must be checked, and gso_segs recomputed. */
>  		shinfo->gso_type |= SKB_GSO_DODGY;
>  		shinfo->gso_segs = 0;
> +	} else {
> +		/* For non-GSO packets, clear encapsulation if decap flags are set */
> +		if ((flags & (BPF_F_ADJ_ROOM_DECAP_L4_MASK |
> +			      BPF_F_ADJ_ROOM_DECAP_IPXIP_MASK)) &&
> +		    skb->encapsulation)
> +			skb->encapsulation = 0;
>  	}

The commit message notes the goal of keeping encapsulation set for cases like
ESP-in-UDP where tunnel state remains. Does unconditionally clearing
skb->encapsulation here for non-GSO packets break hardware ESP offloads for
non-GSO ESP-in-UDP packets?

Hardware NIC drivers and the IPsec stack (such as xfrm_dev_offload_ok) strictly
rely on skb->encapsulation == 1 to offload ESP crypto and checksums, even for
non-GSO packets.

Could this unconditionally clear operation destroy ESP offloading for non-GSO
packets, introducing the inconsistency between GSO and non-GSO handling that
the commit is intending to prevent?

>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504101759.3319427-1-nhudson@akamai.com?part=5