Re: [dm-devel] [dm-crypt] [RFC PATCH 1/1] Add DM_CRYPT_FORCE_INLINE flag to dm-crypt target

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/06/24 14:27, Eric Biggers wrote:
> On Wed, Jun 24, 2020 at 05:21:24AM +0000, Damien Le Moal wrote:
>>>> @@ -1458,13 +1459,18 @@ static void crypt_alloc_req_skcipher(struct crypt_config *cc,
>>>>  
>>>>  	skcipher_request_set_tfm(ctx->r.req, cc->cipher_tfm.tfms[key_index]);
>>>>  
>>>> -	/*
>>>> -	 * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs
>>>> -	 * requests if driver request queue is full.
>>>> -	 */
>>>> -	skcipher_request_set_callback(ctx->r.req,
>>>> -	    CRYPTO_TFM_REQ_MAY_BACKLOG,
>>>> -	    kcryptd_async_done, dmreq_of_req(cc, ctx->r.req));
>>>> +	if (test_bit(DM_CRYPT_FORCE_INLINE, &cc->flags))
>>>> +		/* make sure we zero important fields of the request */
>>>> +		skcipher_request_set_callback(ctx->r.req,
>>>> +	        0, NULL, NULL);
>>>> +	else
>>>> +		/*
>>>> +		 * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs
>>>> +		 * requests if driver request queue is full.
>>>> +		 */
>>>> +		skcipher_request_set_callback(ctx->r.req,
>>>> +	        CRYPTO_TFM_REQ_MAY_BACKLOG,
>>>> +	        kcryptd_async_done, dmreq_of_req(cc, ctx->r.req));
>>>>  }
>>>
>>> This looks wrong.  Unless type=0 and mask=CRYPTO_ALG_ASYNC are passed to
>>> crypto_alloc_skcipher(), the skcipher implementation can still be asynchronous,
>>> in which case providing a callback is required.
>>>
>>> Do you intend that the "force_inline" option forces the use of a synchronous
>>> skcipher (alongside the other things it does)?  Or should it still allow
>>> asynchronous ones?
>>>
>>> We may not actually have a choice in that matter, since xts-aes-aesni has the
>>> CRYPTO_ALG_ASYNC bit set (as I mentioned) despite being synchronous in most
>>> cases; thus, the crypto API won't give you it if you ask for a synchronous
>>> cipher.  So I think you still need to allow async skciphers?  That means a
>>> callback is still always required.
>>
>> Arg... So it means that some skciphers will not be OK at all for SMR writes. I
>> was not aware of these differences (tested with aes-xts-plain64 only). The ugly
>> way to support async ciphers would be to just wait inline for the crypto API to
>> complete using a completion for instance. But that is very ugly. Back to
>> brainstorming, and need to learn more about the crypto API...
>>
> 
> It's easy to wait for crypto API requests to complete if you need to --
> just use crypto_wait_req().

OK. Thanks for the information. I will look into this and the performance
implications. A quick grep shows that a lot of different accelerators for
different architectures have CRYPTO_ALG_ASYNC set. So definitely something that
needs to be checked for SMR, and for Ignat inline patch.

> We do this in fs/crypto/, for example.  (Not many people are using fscrypt with
> crypto API based accelerators, so there hasn't yet been much need to support the
> complexity of issuing multiple async crypto requests like dm-crypt supports.)

Zonefs fscrypt support is on my to do list too :)

Thanks !

>
> - Eric
> 


-- 
Damien Le Moal
Western Digital Research