2023-07-07 10:51:25

by Mikulas Patocka

[permalink] [raw]
Subject: Re: [dm-devel] [PATCH 2/3] crypto: api - adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY



On Wed, 5 Jul 2023, Eric Biggers wrote:

> On Wed, Jul 05, 2023 at 05:40:08PM +0100, Giovanni Cabiddu wrote:
>
> > Algorithms that do not set this flag will guarantee
>
> "will guarantee" => "guarantee"
>
> > that memory is not allocated during request processing, except in
> > the avoidable exception cases described below.
>
> "avoidable exception cases" => "exception cases"
>
> Whether they are avoidable depends on the user.
>
> > * Users can request an algorithm with this flag unset if they can't handle
> > * memory allocation failures or sleeping during request processing.
>
> Why add the "sleeping during request processing" part? Isn't that controlled on
> a per-request basis by CRYPTO_TFM_REQ_MAY_SLEEP which is a separate thing?
>
> > * They should also follow the constraints below.
>
> "should" => "must"
>
> > + * - The input and output scatterlists must have no more than 4 entries.
> > + * If the scatterlists contain more than 4 entries, the algorithm may
> > + * allocate memory.
>
> "If the scatterlists contains" => "If either scatterlist contains"
>
> Otherwise it is unclear whether this is talking about the length of each
> scatterlist individually, or the sum of their lengths.
>
> - Eric

Hi

I wouldn't change the meaning of CRYPTO_ALG_ALLOCATES_MEMORY (because
people will forget about this subtle change anyway).

Also note that dm-integrity allocates arbitrarily large sg-lists when
encrypting the journal, so if you change the meaning of
CRYPTO_ALG_ALLOCATES_MEMORY, there would be no flag left for dm-integrity
to test.

I would introduce a new flag, something like
CRYPTO_ALG_ALLOCATES_MEMORY_FOR_5_OR_MORE_SG_ENTRIES. dm-crypt can then
filter the algorithms based on this flag - and the rest of the kernel code
may stay unchanged.

Mikulas