netdev - Re: [PATCH ipsec 2/3] Revert "xfrm: Remove unneeded device check from validate_xmit

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: <aIol33zSxJk6OQSy@krikkit>
Date: Wed, 30 Jul 2025 16:02:07 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Cosmin Ratiu <cratiu@...dia.com>
Cc: Leon Romanovsky <leonro@...dia.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"razor@...ckwall.org" <razor@...ckwall.org>,
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>
Subject: Re: [PATCH ipsec 2/3] Revert "xfrm: Remove unneeded device check
 from validate_xmit_xfrm"

2025-07-30, 12:32:13 +0000, Cosmin Ratiu wrote:
> On Wed, 2025-07-30 at 12:26 +0200, Sabrina Dubroca wrote:
> > 2025-07-29, 15:27:39 +0000, Cosmin Ratiu wrote:
> > > On Mon, 2025-07-28 at 17:17 +0200, Sabrina Dubroca wrote:
> > > > This reverts commit d53dda291bbd993a29b84d358d282076e3d01506.
> > > > 
> > > > This change causes traffic using GSO with SW crypto running
> > > > through a
> > > > NIC capable of HW offload to no longer get segmented during
> > > > validate_xmit_xfrm.
> > > > 
> > > > Fixes: d53dda291bbd ("xfrm: Remove unneeded device check from
> > > > validate_xmit_xfrm")
> > > > 
> > > 
> > > Thanks for the fix, but I'm curious about details.
> > > 
> > > In that commit, I tried to map all of the possible code paths. Can
> > > you
> > > please explain what code paths I missed that need real_dev given
> > > that
> > > only bonding should use it now?
> > 
> > After running some more tests, it's not about real_dev, it's the
> > other
> > check ("unlikely(x->xso.dev != dev)" below) that you also removed in
> > that patch that causes the issue in my setup. I don't know how you
> > decided that it should be dropped, since it predates bonding's ipsec
> > offload.
> 
> Apologies for that, I think I assumed that if offload is off, then
> xfrm_offload(skb) is NULL and the code bails out early on "if (!xo)".
> Seems I was wrong. On the TX side, the only place that adds a secpath
> and increments sp->olen (and thus add an xfrm_offload) is in
> xfrm_output, after the xfrm_dev_offload_ok check.

Yes, the "offload" code is used for both HW offload and "SW offloads"
(aka GSO/GRO).

> > The codepath is the usual:
> > __dev_queue_xmit -> validate_xmit_skb -> validate_xmit_xfrm
> > 
> > Since the commit message made the incorrect claim "ESP offload off:
> > validate_xmit_xfrm returns early on !xo." I didn't check if a partial
> > revert was enough to fix the issue. My bad.
> > 
> No problem, good that we caught the actual issue. Will you prepare a
> follow-up patch then?

I'll send a v2 of this series with this patch updated. Thanks.

-- 
Sabrina