* [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination
@ 2025-10-28 2:22 Jianbo Liu
2025-10-28 2:22 ` [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst Jianbo Liu
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jianbo Liu @ 2025-10-28 2:22 UTC (permalink / raw)
To: netdev, davem, kuba, steffen.klassert, sd; +Cc: Jianbo Liu
This series contains two patches addressing issues in the XFRM
subsystem where the code incorrectly relied on static family fields
from the xfrm_state instead of determining the family from the actual
packet being processed.
This was particularly problematic in tunnel mode scenarios when using
states that could handle different inner families.
V3:
- Change xfrm_ip2inner_mode for the sel family specified
V2:
- The original first patch was sent separately to "ipsec-next"
Jianbo Liu (2):
xfrm: Check inner packet family directly from skb_dst
xfrm: Determine inner GSO type from packet inner protocol
include/net/xfrm.h | 3 ++-
net/ipv4/esp4_offload.c | 6 ++++--
net/ipv6/esp6_offload.c | 6 ++++--
net/xfrm/xfrm_device.c | 2 +-
net/xfrm/xfrm_output.c | 2 +-
5 files changed, 12 insertions(+), 7 deletions(-)
--
2.49.0
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst 2025-10-28 2:22 [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Jianbo Liu @ 2025-10-28 2:22 ` Jianbo Liu 2025-10-28 14:27 ` Zhu Yanjun 2025-10-28 2:22 ` [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol Jianbo Liu 2025-11-01 12:29 ` [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Steffen Klassert 2 siblings, 1 reply; 11+ messages in thread From: Jianbo Liu @ 2025-10-28 2:22 UTC (permalink / raw) To: netdev, davem, kuba, steffen.klassert, sd Cc: Jianbo Liu, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, Zhu Yanjun, Leon Romanovsky, Raed Salem In the output path, xfrm_dev_offload_ok and xfrm_get_inner_ipproto need to determine the protocol family of the inner packet (skb) before it gets encapsulated. In xfrm_dev_offload_ok, the code checked x->inner_mode.family. This is unreliable because, for states handling both IPv4 and IPv6, the relevant inner family could be either x->inner_mode.family or x->inner_mode_iaf.family. Checking only the former can lead to a mismatch with the actual packet being processed. In xfrm_get_inner_ipproto, the code checked x->outer_mode.family. This is also incorrect for tunnel mode, as the inner packet's family can be different from the outer header's family. At both of these call sites, the skb variable holds the original inner packet. The most direct and reliable source of truth for its protocol family is its destination entry. This patch fixes the issue by using skb_dst(skb)->ops->family to ensure protocol-specific headers are only accessed for the correct packet type. Fixes: 91d8a53db219 ("xfrm: fix offloading of cross-family tunnels") Fixes: 45a98ef4922d ("net/xfrm: IPsec tunnel mode fix inner_ipproto setting in sec_path") Signed-off-by: Jianbo Liu <jianbol@nvidia.com> Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com> --- V2: - Change subject prefix, and send to "ipsec". - Update commit msg. - Add Fixes tag. net/xfrm/xfrm_device.c | 2 +- net/xfrm/xfrm_output.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index 44b9de6e4e77..52ae0e034d29 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x) check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET && x->props.mode == XFRM_MODE_TUNNEL; - switch (x->inner_mode.family) { + switch (skb_dst(skb)->ops->family) { case AF_INET: /* Check for IPv4 options */ if (ip_hdr(skb)->ihl != 5) diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c index 9077730ff7d0..a98b5bf55ac3 100644 --- a/net/xfrm/xfrm_output.c +++ b/net/xfrm/xfrm_output.c @@ -698,7 +698,7 @@ static void xfrm_get_inner_ipproto(struct sk_buff *skb, struct xfrm_state *x) return; if (x->outer_mode.encap == XFRM_MODE_TUNNEL) { - switch (x->outer_mode.family) { + switch (skb_dst(skb)->ops->family) { case AF_INET: xo->inner_ipproto = ip_hdr(skb)->protocol; break; -- 2.49.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst 2025-10-28 2:22 ` [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst Jianbo Liu @ 2025-10-28 14:27 ` Zhu Yanjun 0 siblings, 0 replies; 11+ messages in thread From: Zhu Yanjun @ 2025-10-28 14:27 UTC (permalink / raw) To: Jianbo Liu, netdev, davem, kuba, steffen.klassert, sd Cc: Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, Leon Romanovsky, Raed Salem 在 2025/10/27 19:22, Jianbo Liu 写道: > In the output path, xfrm_dev_offload_ok and xfrm_get_inner_ipproto > need to determine the protocol family of the inner packet (skb) before > it gets encapsulated. > > In xfrm_dev_offload_ok, the code checked x->inner_mode.family. This is > unreliable because, for states handling both IPv4 and IPv6, the > relevant inner family could be either x->inner_mode.family or > x->inner_mode_iaf.family. Checking only the former can lead to a > mismatch with the actual packet being processed. > > In xfrm_get_inner_ipproto, the code checked x->outer_mode.family. This > is also incorrect for tunnel mode, as the inner packet's family can be > different from the outer header's family. > > At both of these call sites, the skb variable holds the original inner > packet. The most direct and reliable source of truth for its protocol > family is its destination entry. This patch fixes the issue by using > skb_dst(skb)->ops->family to ensure protocol-specific headers are only > accessed for the correct packet type. > > Fixes: 91d8a53db219 ("xfrm: fix offloading of cross-family tunnels") > Fixes: 45a98ef4922d ("net/xfrm: IPsec tunnel mode fix inner_ipproto setting in sec_path") > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com> Thanks a lot. I am fine with this. Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> Zhu Yanjun > --- > V2: > - Change subject prefix, and send to "ipsec". > - Update commit msg. > - Add Fixes tag. > > net/xfrm/xfrm_device.c | 2 +- > net/xfrm/xfrm_output.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c > index 44b9de6e4e77..52ae0e034d29 100644 > --- a/net/xfrm/xfrm_device.c > +++ b/net/xfrm/xfrm_device.c > @@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x) > > check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET && > x->props.mode == XFRM_MODE_TUNNEL; > - switch (x->inner_mode.family) { > + switch (skb_dst(skb)->ops->family) { > case AF_INET: > /* Check for IPv4 options */ > if (ip_hdr(skb)->ihl != 5) > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c > index 9077730ff7d0..a98b5bf55ac3 100644 > --- a/net/xfrm/xfrm_output.c > +++ b/net/xfrm/xfrm_output.c > @@ -698,7 +698,7 @@ static void xfrm_get_inner_ipproto(struct sk_buff *skb, struct xfrm_state *x) > return; > > if (x->outer_mode.encap == XFRM_MODE_TUNNEL) { > - switch (x->outer_mode.family) { > + switch (skb_dst(skb)->ops->family) { > case AF_INET: > xo->inner_ipproto = ip_hdr(skb)->protocol; > break; -- Best Regards, Yanjun.Zhu ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-28 2:22 [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Jianbo Liu 2025-10-28 2:22 ` [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst Jianbo Liu @ 2025-10-28 2:22 ` Jianbo Liu 2025-10-28 11:03 ` Sabrina Dubroca 2025-11-01 12:29 ` [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Steffen Klassert 2 siblings, 1 reply; 11+ messages in thread From: Jianbo Liu @ 2025-10-28 2:22 UTC (permalink / raw) To: netdev, davem, kuba, steffen.klassert, sd Cc: Jianbo Liu, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern The GSO segmentation functions for ESP tunnel mode (xfrm4_tunnel_gso_segment and xfrm6_tunnel_gso_segment) were determining the inner packet's L2 protocol type by checking the static x->inner_mode.family field from the xfrm state. This is unreliable. In tunnel mode, the state's actual inner family could be defined by x->inner_mode.family or by x->inner_mode_iaf.family. Checking only the former can lead to a mismatch with the actual packet being processed, causing GSO to create segments with the wrong L2 header type. This patch fixes the bug by deriving the inner mode directly from the packet's inner protocol stored in XFRM_MODE_SKB_CB(skb)->protocol. Instead of replicating the code, this patch modifies the xfrm_ip2inner_mode helper function. It now correctly returns &x->inner_mode if the selector family (x->sel.family) is already specified, thereby handling both specific and AF_UNSPEC cases appropriately. With this change, ESP GSO can use xfrm_ip2inner_mode to get the correct inner mode. It doesn't affect existing callers, as the updated logic now mirrors the checks they were already performing externally. Fixes: 26dbd66eab80 ("esp: choose the correct inner protocol for GSO on inter address family tunnels") Signed-off-by: Jianbo Liu <jianbol@nvidia.com> Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com> --- V3: - Change xfrm_ip2inner_mode for the sel family specified V2: - Change subject prefix, and send to "ipsec". - Add Fixes tag. include/net/xfrm.h | 3 ++- net/ipv4/esp4_offload.c | 6 ++++-- net/ipv6/esp6_offload.c | 6 ++++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/include/net/xfrm.h b/include/net/xfrm.h index f3014e4f54fc..0a14daaa5dd4 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -536,7 +536,8 @@ static inline int xfrm_af2proto(unsigned int family) static inline const struct xfrm_mode *xfrm_ip2inner_mode(struct xfrm_state *x, int ipproto) { - if ((ipproto == IPPROTO_IPIP && x->props.family == AF_INET) || + if ((x->sel.family != AF_UNSPEC) || + (ipproto == IPPROTO_IPIP && x->props.family == AF_INET) || (ipproto == IPPROTO_IPV6 && x->props.family == AF_INET6)) return &x->inner_mode; else diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c index e0d94270da28..05828d4cb6cd 100644 --- a/net/ipv4/esp4_offload.c +++ b/net/ipv4/esp4_offload.c @@ -122,8 +122,10 @@ static struct sk_buff *xfrm4_tunnel_gso_segment(struct xfrm_state *x, struct sk_buff *skb, netdev_features_t features) { - __be16 type = x->inner_mode.family == AF_INET6 ? htons(ETH_P_IPV6) - : htons(ETH_P_IP); + const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x, + XFRM_MODE_SKB_CB(skb)->protocol); + __be16 type = inner_mode->family == AF_INET6 ? htons(ETH_P_IPV6) + : htons(ETH_P_IP); return skb_eth_gso_segment(skb, features, type); } diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c index 7b41fb4f00b5..22410243ebe8 100644 --- a/net/ipv6/esp6_offload.c +++ b/net/ipv6/esp6_offload.c @@ -158,8 +158,10 @@ static struct sk_buff *xfrm6_tunnel_gso_segment(struct xfrm_state *x, struct sk_buff *skb, netdev_features_t features) { - __be16 type = x->inner_mode.family == AF_INET ? htons(ETH_P_IP) - : htons(ETH_P_IPV6); + const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x, + XFRM_MODE_SKB_CB(skb)->protocol); + __be16 type = inner_mode->family == AF_INET ? htons(ETH_P_IP) + : htons(ETH_P_IPV6); return skb_eth_gso_segment(skb, features, type); } -- 2.49.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-28 2:22 ` [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol Jianbo Liu @ 2025-10-28 11:03 ` Sabrina Dubroca 2025-10-28 13:36 ` Jianbo Liu 0 siblings, 1 reply; 11+ messages in thread From: Sabrina Dubroca @ 2025-10-28 11:03 UTC (permalink / raw) To: Jianbo Liu Cc: netdev, davem, kuba, steffen.klassert, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern 2025-10-28, 04:22:48 +0200, Jianbo Liu wrote: > The GSO segmentation functions for ESP tunnel mode > (xfrm4_tunnel_gso_segment and xfrm6_tunnel_gso_segment) were > determining the inner packet's L2 protocol type by checking the static > x->inner_mode.family field from the xfrm state. > > This is unreliable. In tunnel mode, the state's actual inner family > could be defined by x->inner_mode.family or by > x->inner_mode_iaf.family. Checking only the former can lead to a > mismatch with the actual packet being processed, causing GSO to create > segments with the wrong L2 header type. > > This patch fixes the bug by deriving the inner mode directly from the > packet's inner protocol stored in XFRM_MODE_SKB_CB(skb)->protocol. > > Instead of replicating the code, this patch modifies the > xfrm_ip2inner_mode helper function. It now correctly returns > &x->inner_mode if the selector family (x->sel.family) is already > specified, thereby handling both specific and AF_UNSPEC cases > appropriately. (nit: I think this paragraph goes a bit too much into describing the changes between versions) > With this change, ESP GSO can use xfrm_ip2inner_mode to get the > correct inner mode. It doesn't affect existing callers, as the updated > logic now mirrors the checks they were already performing externally. Sorry, maybe I wasn't clear, but I meant that the callers should also be updated to not do the AF_UNSPEC check anymore (note: this will cause merge conflicts with your "NULL inner_mode" cleanup patch [1]). And I think it would be nicer to split the refactoring into a separate patch. So this series would be: patch 1: fix xfrm_dev_offload_ok and xfrm_get_inner_ipproto (same as now) patch 2: modify xfrm_ip2inner_mode and remove the AF_UNSPEC check and setting inner_mode = &x->inner_mode from all callers [no behavior change, just a refactoring to prepare for patch 3] patch 3: use xfrm_ip2inner_mode for GSO (same as your v2 patch 2/2) Does that seem ok to you? And to avoid the merge conflict with [1], maybe it also makes more sense to integrate that clean up in patch 2 from the list above, so for ip_vti we'd have: diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c index 95b6bb78fcd2..89784976c65e 100644 --- a/net/ipv4/ip_vti.c +++ b/net/ipv4/ip_vti.c @@ -118,16 +118,7 @@ static int vti_rcv_cb(struct sk_buff *skb, int err) x = xfrm_input_state(skb); - inner_mode = &x->inner_mode; - - if (x->sel.family == AF_UNSPEC) { - inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol); - if (inner_mode == NULL) { - XFRM_INC_STATS(dev_net(skb->dev), - LINUX_MIB_XFRMINSTATEMODEERROR); - return -EINVAL; - } - } + inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol); family = inner_mode->family; Does that sound reasonable? [1] https://lore.kernel.org/netdev/20251027023818.46446-1-jianbol@nvidia.com/ -- Sabrina ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-28 11:03 ` Sabrina Dubroca @ 2025-10-28 13:36 ` Jianbo Liu 2025-10-28 15:04 ` Sabrina Dubroca 0 siblings, 1 reply; 11+ messages in thread From: Jianbo Liu @ 2025-10-28 13:36 UTC (permalink / raw) To: Sabrina Dubroca Cc: netdev, davem, kuba, steffen.klassert, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern On 10/28/2025 7:03 PM, Sabrina Dubroca wrote: > 2025-10-28, 04:22:48 +0200, Jianbo Liu wrote: >> The GSO segmentation functions for ESP tunnel mode >> (xfrm4_tunnel_gso_segment and xfrm6_tunnel_gso_segment) were >> determining the inner packet's L2 protocol type by checking the static >> x->inner_mode.family field from the xfrm state. >> >> This is unreliable. In tunnel mode, the state's actual inner family >> could be defined by x->inner_mode.family or by >> x->inner_mode_iaf.family. Checking only the former can lead to a >> mismatch with the actual packet being processed, causing GSO to create >> segments with the wrong L2 header type. >> >> This patch fixes the bug by deriving the inner mode directly from the >> packet's inner protocol stored in XFRM_MODE_SKB_CB(skb)->protocol. >> >> Instead of replicating the code, this patch modifies the >> xfrm_ip2inner_mode helper function. It now correctly returns >> &x->inner_mode if the selector family (x->sel.family) is already >> specified, thereby handling both specific and AF_UNSPEC cases >> appropriately. > > (nit: I think this paragraph goes a bit too much into describing the > changes between versions) > >> With this change, ESP GSO can use xfrm_ip2inner_mode to get the >> correct inner mode. It doesn't affect existing callers, as the updated >> logic now mirrors the checks they were already performing externally. > > Sorry, maybe I wasn't clear, but I meant that the callers should also > be updated to not do the AF_UNSPEC check anymore (note: this will > cause merge conflicts with your "NULL inner_mode" cleanup patch [1]). > > And I think it would be nicer to split the refactoring into a separate > patch. So this series would be: > > patch 1: fix xfrm_dev_offload_ok and xfrm_get_inner_ipproto (same as now) > patch 2: modify xfrm_ip2inner_mode and remove the AF_UNSPEC check and > setting inner_mode = &x->inner_mode from all callers > [no behavior change, just a refactoring to prepare for patch 3] > patch 3: use xfrm_ip2inner_mode for GSO (same as your v2 patch 2/2) > > Does that seem ok to you? > > > And to avoid the merge conflict with [1], maybe it also makes more > sense to integrate that clean up in patch 2 from the list above, so > for ip_vti we'd have: > > diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c > index 95b6bb78fcd2..89784976c65e 100644 > --- a/net/ipv4/ip_vti.c > +++ b/net/ipv4/ip_vti.c > @@ -118,16 +118,7 @@ static int vti_rcv_cb(struct sk_buff *skb, int err) > > x = xfrm_input_state(skb); > > - inner_mode = &x->inner_mode; > - > - if (x->sel.family == AF_UNSPEC) { > - inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol); > - if (inner_mode == NULL) { > - XFRM_INC_STATS(dev_net(skb->dev), > - LINUX_MIB_XFRMINSTATEMODEERROR); > - return -EINVAL; > - } > - } > + inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol); > > family = inner_mode->family; > > > > Does that sound reasonable? I have a concern regarding backporting. Patches 1 and 3 in your proposed structure are bug fixes that should ideally go into the ipsec tree and be suitable for stable backports. Patch 2 should be targeted to ipsec-next as refactoring often does. If so, patch 3 becomes dependent on a change that won't exist in older kernels, making it difficult to backport cleanly. To maintain backportability for the GSO fix, I'd prefer to keep the modification to xfrm_ip2inner_mode within the same patch that fixes the GSO code (which is currently my v3 patch 2/2). My proposed plan is: Send the patch 1 and patch 3 (including the xfrm_ip2inner_mode change) together to the ipsec tree. They are self-contained fixes. Separately, after those are accepted, I can modify and re-submit that patch [1] to ipsec-next that removes the now-redundant checks from the other callers (VTI, etc.), leveraging the updated helper function. This way, the critical fixes are self-contained and backportable, while the cleanup of other callers happens later in the development cycle. > > [1] https://lore.kernel.org/netdev/20251027023818.46446-1-jianbol@nvidia.com/ > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-28 13:36 ` Jianbo Liu @ 2025-10-28 15:04 ` Sabrina Dubroca 2025-10-30 8:08 ` Steffen Klassert 0 siblings, 1 reply; 11+ messages in thread From: Sabrina Dubroca @ 2025-10-28 15:04 UTC (permalink / raw) To: Jianbo Liu, steffen.klassert Cc: netdev, davem, kuba, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern 2025-10-28, 21:36:17 +0800, Jianbo Liu wrote: > > > On 10/28/2025 7:03 PM, Sabrina Dubroca wrote: > > 2025-10-28, 04:22:48 +0200, Jianbo Liu wrote: > > > The GSO segmentation functions for ESP tunnel mode > > > (xfrm4_tunnel_gso_segment and xfrm6_tunnel_gso_segment) were > > > determining the inner packet's L2 protocol type by checking the static > > > x->inner_mode.family field from the xfrm state. > > > > > > This is unreliable. In tunnel mode, the state's actual inner family > > > could be defined by x->inner_mode.family or by > > > x->inner_mode_iaf.family. Checking only the former can lead to a > > > mismatch with the actual packet being processed, causing GSO to create > > > segments with the wrong L2 header type. > > > > > > This patch fixes the bug by deriving the inner mode directly from the > > > packet's inner protocol stored in XFRM_MODE_SKB_CB(skb)->protocol. > > > > > > Instead of replicating the code, this patch modifies the > > > xfrm_ip2inner_mode helper function. It now correctly returns > > > &x->inner_mode if the selector family (x->sel.family) is already > > > specified, thereby handling both specific and AF_UNSPEC cases > > > appropriately. > > > > (nit: I think this paragraph goes a bit too much into describing the > > changes between versions) > > > > > With this change, ESP GSO can use xfrm_ip2inner_mode to get the > > > correct inner mode. It doesn't affect existing callers, as the updated > > > logic now mirrors the checks they were already performing externally. > > > > Sorry, maybe I wasn't clear, but I meant that the callers should also > > be updated to not do the AF_UNSPEC check anymore (note: this will > > cause merge conflicts with your "NULL inner_mode" cleanup patch [1]). > > > > And I think it would be nicer to split the refactoring into a separate > > patch. So this series would be: > > > > patch 1: fix xfrm_dev_offload_ok and xfrm_get_inner_ipproto (same as now) > > patch 2: modify xfrm_ip2inner_mode and remove the AF_UNSPEC check and > > setting inner_mode = &x->inner_mode from all callers > > [no behavior change, just a refactoring to prepare for patch 3] > > patch 3: use xfrm_ip2inner_mode for GSO (same as your v2 patch 2/2) > > > > Does that seem ok to you? > > > > > > And to avoid the merge conflict with [1], maybe it also makes more > > sense to integrate that clean up in patch 2 from the list above, so > > for ip_vti we'd have: > > > > diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c > > index 95b6bb78fcd2..89784976c65e 100644 > > --- a/net/ipv4/ip_vti.c > > +++ b/net/ipv4/ip_vti.c > > @@ -118,16 +118,7 @@ static int vti_rcv_cb(struct sk_buff *skb, int err) > > x = xfrm_input_state(skb); > > - inner_mode = &x->inner_mode; > > - > > - if (x->sel.family == AF_UNSPEC) { > > - inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol); > > - if (inner_mode == NULL) { > > - XFRM_INC_STATS(dev_net(skb->dev), > > - LINUX_MIB_XFRMINSTATEMODEERROR); > > - return -EINVAL; > > - } > > - } > > + inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol); > > family = inner_mode->family; > > > > > > Does that sound reasonable? > > I have a concern regarding backporting. > > Patches 1 and 3 in your proposed structure are bug fixes that should ideally > go into the ipsec tree and be suitable for stable backports. > Patch 2 should be targeted to ipsec-next as refactoring often does. If it's part of a bugfix series, I think it's ok to do a small refactoring. > If so, patch 3 becomes dependent on a change that won't exist in older > kernels, making it difficult to backport cleanly. It shouldn't be a problem to backport the refactoring, as this is code that doesn't change much (the code around calls of xfrm_ip2inner_mode hasn't been modified since 2019). > To maintain backportability for the GSO fix, I'd prefer to keep the > modification to xfrm_ip2inner_mode within the same patch that fixes the GSO > code (which is currently my v3 patch 2/2). > > My proposed plan is: > > Send the patch 1 and patch 3 (including the xfrm_ip2inner_mode change) > together to the ipsec tree. They are self-contained fixes. So, keep v3 of this series unchanged. > Separately, after those are accepted, I can modify and re-submit that patch > [1] to ipsec-next that removes the now-redundant checks from the other > callers (VTI, etc.), leveraging the updated helper function. > > This way, the critical fixes are self-contained and backportable, while the > cleanup of other callers happens later in the development cycle. The only (small) drawback is leaving the duplicate code checking AF_UNSPEC in the existing callers of xfrm_ip2inner_mode, but I guess that's ok. Steffen, is it ok for you to - have a duplicate AF_UNSPEC check in callers of xfrm_ip2inner_mode (the existing "default to x->inner_mode, call xfrm_ip2inner_mode if AF_UNSPEC", and the new one added to xfrm_ip2inner_mode by this patch) in the ipsec tree and then in stable? - do the clean up (like the diff I pasted in my previous email, or something smaller if [1] is applied separately) in ipsec-next after ipsec is merged into it? [1] https://lore.kernel.org/netdev/20251027023818.46446-1-jianbol@nvidia.com/ -- Sabrina ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-28 15:04 ` Sabrina Dubroca @ 2025-10-30 8:08 ` Steffen Klassert 2025-10-30 8:35 ` Jianbo Liu 2025-10-30 10:28 ` Sabrina Dubroca 0 siblings, 2 replies; 11+ messages in thread From: Steffen Klassert @ 2025-10-30 8:08 UTC (permalink / raw) To: Sabrina Dubroca Cc: Jianbo Liu, netdev, davem, kuba, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern On Tue, Oct 28, 2025 at 04:04:36PM +0100, Sabrina Dubroca wrote: > 2025-10-28, 21:36:17 +0800, Jianbo Liu wrote: > > > > My proposed plan is: > > > > Send the patch 1 and patch 3 (including the xfrm_ip2inner_mode change) > > together to the ipsec tree. They are self-contained fixes. > > So, keep v3 of this series unchanged. > > > Separately, after those are accepted, I can modify and re-submit that patch > > [1] to ipsec-next that removes the now-redundant checks from the other > > callers (VTI, etc.), leveraging the updated helper function. > > > > This way, the critical fixes are self-contained and backportable, while the > > cleanup of other callers happens later in the development cycle. > > The only (small) drawback is leaving the duplicate code checking > AF_UNSPEC in the existing callers of xfrm_ip2inner_mode, but I guess > that's ok. > > > Steffen, is it ok for you to > > - have a duplicate AF_UNSPEC check in callers of xfrm_ip2inner_mode > (the existing "default to x->inner_mode, call xfrm_ip2inner_mode if > AF_UNSPEC", and the new one added to xfrm_ip2inner_mode by this > patch) in the ipsec tree and then in stable? > > - do the clean up (like the diff I pasted in my previous email, or > something smaller if [1] is applied separately) in ipsec-next after > ipsec is merged into it? I'm OK with this, I can take v3 as is. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-30 8:08 ` Steffen Klassert @ 2025-10-30 8:35 ` Jianbo Liu 2025-10-30 10:28 ` Sabrina Dubroca 1 sibling, 0 replies; 11+ messages in thread From: Jianbo Liu @ 2025-10-30 8:35 UTC (permalink / raw) To: Steffen Klassert, Sabrina Dubroca Cc: netdev, davem, kuba, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern On 10/30/2025 4:08 PM, Steffen Klassert wrote: > On Tue, Oct 28, 2025 at 04:04:36PM +0100, Sabrina Dubroca wrote: >> 2025-10-28, 21:36:17 +0800, Jianbo Liu wrote: >>> >>> My proposed plan is: >>> >>> Send the patch 1 and patch 3 (including the xfrm_ip2inner_mode change) >>> together to the ipsec tree. They are self-contained fixes. >> >> So, keep v3 of this series unchanged. >> >>> Separately, after those are accepted, I can modify and re-submit that patch >>> [1] to ipsec-next that removes the now-redundant checks from the other >>> callers (VTI, etc.), leveraging the updated helper function. >>> >>> This way, the critical fixes are self-contained and backportable, while the >>> cleanup of other callers happens later in the development cycle. >> >> The only (small) drawback is leaving the duplicate code checking >> AF_UNSPEC in the existing callers of xfrm_ip2inner_mode, but I guess >> that's ok. >> >> >> Steffen, is it ok for you to >> >> - have a duplicate AF_UNSPEC check in callers of xfrm_ip2inner_mode >> (the existing "default to x->inner_mode, call xfrm_ip2inner_mode if >> AF_UNSPEC", and the new one added to xfrm_ip2inner_mode by this >> patch) in the ipsec tree and then in stable? >> >> - do the clean up (like the diff I pasted in my previous email, or >> something smaller if [1] is applied separately) in ipsec-next after >> ipsec is merged into it? > > I'm OK with this, I can take v3 as is. Great, thank you for confirming. Once this v3 series is merged, I will update and send that cleanup patch to ipsec-next, as discussed with Sabrina. Thanks! Jianbo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol 2025-10-30 8:08 ` Steffen Klassert 2025-10-30 8:35 ` Jianbo Liu @ 2025-10-30 10:28 ` Sabrina Dubroca 1 sibling, 0 replies; 11+ messages in thread From: Sabrina Dubroca @ 2025-10-30 10:28 UTC (permalink / raw) To: Steffen Klassert Cc: Jianbo Liu, netdev, davem, kuba, Cosmin Ratiu, Herbert Xu, Eric Dumazet, Paolo Abeni, Simon Horman, David Ahern 2025-10-30, 09:08:11 +0100, Steffen Klassert wrote: > On Tue, Oct 28, 2025 at 04:04:36PM +0100, Sabrina Dubroca wrote: > > 2025-10-28, 21:36:17 +0800, Jianbo Liu wrote: > > > > > > My proposed plan is: > > > > > > Send the patch 1 and patch 3 (including the xfrm_ip2inner_mode change) > > > together to the ipsec tree. They are self-contained fixes. > > > > So, keep v3 of this series unchanged. > > > > > Separately, after those are accepted, I can modify and re-submit that patch > > > [1] to ipsec-next that removes the now-redundant checks from the other > > > callers (VTI, etc.), leveraging the updated helper function. > > > > > > This way, the critical fixes are self-contained and backportable, while the > > > cleanup of other callers happens later in the development cycle. > > > > The only (small) drawback is leaving the duplicate code checking > > AF_UNSPEC in the existing callers of xfrm_ip2inner_mode, but I guess > > that's ok. > > > > > > Steffen, is it ok for you to > > > > - have a duplicate AF_UNSPEC check in callers of xfrm_ip2inner_mode > > (the existing "default to x->inner_mode, call xfrm_ip2inner_mode if > > AF_UNSPEC", and the new one added to xfrm_ip2inner_mode by this > > patch) in the ipsec tree and then in stable? > > > > - do the clean up (like the diff I pasted in my previous email, or > > something smaller if [1] is applied separately) in ipsec-next after > > ipsec is merged into it? > > I'm OK with this, I can take v3 as is. Ok. In that case, you can add: Reviewed-by: Sabrina Dubroca <sd@queasysnail.net> for both patches. Thanks. -- Sabrina ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination 2025-10-28 2:22 [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Jianbo Liu 2025-10-28 2:22 ` [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst Jianbo Liu 2025-10-28 2:22 ` [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol Jianbo Liu @ 2025-11-01 12:29 ` Steffen Klassert 2 siblings, 0 replies; 11+ messages in thread From: Steffen Klassert @ 2025-11-01 12:29 UTC (permalink / raw) To: Jianbo Liu; +Cc: netdev, davem, kuba, sd On Tue, Oct 28, 2025 at 04:22:46AM +0200, Jianbo Liu wrote: > This series contains two patches addressing issues in the XFRM > subsystem where the code incorrectly relied on static family fields > from the xfrm_state instead of determining the family from the actual > packet being processed. > > This was particularly problematic in tunnel mode scenarios when using > states that could handle different inner families. > > V3: > - Change xfrm_ip2inner_mode for the sel family specified > > V2: > - The original first patch was sent separately to "ipsec-next" > > Jianbo Liu (2): > xfrm: Check inner packet family directly from skb_dst > xfrm: Determine inner GSO type from packet inner protocol Applied, thaks a lot Jianbo! ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-11-01 12:29 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-28 2:22 [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Jianbo Liu 2025-10-28 2:22 ` [PATCH ipsec v3 1/2] xfrm: Check inner packet family directly from skb_dst Jianbo Liu 2025-10-28 14:27 ` Zhu Yanjun 2025-10-28 2:22 ` [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol Jianbo Liu 2025-10-28 11:03 ` Sabrina Dubroca 2025-10-28 13:36 ` Jianbo Liu 2025-10-28 15:04 ` Sabrina Dubroca 2025-10-30 8:08 ` Steffen Klassert 2025-10-30 8:35 ` Jianbo Liu 2025-10-30 10:28 ` Sabrina Dubroca 2025-11-01 12:29 ` [PATCH ipsec v3 0/2] xfrm: Correct inner packet family determination Steffen Klassert
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).