[PATCH] crypto/crc32: register only one shash_alg

[PATCH] crypto/crc32: register only one shash_alg

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Stop unnecessarily registering a "crc32-generic" shash_alg when a
"crc32-$(ARCH)" shash_alg is registered too.

While every algorithm does need to have a generic implementation to
ensure uniformity of support across platforms, that doesn't mean that we
need to make the generic implementation available through crypto_shash
when an optimized implementation is also available.

Registering the generic shash_alg did allow users of the crypto_shash or
crypto_ahash APIs to request the generic implementation specifically,
instead of an optimized one.  However, the only known use case for that
was the differential fuzz tests in crypto/testmgr.c.  Equivalent test
coverage is now provided by crc_kunit.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---

I'm planning to take this through the crc tree.

 crypto/crc32.c | 69 ++++++++------------------------------------------
 1 file changed, 11 insertions(+), 58 deletions(-)

diff --git a/crypto/crc32.c b/crypto/crc32.c
index cc371d42601fd..b61d5663d0bac 100644
--- a/crypto/crc32.c
+++ b/crypto/crc32.c
@@ -57,33 +57,16 @@ static int crc32_init(struct shash_desc *desc)
 static int crc32_update(struct shash_desc *desc, const u8 *data,
 			unsigned int len)
 {
 	u32 *crcp = shash_desc_ctx(desc);
 
-	*crcp = crc32_le_base(*crcp, data, len);
-	return 0;
-}
-
-static int crc32_update_arch(struct shash_desc *desc, const u8 *data,
-			     unsigned int len)
-{
-	u32 *crcp = shash_desc_ctx(desc);
-
 	*crcp = crc32_le(*crcp, data, len);
 	return 0;
 }
 
 /* No final XOR 0xFFFFFFFF, like crc32_le */
-static int __crc32_finup(u32 *crcp, const u8 *data, unsigned int len,
-			 u8 *out)
-{
-	put_unaligned_le32(crc32_le_base(*crcp, data, len), out);
-	return 0;
-}
-
-static int __crc32_finup_arch(u32 *crcp, const u8 *data, unsigned int len,
-			      u8 *out)
+static int __crc32_finup(u32 *crcp, const u8 *data, unsigned int len, u8 *out)
 {
 	put_unaligned_le32(crc32_le(*crcp, data, len), out);
 	return 0;
 }
 
@@ -91,16 +74,10 @@ static int crc32_finup(struct shash_desc *desc, const u8 *data,
 		       unsigned int len, u8 *out)
 {
 	return __crc32_finup(shash_desc_ctx(desc), data, len, out);
 }
 
-static int crc32_finup_arch(struct shash_desc *desc, const u8 *data,
-		       unsigned int len, u8 *out)
-{
-	return __crc32_finup_arch(shash_desc_ctx(desc), data, len, out);
-}
-
 static int crc32_final(struct shash_desc *desc, u8 *out)
 {
 	u32 *crcp = shash_desc_ctx(desc);
 
 	put_unaligned_le32(*crcp, out);
@@ -111,72 +88,48 @@ static int crc32_digest(struct shash_desc *desc, const u8 *data,
 			unsigned int len, u8 *out)
 {
 	return __crc32_finup(crypto_shash_ctx(desc->tfm), data, len, out);
 }
 
-static int crc32_digest_arch(struct shash_desc *desc, const u8 *data,
-			     unsigned int len, u8 *out)
-{
-	return __crc32_finup_arch(crypto_shash_ctx(desc->tfm), data, len, out);
-}
-
-static struct shash_alg algs[] = {{
+static struct shash_alg alg = {
 	.setkey			= crc32_setkey,
 	.init			= crc32_init,
 	.update			= crc32_update,
 	.final			= crc32_final,
 	.finup			= crc32_finup,
 	.digest			= crc32_digest,
 	.descsize		= sizeof(u32),
 	.digestsize		= CHKSUM_DIGEST_SIZE,
 
 	.base.cra_name		= "crc32",
-	.base.cra_driver_name	= "crc32-generic",
 	.base.cra_priority	= 100,
 	.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 	.base.cra_blocksize	= CHKSUM_BLOCK_SIZE,
 	.base.cra_ctxsize	= sizeof(u32),
 	.base.cra_module	= THIS_MODULE,
 	.base.cra_init		= crc32_cra_init,
-}, {
-	.setkey			= crc32_setkey,
-	.init			= crc32_init,
-	.update			= crc32_update_arch,
-	.final			= crc32_final,
-	.finup			= crc32_finup_arch,
-	.digest			= crc32_digest_arch,
-	.descsize		= sizeof(u32),
-	.digestsize		= CHKSUM_DIGEST_SIZE,
-
-	.base.cra_name		= "crc32",
-	.base.cra_driver_name	= "crc32-" __stringify(ARCH),
-	.base.cra_priority	= 150,
-	.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
-	.base.cra_blocksize	= CHKSUM_BLOCK_SIZE,
-	.base.cra_ctxsize	= sizeof(u32),
-	.base.cra_module	= THIS_MODULE,
-	.base.cra_init		= crc32_cra_init,
-}};
-
-static int num_algs;
+};
 
 static int __init crc32_mod_init(void)
 {
-	/* register the arch flavor only if it differs from the generic one */
-	num_algs = 1 + ((crc32_optimizations() & CRC32_LE_OPTIMIZATION) != 0);
+	const char *driver_name =
+		(crc32_optimizations() & CRC32_LE_OPTIMIZATION) ?
+			"crc32-" __stringify(ARCH) :
+			"crc32-generic";
+
+	strscpy(alg.base.cra_driver_name, driver_name, CRYPTO_MAX_ALG_NAME);
 
-	return crypto_register_shashes(algs, num_algs);
+	return crypto_register_shash(&alg);
 }
 
 static void __exit crc32_mod_fini(void)
 {
-	crypto_unregister_shashes(algs, num_algs);
+	crypto_unregister_shash(&alg);
 }
 
 module_init(crc32_mod_init);
 module_exit(crc32_mod_fini);
 
 MODULE_AUTHOR("Alexander Boyko <alexander_boyko@xyratex.com>");
 MODULE_DESCRIPTION("CRC32 calculations wrapper for lib/crc32");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_CRYPTO("crc32");
-MODULE_ALIAS_CRYPTO("crc32-generic");

base-commit: f66bc387efbee59978e076ce9bf123ac353b389c
-- 
2.49.0

Re: [PATCH] crypto/crc32: register only one shash_alg

[Herbert Xu]

Eric Biggers <ebiggers@kernel.org> wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Stop unnecessarily registering a "crc32-generic" shash_alg when a
> "crc32-$(ARCH)" shash_alg is registered too.
> 
> While every algorithm does need to have a generic implementation to
> ensure uniformity of support across platforms, that doesn't mean that we
> need to make the generic implementation available through crypto_shash
> when an optimized implementation is also available.
> 
> Registering the generic shash_alg did allow users of the crypto_shash or
> crypto_ahash APIs to request the generic implementation specifically,
> instead of an optimized one.  However, the only known use case for that
> was the differential fuzz tests in crypto/testmgr.c.  Equivalent test
> coverage is now provided by crc_kunit.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> 
> I'm planning to take this through the crc tree.
> 
> crypto/crc32.c | 69 ++++++++------------------------------------------
> 1 file changed, 11 insertions(+), 58 deletions(-)

Please don't do this without first removing all drivers providing
"crc32" as otherwise their test coverge will be reduced.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto/crc32: register only one shash_alg

[Eric Biggers]

On Sat, May 31, 2025 at 03:09:37PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Stop unnecessarily registering a "crc32-generic" shash_alg when a
> > "crc32-$(ARCH)" shash_alg is registered too.
> > 
> > While every algorithm does need to have a generic implementation to
> > ensure uniformity of support across platforms, that doesn't mean that we
> > need to make the generic implementation available through crypto_shash
> > when an optimized implementation is also available.
> > 
> > Registering the generic shash_alg did allow users of the crypto_shash or
> > crypto_ahash APIs to request the generic implementation specifically,
> > instead of an optimized one.  However, the only known use case for that
> > was the differential fuzz tests in crypto/testmgr.c.  Equivalent test
> > coverage is now provided by crc_kunit.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> > 
> > I'm planning to take this through the crc tree.
> > 
> > crypto/crc32.c | 69 ++++++++------------------------------------------
> > 1 file changed, 11 insertions(+), 58 deletions(-)
> 
> Please don't do this without first removing all drivers providing
> "crc32" as otherwise their test coverge will be reduced.
> 
> Cheers,

Yes, I'll do that.  It's time to do that anyway.

For other algorithms like sha256 where it's unlikely that all the drivers can be
removed, testmgr.c should just compare against the library implementation, not
the "generic" implementation specifically.

Ideally we'll just stop pretending that cra_driver_name actually matters, and
just name the software algorithms *-lib or *-software.

- Eric