* [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).