Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Leon Romanovsky <leon@kernel.org>]

On Mon, Aug 25, 2025 at 02:50:23PM +0200, Sabrina Dubroca wrote:
> Xiumei reported a regression in IPsec offload tests over xfrmi, where
> IPv6 over IPv4 tunnels are no longer offloaded after commit
> cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> implementation").

What does it mean "tunnels not offloaded"? xdo_dev_offload_ok()
participates in data path and influences packet processing itself,
but not if tunnel offloaded or not.

Also what type of "offload" are you talking? Crypto or packet?

> 
> Commit cc18f482e8b6 added a generic version of existing checks
> attempting to prevent packets with IPv4 options or IPv6 extension
> headers from being sent to HW that doesn't support offloading such
> packets. The check mistakenly uses x->props.family (the outer family)
> to determine the inner packet's family and verify if
> options/extensions are present.

This is how ALL implementations did, so I'm not agree with claimed Fixes
tag (it it not important).

> 
> In the case of IPv6 over IPv4, the check compares some of the traffic
> class bits to the expected no-options ihl value (5). The original
> check was introduced in commit 2ac9cfe78223 ("net/mlx5e: IPSec, Add
> Innova IPSec offload TX data path"), and then duplicated in the other
> drivers. Before commit cc18f482e8b6, the loose check (ihl > 5) passed
> because those traffic class bits were not set to a value that
> triggered the no-offload codepath. Packets with options/extension
> headers that should have been handled in SW went through the offload
> path, and were likely dropped by the NIC or incorrectly
> processed.

The latter is more correct, so it raises question against which
in-kernel driver were these xfrmi tests performed?


> Since commit cc18f482e8b6, the check is now strict (ihl !=
> 5), and in a basic setup (no traffic class configured), all packets go
> through the no-offload codepath.
> 
> The commits that introduced the incorrect family checks in each driver
> are:
> 2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
> 8362ea16f69f ("crypto: chcr - ESN for Inline IPSec Tx")
> 859a497fe80c ("nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer")
> 32188be805d0 ("cn10k-ipsec: Allow ipsec crypto offload for skb with SA")
> [ixgbe/ixgbevf commits are ignored, as that HW does not support tunnel
> mode, thus no cross-family setups are possible]
> 
> Fixes: cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback implementation")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  net/xfrm/xfrm_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index c7a1f080d2de..44b9de6e4e77 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->props.family) {
> +	switch (x->inner_mode.family) {

Will it work for transport mode too? We are taking this path both for
tunnel and transport modes.

Thanks

>  	case AF_INET:
>  		/* Check for IPv4 options */
>  		if (ip_hdr(skb)->ihl != 5)
> -- 
> 2.50.0
> 
>