Hi Milan, Thank you for the review and testing.
On Fri, Feb 17, 2017 at 3:00 PM, Milan Broz <gmazyl...@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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
