<div dir="ltr">Hi Milan,<div><br></div><div>Thank you for the review and testing.<br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 17, 2017 at 3:00 PM, Milan Broz <span dir="ltr"><<a href="mailto:gmazyland@gmail.com" target="_blank">gmazyland@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 02/06/2017 02:58 PM, Gilad Ben-Yossef wrote:<br>
> Use of the synchronous digest API limits dm-verity to using pure<br>
> CPU based algorithm providers and rules out the use of off CPU<br>
> algorithm providers which are normally asynchronous by nature,<br>
> potentially freeing CPU cycles.<br>
><br>
> This can reduce performance per Watt in situations such as during<br>
> boot time when a lot of concurrent file accesses are made to the<br>
> protected volume.<br>
><br>
> Move DM_VERITY to the asynchronous hash API.<br>
<br>
</span>Did you test that async hash completion path?<br>
<br></blockquote><div>Yes, I did - with the Arm TrustZone CryptoCell hardware accelerator.</div><div>I did not try with cryptd though.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I just tried to force async crypto by replacing "sha256"<br>
in mapping table with "cryptd(sha256-generic)" and it kills<br>
kernel immediately.<br>
<a href="https://mbroz.fedorapeople.org/tmp/verity-fail.png" rel="noreferrer" target="_blank">https://mbroz.fedorapeople.<wbr>org/tmp/verity-fail.png</a><br>
<br>
(I hope this trick should cause to use cryptd and use async processing.<br>
In previous version the parameter is properly rejected, because unsupported<br>
by shash api.)<br>
<br>
<br></blockquote><div><br></div><div>Thanks for this trick. I was not aware you can invoke cryptd it like that.</div><div><br></div><div>I will recreate the issue and fix it.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Some more comments below.<br>
...<br>
<span class=""><br>
> -static int verity_hash_update(struct dm_verity *v, struct shash_desc *desc,<br>
> - const u8 *data, size_t len)<br>
> +static int verity_hash_update(struct dm_verity *v, struct ahash_request *req,<br>
> + const u8 *data, size_t len,<br>
> + struct verity_result *res)<br>
> {<br>
> - int r = crypto_shash_update(desc, data, len);<br>
> + struct scatterlist sg;<br>
><br>
> - if (unlikely(r < 0))<br>
> - DMERR("crypto_shash_update failed: %d", r);<br>
> + sg_init_table(&sg, 1);<br>
> + sg_set_buf(&sg, data, len);<br>
<br>
</span>No good reason. I will amend it in the next revision.<br></blockquote><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + ahash_request_set_crypt(req, &sg, NULL, len);<br>
> +<br>
> + return verity_complete_op(res, crypto_ahash_update(req));<br>
> +}<br>
<br>
</span>...<br>
<span class=""><br>
> -int verity_hash(struct dm_verity *v, struct shash_desc *desc,<br>
> +int verity_hash(struct dm_verity *v, struct ahash_request *req,<br>
> const u8 *data, size_t len, u8 *digest)<br>
> {<br>
> int r;<br>
> + struct verity_result res;<br>
><br>
> - r = verity_hash_init(v, desc);<br>
> + r = verity_hash_init(v, req, &res);<br>
> if (unlikely(r < 0))<br>
> - return r;<br>
> + goto out;<br>
<br>
</span>why it is changed to goto? it doesn't simplify anything in this function<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div><br></div><div>I generally prefer for a function to have a single return point, if it does</div><div>not over complicates things. I find it makes code more readable and easier</div><div>to reason about - put debugging log statement for return for example.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><br>
> - r = verity_hash_update(v, desc, data, len);<br>
> + r = verity_hash_update(v, req, data, len, &res);<br>
> if (unlikely(r < 0))<br>
> - return r;<br>
> + goto out;<br>
> +<br>
> + r = verity_hash_final(v, req, digest, &res);<br>
><br>
> - return verity_hash_final(v, desc, digest);<br>
> +out:<br>
> + return r;<br>
> }<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888"></font></span></blockquote><div><br></div><div><br></div><div>I will post a new revision of the patch early next week .</div><div><br></div><div>Thanks,</div><div>Gilad</div></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Gilad Ben-Yossef<br>Chief Coffee Drinker<br><br>"If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?"<br> -- Jean-Baptiste Queru</div>
</div></div></div>