| Message ID | ZfO6zKtvp2jSO4vF@gondor.apana.org.au |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] Crypto Update for 6.9 | expand |
On Thu, 14 Mar 2024 at 20:04, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Drivers: > > - Add queue stop/query debugfs support in hisilicon/qm. There's a lot more than that in there. Fairl ybig Intel qat updates from what I can see, for example. Linus
The pull request you sent on Fri, 15 Mar 2024 11:04:44 +0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.9-p1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c8e769961668ef56acabc67f040c58ed769c57e4
Thank you!
On Fri, Mar 15, 2024 at 02:51:47PM -0700, Linus Torvalds wrote: > On Thu, 14 Mar 2024 at 20:04, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > Drivers: > > > > - Add queue stop/query debugfs support in hisilicon/qm. > > There's a lot more than that in there. Fairl ybig Intel qat updates > from what I can see, for example. Sorry, one line got chopped off while I was creating the signed tag: - Improve error recovery in qat. Cheers,
On Sun, 12 May 2024 at 20:50, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Lukas Wunner (1): > X.509: Introduce scope-based x509_certificate allocation I absolutely hate how this commit tries to remove one single compare instruction by introducing a *very* dangerous hack. The whole 'assume()' thing will generate actively wrong code if that assumption conditional doesn't hold, to the point of being completely impossible to debug. Having random kernel code add random "assume()" lines is absolutely not what we should do. Particularly not in some random code sequence where it absolutely does not matter ONE WHIT. Now, I've pulled this, but I killed that "assume()" hackery in my merge. Because there is no way we will ever encourage random code to make these kinds of patterns, and I most definitely do not want anybody else to try to copy that horrendous thing. Yes, yes, we have "unreachable()" in other places, and yes, you can make compilers generate garbage by using that incorrectly. But they should be about obvious code warning issues, not about "let's save one conditional instruction". Now, if somebody really *really* cares about that one extraneous conditional, particularly if it shows up in some more important place than some random certificate parsing routine where is most definitely is not in the least critical, there are better models for this optimization. Maybe somebody can teach the kernel build in *general* that "kmalloc()" and friends never return an error pointer, only NULL or success? That would not necessarily be a bad idea if the scope-based cleanup otherwise causes issues. But this kind of hacky "one random piece of kernel code uses a very dangerous pattern to state that some *other* piece of kernel code has particular return patterns" - that is not at all acceptable. Put another way: it would probably be ok if the SLAB people added some "this function cannot return error codes" annotation on their core declaration and it fixed an issue in _general_. But it is *not* ok if random kernel code starts randomly asserting the same thing. Quod licet Iovi, non licet bovi. Linus
The pull request you sent on Mon, 13 May 2024 11:50:03 +0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.10-p1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/84c7d76b5ab6a52e1b3d8101b9f910c128dca396
Thank you!
On Mon, May 13, 2024 at 03:12:53PM -0700, Linus Torvalds wrote: > > Maybe somebody can teach the kernel build in *general* that > "kmalloc()" and friends never return an error pointer, only NULL or > success? That would not necessarily be a bad idea if the scope-based > cleanup otherwise causes issues. Yes he did try this out: https://lore.kernel.org/all/20240302082751.GA25828@wunner.de/ It resulted in an increase in total vmlinux size although I don't think anyone looked into the reason for it. > But this kind of hacky "one random piece of kernel code uses a very > dangerous pattern to state that some *other* piece of kernel code has > particular return patterns" - that is not at all acceptable. Agreed. However, this patch still has two outstanding build defects which have not been addressed: https://lore.kernel.org/all/202404240904.Qi3nM37B-lkp@intel.com/ https://lore.kernel.org/all/202404252210.KJE6Uw1h-lkp@intel.com/ So I might end up just reverting it. Thanks,
On Mon, 13 May 2024 at 22:17, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Yes he did try this out: > > https://lore.kernel.org/all/20240302082751.GA25828@wunner.de/ > > It resulted in an increase in total vmlinux size although I don't > think anyone looked into the reason for it. I think the basic issue is that the whole 'assume()' logic of "if (x) unreachable()" is very fragile. Basically, it *can* generate the exact code you want by basically telling the compiler that if some condition is true, then the compiler can jump to unreachable code, and then depending on the phase of the moon, the compiler may get the whole "I can assume this is never true". BUT. The reason I hated seeing it so much is exactly that it's basically depending on everything going just right. When things do *not* go right, it causes the compiler to instead actually generate the extra code for the conditional, and actually generate a conditional jump to something that the compiler then decides it can do anything to, since it's unreachable. So now you generate extra code, and generate a branch to nonsense. > However, this patch still has two outstanding build defects which > have not been addressed: > > https://lore.kernel.org/all/202404240904.Qi3nM37B-lkp@intel.com/ This one just seems to be a sanity check for "you shouldn't check kmalloc() for ERR_PTR", so it's a validation test that then doesn't like the new test in that 'assume()'. And the second one: > https://lore.kernel.org/all/202404252210.KJE6Uw1h-lkp@intel.com/ looks *very* much like the cases we've seen with clang in particular where clang goes "this code isn't reachable, so I'll just drop everything on the floor", and then it just becomes a fallthrough to whatever else code happens to come next. Most of the time that's just more (unrelated) code in the same function, but sometimes it causes that "falls through to next function" instead, entirely randomly depending on how the code was laid out. > So I might end up just reverting it. I suspect that because I removed the whole 'assume()' hackery, neither of the above issues will now happen, and the code nwo works. But yes, the above is *exactly* why I don't want to see that 'unreachable()' hackery. Now, if we had some *other* way to tell the compiler "this condition never happens", that would be fine. Some compiler builtin for asserting some condition. But a conditional "unreachable()" is absolutely not it. Linus
On Mon, May 13, 2024 at 10:41:58PM -0700, Linus Torvalds wrote: > > I suspect that because I removed the whole 'assume()' hackery, neither > of the above issues will now happen, and the code nwo works. Alright I'll let it stay and see if any new issues crop up. Thanks,
On Mon, 13 May 2024 at 23:54, Lukas Wunner <lukas@wunner.de> wrote: > > On Mon, May 13, 2024 at 03:12:53PM -0700, Linus Torvalds wrote: > > > > > https://lore.kernel.org/all/202404252210.KJE6Uw1h-lkp@intel.com/ > > > > looks *very* much like the cases we've seen with clang in particular > > where clang goes "this code isn't reachable, so I'll just drop > > everything on the floor", and then it just becomes a fallthrough to > > whatever else code happens to come next. Most of the time that's just > > more (unrelated) code in the same function, but sometimes it causes > > that "falls through to next function" instead, entirely randomly > > depending on how the code was laid out. > > Curiously, this particular 0-day report is for gcc 13.2.0 though, > not clang. Hmm. I think all the previous reports of "falls through to next function" that I have seen have been with clang, but that is probably be selection bias: the gcc cases of this tend to be found so much more quickly (because gcc is still more common at least on x86) that by the time I see the reports, it's because of some clang issue. And in fact, when I go test this theory by going to search on lore, I do see several gcc reports. So no, it was never just clang-only, it was just that the ones I had looked at were about clang. > The assume() macro had no effect with clang when I tested it. I suspect that the issue is that with *normal* kernel configurations, the code generation is simple and straightforward enough that gcc did the right thing. And then some more complicated setup with more debugging support enabled (particularly things like UBSAN or KASAN) the code gets complicated enough that gcc doesn't do the optimization any more, and then the conditional in assume() doesn't get optimized away at an early stage any more, and remains as a conditional branch to la-la-land. And you actually don't even see this as a warning unless the la-la-land happens to be at the end of a function. IOW, the "branch to nowhere" _could_ just branch to some label inside the function, and the objtool sanity check would never even have triggered. That's why "unreachable()" can be so dangerous. It tells the compiler that code generation in one place no longer matters, and then the compiler can decide to leave things just dangling in odd ways. The code presumably still *works* - because the actual conditional never triggers, so in that sense it's safe and fine. But it's still just horrendous to try to figure out, which is why I was so down on it. Linus
Hi Linus: The following changes since commit c5a2f74db71a849f3a60bc153d684d6d28a0c665: crypto: caam - fix asynchronous hash (2024-01-26 16:35:55 +0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.9-p1 for you to fetch changes up to 6a8dbd71a70620c42d4fa82509204ba18231f28d: Revert "crypto: remove CONFIG_CRYPTO_STATS" (2024-03-13 09:49:37 +0800) ---------------------------------------------------------------- This update includes the following changes: API: - Avoid unnecessary copying in scomp for trivial SG lists. Algorithms: - Optimise NEON CCM implementation on ARM64. Drivers: - Add queue stop/query debugfs support in hisilicon/qm. ---------------------------------------------------------------- Adam Guerin (6): crypto: qat - remove unused macros in qat_comp_alg.c crypto: qat - removed unused macro in adf_cnv_dbgfs.c crypto: qat - avoid division by zero crypto: qat - remove double initialization of value crypto: qat - remove unnecessary description from comment crypto: qat - fix comment structure Ard Biesheuvel (8): crypto: arm64/aes-ccm - Revert "Rewrite skcipher walker loop" crypto: arm64/aes-ccm - Keep NEON enabled during skcipher walk crypto: arm64/aes-ccm - Pass short inputs via stack buffer crypto: arm64/aes-ccm - Replace bytewise tail handling with NEON permute crypto: arm64/aes-ccm - Reuse existing MAC update for AAD input crypto: arm64/aes-ccm - Cache round keys and unroll AES loops crypto: arm64/aes-ccm - Merge encrypt and decrypt tail handling crypto: arm64/aes-ccm - Merge finalization into en/decrypt asm helpers Arnd Bergmann (2): crypto: qat - avoid memcpy() overflow warning crypto: arm/sha - fix function cast warnings Barry Song (3): crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem Borislav Petkov (AMD) (1): crypto: ccp - State in dmesg that TSME is enabled Clay Chang (1): KEYS: include header for EINVAL definition Colin Ian King (2): crypto: pcbc - remove redundant assignment to nbytes crypto: asymmetric_keys - remove redundant pointer secs Damian Muszynski (7): crypto: qat - add heartbeat error simulator crypto: qat - add auto reset on error crypto: qat - change SLAs cleanup flow at shutdown crypto: qat - resolve race condition during AER recovery crypto: qat - fix ring to service map for dcc in 4xxx crypto: qat - fix ring to service map for dcc in 420xx crypto: qat - make ring to service map common for QAT GEN4 Dan Carpenter (1): crypto: qat - uninitialized variable in adf_hb_error_inject_write() Danny Tsen (1): crypto: vmx - Move to arch/powerpc/crypto David Wronek (1): dt-bindings: crypto: ice: Document SC7180 inline crypto engine Eric Biggers (2): crypto: ahash - unexport crypto_hash_alg_has_setkey() crypto: remove CONFIG_CRYPTO_STATS Erick Archer (2): crypto: sun8i-ce - Use kcalloc() instead of kzalloc() crypto: qat - use kcalloc_node() instead of kzalloc_node() Furong Zhou (3): crypto: qat - add fatal error notify method crypto: qat - disable arbitration before reset crypto: qat - limit heartbeat notifications Giovanni Cabiddu (1): Documentation: qat: fix auto_reset section Herbert Xu (2): crypto: dh - Make public key test FIPS-only Revert "crypto: remove CONFIG_CRYPTO_STATS" Joachim Vandersmissen (2): crypto: testmgr - remove unused xts4096 and xts512 algorithms from testmgr.c crypto: rsa - restrict plaintext/ciphertext values more Kilian Zinnecker (1): crypto: rockchip - fix to check return value Li RongQing (1): crypto: virtio - remove duplicate check if queue is broken Luca Weiss (1): dt-bindings: qcom-qce: Add compatible for SM6350 Lukas Bulwahn (1): MAINTAINERS: adjust file entries after crypto vmx file movement Mario Limonciello (2): crypto: ccp - Avoid discarding errors in psp_send_platform_access_msg() crypto: ccp - Update return values for some unit tests Markus Elfring (1): crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection Martin Kaiser (1): hwrng: hisi - use dev_err_probe Minjie Du (1): crypto: iaa - Remove unnecessary debugfs_create_dir() error check in iaa_crypto_debugfs_init() Mun Chun Yep (4): crypto: qat - update PFVF protocol for recovery crypto: qat - re-enable sriov after pf reset crypto: qat - add fatal error notification crypto: qat - improve aer error reset handling Qi Tao (3): crypto: hisilicon/sec2 - updates the sec DFX function register crypto: hisilicon/sec2 - modify nested macro call crypto: hisilicon/sec2 - fix some cleanup issues Quanyang Wang (1): crypto: xilinx - call finalize with bh disabled Randy Dunlap (1): crypto: jitter - fix CRYPTO_JITTERENTROPY help text Tom Zanussi (3): crypto: iaa - Remove header table code crypto: iaa - Fix async_disable descriptor leak crypto: iaa - Fix comp/decomp delay statistics Tudor Ambarus (1): MAINTAINERS: Remove T Ambarus from few mchp entries Varshini Rajendran (4): dt-bindings: crypto: add sam9x7 in Atmel AES dt-bindings: crypto: add sam9x7 in Atmel SHA dt-bindings: crypto: add sam9x7 in Atmel TDES dt-bindings: rng: atmel,at91-trng: add sam9x7 TRNG Vladis Dronov (1): crypto: tcrypt - add ffdhe2048(dh) test Weili Qian (5): crypto: hisilicon/qm - support get device state crypto: hisilicon/qm - dump important registers values before resetting crypto: hisilicon/qm - add stop function by hardware crypto: hisilicon/qm - obtain stop queue status crypto: hisilicon/qm - change function type to void Wenkai Lin (2): crypto: hisilicon - Fix smp_processor_id() warnings crypto: hisilicon/sec - remove unused parameter Documentation/ABI/testing/debugfs-driver-qat | 26 ++ Documentation/ABI/testing/debugfs-hisi-hpre | 22 ++ Documentation/ABI/testing/debugfs-hisi-sec | 22 ++ Documentation/ABI/testing/debugfs-hisi-zip | 22 ++ Documentation/ABI/testing/sysfs-driver-qat | 20 ++ .../bindings/crypto/atmel,at91sam9g46-aes.yaml | 6 +- .../bindings/crypto/atmel,at91sam9g46-sha.yaml | 6 +- .../bindings/crypto/atmel,at91sam9g46-tdes.yaml | 6 +- .../bindings/crypto/qcom,inline-crypto-engine.yaml | 1 + .../devicetree/bindings/crypto/qcom-qce.yaml | 1 + .../devicetree/bindings/rng/atmel,at91-trng.yaml | 4 + MAINTAINERS | 25 +- arch/arm/crypto/sha256_glue.c | 13 +- arch/arm/crypto/sha512-glue.c | 12 +- arch/arm64/crypto/Kconfig | 1 + arch/arm64/crypto/aes-ce-ccm-core.S | 265 ++++++++------------- arch/arm64/crypto/aes-ce-ccm-glue.c | 154 ++++++++---- arch/arm64/crypto/aes-glue.c | 1 + arch/powerpc/crypto/Kconfig | 20 ++ arch/powerpc/crypto/Makefile | 20 +- {drivers/crypto/vmx => arch/powerpc/crypto}/aes.c | 0 .../crypto/vmx => arch/powerpc/crypto}/aes_cbc.c | 0 .../crypto/vmx => arch/powerpc/crypto}/aes_ctr.c | 0 .../crypto/vmx => arch/powerpc/crypto}/aes_xts.c | 0 .../crypto/vmx => arch/powerpc/crypto}/aesp8-ppc.h | 0 .../vmx => arch/powerpc/crypto}/aesp8-ppc.pl | 0 .../crypto/vmx => arch/powerpc/crypto}/ghash.c | 0 .../vmx => arch/powerpc/crypto}/ghashp8-ppc.pl | 0 {drivers/crypto/vmx => arch/powerpc/crypto}/vmx.c | 0 crypto/Kconfig | 5 +- crypto/ahash.c | 21 +- crypto/asymmetric_keys/verify_pefile.c | 4 +- crypto/dh.c | 63 ++--- crypto/pcbc.c | 4 +- crypto/rsa.c | 36 ++- crypto/scompress.c | 38 ++- crypto/tcrypt.c | 3 + crypto/testmgr.c | 8 - drivers/char/hw_random/hisi-rng.c | 6 +- drivers/crypto/Kconfig | 14 +- drivers/crypto/Makefile | 2 +- drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 2 +- drivers/crypto/ccp/platform-access.c | 11 +- drivers/crypto/ccp/psp-dev.c | 11 +- drivers/crypto/hisilicon/debugfs.c | 58 +++++ drivers/crypto/hisilicon/hpre/hpre_main.c | 2 +- drivers/crypto/hisilicon/qm.c | 184 +++++++++----- drivers/crypto/hisilicon/sec2/sec_crypto.c | 33 +-- drivers/crypto/hisilicon/sec2/sec_main.c | 7 +- drivers/crypto/hisilicon/zip/zip_crypto.c | 1 + drivers/crypto/hisilicon/zip/zip_main.c | 2 +- drivers/crypto/intel/iaa/iaa_crypto.h | 25 -- drivers/crypto/intel/iaa/iaa_crypto_comp_fixed.c | 1 - drivers/crypto/intel/iaa/iaa_crypto_main.c | 122 ++-------- drivers/crypto/intel/iaa/iaa_crypto_stats.c | 30 --- drivers/crypto/intel/iaa/iaa_crypto_stats.h | 8 +- drivers/crypto/intel/qat/Kconfig | 14 ++ .../crypto/intel/qat/qat_420xx/adf_420xx_hw_data.c | 64 ++--- .../crypto/intel/qat/qat_4xxx/adf_4xxx_hw_data.c | 64 ++--- drivers/crypto/intel/qat/qat_common/Makefile | 2 + .../intel/qat/qat_common/adf_accel_devices.h | 3 + drivers/crypto/intel/qat/qat_common/adf_aer.c | 138 ++++++++++- .../crypto/intel/qat/qat_common/adf_cfg_strings.h | 1 + drivers/crypto/intel/qat/qat_common/adf_clock.c | 3 + .../crypto/intel/qat/qat_common/adf_cnv_dbgfs.c | 1 - .../crypto/intel/qat/qat_common/adf_common_drv.h | 10 + drivers/crypto/intel/qat/qat_common/adf_dev_mgr.c | 4 +- .../crypto/intel/qat/qat_common/adf_gen4_hw_data.c | 59 +++++ .../crypto/intel/qat/qat_common/adf_gen4_hw_data.h | 1 + drivers/crypto/intel/qat/qat_common/adf_gen4_ras.c | 6 +- .../crypto/intel/qat/qat_common/adf_heartbeat.c | 20 +- .../crypto/intel/qat/qat_common/adf_heartbeat.h | 21 ++ .../intel/qat/qat_common/adf_heartbeat_dbgfs.c | 53 +++++ .../intel/qat/qat_common/adf_heartbeat_inject.c | 76 ++++++ .../crypto/intel/qat/qat_common/adf_hw_arbiter.c | 25 ++ drivers/crypto/intel/qat/qat_common/adf_init.c | 12 + drivers/crypto/intel/qat/qat_common/adf_isr.c | 11 +- drivers/crypto/intel/qat/qat_common/adf_pfvf_msg.h | 7 +- .../crypto/intel/qat/qat_common/adf_pfvf_pf_msg.c | 64 ++++- .../crypto/intel/qat/qat_common/adf_pfvf_pf_msg.h | 21 ++ .../intel/qat/qat_common/adf_pfvf_pf_proto.c | 8 + .../intel/qat/qat_common/adf_pfvf_vf_proto.c | 6 + drivers/crypto/intel/qat/qat_common/adf_rl.c | 20 +- drivers/crypto/intel/qat/qat_common/adf_sriov.c | 38 ++- drivers/crypto/intel/qat/qat_common/adf_sysfs.c | 37 +++ drivers/crypto/intel/qat/qat_common/adf_vf_isr.c | 2 - .../crypto/intel/qat/qat_common/qat_comp_algs.c | 9 - drivers/crypto/intel/qat/qat_common/qat_crypto.c | 4 +- drivers/crypto/rockchip/rk3288_crypto.c | 5 + .../crypto/virtio/virtio_crypto_akcipher_algs.c | 12 +- drivers/crypto/virtio/virtio_crypto_core.c | 2 - drivers/crypto/vmx/.gitignore | 3 - drivers/crypto/vmx/Kconfig | 14 -- drivers/crypto/vmx/Makefile | 23 -- drivers/crypto/vmx/ppc-xlate.pl | 231 ------------------ drivers/crypto/xilinx/zynqmp-aes-gcm.c | 3 + include/crypto/internal/hash.h | 2 - include/crypto/public_key.h | 1 + include/linux/hisi_acc_qm.h | 10 +- tools/crypto/ccp/test_dbc.py | 8 +- 100 files changed, 1450 insertions(+), 1016 deletions(-) rename {drivers/crypto/vmx => arch/powerpc/crypto}/aes.c (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/aes_cbc.c (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/aes_ctr.c (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/aes_xts.c (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/aesp8-ppc.h (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/aesp8-ppc.pl (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/ghash.c (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/ghashp8-ppc.pl (100%) rename {drivers/crypto/vmx => arch/powerpc/crypto}/vmx.c (100%) create mode 100644 drivers/crypto/intel/qat/qat_common/adf_heartbeat_inject.c delete mode 100644 drivers/crypto/vmx/.gitignore delete mode 100644 drivers/crypto/vmx/Kconfig delete mode 100644 drivers/crypto/vmx/Makefile delete mode 100644 drivers/crypto/vmx/ppc-xlate.pl Thanks,