linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] dm: switch dm-verity to async hash crypto API
@ 2017-02-06 13:58 Gilad Ben-Yossef
  2017-02-17 13:00 ` Milan Broz
  0 siblings, 1 reply; 5+ messages in thread
From: Gilad Ben-Yossef @ 2017-02-06 13:58 UTC (permalink / raw)
  To: dm-devel, Alasdair Kergon, Mike Snitzer
  Cc: linux-crypto, gilad.benyossef, ofir.drang, Eric Biggers,
	Ondrej Mosnáček

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 <gilad@benyossef.com>
CC: Eric Biggers <ebiggers3@gmail.com>
CC: Ondrej Mosnáček <omosnacek+linux-crypto@gmail.com>
---

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(&res->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(&res->completion);
+		if (!ret)
+			ret = res->err;
+		reinit_completion(&res->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 failed: %d", r);
+	sg_init_table(&sg, 1);
+	sg_set_buf(&sg, data, len);
+	ahash_request_set_crypt(req, &sg, NULL, len);
+
+	return verity_complete_op(res, crypto_ahash_update(req));
+}
+
+/*
+ * Wrapper for crypto_ahash_init, which handles verity salting.
+ */
+static int verity_hash_init(struct dm_verity *v, struct ahash_request *req,
+				struct verity_result *res)
+{
+	int r;
+
+	ahash_request_set_tfm(req, v->tfm);
+	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP |
+					CRYPTO_TFM_REQ_MAY_BACKLOG,
+					verity_op_done, (void *)res);
+	init_completion(&res->completion);
+
+	r = crypto_ahash_init(req);
+
+	if (unlikely(r < 0)) {
+		DMERR("crypto_ahash_init failed: %d", r);
+		return r;
+	}
+
+	if (likely(v->version >= 1))
+		r = verity_hash_update(v, req, v->salt, v->salt_size, res);
 
 	return r;
 }
 
-static int verity_hash_final(struct dm_verity *v, struct shash_desc *desc,
-			     u8 *digest)
+static int verity_hash_final(struct dm_verity *v, struct ahash_request *req,
+			     u8 *digest, struct verity_result *res)
 {
 	int r;
 
 	if (unlikely(!v->version)) {
-		r = crypto_shash_update(desc, v->salt, v->salt_size);
+		r = verity_hash_update(v, req, v->salt, v->salt_size, res);
 
 		if (r < 0) {
-			DMERR("crypto_shash_update failed: %d", r);
-			return r;
+			DMERR("verity_hash_final failed updating salt: %d", r);
+			goto out;
 		}
 	}
 
-	r = crypto_shash_final(desc, digest);
-
-	if (unlikely(r < 0))
-		DMERR("crypto_shash_final failed: %d", r);
-
+	ahash_request_set_crypt(req, NULL, digest, 0);
+	r = verity_complete_op(res, crypto_ahash_final(req));
+out:
 	return r;
 }
 
-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;
 
-	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;
 }
 
 static void verity_hash_at_level(struct dm_verity *v, sector_t block, int level,
@@ -275,7 +318,7 @@ static int verity_verify_level(struct dm_verity *v, struct dm_verity_io *io,
 			goto release_ret_r;
 		}
 
-		r = verity_hash(v, verity_io_hash_desc(v, io),
+		r = verity_hash(v, verity_io_hash_req(v, io),
 				data, 1 << v->hash_dev_block_bits,
 				verity_io_real_digest(v, io));
 		if (unlikely(r < 0))
@@ -344,6 +387,49 @@ int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io,
 }
 
 /*
+ * Calculates the digest for the given bio
+ */
+int verity_for_io_block(struct dm_verity *v, struct dm_verity_io *io,
+			struct bvec_iter *iter, struct verity_result *res)
+{
+	unsigned int todo = 1 << v->data_dev_block_bits;
+	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
+	struct scatterlist sg;
+	struct ahash_request *req = verity_io_hash_req(v, io);
+
+	do {
+		int r;
+		unsigned int len;
+		struct bio_vec bv = bio_iter_iovec(bio, *iter);
+
+		sg_init_table(&sg, 1);
+
+		len = bv.bv_len;
+
+		if (likely(len >= todo))
+			len = todo;
+		/*
+		 * Operating on a single page at a time looks suboptimal
+		 * until you consider the typical block size is 4,096B.
+		 * Going through this loops twice should be very rare.
+		 */
+		sg_set_page(&sg, bv.bv_page, len, bv.bv_offset);
+		ahash_request_set_crypt(req, &sg, NULL, len);
+		r = verity_complete_op(res, crypto_ahash_update(req));
+
+		if (unlikely(r < 0)) {
+			DMERR("verity_for_io_block crypto op failed: %d", r);
+			return r;
+		}
+
+		bio_advance_iter(bio, iter, len);
+		todo -= len;
+	} while (todo);
+
+	return 0;
+}
+
+/*
  * Calls function process for 1 << v->data_dev_block_bits bytes in the bio_vec
  * starting from iter.
  */
@@ -381,12 +467,6 @@ int verity_for_bv_block(struct dm_verity *v, struct dm_verity_io *io,
 	return 0;
 }
 
-static int verity_bv_hash_update(struct dm_verity *v, struct dm_verity_io *io,
-				 u8 *data, size_t len)
-{
-	return verity_hash_update(v, verity_io_hash_desc(v, io), data, len);
-}
-
 static int verity_bv_zero(struct dm_verity *v, struct dm_verity_io *io,
 			  u8 *data, size_t len)
 {
@@ -403,10 +483,11 @@ static int verity_verify_io(struct dm_verity_io *io)
 	struct dm_verity *v = io->v;
 	struct bvec_iter start;
 	unsigned b;
+	struct verity_result res;
 
 	for (b = 0; b < io->n_blocks; b++) {
 		int r;
-		struct shash_desc *desc = verity_io_hash_desc(v, io);
+		struct ahash_request *req = verity_io_hash_req(v, io);
 
 		r = verity_hash_for_block(v, io, io->block + b,
 					  verity_io_want_digest(v, io),
@@ -427,16 +508,17 @@ static int verity_verify_io(struct dm_verity_io *io)
 			continue;
 		}
 
-		r = verity_hash_init(v, desc);
+		r = verity_hash_init(v, req, &res);
 		if (unlikely(r < 0))
 			return r;
 
 		start = io->iter;
-		r = verity_for_bv_block(v, io, &io->iter, verity_bv_hash_update);
+		r = verity_for_io_block(v, io, &io->iter, &res);
 		if (unlikely(r < 0))
 			return r;
 
-		r = verity_hash_final(v, desc, verity_io_real_digest(v, io));
+		r = verity_hash_final(v, req, verity_io_real_digest(v, io),
+					&res);
 		if (unlikely(r < 0))
 			return r;
 
@@ -705,7 +787,7 @@ static void verity_dtr(struct dm_target *ti)
 	kfree(v->zero_digest);
 
 	if (v->tfm)
-		crypto_free_shash(v->tfm);
+		crypto_free_ahash(v->tfm);
 
 	kfree(v->alg_name);
 
@@ -723,7 +805,7 @@ static void verity_dtr(struct dm_target *ti)
 static int verity_alloc_zero_digest(struct dm_verity *v)
 {
 	int r = -ENOMEM;
-	struct shash_desc *desc;
+	struct ahash_request *req;
 	u8 *zero_data;
 
 	v->zero_digest = kmalloc(v->digest_size, GFP_KERNEL);
@@ -731,9 +813,9 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
 	if (!v->zero_digest)
 		return r;
 
-	desc = kmalloc(v->shash_descsize, GFP_KERNEL);
+	req = kmalloc(v->ahash_reqsize, GFP_KERNEL);
 
-	if (!desc)
+	if (!req)
 		return r; /* verity_dtr will free zero_digest */
 
 	zero_data = kzalloc(1 << v->data_dev_block_bits, GFP_KERNEL);
@@ -741,11 +823,11 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
 	if (!zero_data)
 		goto out;
 
-	r = verity_hash(v, desc, zero_data, 1 << v->data_dev_block_bits,
+	r = verity_hash(v, req, zero_data, 1 << v->data_dev_block_bits,
 			v->zero_digest);
 
 out:
-	kfree(desc);
+	kfree(req);
 	kfree(zero_data);
 
 	return r;
@@ -923,21 +1005,21 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
-	v->tfm = crypto_alloc_shash(v->alg_name, 0, 0);
+	v->tfm = crypto_alloc_ahash(v->alg_name, 0, 0);
 	if (IS_ERR(v->tfm)) {
 		ti->error = "Cannot initialize hash function";
 		r = PTR_ERR(v->tfm);
 		v->tfm = NULL;
 		goto bad;
 	}
-	v->digest_size = crypto_shash_digestsize(v->tfm);
+	v->digest_size = crypto_ahash_digestsize(v->tfm);
 	if ((1 << v->hash_dev_block_bits) < v->digest_size * 2) {
 		ti->error = "Digest size too big";
 		r = -EINVAL;
 		goto bad;
 	}
-	v->shash_descsize =
-		sizeof(struct shash_desc) + crypto_shash_descsize(v->tfm);
+	v->ahash_reqsize = sizeof(struct ahash_request) +
+		crypto_ahash_reqsize(v->tfm);
 
 	v->root_digest = kmalloc(v->digest_size, GFP_KERNEL);
 	if (!v->root_digest) {
@@ -1037,7 +1119,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	}
 
 	ti->per_io_data_size = sizeof(struct dm_verity_io) +
-				v->shash_descsize + v->digest_size * 2;
+				v->ahash_reqsize + v->digest_size * 2;
 
 	r = verity_fec_ctr(v);
 	if (r)
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index fb419f4..a59e0ad 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -37,7 +37,7 @@ struct dm_verity {
 	struct dm_target *ti;
 	struct dm_bufio_client *bufio;
 	char *alg_name;
-	struct crypto_shash *tfm;
+	struct crypto_ahash *tfm;
 	u8 *root_digest;	/* digest of the root block */
 	u8 *salt;		/* salt: its size is salt_size */
 	u8 *zero_digest;	/* digest for a zero block */
@@ -52,7 +52,7 @@ struct dm_verity {
 	unsigned char levels;	/* the number of tree levels */
 	unsigned char version;
 	unsigned digest_size;	/* digest size for the current hash algorithm */
-	unsigned shash_descsize;/* the size of temporary space for crypto */
+	unsigned int ahash_reqsize;/* the size of temporary space for crypto */
 	int hash_failed;	/* set to 1 if hash of any block failed */
 	enum verity_mode mode;	/* mode for handling verification errors */
 	unsigned corrupted_errs;/* Number of errors for corrupted blocks */
@@ -81,31 +81,36 @@ struct dm_verity_io {
 	/*
 	 * Three variably-size fields follow this struct:
 	 *
-	 * u8 hash_desc[v->shash_descsize];
+	 * u8 hash_req[v->ahash_reqsize];
 	 * u8 real_digest[v->digest_size];
 	 * u8 want_digest[v->digest_size];
 	 *
-	 * To access them use: verity_io_hash_desc(), verity_io_real_digest()
+	 * To access them use: verity_io_hash_req(), verity_io_real_digest()
 	 * and verity_io_want_digest().
 	 */
 };
 
-static inline struct shash_desc *verity_io_hash_desc(struct dm_verity *v,
+struct verity_result {
+	struct completion completion;
+	int err;
+};
+
+static inline struct ahash_request *verity_io_hash_req(struct dm_verity *v,
 						     struct dm_verity_io *io)
 {
-	return (struct shash_desc *)(io + 1);
+	return (struct ahash_request *)(io + 1);
 }
 
 static inline u8 *verity_io_real_digest(struct dm_verity *v,
 					struct dm_verity_io *io)
 {
-	return (u8 *)(io + 1) + v->shash_descsize;
+	return (u8 *)(io + 1) + v->ahash_reqsize;
 }
 
 static inline u8 *verity_io_want_digest(struct dm_verity *v,
 					struct dm_verity_io *io)
 {
-	return (u8 *)(io + 1) + v->shash_descsize + v->digest_size;
+	return (u8 *)(io + 1) + v->ahash_reqsize + v->digest_size;
 }
 
 static inline u8 *verity_io_digest_end(struct dm_verity *v,
@@ -120,7 +125,7 @@ extern int verity_for_bv_block(struct dm_verity *v, struct dm_verity_io *io,
 					      struct dm_verity_io *io,
 					      u8 *data, size_t len));
 
-extern int verity_hash(struct dm_verity *v, struct shash_desc *desc,
+extern int verity_hash(struct dm_verity *v, struct ahash_request *req,
 		       const u8 *data, size_t len, u8 *digest);
 
 extern int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io,
-- 
2.1.4

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] dm: switch dm-verity to async hash crypto API
  2017-02-06 13:58 [PATCH v2] dm: switch dm-verity to async hash crypto API Gilad Ben-Yossef
@ 2017-02-17 13:00 ` Milan Broz
  2017-02-17 14:47   ` Gilad Ben-Yossef
  2017-02-17 14:52   ` Gilad Ben-Yossef
  0 siblings, 2 replies; 5+ messages in thread
From: Milan Broz @ 2017-02-17 13:00 UTC (permalink / raw)
  To: Gilad Ben-Yossef, dm-devel, Alasdair Kergon, Mike Snitzer
  Cc: linux-crypto, gilad.benyossef, ofir.drang, Eric Biggers,
	Ondrej Mosnáček

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

why not use sg_init_one?

> +	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

>  
> -	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;
>  }


Milan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] dm: switch dm-verity to async hash crypto API
  2017-02-17 13:00 ` Milan Broz
@ 2017-02-17 14:47   ` Gilad Ben-Yossef
  2017-02-19 12:37     ` Gilad Ben-Yossef
  2017-02-17 14:52   ` Gilad Ben-Yossef
  1 sibling, 1 reply; 5+ messages in thread
From: Gilad Ben-Yossef @ 2017-02-17 14:47 UTC (permalink / raw)
  To: Milan Broz
  Cc: Mike Snitzer, gilad.benyossef, Eric Biggers, dm-devel,
	linux-crypto, Ondrej Mosnáček, Alasdair Kergon,
	Ofir Drang


[-- Attachment #1.1: Type: text/plain, Size: 3492 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 5641 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] dm: switch dm-verity to async hash crypto API
  2017-02-17 13:00 ` Milan Broz
  2017-02-17 14:47   ` Gilad Ben-Yossef
@ 2017-02-17 14:52   ` Gilad Ben-Yossef
  1 sibling, 0 replies; 5+ messages in thread
From: Gilad Ben-Yossef @ 2017-02-17 14:52 UTC (permalink / raw)
  To: Milan Broz
  Cc: dm-devel, Alasdair Kergon, Mike Snitzer, linux-crypto,
	gilad.benyossef, Ofir Drang, Eric Biggers,
	Ondrej Mosnáček

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] dm: switch dm-verity to async hash crypto API
  2017-02-17 14:47   ` Gilad Ben-Yossef
@ 2017-02-19 12:37     ` Gilad Ben-Yossef
  0 siblings, 0 replies; 5+ messages in thread
From: Gilad Ben-Yossef @ 2017-02-19 12:37 UTC (permalink / raw)
  To: Milan Broz
  Cc: dm-devel, Alasdair Kergon, Mike Snitzer, linux-crypto,
	gilad.benyossef, Ofir Drang, Eric Biggers,
	Ondrej Mosnáček

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-02-19 12:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-06 13:58 [PATCH v2] dm: switch dm-verity to async hash crypto API Gilad Ben-Yossef
2017-02-17 13:00 ` Milan Broz
2017-02-17 14:47   ` Gilad Ben-Yossef
2017-02-19 12:37     ` Gilad Ben-Yossef
2017-02-17 14:52   ` Gilad Ben-Yossef

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).