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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250910084027.GL341237@unreal>
Date: Wed, 10 Sep 2025 11:40:27 +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 Wed, Sep 10, 2025 at 10:04:34AM +0200, Sabrina Dubroca wrote:
> 2025-09-10, 08:45:50 +0300, Leon Romanovsky wrote:
> > 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.
> 
> For me "offloaded" can mean either the xfrm state or the packets
> depending on context, and I don't think there's a strict definition,
> but whatever.
> 
> Xiumei reported a regression in IPsec offload tests over xfrmi, where
> the traffic for IPv6 over IPv4 tunnels is processed in SW instead of
> going through crypto offload, after commit [...].
> 
> It's getting too verbose IMO, but does that work for you?

Yes, it is perfectly fine.

> 
> 
> For the subject, are you ok with the current one? It's hard to fit
> more details into such a short space.

Leave subject as is, you describe issue well enough in the commit
message.

> 
> > > > 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                 }
> 
> So it looks like packet offload is also affected. We get to
> xfrm_dev_offload_ok, it does the wrong check, and the packets will get
> dropped instead of being sent through SW crypto. Am I misreading this?

There is no fallback in packet offload, so dropping packet which can't
be handled by HW is right thing to do. I agree that we shouldn't fail
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.
> > 
> > It worked by chance, not by design :)
> 
> Sure.
> 
> [...]
> > > > The latter is more correct, so it raises question against which
> > > > in-kernel driver were these xfrmi tests performed?
> > > 
> > > mlx5
> > 
> > It is artifact.
> 
> Sorry, I'm not sure what you mean here.

I'm saying that "works" depends on FW and HW revision.

> 
> [...]
> > > > 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>
> 
> Thanks. I'll send a v2 when we agree on the wording, to avoid
> resending multiple times.
> 
> -- 
> Sabrina