netdev - Re: [PATCH net-next] macsec: introduce default_async

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: <ZPCfYLsLycX68IeG@hog>
Date: Thu, 31 Aug 2023 16:10:40 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Scott Dial <scott@...ttdial.com>,
	Herbert Xu <herbert@...dor.apana.org.au>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org
Subject: Re: [PATCH net-next] macsec: introduce default_async_crypto sysctl

2023-08-28, 15:04:51 -0400, Scott Dial wrote:
> On 8/28/2023 5:42 AM, Sabrina Dubroca wrote:
> > 2023-08-24, 13:08:41 -0400, Scott Dial wrote:
> > > On 8/24/2023 9:01 AM, Sabrina Dubroca wrote:
> > > > 2023-08-23, 16:22:31 -0400, Scott Dial wrote:
> > > > > AES-NI's implementation of gcm(aes) requires the FPU, so if it's busy the
> > > > > decrypt gets stuck on the cryptd queue, but that queue is not
> > > > > order-preserving.
> > > > 
> > > > It should be (per CPU [*]). The queue itself is a linked list, and if we
> > > > have requests on the queue we don't let new requests skip the queue.
> > > 
> > > My apologies, I'll be the first to admit that I have not tracked all of the
> > > code changes to either the macsec driver or linux-crypto since I first made
> > > the commit. This comment that requests are queued forced me to review the
> > > code again and it appears that the queueing issue was resolved in v5.2-rc1
> > > with commit 1661131a0479, so I no longer believe we need the
> > > CRYPTO_ALG_ASYNC since v5.2 and going forward.
> > 
> > Are you sure about this? 1661131a0479 pre-dates your patch by over a
> > year.
> > 
> > And AFAICT, that series only moved the existing FPU usable +
> > cryptd_aead_queued tests from AESNI's implementation of gcm(aes) to
> > common SIMD helpers.
> 
> My original issue started with a RHEL7 system, so a backport of the macsec
> driver to the 3.10 kernel. I recall building newer kernels and reproducing
> the issue, but I don't have my test setup anymore nor any meaningful notes
> that would indicate to me what kernels I tested. In any case, I didn't
> bisect when the queuing behavior was changed, and maybe I misread the code,
> and maybe my test setup was flawed in some other way.
> 
> 1661131a0479 wasn't obviously just moving code to me, so I didn't trace back
> further, but looking at the longterm maintenance 4.x kernels, I can see that
> the AES-NI code has the same cryptd_aead_queued check

Yes, that's more what I meant. The check exists before and after
commits 1661131a0479 and 149e12252fb3.

(and FWIW, RHEL7 doesn't have it, but that's not a concern for netdev)

> so I think you are
> correct to say that you could revert my change on all of the maintenance
> kernels to restore the performance of MACsec w/ AES-NI.

Ok, thanks.

> Whether that causes any ordering regressions for any other crypto
> accelerations, I have no idea since it would require auditing a lot of
> crypto code.

Herbert, can we expect ASYNC implementations of gcm(aes) to maintain
ordering of completions wrt requests? For AESNI, the use of
cryptd_aead_queued() makes sure of that, but I don't know if other
implementations under drivers/crypto would have the same
guarantee.

[context: we're considering reverting commit ab046a5d4be4 ("net:
macsec: preserve ingress frame ordering"), but Scott is concerned that
the issue he saw would happen with other types of acceleration]

-- 
Sabrina