linux-kernel - Re: [dm-devel] [dm-crypt] [RFC PATCH 1/1] Add DM_CRYPT_FORCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Jun 2020 06:46:33 +0000
From:   Damien Le Moal <Damien.LeMoal@....com>
To:     Eric Biggers <ebiggers@...nel.org>
CC:     Ignat Korchagin <ignat@...udflare.com>,
        "snitzer@...hat.com" <snitzer@...hat.com>,
        "kernel-team@...udflare.com" <kernel-team@...udflare.com>,
        "dm-crypt@...ut.de" <dm-crypt@...ut.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        "agk@...hat.com" <agk@...hat.com>
Subject: Re: [dm-devel] [dm-crypt] [RFC PATCH 1/1] Add DM_CRYPT_FORCE_INLINE
 flag to dm-crypt target

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