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 <mpatocka@xxxxxxxxxx> > >>> > >>> 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.GA6449@xxxxxxxxxxxxxxxxxxx/ > >> > >> 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.5040409@xxxxxxxxxxxxx > >> https://lore.kernel.org/linux-crypto/1426266882-31626-1-git-send-email-horia.geanta@xxxxxxxxxxxxx > >> > >> 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.geanta@xxxxxxxxxxxxx > >> > >> > >> 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.5080301@xxxxxxxxxxxxx > >> > >> 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
