Re: [PATCH ipsec v2 2/2] xfrm: Determine inner GSO type from packet inner protocol

[Jianbo Liu <jianbol@nvidia.com>]

On 10/27/2025 11:33 PM, Sabrina Dubroca wrote:
> 2025-10-27, 04:40:59 +0200, Jianbo Liu wrote:
>> The GSO segmentation functions for ESP tunnel mode
>> (xfrm4_tunnel_gso_segment and xfrm6_tunnel_gso_segment) were
>> determining the inner packet's L2 protocol type by checking the static
>> x->inner_mode.family field from the xfrm state.
>>
>> This is unreliable. In tunnel mode, the state's actual inner family
>> could be defined by x->inner_mode.family or by
>> x->inner_mode_iaf.family. Checking only the former can lead to a
>> mismatch with the actual packet being processed, causing GSO to create
>> segments with the wrong L2 header type.
>>
>> This patch fixes the bug by deriving the inner mode directly from the
>> packet's inner protocol stored in XFRM_MODE_SKB_CB(skb)->protocol.
>>
>> Fixes: 26dbd66eab80 ("esp: choose the correct inner protocol for GSO on inter address family tunnels")
>> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
>> Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
>> ---
>> V2:
>>   - Change subject prefix, and send to "ipsec".
>>   - Add Fixes tag.
>>
>>   net/ipv4/esp4_offload.c | 6 ++++--
>>   net/ipv6/esp6_offload.c | 6 ++++--
>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
>> index e0d94270da28..05828d4cb6cd 100644
>> --- a/net/ipv4/esp4_offload.c
>> +++ b/net/ipv4/esp4_offload.c
>> @@ -122,8 +122,10 @@ static struct sk_buff *xfrm4_tunnel_gso_segment(struct xfrm_state *x,
>>   						struct sk_buff *skb,
>>   						netdev_features_t features)
>>   {
>> -	__be16 type = x->inner_mode.family == AF_INET6 ? htons(ETH_P_IPV6)
>> -						       : htons(ETH_P_IP);
>> +	const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x,
>> +					XFRM_MODE_SKB_CB(skb)->protocol);
> 
> I don't think this is correct. inner_mode_iaf is not always set by
> __xfrm_init_state, only when we have a AF_UNSPEC selector, which is
> not always the case for cross-family tunnels. So we would end up with
> inner_mode->family = 0 here, and pass the wrong type to
> skb_eth_gso_segment.
> 
> Other users of xfrm_ip2inner_mode (in ip_vti/ip6_vti, xfrmi) only call
> it when we have an AF_UNSPEC selector, then we know inner_mode_iaf is
> valid and can be used. Otherwise, the selector (ie x->inner_mode)
> should have the right family for the packet (and all callers of
> xfrm_ip2inner_mode use x->inner_mode when the selector is not
> AF_UNSPEC).
> 
> 
> Maybe it would be better to move the AF_UNSPEC check into
> xfrm_ip2inner_mode, something like:
> 
> static inline const struct xfrm_mode *xfrm_ip2inner_mode(struct xfrm_state *x, int ipproto)
> {
> 	if (x->sel.family != AF_UNSPEC)
> 		return &x->inner_mode;
> 
> 	if ((ipproto == IPPROTO_IPIP && x->props.family == AF_INET) ||
> 	    (ipproto == IPPROTO_IPV6 && x->props.family == AF_INET6))
> 		return &x->inner_mode;
> 	else
> 		return &x->inner_mode_iaf;
> }
> 
> 
> since that's what all the callers are doing anyway?
> 
> Then it would be valid to use xfrm_ip2inner_mode like you're doing.

It makes sense. I will submit v3 soon. Thanks!

>