linux-kernel - Re: [GIT PULL] Crypto Update for 6.15

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: <20250326033404.GD1661@sol.localdomain>
Date: Tue, 25 Mar 2025 20:34:04 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
	"David S. Miller" <davem@...emloft.net>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux Crypto Mailing List <linux-crypto@...r.kernel.org>
Subject: Re: [GIT PULL] Crypto Update for 6.15

On Wed, Mar 26, 2025 at 10:16:10AM +0800, Herbert Xu wrote:
> On Wed, Mar 26, 2025 at 09:49:14AM +0800, Herbert Xu wrote:
> >
> > Let's see how your version is so much better:
> > 
> > https://lore.kernel.org/all/20250212154718.44255-6-ebiggers@kernel.org/
> 
> BTW, I absolutely hate how the fs/block layer uses work queues
> for everything.  It's been used as an argument for async being
> unnecessary because you can always wait for completion since
> you're in a work queue.
> 
> But this is exactly the wrong way to do asynchronous completion.
> In fact, now that async support has been removed because of
> religious opposition to ahash, we now end up with the worst of
> both worlds where hashing is punted off to a work queue where
> it is simply executed on the CPU:
> 
> /**
>  * fsverity_enqueue_verify_work() - enqueue work on the fs-verity workqueue
>  * @work: the work to enqueue
>  *
>  * Enqueue verification work for asynchronous processing.
>  */
> void fsverity_enqueue_verify_work(struct work_struct *work)
> {
>         queue_work(fsverity_read_workqueue, work);
> }
> 
> The correct way to do async offload is to do it conditionally:
> 
> 	ret = submit_request(rq);
> 	if (unlikely(needs_async(ret))) {
> 		allocate for async path with fallback to sync
> 		processing in case of OOM
> 		return;
> 	}
> 
> 	execute normal synchronous path
> 

In the general case, the workqueue is needed anyway because the work can block
(e.g. to read Merkle tree blocks) or can take longer than should be spent in
softirq context.  But in many cases the workqueue is indeed overkill and hurts
I/O performance.  For that reason, dm-verity and dm-crypt already support doing
the read completion work in softirq context in some cases.  It's not enabled by
default though, and isn't implemented in quite the way it should be.  Several
people, including me, have been looking into improving that.

So I think your observation about the workqueue being unhelpful is generally
correct, but fixing that is already partially implemented and is being worked on
further.  And regardless, this does not have that much relevance to the crypto
API.  Yes, you can't sleep from a softirq, which means you can't wait for an
async crypto request to complete (other than polling).  So if you want to do
that, you have to go down the workqueue code path.  But in practice 99% of users
are just using the CPU-based crypto that is synchronous and does not block.

- Eric