Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation

[Sabrina Dubroca <sd@queasysnail.net>]

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/?)

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.


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)


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

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

-- 
Sabrina