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

[Sabrina Dubroca <sd@queasysnail.net>]

2025-09-09, 12:23:15 +0300, Leon Romanovsky wrote:
> 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"?

Offload is no longer performed for those tunnels, or for packets going
through those tunnels if we want to be pedantic.

> xdo_dev_offload_ok()
> participates in data path and influences packet processing itself,
> but not if tunnel offloaded or not.

If for you "tunnel is offloaded" means "xdo_dev_state_add is called",
then yes.


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

Crypto offload, but I don't think packet offload would behave
differently here.

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

Well, prior to your commit, offload seemed to work on mlx5 as I
describe just after this.

But yes, I opted for a Fixes tag more aimed at stable backports with
additional references to the commits. I don't mind putting all the
Fixes tags for each driver as well (except ixgbe/ixgbevf since it's
transport-only so not affected by this, as I wrote in the commit).

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

mlx5

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

Yes, if you look at __xfrm_init_state, inner_mode will always be set
to whatever family is "inside".

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

-- 
Sabrina