Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1283073 > unrolled thread

[PATCH v2 0/2] dm verity: add support for error correction

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/2] dm verity: add support for error correction Sami Tolvanen <samitolvanen@google.com> - 2015-12-03 15:30 +0100
    [PATCH v2 2/2] dm verity: ignore zero blocks Sami Tolvanen <samitolvanen@google.com> - 2015-12-03 15:30 +0100
    Re: [PATCH v2 0/2] dm verity: add support for error correction Mike Snitzer <snitzer@redhat.com> - 2015-12-03 21:00 +0100
      Re: [PATCH v2 0/2] dm verity: add support for error correction Mike Snitzer <snitzer@redhat.com> - 2015-12-04 00:10 +0100
        Re: [PATCH v2 0/2] dm verity: add support for error correction Sami Tolvanen <samitolvanen@google.com> - 2015-12-04 11:10 +0100
          Re: [PATCH v2 0/2] dm verity: add support for error correction Mike Snitzer <snitzer@redhat.com> - 2015-12-04 22:10 +0100
            Re: [PATCH v2 0/2] dm verity: add support for error correction Sami Tolvanen <samitolvanen@google.com> - 2015-12-07 14:30 +0100
              Re: [PATCH v2 0/2] dm verity: add support for error correction Mike Snitzer <snitzer@redhat.com> - 2015-12-07 17:10 +0100
                Re: [PATCH v2 0/2] dm verity: add support for error correction Sami Tolvanen <samitolvanen@google.com> - 2015-12-07 17:40 +0100
                  Re: [PATCH v2 0/2] dm verity: add support for error correction Milan Broz <mbroz@redhat.com> - 2015-12-07 19:10 +0100
                    Re: [PATCH v2 0/2] dm verity: add support for error correction Mike Snitzer <snitzer@redhat.com> - 2015-12-07 20:10 +0100
                      Re: [PATCH v2 0/2] dm verity: add support for error correction Sami Tolvanen <samitolvanen@google.com> - 2015-12-08 11:20 +0100

#1283073 — [PATCH v2 0/2] dm verity: add support for error correction

Changes since v1:

  - Added CONFIG_DM_VERITY_FEC and split error correction into
    dm-verity-fec.[ch] to further separate the functionality from the
    rest of dm-verity. Follows the same pattern as dm-uevent.

  - Added missing dependencies for REED_SOLOMON to Kconfig.

  - Renamed dm-verity.c to dm-verity-target.c to allow an optional
    object to be added. Follows the naming convention of dm-cache and
    dm-era.

  - Changed the algorithm to work with one or more small buffers (~4k)
    instead of a single large one. The more buffers we can allocate,
    the faster it will work, but we don't have to preallocate a large
    amount of memory anymore.

  - Changed memory allocation to use mempools. v2 preallocates all the
    memory required for each worker thread to guarantee forward
    progress in case of memory pressure. The code attempts to allocate
    more buffers (using GFP_NOIO) and uses them if available.

  - Added graceful handling of IO errors, which are now treated as any
    other corruption.

  - Rebased against linux-dm/for-next.

Sami Tolvanen (2):
  dm verity: add support for forward error correction
  dm verity: ignore zero blocks

 Documentation/device-mapper/verity.txt         |  30 +
 drivers/md/Kconfig                             |  12 +
 drivers/md/Makefile                            |   5 +
 drivers/md/dm-verity-fec.c                     | 836 +++++++++++++++++++++++++
 drivers/md/dm-verity-fec.h                     | 146 +++++
 drivers/md/{dm-verity.c => dm-verity-target.c} | 320 ++++++----
 drivers/md/dm-verity.h                         | 129 ++++
 7 files changed, 1343 insertions(+), 135 deletions(-)
 create mode 100644 drivers/md/dm-verity-fec.c
 create mode 100644 drivers/md/dm-verity-fec.h
 rename drivers/md/{dm-verity.c => dm-verity-target.c} (82%)
 create mode 100644 drivers/md/dm-verity.h

-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1283074 — [PATCH v2 2/2] dm verity: ignore zero blocks

Add ignore_zero_blocks option, which returns zeros for blocks matching a
zero hash without validating the content.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 Documentation/device-mapper/verity.txt |  5 ++
 drivers/md/dm-verity-fec.c             |  8 +++-
 drivers/md/dm-verity-target.c          | 84 ++++++++++++++++++++++++++++++----
 drivers/md/dm-verity.h                 |  3 +-
 4 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index 1058f36..4d3bbda 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -79,6 +79,11 @@ restart_on_corruption
     not compatible with ignore_corruption and requires user space support to
     avoid restart loops.
 
+ignore_zero_blocks
+    Do not verify blocks that are expected to contain zeros and always return
+    zeros instead. This may be useful if the partition contains unused blocks
+    that are not guaranteed to contain zeros.
+
 use_fec_from_device
     Use forward error correction (FEC) to recover from corruption if hash
     verification fails. Use encoding data from the specified device. This
diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index c3a2531..599334f 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -210,6 +210,7 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			 u64 rsb, u64 target, unsigned block_offset,
 			 int *neras)
 {
+	bool is_zero;
 	int i, j, target_index = -1;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
@@ -270,7 +271,12 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 		/* locate erasures if the block is on the data device */
 		if (bufio == v->fec->data_bufio &&
-		    verity_hash_for_block(v, io, block, want_digest) == 0) {
+		    verity_hash_for_block(v, io, block, want_digest,
+					  &is_zero) == 0) {
+			/* skip known zero blocks entirely */
+			if (is_zero)
+				continue;
+
 			/*
 			 * skip if we have already found the theoretical
 			 * maximum number (i.e. fec->roots) of erasures
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index ca5857b..a2bb3bc 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -31,8 +31,9 @@
 
 #define DM_VERITY_OPT_LOGGING		"ignore_corruption"
 #define DM_VERITY_OPT_RESTART		"restart_on_corruption"
+#define DM_VERITY_OPT_IGN_ZEROS		"ignore_zero_blocks"
 
-#define DM_VERITY_OPTS_MAX		(1 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC)
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
@@ -309,10 +310,9 @@ release_ret_r:
  * of the hash tree if necessary.
  */
 int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io,
-			  sector_t block, u8 *digest)
+			  sector_t block, u8 *digest, bool *is_zero)
 {
-	int i;
-	int r;
+	int r = 0, i;
 
 	if (likely(v->levels)) {
 		/*
@@ -324,7 +324,7 @@ int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io,
 		 */
 		r = verity_verify_level(v, io, block, 0, true, digest);
 		if (likely(r <= 0))
-			return r;
+			goto out;
 	}
 
 	memcpy(digest, v->root_digest, v->digest_size);
@@ -332,10 +332,16 @@ int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io,
 	for (i = v->levels - 1; i >= 0; i--) {
 		r = verity_verify_level(v, io, block, i, false, digest);
 		if (unlikely(r))
-			return r;
+			goto out;
 	}
 
-	return 0;
+out:
+	if (!r && v->zero_digest)
+		*is_zero = !memcmp(v->zero_digest, digest, v->digest_size);
+	else
+		*is_zero = false;
+
+	return r;
 }
 
 /*
@@ -383,11 +389,19 @@ static int verity_bv_hash_update(struct dm_verity *v, struct dm_verity_io *io,
 	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)
+{
+	memset(data, 0, len);
+	return 0;
+}
+
 /*
  * Verify one "dm_verity_io" structure.
  */
 static int verity_verify_io(struct dm_verity_io *io)
 {
+	bool is_zero;
 	struct dm_verity *v = io->v;
 	struct bvec_iter start;
 	unsigned b;
@@ -397,10 +411,24 @@ static int verity_verify_io(struct dm_verity_io *io)
 		struct shash_desc *desc = verity_io_hash_desc(v, io);
 
 		r = verity_hash_for_block(v, io, io->block + b,
-					  verity_io_want_digest(v, io));
+					  verity_io_want_digest(v, io),
+					  &is_zero);
 		if (unlikely(r < 0))
 			return r;
 
+		if (is_zero) {
+			/*
+			 * If we expect a zero block, don't validate, just
+			 * return zeros.
+			 */
+			r = verity_for_bv_block(v, io, &io->iter,
+						verity_bv_zero);
+			if (unlikely(r < 0))
+				return r;
+
+			continue;
+		}
+
 		r = verity_hash_init(v, desc);
 		if (unlikely(r < 0))
 			return r;
@@ -607,6 +635,8 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 			args++;
 		if (verity_fec_is_enabled(v))
 			args += DM_VERITY_OPTS_FEC;
+		if (v->zero_digest)
+			args++;
 		if (!args)
 			return;
 		DMEMIT(" %u", args);
@@ -623,6 +653,8 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 				BUG();
 			}
 		}
+		if (v->zero_digest)
+			DMEMIT(" " DM_VERITY_OPT_IGN_ZEROS);
 		sz = verity_fec_status_table(v, sz, result, maxlen);
 		break;
 	}
@@ -674,6 +706,7 @@ static void verity_dtr(struct dm_target *ti)
 
 	kfree(v->salt);
 	kfree(v->root_digest);
+	kfree(v->zero_digest);
 
 	if (v->tfm)
 		crypto_free_shash(v->tfm);
@@ -691,6 +724,37 @@ static void verity_dtr(struct dm_target *ti)
 	kfree(v);
 }
 
+static int verity_alloc_zero_digest(struct dm_verity *v)
+{
+	int r = -ENOMEM;
+	struct shash_desc *desc;
+	u8 *zero_data;
+
+	v->zero_digest = kmalloc(v->digest_size, GFP_KERNEL);
+
+	if (!v->zero_digest)
+		return r;
+
+	desc = kmalloc(v->shash_descsize, GFP_KERNEL);
+
+	if (!desc)
+		return r; /* verity_dtr will free zero_digest */
+
+	zero_data = kzalloc(1 << v->data_dev_block_bits, GFP_KERNEL);
+
+	if (!zero_data)
+		goto out;
+
+	r = verity_hash(v, desc, zero_data, 1 << v->data_dev_block_bits,
+			v->zero_digest);
+
+out:
+	kfree(desc);
+	kfree(zero_data);
+
+	return r;
+}
+
 static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 {
 	int r;
@@ -720,6 +784,10 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART)) {
 			v->mode = DM_VERITY_MODE_RESTART;
 			continue;
+		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_IGN_ZEROS)) {
+			r = verity_alloc_zero_digest(v);
+			if (r)
+				ti->error = "Cannot allocate zero digest";
 		} else {
 			r = verity_fec_parse_opt_args(as, v, &argc, arg_name);
 		}
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index 8e85372..fb419f4 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -40,6 +40,7 @@ struct dm_verity {
 	struct crypto_shash *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 */
 	unsigned salt_size;
 	sector_t data_start;	/* data offset in 512-byte sectors */
 	sector_t hash_start;	/* hash start in blocks */
@@ -123,6 +124,6 @@ extern int verity_hash(struct dm_verity *v, struct shash_desc *desc,
 		       const u8 *data, size_t len, u8 *digest);
 
 extern int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io,
-				 sector_t block, u8 *digest);
+				 sector_t block, u8 *digest, bool *is_zero);
 
 #endif /* DM_VERITY_H */
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283298

On Thu, Dec 03 2015 at  9:26am -0500,
Sami Tolvanen <samitolvanen@google.com> wrote:

> Changes since v1:
> 
>   - Added CONFIG_DM_VERITY_FEC and split error correction into
>     dm-verity-fec.[ch] to further separate the functionality from the
>     rest of dm-verity. Follows the same pattern as dm-uevent.
> 
>   - Added missing dependencies for REED_SOLOMON to Kconfig.
> 
>   - Renamed dm-verity.c to dm-verity-target.c to allow an optional
>     object to be added. Follows the naming convention of dm-cache and
>     dm-era.
> 
>   - Changed the algorithm to work with one or more small buffers (~4k)
>     instead of a single large one. The more buffers we can allocate,
>     the faster it will work, but we don't have to preallocate a large
>     amount of memory anymore.
> 
>   - Changed memory allocation to use mempools. v2 preallocates all the
>     memory required for each worker thread to guarantee forward
>     progress in case of memory pressure. The code attempts to allocate
>     more buffers (using GFP_NOIO) and uses them if available.
> 
>   - Added graceful handling of IO errors, which are now treated as any
>     other corruption.
> 
>   - Rebased against linux-dm/for-next.

Thanks a lot for these advances, at a high-level it sounds like you've
handled the issues raised as part of v1 review very well.

I'll review closer now.  Goal is to get these changes staged in
linux-next for upstream inclusion during the 4.5 merge window.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283442

On Thu, Dec 03 2015 at  2:54pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Dec 03 2015 at  9:26am -0500,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> 
> > Changes since v1:
> > 
> >   - Added CONFIG_DM_VERITY_FEC and split error correction into
> >     dm-verity-fec.[ch] to further separate the functionality from the
> >     rest of dm-verity. Follows the same pattern as dm-uevent.
> > 
> >   - Added missing dependencies for REED_SOLOMON to Kconfig.
> > 
> >   - Renamed dm-verity.c to dm-verity-target.c to allow an optional
> >     object to be added. Follows the naming convention of dm-cache and
> >     dm-era.
> > 
> >   - Changed the algorithm to work with one or more small buffers (~4k)
> >     instead of a single large one. The more buffers we can allocate,
> >     the faster it will work, but we don't have to preallocate a large
> >     amount of memory anymore.
> > 
> >   - Changed memory allocation to use mempools. v2 preallocates all the
> >     memory required for each worker thread to guarantee forward
> >     progress in case of memory pressure. The code attempts to allocate
> >     more buffers (using GFP_NOIO) and uses them if available.
> > 
> >   - Added graceful handling of IO errors, which are now treated as any
> >     other corruption.
> > 
> >   - Rebased against linux-dm/for-next.
> 
> Thanks a lot for these advances, at a high-level it sounds like you've
> handled the issues raised as part of v1 review very well.
> 
> I'll review closer now.  Goal is to get these changes staged in
> linux-next for upstream inclusion during the 4.5 merge window.

I took a first pass through your code and pushed the result to this
temporary branch here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=dm-verity-fec

As you'll see, I focused primarily on splitting the dm-verity core
changes out from the FEC and ignore_zero_blocks changes.

Along the way I tweaked some style-nits here or there but the biggest
difference between your v2 and what I pushed is captured in this commit:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=dm-verity-fec&id=cc1339fae225d8cdc8fffeff30f0a9981367d6c0

If you're OK with those changes I'll fold that commit into your main FEC
commit.

I'll carry on with reviewing your new code (outside of dm-verity core)
tomorrow.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283686

On Thu, Dec 03, 2015 at 06:05:38PM -0500, Mike Snitzer wrote:
> If you're OK with those changes I'll fold that commit into your main FEC
> commit.

Yes, these changes look fine. Thanks!

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1284233

On Fri, Dec 04 2015 at  5:03P -0500,
Sami Tolvanen <samitolvanen@google.com> wrote:

> On Thu, Dec 03, 2015 at 06:05:38PM -0500, Mike Snitzer wrote:
> > If you're OK with those changes I'll fold that commit into your main FEC
> > commit.
> 
> Yes, these changes look fine. Thanks!

OK, I reviewed the FEC code and have staged it in linux-next with the
following fixes/tweaks/nits folded in.  Please let me know if you see
any problems with this (I'm obviously not an 80-columns zealot).

I'm going to carry on reviewing aspects of memory allocations, table
loads via ctr, etc.. but the code I've staged in linux-dm.git's
'for-next' is worth handing off to linux-next to see if it catches
anything, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

I'd really appreciate it if you could do some regression testing, etc on
your end to verify I didn't break anything while tweaking things.

Thanks!

From: Mike Snitzer <snitzer@redhat.com>
Date: Fri, 4 Dec 2015 15:49:58 -0500
Subject: [PATCH] dm verity fec: whitespace fixes and other small nits

---
 drivers/md/dm-verity-fec.c | 153 +++++++++++++++++++--------------------------
 1 file changed, 65 insertions(+), 88 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index a40ac01..e722ce5 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -69,18 +69,15 @@ static u8 *fec_read_parity(struct dm_verity *v, u64 rsb, int index,
 
 	position = (index + rsb) * v->fec->roots;
 	block = position >> v->data_dev_block_bits;
-
 	*offset = (unsigned)(position - (block << v->data_dev_block_bits));
 
 	res = dm_bufio_read(v->fec->bufio, v->fec->start + block, buf);
-
 	if (unlikely(IS_ERR(res))) {
 		DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
 		      v->data_dev->name, (unsigned long long)rsb,
 		      (unsigned long long)(v->fec->start + block),
 		      PTR_ERR(res));
 		*buf = NULL;
-		return NULL;
 	}
 
 	return res;
@@ -92,8 +89,7 @@ static u8 *fec_read_parity(struct dm_verity *v, u64 rsb, int index,
 
 /* Loop over each extra buffer slot. */
 #define fec_for_each_extra_buffer(io, __i) \
-	for (__i = DM_VERITY_FEC_BUF_PREALLOC; __i < DM_VERITY_FEC_BUF_MAX; \
-		__i++)
+	for (__i = DM_VERITY_FEC_BUF_PREALLOC; __i < DM_VERITY_FEC_BUF_MAX; __i++)
 
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
@@ -132,14 +128,14 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio,
 			   u64 rsb, int byte_index, unsigned block_offset,
 			   int neras)
 {
-	int r = -1, corrected = 0, res;
+	int r, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned n, i, offset;
 	u8 *par, *block;
 
 	par = fec_read_parity(v, rsb, block_offset, &offset, &buf);
-	if (unlikely(!par))
-		return r;
+	if (IS_ERR(par))
+		return PTR_ERR(par);
 
 	/*
 	 * Decode the RS blocks we have in bufs. Each RS block results in
@@ -148,9 +144,12 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio,
 	fec_for_each_buffer_rs_block(fio, n, i) {
 		block = fec_buffer_rs_block(v, fio, n, i);
 		res = fec_decode_rs8(v, fio, block, &par[offset], neras);
+		if (res < 0) {
+			dm_bufio_release(buf);
 
-		if (res < 0)
+			r = res;
 			goto error;
+		}
 
 		corrected += res;
 		fio->output[block_offset] = block[byte_index];
@@ -164,25 +163,20 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio,
 		if (offset >= 1 << v->data_dev_block_bits) {
 			dm_bufio_release(buf);
 
-			par = fec_read_parity(v, rsb, block_offset, &offset,
-					      &buf);
-			if (unlikely(!par))
-				return r;
+			par = fec_read_parity(v, rsb, block_offset, &offset, &buf);
+			if (unlikely(IS_ERR(par)))
+				return PTR_ERR(par);
 		}
 	}
-
 done:
 	r = corrected;
 error:
-	dm_bufio_release(buf);
-
 	if (r < 0 && neras)
 		DMERR_LIMIT("%s: FEC %llu: failed to correct: %d",
 			    v->data_dev->name, (unsigned long long)rsb, r);
 	else if (r > 0)
 		DMWARN_LIMIT("%s: FEC %llu: corrected %d errors",
-			     v->data_dev->name, (unsigned long long)rsb,
-			     r);
+			     v->data_dev->name, (unsigned long long)rsb, r);
 
 	return r;
 }
@@ -204,7 +198,7 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 
 /*
  * Read data blocks that are part of the RS block and deinterleave as much as
- * fits into buffers. Check for erasure locations if neras is non-NULL.
+ * fits into buffers. Check for erasure locations if @neras is non-NULL.
  */
 static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			 u64 rsb, u64 target, unsigned block_offset,
@@ -299,7 +293,6 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			rs_block = fec_buffer_rs_block(v, fio, n, j);
 			rs_block[i] = bbuf[k];
 		}
-
 done:
 		dm_bufio_release(buf);
 	}
@@ -317,7 +310,6 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 
 	if (!fio->rs) {
 		fio->rs = mempool_alloc(v->fec->rs_pool, 0);
-
 		if (unlikely(!fio->rs)) {
 			DMERR("failed to allocate RS");
 			return -ENOMEM;
@@ -329,7 +321,6 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 			continue;
 
 		fio->bufs[n] = mempool_alloc(v->fec->prealloc_pool, GFP_NOIO);
-
 		if (unlikely(!fio->bufs[n])) {
 			DMERR("failed to allocate FEC buffer");
 			return -ENOMEM;
@@ -342,12 +333,10 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 			continue;
 
 		fio->bufs[n] = mempool_alloc(v->fec->extra_pool, GFP_NOIO);
-
 		/* we can manage with even one buffer if necessary */
 		if (unlikely(!fio->bufs[n]))
 			break;
 	}
-
 	fio->nbufs = n;
 
 	if (!fio->output) {
@@ -363,7 +352,7 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 }
 
 /*
- * Initialize buffers and clear erasures. fec_read_bufs assumes buffers are
+ * Initialize buffers and clear erasures. fec_read_bufs() assumes buffers are
  * zeroed before deinterleaving.
  */
 static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
@@ -371,27 +360,26 @@ static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 	unsigned n;
 
 	fec_for_each_buffer(fio, n)
-		memset(fio->bufs[n], 0,
-		       v->fec->rsn << DM_VERITY_FEC_BUF_RS_BITS);
+		memset(fio->bufs[n], 0, v->fec->rsn << DM_VERITY_FEC_BUF_RS_BITS);
 
 	memset(fio->erasures, 0, sizeof(fio->erasures));
 }
 
 /*
  * Decode all RS blocks in a single data block and return the target block
- * (indicated by "offset") in fio->output. If use_erasures is non-zero, uses
+ * (indicated by @offset) in fio->output. If @use_erasures is non-zero, uses
  * hashes to locate erasures.
  */
 static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 			  struct dm_verity_fec_io *fio, u64 rsb, u64 offset,
-			  int use_erasures)
+			  bool use_erasures)
 {
 	int r, neras = 0;
 	unsigned pos;
 
 	r = fec_alloc_bufs(v, fio);
 	if (unlikely(r < 0))
-		return -1;
+		return r;
 
 	for (pos = 0; pos < 1 << v->data_dev_block_bits; ) {
 		fec_init_bufs(v, fio);
@@ -418,9 +406,8 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 	if (memcmp(verity_io_real_digest(v, io), verity_io_want_digest(v, io),
 		   v->digest_size)) {
 		DMERR_LIMIT("%s: FEC %llu: failed to correct (%d erasures)",
-			    v->data_dev->name, (unsigned long long)rsb,
-			    neras);
-		return -1;
+			    v->data_dev->name, (unsigned long long)rsb, neras);
+		return -EILSEQ;
 	}
 
 	return 0;
@@ -445,12 +432,12 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		      enum verity_block_type type, sector_t block, u8 *dest,
 		      struct bvec_iter *iter)
 {
-	int r = -1;
+	int r;
 	struct dm_verity_fec_io *fio = fec_io(io);
 	u64 offset, res, rsb;
 
 	if (!verity_fec_is_enabled(v))
-		return -1;
+		return -EOPNOTSUPP;
 
 	if (type == DM_VERITY_BLOCK_TYPE_METADATA)
 		block += v->data_blocks;
@@ -481,12 +468,12 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 	 * them first. Do a second attempt with erasures if the corruption is
 	 * bad enough.
 	 */
-	r = fec_decode_rsb(v, io, fio, rsb, offset, 0);
-	if (r < 0)
-		r = fec_decode_rsb(v, io, fio, rsb, offset, 1);
-
-	if (r < 0)
-		return r;
+	r = fec_decode_rsb(v, io, fio, rsb, offset, false);
+	if (r < 0) {
+		r = fec_decode_rsb(v, io, fio, rsb, offset, true);
+		if (r < 0)
+			return r;
+	}
 
 	if (dest)
 		memcpy(dest, fio->output, 1 << v->data_dev_block_bits);
@@ -577,7 +564,6 @@ void verity_fec_dtr(struct dm_verity *v)
 
 	if (f->dev)
 		dm_put_device(v->ti, f->dev);
-
 out:
 	kfree(f);
 	v->fec = NULL;
@@ -610,13 +596,14 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 			      unsigned *argc, const char *arg_name)
 {
 	int r;
+	struct dm_target *ti = v->ti;
 	const char *arg_value;
 	unsigned long long num_ll;
 	unsigned char num_c;
 	char dummy;
 
 	if (!*argc) {
-		v->ti->error = "FEC feature arguments require a value";
+		ti->error = "FEC feature arguments require a value";
 		return -EINVAL;
 	}
 
@@ -624,9 +611,9 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 	(*argc)--;
 
 	if (!strcasecmp(arg_name, DM_VERITY_OPT_FEC_DEV)) {
-		r = dm_get_device(v->ti, arg_value, FMODE_READ, &v->fec->dev);
+		r = dm_get_device(ti, arg_value, FMODE_READ, &v->fec->dev);
 		if (r) {
-			v->ti->error = "FEC device lookup failed";
+			ti->error = "FEC device lookup failed";
 			return r;
 		}
 
@@ -634,7 +621,7 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 		if (sscanf(arg_value, "%llu%c", &num_ll, &dummy) != 1 ||
 		    ((sector_t)(num_ll << (v->data_dev_block_bits - SECTOR_SHIFT))
 		     >> (v->data_dev_block_bits - SECTOR_SHIFT) != num_ll)) {
-			v->ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS;
+			ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS;
 			return -EINVAL;
 		}
 		v->fec->blocks = num_ll;
@@ -643,7 +630,7 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 		if (sscanf(arg_value, "%llu%c", &num_ll, &dummy) != 1 ||
 		    ((sector_t)(num_ll << (v->data_dev_block_bits - SECTOR_SHIFT)) >>
 		     (v->data_dev_block_bits - SECTOR_SHIFT) != num_ll)) {
-			v->ti->error = "Invalid " DM_VERITY_OPT_FEC_START;
+			ti->error = "Invalid " DM_VERITY_OPT_FEC_START;
 			return -EINVAL;
 		}
 		v->fec->start = num_ll;
@@ -652,13 +639,13 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 		if (sscanf(arg_value, "%hhu%c", &num_c, &dummy) != 1 || !num_c ||
 		    num_c < (DM_VERITY_FEC_RSM - DM_VERITY_FEC_MAX_RSN) ||
 		    num_c > (DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN)) {
-			v->ti->error = "Invalid " DM_VERITY_OPT_FEC_ROOTS;
+			ti->error = "Invalid " DM_VERITY_OPT_FEC_ROOTS;
 			return -EINVAL;
 		}
 		v->fec->roots = num_c;
 
 	} else {
-		v->ti->error = "Unrecognized verity FEC feature request";
+		ti->error = "Unrecognized verity FEC feature request";
 		return -EINVAL;
 	}
 
@@ -677,8 +664,8 @@ int verity_fec_ctr_alloc(struct dm_verity *v)
 		v->ti->error = "Cannot allocate FEC structure";
 		return -ENOMEM;
 	}
-
 	v->fec = f;
+
 	return 0;
 }
 
@@ -689,6 +676,7 @@ int verity_fec_ctr_alloc(struct dm_verity *v)
 int verity_fec_ctr(struct dm_verity *v)
 {
 	struct dm_verity_fec *f = v->fec;
+	struct dm_target *ti = v->ti;
 	u64 hash_blocks;
 
 	if (!verity_fec_is_enabled(v)) {
@@ -718,24 +706,22 @@ int verity_fec_ctr(struct dm_verity *v)
 	 * simplicity.
 	 */
 	if (v->data_dev_block_bits != v->hash_dev_block_bits) {
-		v->ti->error = "Block sizes must match to use FEC";
+		ti->error = "Block sizes must match to use FEC";
 		return -EINVAL;
 	}
 
 	if (!f->roots) {
-		v->ti->error = "Missing " DM_VERITY_OPT_FEC_ROOTS;
+		ti->error = "Missing " DM_VERITY_OPT_FEC_ROOTS;
 		return -EINVAL;
 	}
-
 	f->rsn = DM_VERITY_FEC_RSM - f->roots;
 
 	if (!f->blocks) {
-		v->ti->error = "Missing " DM_VERITY_OPT_FEC_BLOCKS;
+		ti->error = "Missing " DM_VERITY_OPT_FEC_BLOCKS;
 		return -EINVAL;
 	}
 
 	f->rounds = f->blocks;
-
 	if (do_div(f->rounds, f->rsn))
 		f->rounds++;
 
@@ -744,7 +730,7 @@ int verity_fec_ctr(struct dm_verity *v)
 	 * data_blocks and hash_blocks combined.
 	 */
 	if (f->blocks < v->data_blocks + hash_blocks || !f->rounds) {
-		v->ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS;
+		ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS;
 		return -EINVAL;
 	}
 
@@ -753,89 +739,80 @@ int verity_fec_ctr(struct dm_verity *v)
 	 * it to be large enough.
 	 */
 	f->hash_blocks = f->blocks - v->data_blocks;
-
 	if (dm_bufio_get_device_size(v->bufio) < f->hash_blocks) {
-		v->ti->error = "Hash device is too small for "
-				DM_VERITY_OPT_FEC_BLOCKS;
+		ti->error = "Hash device is too small for "
+			DM_VERITY_OPT_FEC_BLOCKS;
 		return -E2BIG;
 	}
 
 	f->bufio = dm_bufio_client_create(f->dev->bdev,
-				1 << v->data_dev_block_bits,
-				1, 0, NULL, NULL);
-
+					  1 << v->data_dev_block_bits,
+					  1, 0, NULL, NULL);
 	if (IS_ERR(f->bufio)) {
-		v->ti->error = "Cannot initialize dm-bufio";
+		ti->error = "Cannot initialize FEC bufio client";
 		return PTR_ERR(f->bufio);
 	}
 
 	if (dm_bufio_get_device_size(f->bufio) <
-			(f->start + f->rounds * f->roots)
-				>> v->data_dev_block_bits) {
-		v->ti->error = "FEC device is too small";
+	    ((f->start + f->rounds * f->roots) >> v->data_dev_block_bits)) {
+		ti->error = "FEC device is too small";
 		return -E2BIG;
 	}
 
 	f->data_bufio = dm_bufio_client_create(v->data_dev->bdev,
-				1 << v->data_dev_block_bits,
-				1, 0, NULL, NULL);
-
+					       1 << v->data_dev_block_bits,
+					       1, 0, NULL, NULL);
 	if (IS_ERR(f->data_bufio)) {
-		v->ti->error = "Cannot initialize dm-bufio";
+		ti->error = "Cannot initialize FEC data bufio client";
 		return PTR_ERR(f->data_bufio);
 	}
 
 	if (dm_bufio_get_device_size(f->data_bufio) < v->data_blocks) {
-		v->ti->error = "Data device is too small";
+		ti->error = "Data device is too small";
 		return -E2BIG;
 	}
 
 	/* Preallocate an rs_control structure for each worker thread */
 	f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc,
-				fec_rs_free, (void *) v);
-
+				    fec_rs_free, (void *) v);
 	if (!f->rs_pool) {
-		v->ti->error = "Cannot allocate RS pool";
+		ti->error = "Cannot allocate RS pool";
 		return -ENOMEM;
 	}
 
 	f->cache = kmem_cache_create("dm_verity_fec_buffers",
-				f->rsn << DM_VERITY_FEC_BUF_RS_BITS,
-				0, 0, NULL);
-
+				     f->rsn << DM_VERITY_FEC_BUF_RS_BITS,
+				     0, 0, NULL);
 	if (!f->cache) {
-		v->ti->error = "Cannot create FEC buffer cache";
+		ti->error = "Cannot create FEC buffer cache";
 		return -ENOMEM;
 	}
 
 	/* Preallocate DM_VERITY_FEC_BUF_PREALLOC buffers for each thread */
 	f->prealloc_pool = mempool_create_slab_pool(num_online_cpus() *
-						DM_VERITY_FEC_BUF_PREALLOC,
-					f->cache);
-
+						    DM_VERITY_FEC_BUF_PREALLOC,
+						    f->cache);
 	if (!f->prealloc_pool) {
-		v->ti->error = "Cannot allocate FEC buffer prealloc pool";
+		ti->error = "Cannot allocate FEC buffer prealloc pool";
 		return -ENOMEM;
 	}
 
 	f->extra_pool = mempool_create_slab_pool(0, f->cache);
-
 	if (!f->extra_pool) {
-		v->ti->error = "Cannot allocate FEC buffer extra pool";
+		ti->error = "Cannot allocate FEC buffer extra pool";
 		return -ENOMEM;
 	}
 
 	/* Preallocate an output buffer for each thread */
 	f->output_pool = mempool_create_kmalloc_pool(num_online_cpus(),
-					1 << v->data_dev_block_bits);
-
+						     1 << v->data_dev_block_bits);
 	if (!f->output_pool) {
-		v->ti->error = "Cannot allocate FEC output pool";
+		ti->error = "Cannot allocate FEC output pool";
 		return -ENOMEM;
 	}
 
 	/* Reserve space for our per-bio data */
-	v->ti->per_bio_data_size += sizeof(struct dm_verity_fec_io);
+	ti->per_bio_data_size += sizeof(struct dm_verity_fec_io);
 
 	return 0;
 }
-- 
2.4.9 (Apple Git-60)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285300

On Fri, Dec 04, 2015 at 04:09:35PM -0500, Mike Snitzer wrote:
> I'd really appreciate it if you could do some regression testing,
> etc on your end to verify I didn't break anything while tweaking
> things.

Sure. The changes look fine. I applied them to my tree and everything
still works as expected in my tests.

Thanks for taking the time to review the patches!

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285699

On Mon, Dec 07 2015 at  8:21am -0500,
Sami Tolvanen <samitolvanen@google.com> wrote:

> On Fri, Dec 04, 2015 at 04:09:35PM -0500, Mike Snitzer wrote:
> > I'd really appreciate it if you could do some regression testing,
> > etc on your end to verify I didn't break anything while tweaking
> > things.
> 
> Sure. The changes look fine. I applied them to my tree and everything
> still works as expected in my tests.

Great.  Moving forward it'd be awesome if you could work to get your
verity FEC support regression tests into cryptsetup's tests.  We need a
way to verify future DM and/or other kernel changes don't somehow cause
this FEC support to regress.

Also, I know you said you'd be getting Milan a veritysetup patch soon.
How are things going on that?  Ideally that'll land in conjunction with
the kernel's dm-verity FEC support.

> Thanks for taking the time to review the patches!

No problem, like I said I'll be reviewing the code further but at this
point your changes seem to be on-track for upstream Linux 4.5
inclusion.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285789

On Mon, Dec 07, 2015 at 09:58:14AM -0500, Mike Snitzer wrote:
> Great.  Moving forward it'd be awesome if you could work to get your
> verity FEC support regression tests into cryptsetup's tests.

Sure. These tests would basically involve generating a valid disk
image, corrupting it up to the theoretical maximum, and verifying that
dm-verity can correct it.

> Also, I know you said you'd be getting Milan a veritysetup patch soon.
> How are things going on that?

I'm still planning to have a patch for him this week.

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285834

On 12/07/2015 05:31 PM, Sami Tolvanen wrote:
> On Mon, Dec 07, 2015 at 09:58:14AM -0500, Mike Snitzer wrote:
>> Great.  Moving forward it'd be awesome if you could work to get your
>> verity FEC support regression tests into cryptsetup's tests.
> 
> Sure. These tests would basically involve generating a valid disk
> image, corrupting it up to the theoretical maximum, and verifying that
> dm-verity can correct it.

Does the code handle/report even the same type of data corruption
for RS blocks? Or this is just silently ignored until
data area corruption happens?
Sorry, I did not study the code here, I am just curious :)

(IOW I mean if data are ok but recovery metadata is corrupted.)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285861

On Mon, Dec 07 2015 at  1:07pm -0500,
Milan Broz <mbroz@redhat.com> wrote:

> On 12/07/2015 05:31 PM, Sami Tolvanen wrote:
> > On Mon, Dec 07, 2015 at 09:58:14AM -0500, Mike Snitzer wrote:
> >> Great.  Moving forward it'd be awesome if you could work to get your
> >> verity FEC support regression tests into cryptsetup's tests.
> > 
> > Sure. These tests would basically involve generating a valid disk
> > image, corrupting it up to the theoretical maximum, and verifying that
> > dm-verity can correct it.
> 
> Does the code handle/report even the same type of data corruption
> for RS blocks? Or this is just silently ignored until
> data area corruption happens?
> Sorry, I did not study the code here, I am just curious :)
> 
> (IOW I mean if data are ok but recovery metadata is corrupted.)

There is no preemptive accessing/checking of the RS blocks.  Once a
parity is needed for a given block, due to some corruption, the RS block
will be accessed with:

fec_decode_bufs -> fec_read_parity() -> fec_decode_rs8() -> decode_rs8()

I'm not seeing any verification of the metadata in fec_read_parity() --
so it would seem that corrupt RS blocks would result in -EBADMSG being
returned from decode_rs8() (by virtue of incorrect parity being passed
to decode_rs8).

Sami (or others) am I right?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286372

On Mon, Dec 07, 2015 at 02:07:43PM -0500, Mike Snitzer wrote:
> I'm not seeing any verification of the metadata in fec_read_parity() --
> so it would seem that corrupt RS blocks would result in -EBADMSG being
> returned from decode_rs8() (by virtue of incorrect parity being passed
> to decode_rs8).
> 
> Sami (or others) am I right?

Yes, decode_rs8 failing with -EBADMSG is one option. There are also two
other cases:

 1) If the parity data is only partially corrupted, it may still be
    possible to correct errors, provided that the actual data isn't
    too severely corrupted.

 2) If there's too much corruption for Reed-Solomon to detect, it's
    also possible that decode_rs8 just returns bogus data, which we
    will catch when verifying the hash again. This is why combining
    error correction with integrity checking is essential.

In other words, the worst case is that we cannot correct errors for the
blocks covered by the corrupted parity data.

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web