Re: [PATCH ipsec v3 2/2] xfrm: Determine inner GSO type from packet inner protocol

[Sabrina Dubroca <sd@queasysnail.net>]

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