* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-22 8:33 ` Bartosz Golaszewski
@ 2026-07-22 15:46 ` Demi Marie Obenour
2026-07-22 16:33 ` Eric Biggers
2026-07-22 16:20 ` Eric Biggers
2026-07-24 2:06 ` Eric Biggers
2 siblings, 1 reply; 10+ messages in thread
From: Demi Marie Obenour @ 2026-07-22 15:46 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Herbert Xu, Russell King, David S. Miller,
Eric Biggers, linux-arm-kernel, linux-kernel, linux-crypto,
linux-arm-msm, stable
[-- Attachment #1.1: Type: text/plain, Size: 6144 bytes --]
On 7/22/26 04:33, Bartosz Golaszewski wrote:
> On Tue, 21 Jul 2026 20:29:39 +0200, Demi Marie Obenour
> <demiobenour@gmail.com> said:
>> On 7/21/26 04:24, Bartosz Golaszewski wrote:
>>> On Fri, Jul 17, 2026 at 06:18:48PM +1000, Herbert Xu wrote:
>>>> On Sun, Jul 12, 2026 at 05:31:31PM -0400, Demi Marie Obenour via B4 Relay wrote:
>>>>> From: Demi Marie Obenour <demiobenour@gmail.com>
>>>>>
>>>>> This driver is harmful:
>>>>>
>>>>> - It is much slower than the CPU [1] [2].
>>>>> - It Has a history of bugs [2] [3].
>>>>> - It does not have exclusive access to the hardware [4], causing races
>>>>> with the secure world.
>>>>> - It register its implementations with too low a cra_priority for them
>>>>> to be actually used [5].
>>>>>
>>>>> Therefore, disable it to ensure that nobody builds it into kernels they
>>>>> intend to ship.
>>>>>
>>>>> In the future, the driver will be used for processing restricted media
>>>>> content. However, the kernel does not currently support this. Since
>>>>> the driver will have future uses, allow building it if COMPILE_TEST is
>>>>> enabled.
>>>>>
>>>>> [1]: https://lore.kernel.org/r/20250704070322.20692-1-ebiggers@kernel.org/
>>>>> [2]: https://lore.kernel.org/r/20250615031807.GA81869@sol/
>>>>> [3]: https://lore.kernel.org/r/20260706-qce-fix-self-tests-v5-0-86f461ff1829@oss.qualcomm.com/
>>>>> [4]: https://lore.kernel.org/r/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com/
>>>>> [5]: https://lore.kernel.org/r/20260524204537.GB110177@quark/
>>>>>
>>>>> Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>>>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Add Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>>>> - Add Cc: stable@vger.kernel.org
>>>>> - Link to v1: https://lore.kernel.org/r/20260712-qce-broken-v1-1-85e2bff17871@gmail.com
>>>>> ---
>>>>> arch/arm/configs/multi_v7_defconfig | 1 -
>>>>> arch/arm64/configs/defconfig | 1 -
>>>>> drivers/crypto/Kconfig | 6 +++++-
>>>>> 3 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> Patch applied. Thanks.
>>>
>>> Hi Herbert,
>>>
>>> I'd like to ask you to back this patch out. Admittedly I should have NAKed this
>>> under the v1 thread but I thought the discussion we had under the Shikra series
>>> and the subsequent patch making myself the maintainer of this driver (note: Demi
>>> did not Cc me on this nor the Qcom mailing list at linux-arm-msm) as well as the
>>> pending series[1] fixing the QCE issues was enough to imply we should not mark
>>> it as broken. Please consider applying [1] if the fixes in v6 are correct.
>>>
>>> Bartosz
>>>
>>> [1] https://lore.kernel.org/all/20260717-qce-fix-self-tests-v6-0-455775fe5f6c@oss.qualcomm.com/
>>
>> Is the BAM locking patchset also needed? My understanding is that
>> without it, the QCE driver can still produce wrong results due to races
>> with the TrustZone firmware. Until it is merged and the self-tests
>> are fixed, the driver definitely needs to be marked BROKEN.
>>
>
> No, we currently don't ship TZ firmware that could cause such interactions.
> The only driver that currently suffers from such races is the NAND driver
> which races with the modem firmware and it too will use the BAM locking
> feature once it's upstream. However we do plan to support DRM using this IP
> and this is when it'll need to account for possible races. IOW: BAM locking
> is not strictly required to fix the QCE.
Thanks for the correction.
Will updated firmware alone cause races, or does the kernel need to
opt-in to firmware features that can cause them? I want to make sure
that a firmware update will not cause the driver to start racing with
the secure world.
(Nit: DRM usually stands for "Direct Rendering Manager" in Linux.)
>> Also, what can the current driver do that cannot be done better using
>> software crypto? I understand that you have future plans to add more
>> features to the driver, but those are not relevant to this discussion.
>>
>
> We can offload crypto operations to the QCE saving CPU cycles but more
> importantly, the QCE provides HW based isolation which some vendors care
> about.
Would you mind explaining what you mean by "HW based isolation"?
While some crypto engines support keys that Linux doesn't have access
to, Linux only supports this on IBM mainframes. Linux does support
using hardware-wrapped keys for inline storage encryption, but those
do not use this driver.
>> Finally, I very much appreciate the work you have done. I don't
>> expect that the QCE driver will stay BROKEN forever. This patch is
>> purely a reflection of the current state of the driver, the speed of
>> CPU-accelerated crypto, and the small message sizes used by the kernel.
>>
>
> That may have been true when you sent the v1 but with a set of proposed fixes
> it'll no longer be the case.
I trust (and have always trusted) that you can fix the driver.
I apologize for not CCing you on the patch submission.
>> That said, is drivers/crypto really the right place for a restricted
>> content driver? A driver that uses dmabufs might be a better
>> alternative, and would be easier to integrate with the Direct Rendering
>> Manager (DRM) and Video for Linux 2 (V4L2) subsystems.
>> --
>
> These are future plans, yes. But right now we do have active users of this
> IP and now also a maintainer - yours truly - who stepped up and is actively
> working on this module. With the fixes applied, the driver will again pass
> crypto self-tests and should not be BROKEN.
With your patch, does the driver also pass stress tests, such as
multiple threads using it in parallel with varying algorithms and
message sizes?
If so, then I agree that the driver should no longer depend on
BROKEN, but I still question whether it is useful in its current form.
That's not specific to QCE, though. I would say the same about many
other drivers.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-22 15:46 ` Demi Marie Obenour
@ 2026-07-22 16:33 ` Eric Biggers
0 siblings, 0 replies; 10+ messages in thread
From: Eric Biggers @ 2026-07-22 16:33 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Bartosz Golaszewski, Bartosz Golaszewski, Herbert Xu,
Russell King, David S. Miller, linux-arm-kernel, linux-kernel,
linux-crypto, linux-arm-msm, stable
On Wed, Jul 22, 2026 at 11:46:16AM -0400, Demi Marie Obenour wrote:
> With your patch, does the driver also pass stress tests, such as
> multiple threads using it in parallel with varying algorithms and
> message sizes?
As I've mentioned before, the self-tests (crypto/testmgr.c) don't issue
more than one request concurrently. They are really designed for
testing the software implementations, not the hardware drivers. None of
the hardware driver authors has ever caused enough about quality to make
any significant improvements to the tests. Inevitably it's just push
the driver in, *maybe* run the existing incomplete tests once (for QCE
it's finally getting done just now), and job done I guess.
- Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-22 8:33 ` Bartosz Golaszewski
2026-07-22 15:46 ` Demi Marie Obenour
@ 2026-07-22 16:20 ` Eric Biggers
2026-07-23 9:44 ` Bartosz Golaszewski
2026-07-24 2:06 ` Eric Biggers
2 siblings, 1 reply; 10+ messages in thread
From: Eric Biggers @ 2026-07-22 16:20 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Demi Marie Obenour, Bartosz Golaszewski, Herbert Xu, Russell King,
David S. Miller, linux-arm-kernel, linux-kernel, linux-crypto,
linux-arm-msm, stable
On Wed, Jul 22, 2026 at 01:33:51AM -0700, Bartosz Golaszewski wrote:
> > Also, what can the current driver do that cannot be done better using
> > software crypto? I understand that you have future plans to add more
> > features to the driver, but those are not relevant to this discussion.
> >
>
> We can offload crypto operations to the QCE saving CPU cycles but more
> importantly, the QCE provides HW based isolation which some vendors care
> about.
Do you have any concrete proof of anyone using this with a specific
Linux kernel feature and actually getting a benefit from it?
Just for some additional context, I've been on the Android team for many
years and have worked with lots of companies shipping Android devices
with Qualcomm SoCs. I also upstreamed the support for the
Qualcomm Inline Crypto Engine (ICE)
(https://lore.kernel.org/all/20200710072013.177481-6-ebiggers@kernel.org/
https://lore.kernel.org/linux-mmc/20210126001456.382989-9-ebiggers@kernel.org/),
which is what people actually use and isn't the same as QCE.
The only times the Linux QCE driver has ever come up is when someone
used it accidentally and it destroyed their performance (due to the
20-60x slower performance than the CPU) or caused a filesystem hang.
These issues get reported to me -- sometimes even from Qualcomm
themselves! It's kind of annoying.
(Fortunately it's also quite rare, since the vast majority of people
just ignore QCE and don't make the mistake of using it in Linux.)
As far as I can tell, what actually works and what people actually use
in Linux on Qualcomm SoCs is:
- The Qualcomm Inline Crypto Engine (ICE) for storage encryption
- The ARMv8 Crypto Extensions everywhere else, e.g. dm-verity
> These are future plans, yes. But right now we do have active users of this
> IP
Just because it may be used by the Arm secure world doesn't mean it is
useful in Linux.
- Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-22 16:20 ` Eric Biggers
@ 2026-07-23 9:44 ` Bartosz Golaszewski
2026-07-23 14:17 ` Eric Biggers
0 siblings, 1 reply; 10+ messages in thread
From: Bartosz Golaszewski @ 2026-07-23 9:44 UTC (permalink / raw)
To: Eric Biggers
Cc: Demi Marie Obenour, Bartosz Golaszewski, Herbert Xu, Russell King,
David S. Miller, linux-arm-kernel, linux-kernel, linux-crypto,
linux-arm-msm, stable, Bartosz Golaszewski
On Wed, 22 Jul 2026 18:20:40 +0200, Eric Biggers <ebiggers@kernel.org> said:
> On Wed, Jul 22, 2026 at 01:33:51AM -0700, Bartosz Golaszewski wrote:
>> > Also, what can the current driver do that cannot be done better using
>> > software crypto? I understand that you have future plans to add more
>> > features to the driver, but those are not relevant to this discussion.
>> >
>>
>> We can offload crypto operations to the QCE saving CPU cycles but more
>> importantly, the QCE provides HW based isolation which some vendors care
>> about.
>
> Do you have any concrete proof of anyone using this with a specific
> Linux kernel feature and actually getting a benefit from it?
>
I'm not required to produce a proof of Qualcomm's clients using it. That's
never been a requirement to keep a driver in mainline. I'm saying that Qualcomm
wants to keep it and I've stepped up to fix and maintain it, which is what I'm
doing.
But yes, I know about companies that do use this driver as is for whatever
reasons. I'm not sure I can mention them so I won't.
> Just for some additional context, I've been on the Android team for many
> years and have worked with lots of companies shipping Android devices
> with Qualcomm SoCs. I also upstreamed the support for the
> Qualcomm Inline Crypto Engine (ICE)
> (https://lore.kernel.org/all/20200710072013.177481-6-ebiggers@kernel.org/
> https://lore.kernel.org/linux-mmc/20210126001456.382989-9-ebiggers@kernel.org/),
> which is what people actually use and isn't the same as QCE.
>
Yes, I know about your work. Thank you for doing all of that.
Best Regards,
Bartosz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-23 9:44 ` Bartosz Golaszewski
@ 2026-07-23 14:17 ` Eric Biggers
2026-07-23 20:58 ` Eric Biggers
0 siblings, 1 reply; 10+ messages in thread
From: Eric Biggers @ 2026-07-23 14:17 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Demi Marie Obenour, Bartosz Golaszewski, Herbert Xu, Russell King,
David S. Miller, linux-arm-kernel, linux-kernel, linux-crypto,
linux-arm-msm, stable
On Thu, Jul 23, 2026 at 02:44:27AM -0700, Bartosz Golaszewski wrote:
> On Wed, 22 Jul 2026 18:20:40 +0200, Eric Biggers <ebiggers@kernel.org> said:
> > On Wed, Jul 22, 2026 at 01:33:51AM -0700, Bartosz Golaszewski wrote:
> >> > Also, what can the current driver do that cannot be done better using
> >> > software crypto? I understand that you have future plans to add more
> >> > features to the driver, but those are not relevant to this discussion.
> >> >
> >>
> >> We can offload crypto operations to the QCE saving CPU cycles but more
> >> importantly, the QCE provides HW based isolation which some vendors care
> >> about.
> >
> > Do you have any concrete proof of anyone using this with a specific
> > Linux kernel feature and actually getting a benefit from it?
> >
>
> I'm not required to produce a proof of Qualcomm's clients using it. That's
> never been a requirement to keep a driver in mainline. I'm saying that Qualcomm
> wants to keep it and I've stepped up to fix and maintain it, which is what I'm
> doing.
>
> But yes, I know about companies that do use this driver as is for whatever
> reasons. I'm not sure I can mention them so I won't.
I know that it's enabled in the defconfig and Qualcomm's BSP, so some
people have the module loaded on their systems. That doesn't count as
use, let alone it being useful.
I'm not asking for a client name or any confidential information. This
is a technical discussion, and I'm asking for proof that this driver can
actually provide a benefit for any specific Linux kernel feature over
the widely used and well-vetted alternatives (ICE and ARMv8 CE).
"We want this upstream for reasons that we refuse to disclose" is never
a reason for adding (or keeping) code in Linux. There has to be a
technical justification.
Meanwhile, the strong technical arguments in favor of removal or marking
it BROKEN that have been presented remain unaddressed.
- Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-23 14:17 ` Eric Biggers
@ 2026-07-23 20:58 ` Eric Biggers
0 siblings, 0 replies; 10+ messages in thread
From: Eric Biggers @ 2026-07-23 20:58 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Demi Marie Obenour, Bartosz Golaszewski, Herbert Xu, Russell King,
David S. Miller, linux-arm-kernel, linux-kernel, linux-crypto,
linux-arm-msm, stable
On Thu, Jul 23, 2026 at 07:17:19AM -0700, Eric Biggers wrote:
> On Thu, Jul 23, 2026 at 02:44:27AM -0700, Bartosz Golaszewski wrote:
> > On Wed, 22 Jul 2026 18:20:40 +0200, Eric Biggers <ebiggers@kernel.org> said:
> > > On Wed, Jul 22, 2026 at 01:33:51AM -0700, Bartosz Golaszewski wrote:
> > >> > Also, what can the current driver do that cannot be done better using
> > >> > software crypto? I understand that you have future plans to add more
> > >> > features to the driver, but those are not relevant to this discussion.
> > >> >
> > >>
> > >> We can offload crypto operations to the QCE saving CPU cycles but more
> > >> importantly, the QCE provides HW based isolation which some vendors care
> > >> about.
> > >
> > > Do you have any concrete proof of anyone using this with a specific
> > > Linux kernel feature and actually getting a benefit from it?
> > >
> >
> > I'm not required to produce a proof of Qualcomm's clients using it. That's
> > never been a requirement to keep a driver in mainline. I'm saying that Qualcomm
> > wants to keep it and I've stepped up to fix and maintain it, which is what I'm
> > doing.
> >
> > But yes, I know about companies that do use this driver as is for whatever
> > reasons. I'm not sure I can mention them so I won't.
>
> I know that it's enabled in the defconfig and Qualcomm's BSP, so some
> people have the module loaded on their systems. That doesn't count as
> use, let alone it being useful.
>
> I'm not asking for a client name or any confidential information. This
> is a technical discussion, and I'm asking for proof that this driver can
> actually provide a benefit for any specific Linux kernel feature over
> the widely used and well-vetted alternatives (ICE and ARMv8 CE).
>
> "We want this upstream for reasons that we refuse to disclose" is never
> a reason for adding (or keeping) code in Linux. There has to be a
> technical justification.
>
> Meanwhile, the strong technical arguments in favor of removal or marking
> it BROKEN that have been presented remain unaddressed.
FYI, I took some time to take a closer look at this driver, and I found
a bunch more bugs:
- https://lore.kernel.org/linux-crypto/20260723183438.GB93534@quark/
- https://lore.kernel.org/linux-crypto/20260723192514.GA110634@quark/
- https://lore.kernel.org/linux-crypto/20260723202704.GB110634@quark/
- https://lore.kernel.org/linux-crypto/20260723205342.GC110634@quark/
Overall, I think this demotion is extraordinarily well justified.
The pending fixes for some bugs don't change the big picture. In fact,
the introduction of BAM locking seems likely to exacerbate the
performance issues.
It would not be responsible to defer to the hardware vendor here. The
community has a responsibility to uphold high technical standards,
especially for cryptography.
- Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN
2026-07-22 8:33 ` Bartosz Golaszewski
2026-07-22 15:46 ` Demi Marie Obenour
2026-07-22 16:20 ` Eric Biggers
@ 2026-07-24 2:06 ` Eric Biggers
2 siblings, 0 replies; 10+ messages in thread
From: Eric Biggers @ 2026-07-24 2:06 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Demi Marie Obenour, Bartosz Golaszewski, Herbert Xu, Russell King,
David S. Miller, linux-arm-kernel, linux-kernel, linux-crypto,
linux-arm-msm, stable
On Wed, Jul 22, 2026 at 01:33:51AM -0700, Bartosz Golaszewski wrote:
> We can offload crypto operations to the QCE saving CPU cycles
Here are benchmark results for SHA-256 on the SM8650 HDK, hashing 50000
4096-byte messages (matching dm-verity's and fsverity's workloads), with
all your pending patches applied:
sha256-lib (i.e. ARMv8 CE):
Wall-clock elapsed time: 0.10s
CPU time used: 0.10s
CPU time spent in hardirq context: 0.00s
CPU time spent in softirq context: 0.00s
sha256-qce:
Wall-clock elapsed time: 10.76s
CPU time used: 5.14s
CPU time spent in hardirq context: 0.77s
CPU time spent in softirq context: 2.31s
So sha256-qce is over 100 times slower and uses over 50 times more CPU
than ARMv8 CE, over half of that in hardirq or softirq context.
This was even worse than what I got last time I tried this! So I tried
rolling back your BAM locking patches:
Wall-clock elapsed time: 4.85s
CPU time used: 2.78s
CPU time spent in hardirq context: 0.55s
CPU time spent in softirq context: 0.94s
That's more in line with what I measured last time I tried this, but
still atrocious. It's still far more CPU time used than just doing the
hashing on the CPU, and much of it spent in hardirq and softirq
contexts. The BAM locking just makes it even worse.
With that, plus the security claims not panning out either when
considering the actual code and usage in Linux, and also all the bugs
(both historical and current unfixed ones), I don't think there's any
credibility left. This legacy driver should just be deleted.
To be clear: completely separate from QCE, Qualcomm SoCs have good
support for crypto acceleration in ICE and the ARMv8 CE. That's great!
And that is, of course, what is *actually* being used in Linux. That's
what we'll support. Let's stop wasting time on a dead-end approach.
I understand there may be a desire to keep this driver around so that
support for restricted media content can be added to it. However, that
would be new functionality that has nothing to do with what the driver
currently does and doesn't use the kernel's crypto API. There's no need
to keep the driver around for this. Just put together a clean proposal
to add a new version of the driver with just the needed functionality
(note that drivers/crypto/ probably won't be the right place for it).
- Eric
^ permalink raw reply [flat|nested] 10+ messages in thread