| Message ID | 20211222143628.618436-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,kbuild] lib/crypto: blake2s: include as built-in | expand |
On 12/22/21 06:36, Jason A. Donenfeld wrote: > In preparation for using blake2s in the RNG, we change the way that it > is wired-in to the build system. Instead of kconfig mazes and ifdefs, we > use weak symbols, so that an arch version can override the generic > version. Then we include the generic version in lib-y, so that it can be > removed from the image if the arch version doesn't fallback to it (as is > the case on arm though not x86). The result should be a bit simpler and > smaller than the code it replaces. > > Discussed-with: Ard Biesheuvel <ardb@kernel.org> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > Hi Masahiro, > > This is a commit I'm working on as part of a change to the RNG, which > got broken out into something standalone because it wound up being a bit > more involved than originally conceived. With Ard's help, this is using > weak symbols with the generic code in lib-y, but the only way we figured > to actually include the lib.a file was via this unseemly > KBUILD_VMLINUX_LIBS line below, which doesn't seem very okay. I tried > adding `libs-y += lib/crypto/` to lib/Makefile, but it seemed like the > build system ignored it. I figure there's probably a right way to do > this, so thought I should send you this RFC to look at first. If lib-y ignores (drops) an object file (usually because it is not used), the usual way to force it to be included is to add it to obj-y instead of lib-y (see many examples in lib/Makefile). However, this may cause problems with weak symbols. I don't recall it being used in that scenario. > Alternatively, if the KBUILD_VMLINUX_LIBS trick is somehow okay, that'd > be useful to know too. > > Thanks, > Jason
On Wed, Dec 22, 2021 at 4:49 PM Randy Dunlap <rdunlap@infradead.org> wrote: > If lib-y ignores (drops) an object file (usually because it is not used), > the usual way to force it to be included is to add it to obj-y instead > of lib-y (see many examples in lib/Makefile). This is not a problem with lib-y. This is a problem with libs-y. Note the 's'. The former is working as expected. The latter controls whether a directory's lib.a is picked up. For whatever reason, the build system isn't respecting a libs-y declaration added to lib/Makefile like it respects for one added to arch/*/Makefile. > However, this may cause problems with weak symbols. I don't recall it being > used in that scenario. The reason we're using lib-y is so that unused code is pruned in the case that the weak symbol isn't used. IOW, a usual use for lib-y. And it works just fine. As mentioned, the issue is just with libs-y not finding that lib.a file. Jason
Hey Masahiro, On Wed, Dec 22, 2021 at 3:36 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Hi Masahiro, > > This is a commit I'm working on as part of a change to the RNG, which > got broken out into something standalone because it wound up being a bit > more involved than originally conceived. With Ard's help, this is using > weak symbols with the generic code in lib-y, but the only way we figured > to actually include the lib.a file was via this unseemly > KBUILD_VMLINUX_LIBS line below, which doesn't seem very okay. I tried > adding `libs-y += lib/crypto/` to lib/Makefile, but it seemed like the > build system ignored it. I figure there's probably a right way to do > this, so thought I should send you this RFC to look at first. > Alternatively, if the KBUILD_VMLINUX_LIBS trick is somehow okay, that'd > be useful to know too. > > Thanks, > Jason I found something else that works. Here's the original (bad) idea: > diff --git a/Makefile b/Makefile > index d85f1ff79f5c..4b4a2bec020c 100644 > --- a/Makefile > +++ b/Makefile > @@ -1120,6 +1120,7 @@ else > KBUILD_VMLINUX_LIBS := $(patsubst %/,%/lib.a, $(libs-y)) > endif > KBUILD_VMLINUX_OBJS += $(patsubst %/,%/built-in.a, $(drivers-y)) > +KBUILD_VMLINUX_LIBS += lib/crypto/lib.a > > export KBUILD_VMLINUX_OBJS KBUILD_VMLINUX_LIBS > export KBUILD_LDS := arch/$(SRCARCH)/kernel/vmlinux.lds Here's a new idea: diff --git a/Makefile b/Makefile index d85f1ff79f5c..235a3b712d7b 100644 --- a/Makefile +++ b/Makefile @@ -700,6 +700,7 @@ export RETPOLINE_CFLAGS export RETPOLINE_VDSO_CFLAGS include $(srctree)/arch/$(SRCARCH)/Makefile +libs-y += lib/crypto/ ifdef need-config ifdef may-sync-config diff --git a/lib/Makefile b/lib/Makefile index 364c23f15578..bb57b2e466fa 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -139,7 +139,7 @@ endif obj-$(CONFIG_DEBUG_INFO_REDUCED) += debug_info.o CFLAGS_debug_info.o += $(call cc-option, -femit-struct-debug-detailed=any) -obj-y += math/ crypto/ +obj-y += math/ obj-$(CONFIG_GENERIC_IOMAP) += iomap.o obj-$(CONFIG_GENERIC_PCI_IOMAP) += pci_iomap.o Perhaps there's yet a third way that you see. Thanks, Jason
On Thu, Dec 23, 2021 at 12:55 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Wed, Dec 22, 2021 at 4:49 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > If lib-y ignores (drops) an object file (usually because it is not used), > > the usual way to force it to be included is to add it to obj-y instead > > of lib-y (see many examples in lib/Makefile). > > This is not a problem with lib-y. This is a problem with libs-y. Note > the 's'. The former is working as expected. The latter controls > whether a directory's lib.a is picked up. For whatever reason, the > build system isn't respecting a libs-y declaration added to > lib/Makefile like it respects for one added to arch/*/Makefile. > > > However, this may cause problems with weak symbols. I don't recall it being > > used in that scenario. > > The reason we're using lib-y is so that unused code is pruned in the > case that the weak symbol isn't used. IOW, a usual use for lib-y. And > it works just fine. As mentioned, the issue is just with libs-y not > finding that lib.a file. lib-y does not work like that. See commit 7273ad2b08f8ac9563579d16a3cf528857b26f49 When CONFIG_MODULES=y (this is true for most of usecases), there is not much difference between obj-y and lib-y. So, weak functions will remain in vmlinux even if they are unused.
diff --git a/Makefile b/Makefile index d85f1ff79f5c..4b4a2bec020c 100644 --- a/Makefile +++ b/Makefile @@ -1120,6 +1120,7 @@ else KBUILD_VMLINUX_LIBS := $(patsubst %/,%/lib.a, $(libs-y)) endif KBUILD_VMLINUX_OBJS += $(patsubst %/,%/built-in.a, $(drivers-y)) +KBUILD_VMLINUX_LIBS += lib/crypto/lib.a export KBUILD_VMLINUX_OBJS KBUILD_VMLINUX_LIBS export KBUILD_LDS := arch/$(SRCARCH)/kernel/vmlinux.lds diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig index 2b575792363e..47cb22645746 100644 --- a/arch/arm/crypto/Kconfig +++ b/arch/arm/crypto/Kconfig @@ -63,8 +63,7 @@ config CRYPTO_SHA512_ARM using optimized ARM assembler and NEON, when available. config CRYPTO_BLAKE2S_ARM - tristate "BLAKE2s digest algorithm (ARM)" - select CRYPTO_ARCH_HAVE_LIB_BLAKE2S + bool "BLAKE2s digest algorithm (ARM)" help BLAKE2s digest algorithm optimized with ARM scalar instructions. This is faster than the generic implementations of BLAKE2s and BLAKE2b, but diff --git a/arch/arm/crypto/blake2s-core.S b/arch/arm/crypto/blake2s-core.S index 86345751bbf3..df40e46601f1 100644 --- a/arch/arm/crypto/blake2s-core.S +++ b/arch/arm/crypto/blake2s-core.S @@ -167,8 +167,8 @@ .endm // -// void blake2s_compress_arch(struct blake2s_state *state, -// const u8 *block, size_t nblocks, u32 inc); +// void blake2s_compress(struct blake2s_state *state, +// const u8 *block, size_t nblocks, u32 inc); // // Only the first three fields of struct blake2s_state are used: // u32 h[8]; (inout) @@ -176,7 +176,7 @@ // u32 f[2]; (in) // .align 5 -ENTRY(blake2s_compress_arch) +ENTRY(blake2s_compress) push {r0-r2,r4-r11,lr} // keep this an even number .Lnext_block: @@ -303,4 +303,4 @@ ENTRY(blake2s_compress_arch) str r3, [r12], #4 bne 1b b .Lcopy_block_done -ENDPROC(blake2s_compress_arch) +ENDPROC(blake2s_compress) diff --git a/arch/arm/crypto/blake2s-glue.c b/arch/arm/crypto/blake2s-glue.c index f2cc1e5fc9ec..09d3a0cabd2c 100644 --- a/arch/arm/crypto/blake2s-glue.c +++ b/arch/arm/crypto/blake2s-glue.c @@ -11,17 +11,17 @@ #include <linux/module.h> /* defined in blake2s-core.S */ -EXPORT_SYMBOL(blake2s_compress_arch); +EXPORT_SYMBOL(blake2s_compress); static int crypto_blake2s_update_arm(struct shash_desc *desc, const u8 *in, unsigned int inlen) { - return crypto_blake2s_update(desc, in, inlen, blake2s_compress_arch); + return crypto_blake2s_update(desc, in, inlen, blake2s_compress); } static int crypto_blake2s_final_arm(struct shash_desc *desc, u8 *out) { - return crypto_blake2s_final(desc, out, blake2s_compress_arch); + return crypto_blake2s_final(desc, out, blake2s_compress); } #define BLAKE2S_ALG(name, driver_name, digest_size) \ diff --git a/arch/s390/configs/debug_defconfig b/arch/s390/configs/debug_defconfig index e45cc27716de..caa3d1d6a0e8 100644 --- a/arch/s390/configs/debug_defconfig +++ b/arch/s390/configs/debug_defconfig @@ -757,7 +757,6 @@ CONFIG_CRYPTO_USER_API_SKCIPHER=m CONFIG_CRYPTO_USER_API_RNG=m CONFIG_CRYPTO_USER_API_AEAD=m CONFIG_CRYPTO_STATS=y -CONFIG_CRYPTO_LIB_BLAKE2S=m CONFIG_CRYPTO_LIB_CURVE25519=m CONFIG_CRYPTO_LIB_CHACHA20POLY1305=m CONFIG_ZCRYPT=m diff --git a/arch/s390/configs/defconfig b/arch/s390/configs/defconfig index 1c750bfca2d8..fffc6af5358c 100644 --- a/arch/s390/configs/defconfig +++ b/arch/s390/configs/defconfig @@ -744,7 +744,6 @@ CONFIG_CRYPTO_USER_API_SKCIPHER=m CONFIG_CRYPTO_USER_API_RNG=m CONFIG_CRYPTO_USER_API_AEAD=m CONFIG_CRYPTO_STATS=y -CONFIG_CRYPTO_LIB_BLAKE2S=m CONFIG_CRYPTO_LIB_CURVE25519=m CONFIG_CRYPTO_LIB_CHACHA20POLY1305=m CONFIG_ZCRYPT=m diff --git a/arch/x86/crypto/blake2s-glue.c b/arch/x86/crypto/blake2s-glue.c index a40365ab301e..ef91a3167d27 100644 --- a/arch/x86/crypto/blake2s-glue.c +++ b/arch/x86/crypto/blake2s-glue.c @@ -28,9 +28,8 @@ asmlinkage void blake2s_compress_avx512(struct blake2s_state *state, static __ro_after_init DEFINE_STATIC_KEY_FALSE(blake2s_use_ssse3); static __ro_after_init DEFINE_STATIC_KEY_FALSE(blake2s_use_avx512); -void blake2s_compress_arch(struct blake2s_state *state, - const u8 *block, size_t nblocks, - const u32 inc) +void blake2s_compress(struct blake2s_state *state, const u8 *block, + size_t nblocks, const u32 inc) { /* SIMD disables preemption, so relax after processing each page. */ BUILD_BUG_ON(SZ_4K / BLAKE2S_BLOCK_SIZE < 8); @@ -56,17 +55,17 @@ void blake2s_compress_arch(struct blake2s_state *state, block += blocks * BLAKE2S_BLOCK_SIZE; } while (nblocks); } -EXPORT_SYMBOL(blake2s_compress_arch); +EXPORT_SYMBOL(blake2s_compress); static int crypto_blake2s_update_x86(struct shash_desc *desc, const u8 *in, unsigned int inlen) { - return crypto_blake2s_update(desc, in, inlen, blake2s_compress_arch); + return crypto_blake2s_update(desc, in, inlen, blake2s_compress); } static int crypto_blake2s_final_x86(struct shash_desc *desc, u8 *out) { - return crypto_blake2s_final(desc, out, blake2s_compress_arch); + return crypto_blake2s_final(desc, out, blake2s_compress); } #define BLAKE2S_ALG(name, driver_name, digest_size) \ diff --git a/crypto/Kconfig b/crypto/Kconfig index 285f82647d2b..bfda2c82774d 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -685,7 +685,6 @@ config CRYPTO_BLAKE2B config CRYPTO_BLAKE2S tristate "BLAKE2s digest algorithm" - select CRYPTO_LIB_BLAKE2S_GENERIC select CRYPTO_HASH help Implementation of cryptographic hash function BLAKE2s @@ -702,10 +701,8 @@ config CRYPTO_BLAKE2S See https://blake2.net for further information. config CRYPTO_BLAKE2S_X86 - tristate "BLAKE2s digest algorithm (x86 accelerated version)" + bool "BLAKE2s digest algorithm (x86 accelerated version)" depends on X86 && 64BIT - select CRYPTO_LIB_BLAKE2S_GENERIC - select CRYPTO_ARCH_HAVE_LIB_BLAKE2S config CRYPTO_CRCT10DIF tristate "CRCT10DIF algorithm" diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 6cccc3dc00bc..b2a4f998c180 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -81,7 +81,6 @@ config WIREGUARD select CRYPTO select CRYPTO_LIB_CURVE25519 select CRYPTO_LIB_CHACHA20POLY1305 - select CRYPTO_LIB_BLAKE2S select CRYPTO_CHACHA20_X86_64 if X86 && 64BIT select CRYPTO_POLY1305_X86_64 if X86 && 64BIT select CRYPTO_BLAKE2S_X86 if X86 && 64BIT diff --git a/include/crypto/internal/blake2s.h b/include/crypto/internal/blake2s.h index 8e50d487500f..d39cfa0d333e 100644 --- a/include/crypto/internal/blake2s.h +++ b/include/crypto/internal/blake2s.h @@ -11,11 +11,11 @@ #include <crypto/internal/hash.h> #include <linux/string.h> -void blake2s_compress_generic(struct blake2s_state *state,const u8 *block, +void blake2s_compress_generic(struct blake2s_state *state, const u8 *block, size_t nblocks, const u32 inc); -void blake2s_compress_arch(struct blake2s_state *state,const u8 *block, - size_t nblocks, const u32 inc); +void blake2s_compress(struct blake2s_state *state, const u8 *block, + size_t nblocks, const u32 inc); bool blake2s_selftest(void); diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig index 545ccbddf6a1..31c6e2be3b84 100644 --- a/lib/crypto/Kconfig +++ b/lib/crypto/Kconfig @@ -8,31 +8,6 @@ config CRYPTO_LIB_AES config CRYPTO_LIB_ARC4 tristate -config CRYPTO_ARCH_HAVE_LIB_BLAKE2S - tristate - help - Declares whether the architecture provides an arch-specific - accelerated implementation of the Blake2s library interface, - either builtin or as a module. - -config CRYPTO_LIB_BLAKE2S_GENERIC - tristate - help - This symbol can be depended upon by arch implementations of the - Blake2s library interface that require the generic code as a - fallback, e.g., for SIMD implementations. If no arch specific - implementation is enabled, this implementation serves the users - of CRYPTO_LIB_BLAKE2S. - -config CRYPTO_LIB_BLAKE2S - tristate "BLAKE2s hash function library" - depends on CRYPTO_ARCH_HAVE_LIB_BLAKE2S || !CRYPTO_ARCH_HAVE_LIB_BLAKE2S - select CRYPTO_LIB_BLAKE2S_GENERIC if CRYPTO_ARCH_HAVE_LIB_BLAKE2S=n - help - Enable the Blake2s library interface. This interface may be fulfilled - by either the generic implementation or an arch-specific one, if one - is available and enabled. - config CRYPTO_ARCH_HAVE_LIB_CHACHA tristate help diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile index 73205ed269ba..42e1d932c077 100644 --- a/lib/crypto/Makefile +++ b/lib/crypto/Makefile @@ -10,10 +10,9 @@ libaes-y := aes.o obj-$(CONFIG_CRYPTO_LIB_ARC4) += libarc4.o libarc4-y := arc4.o -obj-$(CONFIG_CRYPTO_LIB_BLAKE2S_GENERIC) += libblake2s-generic.o -libblake2s-generic-y += blake2s-generic.o - -obj-$(CONFIG_CRYPTO_LIB_BLAKE2S) += libblake2s.o +# blake2s is used by the /dev/random driver which is always builtin +lib-y += blake2s-generic.o +obj-y += libblake2s.o libblake2s-y += blake2s.o obj-$(CONFIG_CRYPTO_LIB_CHACHA20POLY1305) += libchacha20poly1305.o diff --git a/lib/crypto/blake2s-generic.c b/lib/crypto/blake2s-generic.c index 04ff8df24513..75ccb3e633e6 100644 --- a/lib/crypto/blake2s-generic.c +++ b/lib/crypto/blake2s-generic.c @@ -37,7 +37,11 @@ static inline void blake2s_increment_counter(struct blake2s_state *state, state->t[1] += (state->t[0] < inc); } -void blake2s_compress_generic(struct blake2s_state *state,const u8 *block, +void blake2s_compress(struct blake2s_state *state, const u8 *block, + size_t nblocks, const u32 inc) + __weak __alias(blake2s_compress_generic); + +void blake2s_compress_generic(struct blake2s_state *state, const u8 *block, size_t nblocks, const u32 inc) { u32 m[16]; diff --git a/lib/crypto/blake2s.c b/lib/crypto/blake2s.c index 4055aa593ec4..93f2ae051370 100644 --- a/lib/crypto/blake2s.c +++ b/lib/crypto/blake2s.c @@ -16,12 +16,6 @@ #include <linux/init.h> #include <linux/bug.h> -#if IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_BLAKE2S) -# define blake2s_compress blake2s_compress_arch -#else -# define blake2s_compress blake2s_compress_generic -#endif - void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen) { __blake2s_update(state, in, inlen, blake2s_compress);
In preparation for using blake2s in the RNG, we change the way that it is wired-in to the build system. Instead of kconfig mazes and ifdefs, we use weak symbols, so that an arch version can override the generic version. Then we include the generic version in lib-y, so that it can be removed from the image if the arch version doesn't fallback to it (as is the case on arm though not x86). The result should be a bit simpler and smaller than the code it replaces. Discussed-with: Ard Biesheuvel <ardb@kernel.org> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- Hi Masahiro, This is a commit I'm working on as part of a change to the RNG, which got broken out into something standalone because it wound up being a bit more involved than originally conceived. With Ard's help, this is using weak symbols with the generic code in lib-y, but the only way we figured to actually include the lib.a file was via this unseemly KBUILD_VMLINUX_LIBS line below, which doesn't seem very okay. I tried adding `libs-y += lib/crypto/` to lib/Makefile, but it seemed like the build system ignored it. I figure there's probably a right way to do this, so thought I should send you this RFC to look at first. Alternatively, if the KBUILD_VMLINUX_LIBS trick is somehow okay, that'd be useful to know too. Thanks, Jason Makefile | 1 + arch/arm/crypto/Kconfig | 3 +-- arch/arm/crypto/blake2s-core.S | 8 ++++---- arch/arm/crypto/blake2s-glue.c | 6 +++--- arch/s390/configs/debug_defconfig | 1 - arch/s390/configs/defconfig | 1 - arch/x86/crypto/blake2s-glue.c | 11 +++++------ crypto/Kconfig | 5 +---- drivers/net/Kconfig | 1 - include/crypto/internal/blake2s.h | 6 +++--- lib/crypto/Kconfig | 25 ------------------------- lib/crypto/Makefile | 7 +++---- lib/crypto/blake2s-generic.c | 6 +++++- lib/crypto/blake2s.c | 6 ------ 14 files changed, 26 insertions(+), 61 deletions(-)