| Message ID | 20190816211611.2568-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: sha256 - Merge 2 separate C implementations into 1, put into separate library | expand |
On Fri, Aug 16, 2019 at 11:16:08PM +0200, Hans de Goede wrote: > diff --git a/include/linux/sha256.h b/include/crypto/sha256.h > similarity index 100% > rename from include/linux/sha256.h > rename to include/crypto/sha256.h <crypto/sha.h> already has the declarations for both SHA-1 and SHA-2, including SHA-256. So I'm not sure a separate sha256.h is appropriate. How about putting these declarations in <crypto/sha.h>? - Eric
Hi, On 17-08-19 07:19, Eric Biggers wrote: > On Fri, Aug 16, 2019 at 11:16:08PM +0200, Hans de Goede wrote: >> diff --git a/include/linux/sha256.h b/include/crypto/sha256.h >> similarity index 100% >> rename from include/linux/sha256.h >> rename to include/crypto/sha256.h > > <crypto/sha.h> already has the declarations for both SHA-1 and SHA-2, including > SHA-256. So I'm not sure a separate sha256.h is appropriate. How about putting > these declarations in <crypto/sha.h>? The problems with that is that the sha256_init, etc. names are quite generic and they have not been reserved before, so a lot of the crypto hw-accel drivers use them, for private file-local (static) code, e.g.: [hans@shalem linux]$ ack -l sha256_init include/crypto/sha256.h drivers/crypto/marvell/hash.c drivers/crypto/ccp/ccp-ops.c drivers/crypto/nx/nx-sha256.c drivers/crypto/ux500/hash/hash_core.c drivers/crypto/inside-secure/safexcel_hash.c drivers/crypto/chelsio/chcr_algo.h drivers/crypto/stm32/stm32-hash.c drivers/crypto/omap-sham.c drivers/crypto/padlock-sha.c drivers/crypto/n2_core.c drivers/crypto/atmel-aes.c drivers/crypto/axis/artpec6_crypto.c drivers/crypto/mediatek/mtk-sha.c drivers/crypto/qat/qat_common/qat_algs.c drivers/crypto/img-hash.c drivers/crypto/ccree/cc_hash.c lib/crypto/sha256.c arch/powerpc/crypto/sha256-spe-glue.c arch/mips/cavium-octeon/crypto/octeon-sha256.c arch/x86/purgatory/purgatory.c arch/s390/crypto/sha256_s390.c arch/s390/purgatory/purgatory.c (in case you do not know ack is a smarter grep, which skips .o files, etc.) All these do include crypto/sha.h and putting the stuff which is in what was linux/sha256.h into crypto/sha.h leads to name collisions which causes more churn then I would like this series to cause. I guess we could do a cleanup afterwards, with one patch per file above to fix the name collision issue, and then merge the 2 headers. I do not want to do that for this series, as I want to keep this series as KISS as possible since it is messing with somewhat sensitive stuff. And TBH I even wonder if a follow-up series is worth the churn... Regards, Hans
On Sat, Aug 17, 2019 at 10:28:04AM +0200, Hans de Goede wrote: > Hi, > > On 17-08-19 07:19, Eric Biggers wrote: > > On Fri, Aug 16, 2019 at 11:16:08PM +0200, Hans de Goede wrote: > > > diff --git a/include/linux/sha256.h b/include/crypto/sha256.h > > > similarity index 100% > > > rename from include/linux/sha256.h > > > rename to include/crypto/sha256.h > > > > <crypto/sha.h> already has the declarations for both SHA-1 and SHA-2, including > > SHA-256. So I'm not sure a separate sha256.h is appropriate. How about putting > > these declarations in <crypto/sha.h>? > > The problems with that is that the sha256_init, etc. names are quite generic > and they have not been reserved before, so a lot of the crypto hw-accel > drivers use them, for private file-local (static) code, e.g.: > > [hans@shalem linux]$ ack -l sha256_init > include/crypto/sha256.h > drivers/crypto/marvell/hash.c > drivers/crypto/ccp/ccp-ops.c > drivers/crypto/nx/nx-sha256.c > drivers/crypto/ux500/hash/hash_core.c > drivers/crypto/inside-secure/safexcel_hash.c > drivers/crypto/chelsio/chcr_algo.h > drivers/crypto/stm32/stm32-hash.c > drivers/crypto/omap-sham.c > drivers/crypto/padlock-sha.c > drivers/crypto/n2_core.c > drivers/crypto/atmel-aes.c > drivers/crypto/axis/artpec6_crypto.c > drivers/crypto/mediatek/mtk-sha.c > drivers/crypto/qat/qat_common/qat_algs.c > drivers/crypto/img-hash.c > drivers/crypto/ccree/cc_hash.c > lib/crypto/sha256.c > arch/powerpc/crypto/sha256-spe-glue.c > arch/mips/cavium-octeon/crypto/octeon-sha256.c > arch/x86/purgatory/purgatory.c > arch/s390/crypto/sha256_s390.c > arch/s390/purgatory/purgatory.c > > (in case you do not know ack is a smarter grep, which skips .o files, etc.) You need to match at word boundaries to avoid matching on ${foo}_sha256_init(). So it's actually a somewhat shorter list: $ git grep -l -E '\<sha(224|256)_(init|update|final)\>' arch/arm/crypto/sha256_glue.c arch/arm/crypto/sha256_neon_glue.c arch/arm64/crypto/sha256-glue.c arch/s390/crypto/sha256_s390.c arch/s390/purgatory/purgatory.c arch/x86/crypto/sha256_ssse3_glue.c arch/x86/purgatory/purgatory.c crypto/sha256_generic.c drivers/crypto/ccree/cc_hash.c drivers/crypto/chelsio/chcr_algo.h drivers/crypto/n2_core.c include/linux/sha256.h lib/sha256.c 5 of these are already edited by this patchset, so that leaves only 8 files. > > All these do include crypto/sha.h and putting the stuff which is in what > was linux/sha256.h into crypto/sha.h leads to name collisions which causes > more churn then I would like this series to cause. > > I guess we could do a cleanup afterwards, with one patch per file above > to fix the name collision issue, and then merge the 2 headers. I do not > want to do that for this series, as I want to keep this series as KISS > as possible since it is messing with somewhat sensitive stuff. > > And TBH I even wonder if a follow-up series is worth the churn... > I think it should be done; the same was done when introducing the AES library. But I'm okay with it being done later, if you want to keep this patchset shorter. - Eric
Hi, On 18-08-19 17:54, Eric Biggers wrote: > On Sat, Aug 17, 2019 at 10:28:04AM +0200, Hans de Goede wrote: >> Hi, >> >> On 17-08-19 07:19, Eric Biggers wrote: >>> On Fri, Aug 16, 2019 at 11:16:08PM +0200, Hans de Goede wrote: >>>> diff --git a/include/linux/sha256.h b/include/crypto/sha256.h >>>> similarity index 100% >>>> rename from include/linux/sha256.h >>>> rename to include/crypto/sha256.h >>> >>> <crypto/sha.h> already has the declarations for both SHA-1 and SHA-2, including >>> SHA-256. So I'm not sure a separate sha256.h is appropriate. How about putting >>> these declarations in <crypto/sha.h>? >> >> The problems with that is that the sha256_init, etc. names are quite generic >> and they have not been reserved before, so a lot of the crypto hw-accel >> drivers use them, for private file-local (static) code, e.g.: >> >> [hans@shalem linux]$ ack -l sha256_init >> include/crypto/sha256.h >> drivers/crypto/marvell/hash.c >> drivers/crypto/ccp/ccp-ops.c >> drivers/crypto/nx/nx-sha256.c >> drivers/crypto/ux500/hash/hash_core.c >> drivers/crypto/inside-secure/safexcel_hash.c >> drivers/crypto/chelsio/chcr_algo.h >> drivers/crypto/stm32/stm32-hash.c >> drivers/crypto/omap-sham.c >> drivers/crypto/padlock-sha.c >> drivers/crypto/n2_core.c >> drivers/crypto/atmel-aes.c >> drivers/crypto/axis/artpec6_crypto.c >> drivers/crypto/mediatek/mtk-sha.c >> drivers/crypto/qat/qat_common/qat_algs.c >> drivers/crypto/img-hash.c >> drivers/crypto/ccree/cc_hash.c >> lib/crypto/sha256.c >> arch/powerpc/crypto/sha256-spe-glue.c >> arch/mips/cavium-octeon/crypto/octeon-sha256.c >> arch/x86/purgatory/purgatory.c >> arch/s390/crypto/sha256_s390.c >> arch/s390/purgatory/purgatory.c >> >> (in case you do not know ack is a smarter grep, which skips .o files, etc.) > > You need to match at word boundaries to avoid matching on ${foo}_sha256_init(). > So it's actually a somewhat shorter list: > > $ git grep -l -E '\<sha(224|256)_(init|update|final)\>' > arch/arm/crypto/sha256_glue.c > arch/arm/crypto/sha256_neon_glue.c > arch/arm64/crypto/sha256-glue.c > arch/s390/crypto/sha256_s390.c > arch/s390/purgatory/purgatory.c > arch/x86/crypto/sha256_ssse3_glue.c > arch/x86/purgatory/purgatory.c > crypto/sha256_generic.c > drivers/crypto/ccree/cc_hash.c > drivers/crypto/chelsio/chcr_algo.h > drivers/crypto/n2_core.c > include/linux/sha256.h > lib/sha256.c > > 5 of these are already edited by this patchset, so that leaves only 8 files. Good point. >> All these do include crypto/sha.h and putting the stuff which is in what >> was linux/sha256.h into crypto/sha.h leads to name collisions which causes >> more churn then I would like this series to cause. >> >> I guess we could do a cleanup afterwards, with one patch per file above >> to fix the name collision issue, and then merge the 2 headers. I do not >> want to do that for this series, as I want to keep this series as KISS >> as possible since it is messing with somewhat sensitive stuff. >> >> And TBH I even wonder if a follow-up series is worth the churn... >> > > I think it should be done; the same was done when introducing the AES library. > But I'm okay with it being done later, if you want to keep this patchset > shorter. I would prefer to do this later, so that we can focus on the basis of merging the 2 implementations now. I'm willing to commit to doing the cleanup once the base series has been merged. Regards, Hans
diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile index dc1ae4ff79d7..85b05c9e40f5 100644 --- a/arch/s390/purgatory/Makefile +++ b/arch/s390/purgatory/Makefile @@ -7,7 +7,7 @@ purgatory-y := head.o purgatory.o string.o sha256.o mem.o targets += $(purgatory-y) purgatory.lds purgatory purgatory.ro PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y)) -$(obj)/sha256.o: $(srctree)/lib/sha256.c FORCE +$(obj)/sha256.o: $(srctree)/lib/crypto/sha256.c FORCE $(call if_changed_rule,cc_o_c) $(obj)/mem.o: $(srctree)/arch/s390/lib/mem.S FORCE diff --git a/arch/s390/purgatory/purgatory.c b/arch/s390/purgatory/purgatory.c index 3528e6da4e87..a80c78da9985 100644 --- a/arch/s390/purgatory/purgatory.c +++ b/arch/s390/purgatory/purgatory.c @@ -8,8 +8,8 @@ */ #include <linux/kexec.h> -#include <linux/sha256.h> #include <linux/string.h> +#include <crypto/sha256.h> #include <asm/purgatory.h> int verify_sha256_digest(void) diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 8901a1f89cf5..6ebd0739106e 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -9,7 +9,7 @@ PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y)) $(obj)/string.o: $(srctree)/arch/x86/boot/compressed/string.c FORCE $(call if_changed_rule,cc_o_c) -$(obj)/sha256.o: $(srctree)/lib/sha256.c FORCE +$(obj)/sha256.o: $(srctree)/lib/crypto/sha256.c FORCE $(call if_changed_rule,cc_o_c) LDFLAGS_purgatory.ro := -e purgatory_start -r --no-undefined -nostdlib -z nodefaultlib diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c index b607bda786f6..7f90a86eff49 100644 --- a/arch/x86/purgatory/purgatory.c +++ b/arch/x86/purgatory/purgatory.c @@ -9,7 +9,7 @@ */ #include <linux/bug.h> -#include <linux/sha256.h> +#include <crypto/sha256.h> #include <asm/purgatory.h> #include "../boot/string.h" diff --git a/include/linux/sha256.h b/include/crypto/sha256.h similarity index 100% rename from include/linux/sha256.h rename to include/crypto/sha256.h diff --git a/lib/sha256.c b/lib/crypto/sha256.c similarity index 99% rename from lib/sha256.c rename to lib/crypto/sha256.c index ba4dce0b3711..b8114028d06f 100644 --- a/lib/sha256.c +++ b/lib/crypto/sha256.c @@ -12,8 +12,8 @@ */ #include <linux/bitops.h> -#include <linux/sha256.h> #include <linux/string.h> +#include <crypto/sha256.h> #include <asm/byteorder.h> static inline u32 Ch(u32 x, u32 y, u32 z)
Generic crypto implementations belong under lib/crypto not directly in lib, likewise the header should be in include/crypto, not include/linux. Note that the code in lib/crypto/sha256.c is not yet available for generic use after this commit, it is still only used by the s390 and x86 purgatory code. Making it suitable for generic use is done in further patches in this series. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/s390/purgatory/Makefile | 2 +- arch/s390/purgatory/purgatory.c | 2 +- arch/x86/purgatory/Makefile | 2 +- arch/x86/purgatory/purgatory.c | 2 +- include/{linux => crypto}/sha256.h | 0 lib/{ => crypto}/sha256.c | 2 +- 6 files changed, 5 insertions(+), 5 deletions(-) rename include/{linux => crypto}/sha256.h (100%) rename lib/{ => crypto}/sha256.c (99%)