[dm-devel] [PATCH v2] dm: switch dm-verity to async hash crypto API

[Gilad Ben-Yossef]

Hi Milan,

Thank you for the review and testing.

On Fri, Feb 17, 2017 at 3:00 PM, Milan Broz <gmazyland at gmail.com> wrote:
> On 02/06/2017 02:58 PM, Gilad Ben-Yossef wrote:
>> Use of the synchronous digest API limits dm-verity to using pure
>> CPU based algorithm providers and rules out the use of off CPU
>> algorithm providers which are normally asynchronous by nature,
>> potentially freeing CPU cycles.
>>
>> This can reduce performance per Watt in situations such as during
>> boot time when a lot of concurrent file accesses are made to the
>> protected volume.
>>
>> Move DM_VERITY to the asynchronous hash API.
>
> Did you test that async hash completion path?

Yes, I did - with the Arm TrustZone CryptoCell hardware accelerator.
I did not try with cryptd though.
>
> I just tried to force async crypto by replacing "sha256"
> in mapping table with "cryptd(sha256-generic)" and it kills
> kernel immediately.
> https://mbroz.fedorapeople.org/tmp/verity-fail.png
>
> (I hope this trick should cause to use cryptd and use async processing.
> In previous version the parameter is properly rejected, because unsupported
> by shash api.)
>

Thanks for this trick. I was not aware you can invoke cryptd it like that.

I will recreate the issue and fix it.

>
> Some more comments below.
> ...
>
>> -static int verity_hash_update(struct dm_verity *v, struct shash_desc *desc,
>> -                           const u8 *data, size_t len)
>> +static int verity_hash_update(struct dm_verity *v, struct ahash_request *req,
>> +                             const u8 *data, size_t len,
>> +                             struct verity_result *res)
>>  {
>> -     int r = crypto_shash_update(desc, data, len);
>> +     struct scatterlist sg;
>>
>> -     if (unlikely(r < 0))
>> -             DMERR("crypto_shash_update failed: %d", r);
>> +     sg_init_table(&sg, 1);
>> +     sg_set_buf(&sg, data, len);
>
> why not use sg_init_one?

No good reason. I will amend it in the next revision.

>
>> +     ahash_request_set_crypt(req, &sg, NULL, len);
>> +
>> +     return verity_complete_op(res, crypto_ahash_update(req));
>> +}
>
> ...
>
>> -int verity_hash(struct dm_verity *v, struct shash_desc *desc,
>> +int verity_hash(struct dm_verity *v, struct ahash_request *req,
>>               const u8 *data, size_t len, u8 *digest)
>>  {
>>       int r;
>> +     struct verity_result res;
>>
>> -     r = verity_hash_init(v, desc);
>> +     r = verity_hash_init(v, req, &res);
>>       if (unlikely(r < 0))
>> -             return r;
>> +             goto out;
>
> why it is changed to goto? it doesn't simplify anything in this function
>

I generally prefer for a function to have a single return point, if it does
not over complicates things. I find it makes code more readable and easier
to reason about - put debugging log statement for return for example.

>>
>> -     r = verity_hash_update(v, desc, data, len);
>> +     r = verity_hash_update(v, req, data, len, &res);
>>       if (unlikely(r < 0))
>> -             return r;
>> +             goto out;
>> +
>> +     r = verity_hash_final(v, req, digest, &res);
>>
>> -     return verity_hash_final(v, desc, digest);
>> +out:
>> +     return r;
>>  }

I will post a new revision of the patch early next week  .

Thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru