Simplify the shash wrappers for the CRC32 library
Subsystems (3)
cover Simplify the shash wrappers for the CRC32 library
This series simplifies how the CRC32 library functions are exposed through the crypto_shash API. We'll now have just one shash algorithm each for "crc32" and "crc32c", and their driver names will just always be "crc32-lib" and "crc32c-lib" respectively. This seems to be all that's actually needed.
As mentioned in patch 2, this does change the content of /sys/fs/btrfs/$uuid/checksum again, but that should be fine.
This is based on v6.16-rc1, and I'm planning to take these patches through the crc-next tree. These supersede
https://lore.kernel.org/r/[email protected]/
and
https://lore.kernel.org/r/[email protected]/,
and they fix the warning in the full crypto self-tests reported at
https://lore.kernel.org/r/[email protected]/
Eric Biggers (2):
btrfs: stop parsing crc32c driver name
crypto/crc32[c]: register only "-lib" drivers
crypto/Makefile | 2 -- crypto/crc32.c | 65 +++++---------------------------------------- crypto/crc32c.c | 66 ++++------------------------------------------ crypto/testmgr.c | 2 ++ fs/btrfs/disk-io.c | 8 ++---- 5 files changed, 15 insertions(+), 128 deletions(-)
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
Signature · 1 line
2.49.0
PATCH 1/2 btrfs: stop parsing crc32c driver name
From: Eric Biggers <[email protected]>
To determine whether the crc32c implementation is "fast", use crc32_optimizations() instead of parsing the crypto_shash driver name. This keeps the code working as intended after the driver name is changed by the next patch.
--- fs/btrfs/disk-io.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
fs/btrfs/disk-io.c +2 -6
@@ -2024,18 +2024,14 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)return PTR_ERR (csum_shash); } fs_info->csum_shash = csum_shash; -/* -* Check if the checksum implementation is a fast accelerated one. -* As-is this is a bit of a hack and should be replaced once the csum -* implementations provide that information themselves. -*/ +/* Check if the checksum implementation is a fast accelerated one. */ switch (csum_type) {case BTRFS_CSUM_TYPE_CRC32 : -if (!strstr (crypto_shash_driver_name (csum_shash),"generic" )) +if (crc32_optimizations () &CRC32C_OPTIMIZATION )set_bit (BTRFS_FS_CSUM_IMPL_FAST , &fs_info->flags);break ;case BTRFS_CSUM_TYPE_XXHASH :set_bit (BTRFS_FS_CSUM_IMPL_FAST , &fs_info->flags);break ;
Signature · 1 line
2.49.0
PATCH 2/2 crypto/crc32[c]: register only "-lib" drivers
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
For the "crc32" and "crc32c" shash algorithms, instead of registering "*-generic" drivers as well as conditionally registering "*-$(ARCH)" drivers, instead just register "*-lib" drivers. These just use the regular library functions crc32_le() and crc32c(), so they just do the right thing and are fully accelerated when supported by the CPU.
This eliminates the need for the CRC library to export crc32_le_base() and crc32c_base(). Separate patches make those static functions.
Since this patch removes the "crc32-generic" and "crc32c-generic" driver names which crypto/testmgr.c expects to exist, update crypto/testmgr.c accordingly. This does mean that crypto/testmgr.c will no longer fuzz-test the "generic" implementation against the "arch" implementation for crc32 and crc32c, but this was redundant with crc_kunit anyway.
Besides the above, and btrfs_init_csum_hash() which the previous patch fixed, no code appears to have been relying on the "crc32-generic" or "crc32c-generic" driver names specifically.
btrfs does export the checksum driver name in /sys/fs/btrfs/$uuid/checksum. This patch makes that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and this file is basically obsolete. (Also, note that the contents of this file already changed in 6.14.)
--- crypto/Makefile | 2 -- crypto/crc32.c | 65 +++++------------------------------------------ crypto/crc32c.c | 66 ++++-------------------------------------------- crypto/testmgr.c | 2 ++ 4 files changed, 13 insertions(+), 122 deletions(-)
crypto/Makefile +0 -2
@@ -152,14 +152,12 @@ obj-$(CONFIG_CRYPTO_CHACHA20) += chacha.o CFLAGS_chacha.o += -DARCH=$(ARCH) obj-$(CONFIG_CRYPTO_DEFLATE) += deflate.o obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o obj-$(CONFIG_CRYPTO_CRC32C) += crc32c-cryptoapi.o crc32c-cryptoapi-y := crc32c.o -CFLAGS_crc32c.o += -DARCH=$(ARCH) obj-$(CONFIG_CRYPTO_CRC32) += crc32-cryptoapi.o crc32-cryptoapi-y := crc32.o -CFLAGS_crc32.o += -DARCH=$(ARCH) obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o obj-$(CONFIG_CRYPTO_KRB5ENC) += krb5enc.o obj-$(CONFIG_CRYPTO_LZO) += lzo.o lzo-rle.o obj-$(CONFIG_CRYPTO_LZ4) += lz4.o obj-$(CONFIG_CRYPTO_LZ4HC) += lz4hc.o
crypto/crc32.c +6 -59
127 lines not on this page · raw message
crypto/crc32c.c +5 -61
139 lines not on this page · raw message
crypto/testmgr.c +2 -0
@@ -4544,17 +4544,19 @@ static const struct alg_test_desc alg_test_descs[] = { .suite = { .hash = __VECS(sm4_cmac128_tv_template) } }, { .alg = "crc32", + .generic_driver = "crc32-lib", .test = alg_test_hash, .fips_allowed = 1, .suite = { .hash = __VECS(crc32_tv_template) } }, { .alg = "crc32c", + .generic_driver = "crc32c-lib", .test = alg_test_crc32c, .fips_allowed = 1, .suite = { .hash =__VECS (crc32c_tv_template) }
Signature · 1 line
2.49.0
Re: [PATCH 2/2] crypto/crc32[c]: register only "-lib" drivers
Quoted 21 lines · Eric Biggers
From: Eric Biggers <[email protected]>
For the "crc32" and "crc32c" shash algorithms, instead of registering "*-generic" drivers as well as conditionally registering "*-$(ARCH)" drivers, instead just register "*-lib" drivers. These just use the regular library functions
crc32_le()andcrc32c(), so they just do the right thing and are fully accelerated when supported by the CPU.This eliminates the need for the CRC library to export
crc32_le_base()andcrc32c_base(). Separate patches make those static functions.Since this patch removes the "crc32-generic" and "crc32c-generic" driver names which
crypto/testmgr.cexpects to exist, updatecrypto/testmgr.caccordingly. This does mean thatcrypto/testmgr.cwill no longer fuzz-test the "generic" implementation against the "arch" implementation for crc32 and crc32c, but this was redundant withcrc_kunitanyway.Besides the above, and
btrfs_init_csum_hash()which the previous patch fixed, no code appears to have been relying on the "crc32-generic" or "crc32c-generic" driver names specifically.
Quoted 10 lines
btrfs does export the checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This patch makes that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and this file is basically obsolete.
Well, not the whole file, because it says which checksumming algo is used for the filesystem, but the implementation part is.
Re: [PATCH 2/2] crypto/crc32[c]: register only "-lib" drivers
Quoted 33 lines · David Sterba
Quoted 21 lines · Eric Biggers
From: Eric Biggers <[email protected]>
For the "crc32" and "crc32c" shash algorithms, instead of registering "*-generic" drivers as well as conditionally registering "*-$(ARCH)" drivers, instead just register "*-lib" drivers. These just use the regular library functions
crc32_le()andcrc32c(), so they just do the right thing and are fully accelerated when supported by the CPU.This eliminates the need for the CRC library to export
crc32_le_base()andcrc32c_base(). Separate patches make those static functions.Since this patch removes the "crc32-generic" and "crc32c-generic" driver names which
crypto/testmgr.cexpects to exist, updatecrypto/testmgr.caccordingly. This does mean thatcrypto/testmgr.cwill no longer fuzz-test the "generic" implementation against the "arch" implementation for crc32 and crc32c, but this was redundant withcrc_kunitanyway.Besides the above, and
btrfs_init_csum_hash()which the previous patch fixed, no code appears to have been relying on the "crc32-generic" or "crc32c-generic" driver names specifically.Quoted 10 lines
btrfs does export the checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This patch makes that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and this file is basically obsolete.
Quoted 2 lines
Well, not the whole file, because it says which checksumming algo is used for the filesystem, but the implementation part is.
Oh, right. It's one of those sysfs files that don't follow the normal sysfs convention and contain multiple values. I'll update the paragraph above to clarify that it's referring to the driver name part of the file.
- Eric
Re: [PATCH 2/2] crypto/crc32[c]: register only "-lib" drivers
Quoted 34 lines · Eric Biggers
Quoted 33 lines · David Sterba
Quoted 31 lines · Eric Biggers
From: Eric Biggers <[email protected]>
For the "crc32" and "crc32c" shash algorithms, instead of registering "*-generic" drivers as well as conditionally registering "*-$(ARCH)" drivers, instead just register "*-lib" drivers. These just use the regular library functions
crc32_le()andcrc32c(), so they just do the right thing and are fully accelerated when supported by the CPU.This eliminates the need for the CRC library to export
crc32_le_base()andcrc32c_base(). Separate patches make those static functions.Since this patch removes the "crc32-generic" and "crc32c-generic" driver names which
crypto/testmgr.cexpects to exist, updatecrypto/testmgr.caccordingly. This does mean thatcrypto/testmgr.cwill no longer fuzz-test the "generic" implementation against the "arch" implementation for crc32 and crc32c, but this was redundant withcrc_kunitanyway.Besides the above, and
btrfs_init_csum_hash()which the previous patch fixed, no code appears to have been relying on the "crc32-generic" or "crc32c-generic" driver names specifically.btrfs does export the checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This patch makes that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and this file is basically obsolete.
Quoted 6 lines
Quoted 2 lines
Well, not the whole file, because it says which checksumming algo is used for the filesystem, but the implementation part is.
Oh, right. It's one of those sysfs files that don't follow the normal sysfs convention and contain multiple values. I'll update the paragraph above to clarify that it's referring to the driver name part of the file.
I revised it to:
btrfs does export the checksum name and checksum driver name in /sys/fs/btrfs/$uuid/checksum. This commit makes the driver name portion of that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of the driver name portion of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and the driver name portion of this is basically obsolete. (Also, note that the crc32c driver name already changed in 6.14.)
Re: [PATCH 2/2] crypto/crc32[c]: register only "-lib" drivers
Quoted 44 lines · Eric Biggers
Quoted 34 lines · Eric Biggers
Quoted 33 lines · David Sterba
Quoted 31 lines · Eric Biggers
From: Eric Biggers <[email protected]>
For the "crc32" and "crc32c" shash algorithms, instead of registering "*-generic" drivers as well as conditionally registering "*-$(ARCH)" drivers, instead just register "*-lib" drivers. These just use the regular library functions
crc32_le()andcrc32c(), so they just do the right thing and are fully accelerated when supported by the CPU.This eliminates the need for the CRC library to export
crc32_le_base()andcrc32c_base(). Separate patches make those static functions.Since this patch removes the "crc32-generic" and "crc32c-generic" driver names which
crypto/testmgr.cexpects to exist, updatecrypto/testmgr.caccordingly. This does mean thatcrypto/testmgr.cwill no longer fuzz-test the "generic" implementation against the "arch" implementation for crc32 and crc32c, but this was redundant withcrc_kunitanyway.Besides the above, and
btrfs_init_csum_hash()which the previous patch fixed, no code appears to have been relying on the "crc32-generic" or "crc32c-generic" driver names specifically.btrfs does export the checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This patch makes that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and this file is basically obsolete.Quoted 6 lines
Quoted 2 lines
Well, not the whole file, because it says which checksumming algo is used for the filesystem, but the implementation part is.
Oh, right. It's one of those sysfs files that don't follow the normal sysfs convention and contain multiple values. I'll update the paragraph above to clarify that it's referring to the driver name part of the file.
I revised it to:
Quoted 12 lines
btrfs does export the checksum name and checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This commit makes the driver name portion of that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of the driver name portion of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and the driver name portion of this is basically obsolete. (Also, note that the crc32c driver name already changed in 6.14.)
This is OK, thanks.
Re: [PATCH 2/2] crypto/crc32[c]: register only "-lib" drivers
In reply to Eric BiggersQuoted 45 lines · Eric Biggers
Quoted 34 lines · Eric Biggers
Quoted 33 lines · David Sterba
Quoted 31 lines · Eric Biggers
From: Eric Biggers <[email protected]>
For the "crc32" and "crc32c" shash algorithms, instead of registering "*-generic" drivers as well as conditionally registering "*-$(ARCH)" drivers, instead just register "*-lib" drivers. These just use the regular library functions
crc32_le()andcrc32c(), so they just do the right thing and are fully accelerated when supported by the CPU.This eliminates the need for the CRC library to export
crc32_le_base()andcrc32c_base(). Separate patches make those static functions.Since this patch removes the "crc32-generic" and "crc32c-generic" driver names which
crypto/testmgr.cexpects to exist, updatecrypto/testmgr.caccordingly. This does mean thatcrypto/testmgr.cwill no longer fuzz-test the "generic" implementation against the "arch" implementation for crc32 and crc32c, but this was redundant withcrc_kunitanyway.Besides the above, and
btrfs_init_csum_hash()which the previous patch fixed, no code appears to have been relying on the "crc32-generic" or "crc32c-generic" driver names specifically.btrfs does export the checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This patch makes that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and this file is basically obsolete.Quoted 6 lines
Quoted 2 lines
Well, not the whole file, because it says which checksumming algo is used for the filesystem, but the implementation part is.
Oh, right. It's one of those sysfs files that don't follow the normal sysfs convention and contain multiple values. I'll update the paragraph above to clarify that it's referring to the driver name part of the file.
I revised it to:
Quoted 12 lines
btrfs does export the checksum name and checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This commit makes the driver name portion of that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of the driver name portion of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and the driver name portion of this is basically obsolete. (Also, note that the crc32c driver name already changed in 6.14.)
How about instead removing that part since it's useless now?
Re: [PATCH 2/2] crypto/crc32[c]: register only "-lib" drivers
Quoted 15 lines · Daniel Vacek
Quoted 14 lines
I revised it to:
btrfs does export the checksum name and checksum driver name in
/sys/fs/btrfs/$uuid/checksum. This commit makes the driver name portion of that file contain "crc32c-lib" instead of "crc32c-generic" or "crc32c-$(ARCH)". This should be fine, since in practice the purpose of the driver name portion of this file seems to have been just to allow users to manually check whether they needed to enable the optimized CRC32C code. This was needed only because of the bug in old kernels where the optimized CRC32C code defaulted to off and even needed to be explicitly added to the ramdisk to be used. Now that it just works in Linux 6.14 and later, there's no need for users to take any action and the driver name portion of this is basically obsolete. (Also, note that the crc32c driver name already changed in 6.14.)
Quoted 1 line
How about instead removing that part since it's useless now?
There's no best answer, removing it makes sense but could break somebody's scripts parsing that file. The information put to "(...)" might be useful in the future again. It's less harm to leave it there than to deal with potential fallout if it's removed.
Quoted 17 lines · Eric Biggers
This series simplifies how the CRC32 library functions are exposed through the
crypto_shashAPI. We'll now have just one shash algorithm each for "crc32" and "crc32c", and their driver names will just always be "crc32-lib" and "crc32c-lib" respectively. This seems to be all that's actually needed.As mentioned in patch 2, this does change the content of
/sys/fs/btrfs/$uuid/checksumagain, but that should be fine.This is based on v6.16-rc1, and I'm planning to take these patches through the crc-next tree. These supersede
https://lore.kernel.org/r/[email protected]/
and
https://lore.kernel.org/r/[email protected]/,
and they fix the warning in the full crypto self-tests reported at
https://lore.kernel.org/r/[email protected]/
Quoted 3 lines
Eric Biggers (2):
btrfs: stop parsing crc32c driver name
crypto/crc32[c]: register only "-lib" drivers
FYI, applied to
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/log/?h=crc-next
as per the plan above. An ack from the btrfs folks on both patches would be appreciated, though.
- Eric
Quoted 8 lines · Eric Biggers
This series simplifies how the CRC32 library functions are exposed through the
crypto_shashAPI. We'll now have just one shash algorithm each for "crc32" and "crc32c", and their driver names will just always be "crc32-lib" and "crc32c-lib" respectively. This seems to be all that's actually needed.As mentioned in patch 2, this does change the content of
/sys/fs/btrfs/$uuid/checksumagain, but that should be fine.
Yes, this is fine, I don't think any ABI applies here and the implementation was only informative.
Quoted 11 lines
This is based on v6.16-rc1, and I'm planning to take these patches through the crc-next tree. These supersede
https://lore.kernel.org/r/[email protected]/
and
https://lore.kernel.org/r/[email protected]/,
and they fix the warning in the full crypto self-tests reported at
https://lore.kernel.org/r/[email protected]/Eric Biggers (2):
btrfs: stop parsing crc32c driver name
crypto/crc32[c]: register only "-lib" drivers
Thanks.