Message-ID: <86801357-7262-40e5-b2bc-8429ac80ec7e@nvidia.com>
Date: Thu, 20 Nov 2025 09:20:11 +0800
From: Jianbo Liu <jianbol@...dia.com>
To: Sabrina Dubroca <sd@...asysnail.net>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
	<steffen.klassert@...unet.com>, Herbert Xu <herbert@...dor.apana.org.au>,
	David Ahern <dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com>, "Paolo
 Abeni" <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Cosmin Ratiu
	<cratiu@...dia.com>
Subject: Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO
 segmentation



On 11/19/2025 8:58 PM, Sabrina Dubroca wrote:
> 2025-11-17, 10:12:32 +0800, Jianbo Liu wrote:
>>
>>
>> On 11/17/2025 7:11 AM, Sabrina Dubroca wrote:
>>> 2025-11-14, 05:56:17 +0200, Jianbo Liu wrote:
>>>> Commit 61fafbee6cfe ("xfrm: Determine inner GSO type from packet
>>>> inner protocol") attempted to fix GSO segmentation by reading the
>>>> inner protocol from XFRM_MODE_SKB_CB(skb)->protocol. This was
>>>> incorrect as the XFRM_MODE_SKB_CB(skb)->protocol field is not assigned
>>>> a value in this code path and led to selecting the wrong inner mode.
>>>
>>> Your testing didn't catch it before the patch was submitted? :(
>>>
>>
>> I admit I didn't test all the cases for the previous submission, but I have
>> tested all the cases now with this fix.
>>
>>>
>>>> The correct value is in xfrm_offload(skb)->proto, which is set from
>>>> the outer tunnel header's protocol field by esp[4|6]_gso_encap(). It
>>>> is initialized by xfrm[4|6]_tunnel_encap_add() to either IPPROTO_IPIP
>>>> or IPPROTO_IPV6, using xfrm_af2proto() and correctly reflects the
>>>> inner packet's address family.
>>>
>>> What's the call sequence that leads to calling
>>> xfrm4_tunnel_gso_segment without setting
>>> XFRM_MODE_SKB_CB(skb)->protocol? I'm seeing
>>>
>>> xfrm_output -> xfrm_output2 -> xfrm_output_one
>>>    -> xfrm_outer_mode_output -> xfrm4_prepare_output
>>>    -> xfrm_inner_extract_output -> xfrm4_extract_output
>>>
>>> (almost same as what ends up calling xfrm[4|6]_tunnel_encap_add)
>>> so XFRM_MODE_SKB_CB(skb)->protocol should be set?
>>>
>>
>> I think we both made mistaken.
>> a. XFRM_MODE_SKB_CB(skb)->protocol is assigned in that path, but it is
>> assigned the value from ip_hdr(skb)->protocol. This means it holds the L4
>> protocol (e.g., IPPROTO_TCP or IPPROTO_UDP). However, to correctly determine
>> the inner mode family, we need the tunnel protocols (IPPROTO_IPIP or
>> IPPROTO_IPV6), which xfrm_af2proto() expects.
> 
> (not "expects" but "returns"? or did you mean
> s/xfrm_af2proto/xfrm_ip2inner_mode/?)
> 

Yes, I meant xfrm_ip2inner_mode. I apologize for the confusing mix-up in 
helper function names.

> Ah, right. Thanks. Then please update the commit message to explain
> that XFRM_MODE_SKB_CB(skb)->protocol is not the right value, rather
> than being unset.
> 
>> b. Furthermore, XFRM_MODE_SKB_CB(skb) shares the same memory layout as
>> XFRM_SKB_CB(skb). This area can be overwritten during the transformation
>> process (for example, in xfrm_replay_overflow and others), making the value
>> in XFRM_MODE_SKB_CB unreliable by the time we reach GSO segmentation.
> 
> Ok, that could also happen.
> 
>>> Also, after thinking about it more, I'm not so sure that
>>> xfrm_ip2inner_mode is wanted/needed in this context. Since we already
>>> have the inner protocol (whether it's via xo->proto or
>>> XFRM_MODE_SKB_CB(skb)->protocol), and all we care about is the inner
>>> family (to get the corresponding ethertype), we can just get it
>>> directly from the inner protocol without looking at
>>> x->inner_mode{,_iaf}? (pretty much just the reverse of xfrm_af2proto)
>>>
>>
>> I still prefer to reuse the logic in xfrm_af2proto()/xfrm_ip2inner_mode for
>> two main reasons: a. It keeps the code easier to understand by using
>> standard helpers rather than open-coding the reverse mapping.
> 
> We don't have to open-code it, we can add something like
> 
> static inline int xfrm_proto2af(unsigned int ipproto)
> {
> 	switch(ipproto) {
> 	case IPPROTO_IPIP:
> 		return AF_INET;
> 	case IPPROTO_IPV6:
> 		return AF_INET6;
> 	default:
> 		return 0;
> 	}
> }
> 
> 
> I don't think xfrm_ip2inner_mode, which does "if [some ipproto value]
> and [some x->* property] match then use inner_mode, otherwise use
> _iaf", is easier to understand. To me it seems clearer to add
> xfrm_proto2af.
> 

The simplicity of your helper is appealing, but I think we need to 
preserve the functionality of the existing helper for now.

> 
> And looking for all uses of inner_mode_iaf, I'm not sure we need this
> at all anymore. We only use inner_mode_iaf->family nowadays, and
> ->family is always "not x->props.family" (one of AF_INET/AF_INET6), or
> 0 with unspec selector on transport mode (makes sense, there's no
> "inner" AF there). (but that's a separate issue)
> 

The inner_mode_iaf is required because it holds several fields (maybe 
more if extended in the future) for the inner mode, not just the address 
family.

> 
> I'd be ok with using xfrm_ip2inner_mode for this fix and trying to
> clean this up later in -next.

I will incorporate the feedback into the commit message and push the new 
version soon.

Thanks!
Jianbo

> 
>> b. It keeps
>> the logic directly related to the xfrm configuration and state properties.
>>
>> Thanks!
>> Jianbo
>>
> 

Powered by blists - more mailing lists