| Message ID | 20210108171706.10306-1-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] crypto: reduce minimum alignment of on-stack structures | expand |
On Fri, Jan 08, 2021 at 11:49:32PM +0100, Ard Biesheuvel wrote: > > The assumption is that ARCH_SLAB_MINALIGN should be sufficient for any > POD type, But I guess that in order to be fully correct, the actual > alignment of the struct type should be ARCH_SLAB_MINALIGN, and __ctx > should just be padded out so it appears at an offset that is a > multiple of ARCH_KMALLOC_ALIGN. How about just leaving the skcpiher alone for now and change shash only? Thanks, -- 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
On Wed, 13 Jan 2021 at 07:27, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Fri, Jan 08, 2021 at 11:49:32PM +0100, Ard Biesheuvel wrote: > > > > The assumption is that ARCH_SLAB_MINALIGN should be sufficient for any > > POD type, But I guess that in order to be fully correct, the actual > > alignment of the struct type should be ARCH_SLAB_MINALIGN, and __ctx > > should just be padded out so it appears at an offset that is a > > multiple of ARCH_KMALLOC_ALIGN. > > How about just leaving the skcpiher alone for now and change shash > only? > Good point. I'll spin a v3 with only the shash_desc change and the updated comment for CRYPTO_MINALIGN.
diff --git a/include/crypto/hash.h b/include/crypto/hash.h index af2ff31ff619..13f8a6a54ca8 100644 --- a/include/crypto/hash.h +++ b/include/crypto/hash.h @@ -149,7 +149,7 @@ struct ahash_alg { struct shash_desc { struct crypto_shash *tfm; - void *__ctx[] CRYPTO_MINALIGN_ATTR; + void *__ctx[] __aligned(ARCH_SLAB_MINALIGN); }; #define HASH_MAX_DIGESTSIZE 64 @@ -162,9 +162,9 @@ struct shash_desc { #define HASH_MAX_STATESIZE 512 -#define SHASH_DESC_ON_STACK(shash, ctx) \ - char __##shash##_desc[sizeof(struct shash_desc) + \ - HASH_MAX_DESCSIZE] CRYPTO_MINALIGN_ATTR; \ +#define SHASH_DESC_ON_STACK(shash, ctx) \ + char __##shash##_desc[sizeof(struct shash_desc) + HASH_MAX_DESCSIZE] \ + __aligned(__alignof__(struct shash_desc)); \ struct shash_desc *shash = (struct shash_desc *)__##shash##_desc /** diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h index 6a733b171a5d..aa133dc3bf39 100644 --- a/include/crypto/skcipher.h +++ b/include/crypto/skcipher.h @@ -128,7 +128,7 @@ struct skcipher_alg { MAX_SYNC_SKCIPHER_REQSIZE + \ (!(sizeof((struct crypto_sync_skcipher *)1 == \ (typeof(tfm))1))) \ - ] CRYPTO_MINALIGN_ATTR; \ + ] __aligned(ARCH_SLAB_MINALIGN); \ struct skcipher_request *name = (void *)__##name##_desc /** diff --git a/include/linux/crypto.h b/include/linux/crypto.h index 9b55cd6b1f1b..aec33c95f9c3 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -151,9 +151,12 @@ * The macro CRYPTO_MINALIGN_ATTR (along with the void * type in the actual * declaration) is used to ensure that the crypto_tfm context structure is * aligned correctly for the given architecture so that there are no alignment - * faults for C data types. In particular, this is required on platforms such - * as arm where pointers are 32-bit aligned but there are data types such as - * u64 which require 64-bit alignment. + * faults for C data types. On architectures that support non-cache coherent + * DMA, such as ARM or arm64, it also takes into account the minimal alignment + * that is required to ensure that the context struct member does not share any + * cachelines with the rest of the struct. This is needed to ensure that cache + * maintenance for non-coherent DMA does not affect data that may be accessed + * by the CPU concurrently. */ #define CRYPTO_MINALIGN ARCH_KMALLOC_MINALIGN
Unlike many other structure types defined in the crypto API, the 'shash_desc' structure is permitted to live on the stack, which implies its contents may not be accessed by DMA masters. (This is due to the fact that the stack may be located in the vmalloc area, which requires a different virtual-to-physical translation than the one implemented by the DMA subsystem) Our definition of CRYPTO_MINALIGN_ATTR is based on ARCH_KMALLOC_MINALIGN, which may take DMA constraints into account on architectures that support non-cache coherent DMA such as ARM and arm64. In this case, the value is chosen to reflect the largest cacheline size in the system, in order to ensure that explicit cache maintenance as required by non-coherent DMA masters does not affect adjacent, unrelated slab allocations. On arm64, this value is currently set at 128 bytes. This means that applying CRYPTO_MINALIGN_ATTR to struct shash_desc is both unnecessary (as it is never used for DMA), and undesirable, given that it wastes stack space (on arm64, performing the alignment costs 112 bytes in the worst case, and the hole between the 'tfm' and '__ctx' members takes up another 120 bytes, resulting in an increased stack footprint of up to 232 bytes.) So instead, let's switch to the minimum SLAB alignment, which does not take DMA constraints into account. By the same reasoning, we can reduce the alignment of the skcipher_request structure when it lives on the stack. Note that in this case, we cannot reduce the alignment of the __ctx[] struct member, because the structure type is shared between synchronous and asynchronous users. However, there is no need to apply CRYPTO_MINALIGN_ATTR to SYNC_SKCIPHER_REQUEST_ON_STACK so let's use ARCH_SLAB_MINALIGN here as well. Also, document the DMA aspect of this in the comment that explains the purpose of CRYPTO_MINALIGN_ATTR. Note that this is a no-op for x86. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- v2: - reduce alignment for SYNC_SKCIPHER_REQUEST_ON_STACK as well - update CRYPTO_MINALIGN_ATTR comment with DMA requirements. include/crypto/hash.h | 8 ++++---- include/crypto/skcipher.h | 2 +- include/linux/crypto.h | 9 ++++++--- 3 files changed, 11 insertions(+), 8 deletions(-)