Message-ID: <20250910054550.GI341237@unreal>
Date: Wed, 10 Sep 2025 08:45:50 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Zhu Yanjun <yanjun.zhu@...ux.dev>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Xiumei Mu <xmu@...hat.com>
Subject: Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

On Tue, Sep 09, 2025 at 08:29:20PM +0200, Sabrina Dubroca wrote:
> 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.

Yes, "offloaded" means that we created HW objects.

> 
> 
> > Also what type of "offload" are you talking? Crypto or packet?
> 
> Crypto offload, but I don't think packet offload would behave
> differently here.

It will, at least in the latest code, we have an extra check before
passing packet to HW.

  765         if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
  766                 if (!xfrm_dev_offload_ok(skb, x)) {
  767                         XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
  768                         kfree_skb(skb);
  769                         return -EHOSTUNREACH;
  770                 }

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

It worked by chance, not by design :)

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

No problem, like I wrote, it is 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?
> 
> mlx5

It is artifact.

> 
> > > 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@...hat.com>
> > > Signed-off-by: Sabrina Dubroca <sd@...asysnail.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".

I believe that you need to rephrase commit message around meaning of "offloaded"
but the change looks ok to me.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@...dia.com>

Powered by blists - more mailing lists