Hi Milan,

Thank you for the review and testing.

On Fri, Feb 17, 2017 at 3:00 PM, Milan Broz <gmazyland@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);
>
> 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