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

[Sabrina Dubroca <sd@queasysnail.net>]

2025-11-20, 09:20:11 +0800, Jianbo Liu wrote:
> 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:
> > > > > 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.

No worries. Thanks for clarifying.

[...]
> > 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.

But the other fields are never used (and have the same value as those
from x->inner_mode, no need to check _iaf). Anyway, I'll propose a
cleanup later.

-- 
Sabrina