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

[Gilad Ben-Yossef]

On Fri, Feb 17, 2017 at 4:47 PM, Gilad Ben-Yossef  wrote:
> 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.

I found out what the problem was and why I haven't seen it  - I
thought crypto_ahash_init() only
sets up internal data structures and such and doesn't talk to the
device and therefore never
goes in the asynchronous completion path, which is true for the
hardware based provider
I was using for testing and probably for other HW based providers as well.

BUT, it isn't true for cryptd, which I guess uses the init function to
spawn kernel threads and does
use the asynchronous code path there.

Thanks for the review and testing. The next revision is coming up in a
jiffie. I'd be delighted if you
can take it out for a spin.

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

Re: [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  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(, 1);
>> + sg_set_buf(, data, len);
>
> why not use sg_init_one?

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

>
>> + ahash_request_set_crypt(req, , 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, );
>>   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, );
>>   if (unlikely(r < 0))
>> - return r;
>> + goto out;
>> +
>> + r = verity_hash_final(v, req, digest, );
>>
>> - 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

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

[Milan Broz]

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?

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.)


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(, 1);
> + sg_set_buf(, data, len);

why not use sg_init_one?

> + ahash_request_set_crypt(req, , 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, );
>   if (unlikely(r < 0))
> - return r;
> + goto out;

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

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


Milan

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

[Gilad Ben-Yossef]

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.

Signed-off-by: Gilad Ben-Yossef 
CC: Eric Biggers 
CC: Ondrej Mosnáček 
---

The patch was tested on an Armv7 based dual core Zynq ZC706 development
board with SHA256-asm, SHA256-neon synchronous providers with no visible
degradation of performance and with off tree Arm CryptoCell asynchronous
provider.

Changes from v1:

- Use a 0 mask to allocate crypto alg indicating we welcome async algo 
  providers, as suggested by Ondrej Mosnáček.
- Fix use of un-initialized completion when using async provider for IO
  blocks hashing
- Pass flag indicating we are OK with crypto provider backlog
- Re-factor checking for need to wait into a common function

 drivers/md/dm-verity-fec.c|   4 +-
 drivers/md/dm-verity-target.c | 202 +-
 drivers/md/dm-verity.h|  23 +++--
 3 files changed, 158 insertions(+), 71 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 0f0eb8a..dab98fe 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -188,7 +188,7 @@ static int fec_decode_bufs(struct dm_verity *v, struct 
dm_verity_fec_io *fio,
 static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
  u8 *want_digest, u8 *data)
 {
-   if (unlikely(verity_hash(v, verity_io_hash_desc(v, io),
+   if (unlikely(verity_hash(v, verity_io_hash_req(v, io),
 data, 1 << v->data_dev_block_bits,
 verity_io_real_digest(v, io
return 0;
@@ -397,7 +397,7 @@ static int fec_decode_rsb(struct dm_verity *v, struct 
dm_verity_io *io,
}
 
/* Always re-validate the corrected block against the expected hash */
-   r = verity_hash(v, verity_io_hash_desc(v, io), fio->output,
+   r = verity_hash(v, verity_io_hash_req(v, io), fio->output,
1 << v->data_dev_block_bits,
verity_io_real_digest(v, io));
if (unlikely(r < 0))
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 7335d8a..f0b24cc 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -93,81 +93,124 @@ static sector_t verity_position_at_level(struct dm_verity 
*v, sector_t block,
 }
 
 /*
- * Wrapper for crypto_shash_init, which handles verity salting.
+ * Callback function for asynchrnous crypto API completion notification
  */
-static int verity_hash_init(struct dm_verity *v, struct shash_desc *desc)
+static void verity_op_done(struct crypto_async_request *base, int err)
 {
-   int r;
+   struct verity_result *res = (struct verity_result *)base->data;
 
-   desc->tfm = v->tfm;
-   desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+   if (err == -EINPROGRESS)
+   return;
 
-   r = crypto_shash_init(desc);
+   res->err = err;
+   complete(>completion);
+}
 
-   if (unlikely(r < 0)) {
-   DMERR("crypto_shash_init failed: %d", r);
-   return r;
-   }
+/*
+ * Wait for async crypto API callback
+ */
+static inline int verity_complete_op(struct verity_result *res, int ret)
+{
+   switch (ret) {
+   case 0:
+   break;
 
-   if (likely(v->version >= 1)) {
-   r = crypto_shash_update(desc, v->salt, v->salt_size);
+   case -EINPROGRESS:
+   case -EBUSY:
+   ret = wait_for_completion_interruptible(>completion);
+   if (!ret)
+   ret = res->err;
+   reinit_completion(>completion);
+   break;
 
-   if (unlikely(r < 0)) {
-   DMERR("crypto_shash_update failed: %d", r);
-   return r;
-   }
+   default:
+   DMERR("verity_wait_hash: crypto op submission failed: %d", ret);
}
 
-   return 0;
+   if (unlikely(ret < 0))
+   DMERR("verity_wait_hash: crypto op failed: %d", ret);
+
+   return ret;
 }
 
-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