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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMExEjj3I4ahnMHc@krikkit>
Date: Wed, 10 Sep 2025 10:04:34 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Leon Romanovsky <leon@...nel.org>
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

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?


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

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


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

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