Message ID | 20250227123338.3033402-1-ardb+git@google.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Herbert Xu |
Headers | show |
Series | crypto: lib/chachapoly - Drop dependency on CRYPTO_ALGAPI | expand |
On Thu, Feb 27, 2025, at 13:33, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > The ChaCha20-Poly1305 library code uses the sg_miter API to process > input presented via scatterlists, except for the special case where the > digest buffer is not covered entirely by the same scatterlist entry as > the last byte of input. In that case, it uses scatterwalk_map_and_copy() > to access the memory in the input scatterlist where the digest is stored. > > This results in a dependency on crypto/scatterwalk.c and therefore on > CONFIG_CRYPTO_ALGAPI, which is unnecessary, as the sg_miter API already > provides this functionality via sg_copy_to_buffer(). So use that > instead, and drop the CRYPTO_ALGAPI dependency. > > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Thanks for the fixup! > --- > lib/crypto/Kconfig | 1 - > lib/crypto/chacha20poly1305.c | 5 ++--- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig > index b01253cac70a..a759e6f6a939 100644 > --- a/lib/crypto/Kconfig > +++ b/lib/crypto/Kconfig > @@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305 > depends on CRYPTO > select CRYPTO_LIB_CHACHA > select CRYPTO_LIB_POLY1305 > - select CRYPTO_ALGAPI I think importantly, dropping the 'select CRYPTO_ALGAPI' means that the 'depends on CRYPTO' here can also be dropped: CRYPTO_LIB_CHACHA20POLY1305 needs nothing else from the crypto subsystem. Moreover, CONFIG_WIREGUARD can now drop the 'select CRYPTO' because it seems to only need this in order to 'select CRYPTO_LIB_CHACHA20POLY1305'. Arnd
On Thu, Feb 27, 2025 at 01:33:38PM +0100, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > The ChaCha20-Poly1305 library code uses the sg_miter API to process > input presented via scatterlists, except for the special case where the > digest buffer is not covered entirely by the same scatterlist entry as > the last byte of input. In that case, it uses scatterwalk_map_and_copy() > to access the memory in the input scatterlist where the digest is stored. > > This results in a dependency on crypto/scatterwalk.c and therefore on > CONFIG_CRYPTO_ALGAPI, which is unnecessary, as the sg_miter API already > provides this functionality via sg_copy_to_buffer(). So use that > instead, and drop the CRYPTO_ALGAPI dependency. > > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Acked-by: Eric Biggers <ebiggers@kernel.org> There's indeed redundancy between crypto/scatterwalk.c and lib/scatterlist.c, and switching to lib/scatterlist.c makes sense here. I do think that the implementation in crypto/scatterwalk.c is slightly better (including being slightly more efficient), especially if my patchset https://lore.kernel.org/linux-crypto/20250219182341.43961-1-ebiggers@kernel.org/ gets applied. It would be a good future project to unify them into a single version, which of course would not depend on CRYPTO. But for now, just using the one that already does not depend on CRYPTO here is the right choice. > diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig > index b01253cac70a..a759e6f6a939 100644 > --- a/lib/crypto/Kconfig > +++ b/lib/crypto/Kconfig > @@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305 > depends on CRYPTO > select CRYPTO_LIB_CHACHA > select CRYPTO_LIB_POLY1305 > - select CRYPTO_ALGAPI As Arnd mentioned, 'depends on CRYPTO' should be dropped. > diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c > index a839c0ac60b2..280a4925dd17 100644 > --- a/lib/crypto/chacha20poly1305.c > +++ b/lib/crypto/chacha20poly1305.c > @@ -11,7 +11,6 @@ > #include <crypto/algapi.h> > #include <crypto/chacha20poly1305.h> > #include <crypto/chacha.h> > #include <crypto/poly1305.h> > -#include <crypto/scatterwalk.h> Also replace the include of <crypto/algapi.h> with <crypto/utils.h>, for consistency with the removal of the selection of CRYPTO_ALGAPI. - Eric
On Thu, Feb 27, 2025 at 01:59:42PM +0100, Arnd Bergmann wrote: > > I think importantly, dropping the 'select CRYPTO_ALGAPI' > means that the 'depends on CRYPTO' here can also be dropped: > CRYPTO_LIB_CHACHA20POLY1305 needs nothing else from the > crypto subsystem. Moreover, CONFIG_WIREGUARD can now drop the > 'select CRYPTO' because it seems to only need this in > order to 'select CRYPTO_LIB_CHACHA20POLY1305'. Yes you can drop it from CHACHA20POLY1305, but you must keep the 'select CRYPTO' on the individual CHACHA and POLY1305 LIB options as otherwise the arch options cannot be selected at all since they're all under CRYPTO. Cheers,
On Thu, Feb 27, 2025 at 01:33:38PM +0100, Ard Biesheuvel wrote: > > diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig > index b01253cac70a..a759e6f6a939 100644 > --- a/lib/crypto/Kconfig > +++ b/lib/crypto/Kconfig > @@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305 > depends on CRYPTO > select CRYPTO_LIB_CHACHA > select CRYPTO_LIB_POLY1305 > - select CRYPTO_ALGAPI You should add CRYPTO_LIB_UTILS. It's coming indirectly through CRYPTO but as Arnd suggested that can now be removed. > diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c > index a839c0ac60b2..280a4925dd17 100644 > --- a/lib/crypto/chacha20poly1305.c > +++ b/lib/crypto/chacha20poly1305.c > @@ -11,7 +11,6 @@ > #include <crypto/chacha20poly1305.h> > #include <crypto/chacha.h> > #include <crypto/poly1305.h> > -#include <crypto/scatterwalk.h> Please also replace algapi.h with utils.h. Thanks,
diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig index b01253cac70a..a759e6f6a939 100644 --- a/lib/crypto/Kconfig +++ b/lib/crypto/Kconfig @@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305 depends on CRYPTO select CRYPTO_LIB_CHACHA select CRYPTO_LIB_POLY1305 - select CRYPTO_ALGAPI config CRYPTO_LIB_SHA1 tristate diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c index a839c0ac60b2..280a4925dd17 100644 --- a/lib/crypto/chacha20poly1305.c +++ b/lib/crypto/chacha20poly1305.c @@ -11,7 +11,6 @@ #include <crypto/chacha20poly1305.h> #include <crypto/chacha.h> #include <crypto/poly1305.h> -#include <crypto/scatterwalk.h> #include <linux/unaligned.h> #include <linux/kernel.h> @@ -318,8 +317,8 @@ bool chacha20poly1305_crypt_sg_inplace(struct scatterlist *src, if (unlikely(sl > -POLY1305_DIGEST_SIZE)) { poly1305_final(&poly1305_state, b.mac[1]); - scatterwalk_map_and_copy(b.mac[encrypt], src, src_len, - sizeof(b.mac[1]), encrypt); + sg_copy_buffer(src, sg_nents(src), b.mac[encrypt], + sizeof(b.mac[1]), src_len, !encrypt); ret = encrypt || !crypto_memneq(b.mac[0], b.mac[1], POLY1305_DIGEST_SIZE); }