| Message ID | 20220406142715.2270256-3-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: avoid DMA padding for request structures | expand |
On Wed, Apr 06, 2022 at 04:27:09PM +0200, Ard Biesheuvel wrote: > > +#define EIP197_SKCIPHER_REQ_SIZE (ALIGN(sizeof(struct skcipher_request), \ > + CRYPTO_MINALIGN) + \ The whole point of CRYPTO_MINALIGN is that it comes for free via kmalloc. If you need alignment over and above that of kmalloc, you need to do it explicitly in the driver. Cheers,
On Thu, 7 Apr 2022 at 06:32, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Wed, Apr 06, 2022 at 04:27:09PM +0200, Ard Biesheuvel wrote: > > > > +#define EIP197_SKCIPHER_REQ_SIZE (ALIGN(sizeof(struct skcipher_request), \ > > + CRYPTO_MINALIGN) + \ > > The whole point of CRYPTO_MINALIGN is that it comes for free > via kmalloc. > Yes, but kmalloc allocates the entire block at once, and so all the concatenated structures need to use the same alignment, which results in substantial padding overhead. > If you need alignment over and above that of kmalloc, you need > to do it explicitly in the driver. > But none of the drivers currently do so, and rely on the API to produce ctx struct pointers that are suitably aligned for DMA. Note that the main issue is not the alignment, but the rounding up of the size due to that. For instance, looking at crypto/adiantum.c: struct adiantum_request_ctx { ... other fields ... /* Sub-requests, must be last */ union { struct shash_desc hash_desc; struct skcipher_request streamcipher_req; } u; }; So on arm64, every skcipher_request that gets allocated will be: 128 bytes for the outer skcipher_request + padding 128 bytes for the adiantum request context + padding 128 bytes for the inner skcipher_request + padding n bytes for the inner context Given that the skcipher_request only needs 72 bytes on 64-bit architectures, and Adiantum's reqctx size of ~50 bytes, this results in an overhead of ~200 bytes for every allocation, which is rather wasteful. I think permitting DMA directly into these buffers was a mistake, but it is the situation we are in today. I am only trying to reduce the memory overhead for cases where it is not needed.
On Thu, 7 Apr 2022 at 10:32, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 7 Apr 2022 at 06:32, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > On Wed, Apr 06, 2022 at 04:27:09PM +0200, Ard Biesheuvel wrote: > > > > > > +#define EIP197_SKCIPHER_REQ_SIZE (ALIGN(sizeof(struct skcipher_request), \ > > > + CRYPTO_MINALIGN) + \ > > > > The whole point of CRYPTO_MINALIGN is that it comes for free > > via kmalloc. > > > BTW the definition above is only used for request allocations on the stack.
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h index ce1e611a163e..b5033803714a 100644 --- a/drivers/crypto/inside-secure/safexcel.h +++ b/drivers/crypto/inside-secure/safexcel.h @@ -56,12 +56,15 @@ GFP_KERNEL : GFP_ATOMIC) /* Custom on-stack requests (for invalidation) */ -#define EIP197_SKCIPHER_REQ_SIZE sizeof(struct skcipher_request) + \ - sizeof(struct safexcel_cipher_req) -#define EIP197_AHASH_REQ_SIZE sizeof(struct ahash_request) + \ - sizeof(struct safexcel_ahash_req) -#define EIP197_AEAD_REQ_SIZE sizeof(struct aead_request) + \ - sizeof(struct safexcel_cipher_req) +#define EIP197_SKCIPHER_REQ_SIZE (ALIGN(sizeof(struct skcipher_request), \ + CRYPTO_MINALIGN) + \ + sizeof(struct safexcel_cipher_req)) +#define EIP197_AHASH_REQ_SIZE (ALIGN(sizeof(struct ahash_request), \ + CRYPTO_MINALIGN) + \ + sizeof(struct safexcel_ahash_req)) +#define EIP197_AEAD_REQ_SIZE (ALIGN(sizeof(struct aead_request), \ + CRYPTO_MINALIGN) + \ + sizeof(struct safexcel_cipher_req)) #define EIP197_REQUEST_ON_STACK(name, type, size) \ char __##name##_desc[size] CRYPTO_MINALIGN_ATTR; \ struct type##_request *name = (void *)__##name##_desc diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c index d68ef16650d4..6dc3e171f474 100644 --- a/drivers/crypto/inside-secure/safexcel_cipher.c +++ b/drivers/crypto/inside-secure/safexcel_cipher.c @@ -1108,7 +1108,6 @@ static int safexcel_cipher_exit_inv(struct crypto_tfm *tfm, static int safexcel_skcipher_exit_inv(struct crypto_tfm *tfm) { EIP197_REQUEST_ON_STACK(req, skcipher, EIP197_SKCIPHER_REQ_SIZE); - struct safexcel_cipher_req *sreq = skcipher_request_ctx(req); struct safexcel_inv_result result = {}; memset(req, 0, sizeof(struct skcipher_request)); @@ -1117,13 +1116,13 @@ static int safexcel_skcipher_exit_inv(struct crypto_tfm *tfm) safexcel_inv_complete, &result); skcipher_request_set_tfm(req, __crypto_skcipher_cast(tfm)); - return safexcel_cipher_exit_inv(tfm, &req->base, sreq, &result); + return safexcel_cipher_exit_inv(tfm, &req->base, + skcipher_request_ctx(req), &result); } static int safexcel_aead_exit_inv(struct crypto_tfm *tfm) { EIP197_REQUEST_ON_STACK(req, aead, EIP197_AEAD_REQ_SIZE); - struct safexcel_cipher_req *sreq = aead_request_ctx(req); struct safexcel_inv_result result = {}; memset(req, 0, sizeof(struct aead_request)); @@ -1132,7 +1131,8 @@ static int safexcel_aead_exit_inv(struct crypto_tfm *tfm) safexcel_inv_complete, &result); aead_request_set_tfm(req, __crypto_aead_cast(tfm)); - return safexcel_cipher_exit_inv(tfm, &req->base, sreq, &result); + return safexcel_cipher_exit_inv(tfm, &req->base, aead_request_ctx(req), + &result); } static int safexcel_queue_req(struct crypto_async_request *base,
The skcipher, aead and ahash request structure types will no longer be aligned for DMA, and the padding and re-alignment of the context buffer region will be taken care of at runtime. This means that we need to update the stack representation accordingly, to ensure that the context pointer doesn't point past the allocation after rounding. Also, as getting at the context pointer of a skcipher_request will involve a check of the underlying algo's cra_flags field, as it may need to be aligned for DMA, defer grabbing the context pointer until after setting the TFM. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/crypto/inside-secure/safexcel.h | 15 +++++++++------ drivers/crypto/inside-secure/safexcel_cipher.c | 8 ++++---- 2 files changed, 13 insertions(+), 10 deletions(-)