mbox series

[0/9] Remove per-architecture ChaCha skcipher glue code

Message ID 20250405182609.404216-1-ebiggers@kernel.org (mailing list archive)
Headers show
Series Remove per-architecture ChaCha skcipher glue code | expand

Message

Eric Biggers April 5, 2025, 6:26 p.m. UTC
Currently each architecture exposes ChaCha not only through the library
API, but also through the crypto_skcipher API.  That requires each
architecture to implement essentially the same skcipher glue code.

Following the example of what's been done for crc32 and crc32c,
eliminate this redundancy by making crypto/chacha.c register both the
generic and architecture-optimized skcipher algorithms, implemented on
top of the appropriate library functions.  This removes almost 800 lines
of code and disentangles the library code from the skcipher API.

From what I remember, the following are the reasons why it wasn't just
done this way originally.  But none of these really hold water:

- The skcipher code was there first, so it may have seemed more natural
  to add onto it rather than replace it.

- Architectures could register multiple skcipher algorithms using
  different CPU features and have them all be tested in a single boot.
  This was convenient in theory, but it never really worked properly.
  It didn't apply to the library code, the x86 ChaCha code wasn't
  actually doing this (it used static keys instead), and this cannot
  catch bugs like accidentally using an AVX instruction in SSE code.
  Instead, a correct solution, which also doesn't require any special
  kernel support, is to just boot the kernel in QEMU using different
  -cpu arguments as needed to test all the code.

- There was a concern about changing cra_driver_names potentially
  breaking users.  But in practice users rely on cra_name, not
  cra_driver_name.  We already change, add, and remove cra_driver_names
  occasionally for various reasons.  And even if someone was relying on
  a specific cra_driver_name, there are some more lightweight
  compatibility tricks that could be used.

- There was a desire for users to be able to override the kernel's
  choice of ChaCha implementation by blacklisting the arch-optimized
  ChaCha module.  But that already became mostly impossible when the
  library functions were added to the same module.  And in practice
  users don't do this anyway.  Even if, hypothetically, someone really
  needed to do this and for some reason the kernel couldn't be fixed to
  make the right choice in their case automatically, there are other
  ways this could be implemented such as a module parameter.

Eric Biggers (9):
  crypto: riscv/chacha - implement library instead of skcipher
  crypto: chacha - centralize the skcipher wrappers for arch code
  crypto: arm/chacha - remove the redundant skcipher algorithms
  crypto: arm64/chacha - remove the skcipher algorithms
  crypto: mips/chacha - remove the skcipher algorithms
  crypto: powerpc/chacha - remove the skcipher algorithms
  crypto: s390/chacha - remove the skcipher algorithms
  crypto: x86/chacha - remove the skcipher algorithms
  crypto: chacha - remove <crypto/internal/chacha.h>

 arch/arm/crypto/Kconfig                 |   7 -
 arch/arm/crypto/chacha-glue.c           | 243 +---------------------
 arch/arm/crypto/chacha-neon-core.S      |   2 +-
 arch/arm64/crypto/Kconfig               |   7 -
 arch/arm64/crypto/chacha-neon-core.S    |   2 +-
 arch/arm64/crypto/chacha-neon-glue.c    | 146 +------------
 arch/mips/crypto/Kconfig                |   6 -
 arch/mips/crypto/chacha-glue.c          | 131 +-----------
 arch/powerpc/crypto/Kconfig             |   8 -
 arch/powerpc/crypto/chacha-p10-glue.c   | 147 +-------------
 arch/riscv/crypto/Kconfig               |  11 +-
 arch/riscv/crypto/chacha-riscv64-glue.c | 112 ++++------
 arch/riscv/crypto/chacha-riscv64-zvkb.S |  71 +++----
 arch/s390/crypto/Kconfig                |   7 -
 arch/s390/crypto/chacha-glue.c          |  99 ++-------
 arch/x86/crypto/Kconfig                 |   9 -
 arch/x86/crypto/chacha_glue.c           | 144 +------------
 crypto/Makefile                         |   3 +-
 crypto/chacha.c                         | 260 ++++++++++++++++++++++++
 crypto/chacha_generic.c                 | 139 -------------
 include/crypto/chacha.h                 |   9 +
 include/crypto/internal/chacha.h        |  43 ----
 22 files changed, 413 insertions(+), 1193 deletions(-)
 create mode 100644 crypto/chacha.c
 delete mode 100644 crypto/chacha_generic.c
 delete mode 100644 include/crypto/internal/chacha.h

base-commit: 56f944529ec2292cbe63377a76df3759d702dd39

Comments

Herbert Xu April 7, 2025, 5:28 a.m. UTC | #1
Eric Biggers <ebiggers@kernel.org> wrote:
> Currently each architecture exposes ChaCha not only through the library
> API, but also through the crypto_skcipher API.  That requires each
> architecture to implement essentially the same skcipher glue code.
> 
> Following the example of what's been done for crc32 and crc32c,
> eliminate this redundancy by making crypto/chacha.c register both the
> generic and architecture-optimized skcipher algorithms, implemented on
> top of the appropriate library functions.  This removes almost 800 lines
> of code and disentangles the library code from the skcipher API.
> 
> From what I remember, the following are the reasons why it wasn't just
> done this way originally.  But none of these really hold water:
> 
> - The skcipher code was there first, so it may have seemed more natural
>  to add onto it rather than replace it.
> 
> - Architectures could register multiple skcipher algorithms using
>  different CPU features and have them all be tested in a single boot.
>  This was convenient in theory, but it never really worked properly.
>  It didn't apply to the library code, the x86 ChaCha code wasn't
>  actually doing this (it used static keys instead), and this cannot
>  catch bugs like accidentally using an AVX instruction in SSE code.
>  Instead, a correct solution, which also doesn't require any special
>  kernel support, is to just boot the kernel in QEMU using different
>  -cpu arguments as needed to test all the code.
> 
> - There was a concern about changing cra_driver_names potentially
>  breaking users.  But in practice users rely on cra_name, not
>  cra_driver_name.  We already change, add, and remove cra_driver_names
>  occasionally for various reasons.  And even if someone was relying on
>  a specific cra_driver_name, there are some more lightweight
>  compatibility tricks that could be used.
> 
> - There was a desire for users to be able to override the kernel's
>  choice of ChaCha implementation by blacklisting the arch-optimized
>  ChaCha module.  But that already became mostly impossible when the
>  library functions were added to the same module.  And in practice
>  users don't do this anyway.  Even if, hypothetically, someone really
>  needed to do this and for some reason the kernel couldn't be fixed to
>  make the right choice in their case automatically, there are other
>  ways this could be implemented such as a module parameter.
> 
> Eric Biggers (9):
>  crypto: riscv/chacha - implement library instead of skcipher
>  crypto: chacha - centralize the skcipher wrappers for arch code
>  crypto: arm/chacha - remove the redundant skcipher algorithms
>  crypto: arm64/chacha - remove the skcipher algorithms
>  crypto: mips/chacha - remove the skcipher algorithms
>  crypto: powerpc/chacha - remove the skcipher algorithms
>  crypto: s390/chacha - remove the skcipher algorithms
>  crypto: x86/chacha - remove the skcipher algorithms
>  crypto: chacha - remove <crypto/internal/chacha.h>
> 
> arch/arm/crypto/Kconfig                 |   7 -
> arch/arm/crypto/chacha-glue.c           | 243 +---------------------
> arch/arm/crypto/chacha-neon-core.S      |   2 +-
> arch/arm64/crypto/Kconfig               |   7 -
> arch/arm64/crypto/chacha-neon-core.S    |   2 +-
> arch/arm64/crypto/chacha-neon-glue.c    | 146 +------------
> arch/mips/crypto/Kconfig                |   6 -
> arch/mips/crypto/chacha-glue.c          | 131 +-----------
> arch/powerpc/crypto/Kconfig             |   8 -
> arch/powerpc/crypto/chacha-p10-glue.c   | 147 +-------------
> arch/riscv/crypto/Kconfig               |  11 +-
> arch/riscv/crypto/chacha-riscv64-glue.c | 112 ++++------
> arch/riscv/crypto/chacha-riscv64-zvkb.S |  71 +++----
> arch/s390/crypto/Kconfig                |   7 -
> arch/s390/crypto/chacha-glue.c          |  99 ++-------
> arch/x86/crypto/Kconfig                 |   9 -
> arch/x86/crypto/chacha_glue.c           | 144 +------------
> crypto/Makefile                         |   3 +-
> crypto/chacha.c                         | 260 ++++++++++++++++++++++++
> crypto/chacha_generic.c                 | 139 -------------
> include/crypto/chacha.h                 |   9 +
> include/crypto/internal/chacha.h        |  43 ----
> 22 files changed, 413 insertions(+), 1193 deletions(-)
> create mode 100644 crypto/chacha.c
> delete mode 100644 crypto/chacha_generic.c
> delete mode 100644 include/crypto/internal/chacha.h
> 
> base-commit: 56f944529ec2292cbe63377a76df3759d702dd39

All applied.  Thanks.