netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
@ 2025-11-14  3:56 Jianbo Liu
  2025-11-16 23:11 ` Sabrina Dubroca
  0 siblings, 1 reply; 7+ messages in thread
From: Jianbo Liu @ 2025-11-14  3:56 UTC (permalink / raw)
  To: netdev, davem, kuba, steffen.klassert
  Cc: Jianbo Liu, Herbert Xu, David Ahern, Eric Dumazet, Paolo Abeni,
	Simon Horman, Sabrina Dubroca, Cosmin Ratiu

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.

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.

Fixes: 61fafbee6cfe ("xfrm: Determine inner GSO type from packet inner protocol")
Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
---
 net/ipv4/esp4_offload.c | 4 ++--
 net/ipv6/esp6_offload.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index 05828d4cb6cd..abd77162f5e7 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -122,8 +122,8 @@ static struct sk_buff *xfrm4_tunnel_gso_segment(struct xfrm_state *x,
 						struct sk_buff *skb,
 						netdev_features_t features)
 {
-	const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x,
-					XFRM_MODE_SKB_CB(skb)->protocol);
+	struct xfrm_offload *xo = xfrm_offload(skb);
+	const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x, xo->proto);
 	__be16 type = inner_mode->family == AF_INET6 ? htons(ETH_P_IPV6)
 						     : htons(ETH_P_IP);
 
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 22410243ebe8..22895521a57d 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -158,8 +158,8 @@ static struct sk_buff *xfrm6_tunnel_gso_segment(struct xfrm_state *x,
 						struct sk_buff *skb,
 						netdev_features_t features)
 {
-	const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x,
-					XFRM_MODE_SKB_CB(skb)->protocol);
+	struct xfrm_offload *xo = xfrm_offload(skb);
+	const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x, xo->proto);
 	__be16 type = inner_mode->family == AF_INET ? htons(ETH_P_IP)
 						    : htons(ETH_P_IPV6);
 
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
  2025-11-14  3:56 [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation Jianbo Liu
@ 2025-11-16 23:11 ` Sabrina Dubroca
  2025-11-17  2:12   ` Jianbo Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Sabrina Dubroca @ 2025-11-16 23:11 UTC (permalink / raw)
  To: Jianbo Liu
  Cc: netdev, davem, kuba, steffen.klassert, Herbert Xu, David Ahern,
	Eric Dumazet, Paolo Abeni, Simon Horman, Cosmin Ratiu

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? :(


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


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)

-- 
Sabrina

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
  2025-11-16 23:11 ` Sabrina Dubroca
@ 2025-11-17  2:12   ` Jianbo Liu
  2025-11-19 12:58     ` Sabrina Dubroca
  0 siblings, 1 reply; 7+ messages in thread
From: Jianbo Liu @ 2025-11-17  2:12 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: netdev, davem, kuba, steffen.klassert, Herbert Xu, David Ahern,
	Eric Dumazet, Paolo Abeni, Simon Horman, Cosmin Ratiu



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.

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.

> 
> 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. b. It 
keeps the logic directly related to the xfrm configuration and state 
properties.

Thanks!
Jianbo


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
  2025-11-17  2:12   ` Jianbo Liu
@ 2025-11-19 12:58     ` Sabrina Dubroca
  2025-11-20  1:20       ` Jianbo Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Sabrina Dubroca @ 2025-11-19 12:58 UTC (permalink / raw)
  To: Jianbo Liu
  Cc: netdev, davem, kuba, steffen.klassert, Herbert Xu, David Ahern,
	Eric Dumazet, Paolo Abeni, Simon Horman, Cosmin Ratiu

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
  2025-11-19 12:58     ` Sabrina Dubroca
@ 2025-11-20  1:20       ` Jianbo Liu
  2025-11-20 11:41         ` Sabrina Dubroca
  0 siblings, 1 reply; 7+ messages in thread
From: Jianbo Liu @ 2025-11-20  1:20 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: netdev, davem, kuba, steffen.klassert, Herbert Xu, David Ahern,
	Eric Dumazet, Paolo Abeni, Simon Horman, Cosmin Ratiu



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
>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
  2025-11-20  1:20       ` Jianbo Liu
@ 2025-11-20 11:41         ` Sabrina Dubroca
  2025-11-21  2:03           ` Jianbo Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Sabrina Dubroca @ 2025-11-20 11:41 UTC (permalink / raw)
  To: Jianbo Liu
  Cc: netdev, davem, kuba, steffen.klassert, Herbert Xu, David Ahern,
	Eric Dumazet, Paolo Abeni, Simon Horman, Cosmin Ratiu

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation
  2025-11-20 11:41         ` Sabrina Dubroca
@ 2025-11-21  2:03           ` Jianbo Liu
  0 siblings, 0 replies; 7+ messages in thread
From: Jianbo Liu @ 2025-11-21  2:03 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: netdev, davem, kuba, steffen.klassert, Herbert Xu, David Ahern,
	Eric Dumazet, Paolo Abeni, Simon Horman, Cosmin Ratiu



On 11/20/2025 7:41 PM, Sabrina Dubroca wrote:
> 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.
> 

OK, I'm happy to see your proposed cleanup patch soon.

Just a friendly reminder. Could you please confirm the latest version of 
this patch is okay and add your RB?
https://lore.kernel.org/netdev/20251120035856.12337-1-jianbol@nvidia.com/

Thanks!
Jianbo


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-11-21  2:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-14  3:56 [PATCH ipsec] xfrm: Fix inner mode lookup in tunnel mode GSO segmentation Jianbo Liu
2025-11-16 23:11 ` Sabrina Dubroca
2025-11-17  2:12   ` Jianbo Liu
2025-11-19 12:58     ` Sabrina Dubroca
2025-11-20  1:20       ` Jianbo Liu
2025-11-20 11:41         ` Sabrina Dubroca
2025-11-21  2:03           ` Jianbo Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).