From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gilad Ben-Yossef Subject: Re: [PATCH v2] dm: switch dm-verity to async hash crypto API Date: Fri, 17 Feb 2017 16:47:11 +0200 Message-ID: References: <1486389482-26992-1-git-send-email-gilad@benyossef.com> <74bc1a00-6fb4-4b2f-b57b-a7fea0969eeb@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3511730223519518954==" Cc: Mike Snitzer , gilad.benyossef@arm.com, Eric Biggers , dm-devel@redhat.com, linux-crypto@vger.kernel.org, =?UTF-8?B?T25kcmVqIE1vc27DocSNZWs=?= , Alasdair Kergon , Ofir Drang To: Milan Broz Return-path: In-Reply-To: <74bc1a00-6fb4-4b2f-b57b-a7fea0969eeb@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com List-Id: linux-crypto.vger.kernel.org --===============3511730223519518954== Content-Type: multipart/alternative; boundary=001a113f26ba2b17500548bafd01 --001a113f26ba2b17500548bafd01 Content-Type: text/plain; charset=UTF-8 Hi Milan, Thank you for the review and testing. On Fri, Feb 17, 2017 at 3:00 PM, Milan Broz 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 --001a113f26ba2b17500548bafd01 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Milan,

Thank you for the review and = testing.

On Fr= i, 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 hardwa= re accelerator.
I did not try with cryptd though.

<= /div>
=C2=A0
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-fai= l.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 recrea= te the issue and fix it.

Some more comments below.
...

> -static int verity_hash_update(struct dm_verity *v, struct shash_desc = *desc,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0const u8 *data, size_t len)
> +static int verity_hash_update(struct dm_verity *v, struct ahash_reque= st *req,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const u8 *data, size_t len,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct verity_result *res)
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0int r =3D crypto_shash_update(desc, data, len); > +=C2=A0 =C2=A0 =C2=A0struct scatterlist sg;
>
> -=C2=A0 =C2=A0 =C2=A0if (unlikely(r < 0))
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DMERR("crypto_sh= ash_update failed: %d", r);
> +=C2=A0 =C2=A0 =C2=A0sg_init_table(&sg, 1);
> +=C2=A0 =C2=A0 =C2=A0sg_set_buf(&sg, data, len);

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



> +=C2=A0 =C2=A0 =C2=A0ahash_request_set_crypt(req, &sg, NULL, len);=
> +
> +=C2=A0 =C2=A0 =C2=A0return verity_complete_op(res, crypto_ahash_updat= e(req));
> +}

...

> -int verity_hash(struct dm_verity *v, struct shash_desc *desc,
> +int verity_hash(struct dm_verity *v, struct ahash_request *req,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const u8 *data, = size_t len, u8 *digest)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int r;
> +=C2=A0 =C2=A0 =C2=A0struct verity_result res;
>
> -=C2=A0 =C2=A0 =C2=A0r =3D verity_hash_init(v, desc);
> +=C2=A0 =C2=A0 =C2=A0r =3D verity_hash_init(v, req, &res);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (unlikely(r < 0))
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return r;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;

why it is changed to goto? it doesn't simplify anything in this = function

<= br>
I generally prefer for a function to have a single return poi= nt, if it does
not over complicates things. I find it makes code = more readable and easier
to reason about - put debugging log stat= ement for return for example.

=C2=A0
>
> -=C2=A0 =C2=A0 =C2=A0r =3D verity_hash_update(v, desc, data, len);
> +=C2=A0 =C2=A0 =C2=A0r =3D verity_hash_update(v, req, data, len, &= res);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (unlikely(r < 0))
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return r;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> +
> +=C2=A0 =C2=A0 =C2=A0r =3D verity_hash_final(v, req, digest, &res)= ;
>
> -=C2=A0 =C2=A0 =C2=A0return verity_hash_final(v, desc, digest);
> +out:
> +=C2=A0 =C2=A0 =C2=A0return r;
>=C2=A0 }



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

Thanks,
Gilad



--
Gilad Ben-YossefChief Coffee Drinker

"If you take a class in large-scale robot= ics, can you end up in a situation where the homework eats your dog?"<= br>=C2=A0-- Jean-Baptiste Queru
--001a113f26ba2b17500548bafd01-- --===============3511730223519518954== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============3511730223519518954==--