Re: [dm-devel] [PATCH 5/6] crypto: set the flag CRYPTO_ALG_ALLOCATES_MEMORY
On Mon, 13 Jul 2020, Horia Geantă wrote: > On 7/13/2020 7:01 PM, Eric Biggers wrote: > > On Mon, Jul 13, 2020 at 06:49:00PM +0300, Horia Geantă wrote: > >> On 7/1/2020 7:52 AM, Eric Biggers wrote: > >>> From: Mikulas Patocka > >>> > >>> Set the flag CRYPTO_ALG_ALLOCATES_MEMORY in the crypto drivers that > >>> allocate memory. > >>> > >> Quite a few drivers are impacted. > >> > >> I wonder what's the proper way to address the memory allocation. > >> > >> Herbert mentioned setting up reqsize: > >> https://lore.kernel.org/linux-crypto/20200610010450.ga6...@gondor.apana.org.au/ > >> > >> I see at least two hurdles in converting the drivers to using reqsize: > >> > >> 1. Some drivers allocate the memory using GFP_DMA > >> > >> reqsize does not allow drivers to control gfp allocation flags. > >> > >> I've tried converting talitos driver (to use reqsize) at some point, > >> and in the process adding a generic CRYPTO_TFM_REQ_DMA flag: > >> https://lore.kernel.org/linux-crypto/54fd8d3b.5040...@freescale.com > >> https://lore.kernel.org/linux-crypto/1426266882-31626-1-git-send-email-horia.gea...@freescale.com > >> > >> The flag was supposed to be transparent for the user, > >> however there were users that open-coded the request allocation, > >> for example esp_alloc_tmp() in net/ipv4/esp4.c. > >> At that time, Dave NACK-ed the change: > >> https://lore.kernel.org/linux-crypto/1426266922-31679-1-git-send-email-horia.gea...@freescale.com > >> > >> > >> 2. Memory requirements cannot be determined / are not known > >> at request allocation time > >> > >> An analysis for talitos driver is here: > >> https://lore.kernel.org/linux-crypto/54f8235b.5080...@freescale.com > >> > >> In general, drivers would be forced to ask more memory than needed, > >> to handle the "worst-case". > >> Logic will be needed to fail in case the "worst-case" isn't correctly > >> estimated. > >> > >> However, this is still problematic. > >> > >> For example, a driver could set up reqsize to accommodate for 32 S/G > >> entries > >> (in the HW S/G table). In case a dm-crypt encryption request would require > >> more, > >> then driver's .encrypt callback would fail, possibly with -ENOMEM, > >> since there's not enough pre-allocated memory. > >> This brings us back to the same problem we're trying to solve, > >> since in this case the driver would be forced to either fail immediately or > >> to allocate memory at .encrypt/.decrypt time. > >> > > > > We have to place restrictions on what cases > > !(flags & CRYPTO_ALG_ALLOCATES_MEMORY) applies to anyway; see the patch that > > introduces it. If needed we could add more restrictions, like limit the > > number > > of scatterlist elements. If we did that, the driver could allocate memory > > if > > the number of scatterlist elements is large, without having to set > > CRYPTO_ALG_ALLOCATES_MEMORY. > > > This sounds reasonable. > > > Also, have you considered using a mempool? A mempool allows allocations > > without > > a possibility of failure, at the cost of pre-allocations. > > > Thanks for the suggestion. > > Would this be safe for all cases, e.g. IPsec - where .encrypt/.decrypt > callbacks > execute in (soft)IRQ context? > kernel-doc for mempool_alloc() mentions it could fail when called from > "IRQ context". In IPsec, you can drop packets (and TCP will retransmit them), so there is no problem with memory allocation failures. > Thanks, > Horia Mikulas-- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 5/6] crypto: set the flag CRYPTO_ALG_ALLOCATES_MEMORY
On 7/13/2020 7:01 PM, Eric Biggers wrote: > On Mon, Jul 13, 2020 at 06:49:00PM +0300, Horia Geantă wrote: >> On 7/1/2020 7:52 AM, Eric Biggers wrote: >>> From: Mikulas Patocka >>> >>> Set the flag CRYPTO_ALG_ALLOCATES_MEMORY in the crypto drivers that >>> allocate memory. >>> >> Quite a few drivers are impacted. >> >> I wonder what's the proper way to address the memory allocation. >> >> Herbert mentioned setting up reqsize: >> https://lore.kernel.org/linux-crypto/20200610010450.ga6...@gondor.apana.org.au/ >> >> I see at least two hurdles in converting the drivers to using reqsize: >> >> 1. Some drivers allocate the memory using GFP_DMA >> >> reqsize does not allow drivers to control gfp allocation flags. >> >> I've tried converting talitos driver (to use reqsize) at some point, >> and in the process adding a generic CRYPTO_TFM_REQ_DMA flag: >> https://lore.kernel.org/linux-crypto/54fd8d3b.5040...@freescale.com >> https://lore.kernel.org/linux-crypto/1426266882-31626-1-git-send-email-horia.gea...@freescale.com >> >> The flag was supposed to be transparent for the user, >> however there were users that open-coded the request allocation, >> for example esp_alloc_tmp() in net/ipv4/esp4.c. >> At that time, Dave NACK-ed the change: >> https://lore.kernel.org/linux-crypto/1426266922-31679-1-git-send-email-horia.gea...@freescale.com >> >> >> 2. Memory requirements cannot be determined / are not known >> at request allocation time >> >> An analysis for talitos driver is here: >> https://lore.kernel.org/linux-crypto/54f8235b.5080...@freescale.com >> >> In general, drivers would be forced to ask more memory than needed, >> to handle the "worst-case". >> Logic will be needed to fail in case the "worst-case" isn't correctly >> estimated. >> >> However, this is still problematic. >> >> For example, a driver could set up reqsize to accommodate for 32 S/G entries >> (in the HW S/G table). In case a dm-crypt encryption request would require >> more, >> then driver's .encrypt callback would fail, possibly with -ENOMEM, >> since there's not enough pre-allocated memory. >> This brings us back to the same problem we're trying to solve, >> since in this case the driver would be forced to either fail immediately or >> to allocate memory at .encrypt/.decrypt time. >> > > We have to place restrictions on what cases > !(flags & CRYPTO_ALG_ALLOCATES_MEMORY) applies to anyway; see the patch that > introduces it. If needed we could add more restrictions, like limit the > number > of scatterlist elements. If we did that, the driver could allocate memory if > the number of scatterlist elements is large, without having to set > CRYPTO_ALG_ALLOCATES_MEMORY. > This sounds reasonable. > Also, have you considered using a mempool? A mempool allows allocations > without > a possibility of failure, at the cost of pre-allocations. > Thanks for the suggestion. Would this be safe for all cases, e.g. IPsec - where .encrypt/.decrypt callbacks execute in (soft)IRQ context? kernel-doc for mempool_alloc() mentions it could fail when called from "IRQ context". Thanks, Horia -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 5/6] crypto: set the flag CRYPTO_ALG_ALLOCATES_MEMORY
On Mon, Jul 13, 2020 at 06:49:00PM +0300, Horia Geantă wrote: > On 7/1/2020 7:52 AM, Eric Biggers wrote: > > From: Mikulas Patocka > > > > Set the flag CRYPTO_ALG_ALLOCATES_MEMORY in the crypto drivers that > > allocate memory. > > > Quite a few drivers are impacted. > > I wonder what's the proper way to address the memory allocation. > > Herbert mentioned setting up reqsize: > https://lore.kernel.org/linux-crypto/20200610010450.ga6...@gondor.apana.org.au/ > > I see at least two hurdles in converting the drivers to using reqsize: > > 1. Some drivers allocate the memory using GFP_DMA > > reqsize does not allow drivers to control gfp allocation flags. > > I've tried converting talitos driver (to use reqsize) at some point, > and in the process adding a generic CRYPTO_TFM_REQ_DMA flag: > https://lore.kernel.org/linux-crypto/54fd8d3b.5040...@freescale.com > https://lore.kernel.org/linux-crypto/1426266882-31626-1-git-send-email-horia.gea...@freescale.com > > The flag was supposed to be transparent for the user, > however there were users that open-coded the request allocation, > for example esp_alloc_tmp() in net/ipv4/esp4.c. > At that time, Dave NACK-ed the change: > https://lore.kernel.org/linux-crypto/1426266922-31679-1-git-send-email-horia.gea...@freescale.com > > > 2. Memory requirements cannot be determined / are not known > at request allocation time > > An analysis for talitos driver is here: > https://lore.kernel.org/linux-crypto/54f8235b.5080...@freescale.com > > In general, drivers would be forced to ask more memory than needed, > to handle the "worst-case". > Logic will be needed to fail in case the "worst-case" isn't correctly > estimated. > > However, this is still problematic. > > For example, a driver could set up reqsize to accommodate for 32 S/G entries > (in the HW S/G table). In case a dm-crypt encryption request would require > more, > then driver's .encrypt callback would fail, possibly with -ENOMEM, > since there's not enough pre-allocated memory. > This brings us back to the same problem we're trying to solve, > since in this case the driver would be forced to either fail immediately or > to allocate memory at .encrypt/.decrypt time. > We have to place restrictions on what cases !(flags & CRYPTO_ALG_ALLOCATES_MEMORY) applies to anyway; see the patch that introduces it. If needed we could add more restrictions, like limit the number of scatterlist elements. If we did that, the driver could allocate memory if the number of scatterlist elements is large, without having to set CRYPTO_ALG_ALLOCATES_MEMORY. Also, have you considered using a mempool? A mempool allows allocations without a possibility of failure, at the cost of pre-allocations. - Eric -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 5/6] crypto: set the flag CRYPTO_ALG_ALLOCATES_MEMORY
On 7/1/2020 7:52 AM, Eric Biggers wrote: > From: Mikulas Patocka > > Set the flag CRYPTO_ALG_ALLOCATES_MEMORY in the crypto drivers that > allocate memory. > Quite a few drivers are impacted. I wonder what's the proper way to address the memory allocation. Herbert mentioned setting up reqsize: https://lore.kernel.org/linux-crypto/20200610010450.ga6...@gondor.apana.org.au/ I see at least two hurdles in converting the drivers to using reqsize: 1. Some drivers allocate the memory using GFP_DMA reqsize does not allow drivers to control gfp allocation flags. I've tried converting talitos driver (to use reqsize) at some point, and in the process adding a generic CRYPTO_TFM_REQ_DMA flag: https://lore.kernel.org/linux-crypto/54fd8d3b.5040...@freescale.com https://lore.kernel.org/linux-crypto/1426266882-31626-1-git-send-email-horia.gea...@freescale.com The flag was supposed to be transparent for the user, however there were users that open-coded the request allocation, for example esp_alloc_tmp() in net/ipv4/esp4.c. At that time, Dave NACK-ed the change: https://lore.kernel.org/linux-crypto/1426266922-31679-1-git-send-email-horia.gea...@freescale.com 2. Memory requirements cannot be determined / are not known at request allocation time An analysis for talitos driver is here: https://lore.kernel.org/linux-crypto/54f8235b.5080...@freescale.com In general, drivers would be forced to ask more memory than needed, to handle the "worst-case". Logic will be needed to fail in case the "worst-case" isn't correctly estimated. However, this is still problematic. For example, a driver could set up reqsize to accommodate for 32 S/G entries (in the HW S/G table). In case a dm-crypt encryption request would require more, then driver's .encrypt callback would fail, possibly with -ENOMEM, since there's not enough pre-allocated memory. This brings us back to the same problem we're trying to solve, since in this case the driver would be forced to either fail immediately or to allocate memory at .encrypt/.decrypt time. Thanks, Horia -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 5/6] crypto: set the flag CRYPTO_ALG_ALLOCATES_MEMORY
From: Mikulas Patocka
Set the flag CRYPTO_ALG_ALLOCATES_MEMORY in the crypto drivers that
allocate memory.
drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c: sun8i_ce_cipher
drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c: sun8i_ss_cipher
drivers/crypto/amlogic/amlogic-gxl-core.c: meson_cipher
drivers/crypto/axis/artpec6_crypto.c: artpec6_crypto_common_init
drivers/crypto/bcm/cipher.c: spu_skcipher_rx_sg_create
drivers/crypto/caam/caamalg.c: aead_edesc_alloc
drivers/crypto/caam/caamalg_qi.c: aead_edesc_alloc
drivers/crypto/caam/caamalg_qi2.c: aead_edesc_alloc
drivers/crypto/caam/caamhash.c: hash_digest_key
drivers/crypto/cavium/cpt/cptvf_algs.c: process_request
drivers/crypto/cavium/nitrox/nitrox_aead.c: nitrox_process_se_request
drivers/crypto/cavium/nitrox/nitrox_skcipher.c: nitrox_process_se_request
drivers/crypto/ccp/ccp-crypto-aes-cmac.c: ccp_do_cmac_update
drivers/crypto/ccp/ccp-crypto-aes-galois.c: ccp_crypto_enqueue_request
drivers/crypto/ccp/ccp-crypto-aes-xts.c: ccp_crypto_enqueue_request
drivers/crypto/ccp/ccp-crypto-aes.c: ccp_crypto_enqueue_request
drivers/crypto/ccp/ccp-crypto-des3.c: ccp_crypto_enqueue_request
drivers/crypto/ccp/ccp-crypto-sha.c: ccp_crypto_enqueue_request
drivers/crypto/chelsio/chcr_algo.c: create_cipher_wr
drivers/crypto/hisilicon/sec/sec_algs.c: sec_alloc_and_fill_hw_sgl
drivers/crypto/hisilicon/sec2/sec_crypto.c: sec_alloc_req_id
drivers/crypto/inside-secure/safexcel_cipher.c: safexcel_queue_req
drivers/crypto/inside-secure/safexcel_hash.c: safexcel_ahash_enqueue
drivers/crypto/ixp4xx_crypto.c: ablk_perform
drivers/crypto/marvell/cesa/cipher.c: mv_cesa_skcipher_dma_req_init
drivers/crypto/marvell/cesa/hash.c: mv_cesa_ahash_dma_req_init
drivers/crypto/marvell/octeontx/otx_cptvf_algs.c: create_ctx_hdr
drivers/crypto/n2_core.c: n2_compute_chunks
drivers/crypto/picoxcell_crypto.c: spacc_sg_to_ddt
drivers/crypto/qat/qat_common/qat_algs.c: qat_alg_skcipher_encrypt
drivers/crypto/qce/skcipher.c: qce_skcipher_async_req_handle
drivers/crypto/talitos.c : talitos_edesc_alloc
drivers/crypto/virtio/virtio_crypto_algs.c: __virtio_crypto_skcipher_do_req
drivers/crypto/xilinx/zynqmp-aes-gcm.c: zynqmp_aes_aead_cipher
Signed-off-by: Mikulas Patocka
[EB: avoid overly-long lines]
Signed-off-by: Eric Biggers
---
.../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 12 +-
.../crypto/allwinner/sun8i-ss/sun8i-ss-core.c | 12 +-
drivers/crypto/amlogic/amlogic-gxl-core.c | 6 +-
drivers/crypto/axis/artpec6_crypto.c | 20 ++-
drivers/crypto/bcm/cipher.c | 72 ---
drivers/crypto/caam/caamalg.c | 6 +-
drivers/crypto/caam/caamalg_qi.c | 6 +-
drivers/crypto/caam/caamalg_qi2.c | 8 +-
drivers/crypto/caam/caamhash.c| 2 +-
drivers/crypto/cavium/cpt/cptvf_algs.c| 18 ++-
drivers/crypto/cavium/nitrox/nitrox_aead.c| 4 +-
.../crypto/cavium/nitrox/nitrox_skcipher.c| 16 +--
drivers/crypto/ccp/ccp-crypto-aes-cmac.c | 1 +
drivers/crypto/ccp/ccp-crypto-aes-galois.c| 1 +
drivers/crypto/ccp/ccp-crypto-aes-xts.c | 1 +
drivers/crypto/ccp/ccp-crypto-aes.c | 2 +
drivers/crypto/ccp/ccp-crypto-des3.c | 1 +
drivers/crypto/ccp/ccp-crypto-sha.c | 1 +
drivers/crypto/chelsio/chcr_algo.c| 7 +-
drivers/crypto/hisilicon/sec/sec_algs.c | 24 ++--
drivers/crypto/hisilicon/sec2/sec_crypto.c| 4 +-
.../crypto/inside-secure/safexcel_cipher.c| 47 +++
drivers/crypto/inside-secure/safexcel_hash.c | 18 +++
drivers/crypto/ixp4xx_crypto.c| 6 +-
drivers/crypto/marvell/cesa/cipher.c | 18 ++-
drivers/crypto/marvell/cesa/hash.c| 6 +
.../crypto/marvell/octeontx/otx_cptvf_algs.c | 30 ++---
drivers/crypto/n2_core.c | 3 +-
drivers/crypto/picoxcell_crypto.c | 17 ++-
drivers/crypto/qat/qat_common/qat_algs.c | 12 +-
drivers/crypto/qce/skcipher.c | 1 +
drivers/crypto/talitos.c | 117 --
drivers/crypto/virtio/virtio_crypto_algs.c| 3 +-
drivers/crypto/xilinx/zynqmp-aes-gcm.c| 1 +
34 files changed, 360 insertions(+), 143 deletions(-)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
index b957061424a1..138759dc8190 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
@@ -185,7 +185,8 @@ static struct sun8i_ce_alg_template ce_algs[] = {
.cra_priority = 400,
.cra_blocksize = AES_BLOCK_SIZE,
.cra_flags = CRYPTO_ALG_TYPE_SKCIPHER |
- CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK,
+ CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY |
+ CRYPTO_ALG_NEED_FALLBACK,
