[PATCH] dm-verity: Add error handling modes for corrupted blocks

[PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

Add device specific modes to dm-verity to specify how corrupted
blocks should be handled. The following modes are defined:

  - DM_VERITY_MODE_EIO is the default behavior, where reading a
    corrupted block results in -EIO.

  - DM_VERITY_MODE_LOGGING only logs corrupted blocks, but does
    not block the read.

  - DM_VERITY_MODE_RESTART calls kernel_restart when a corrupted
    block is discovered.

In addition, each mode sends a uevent to notify userspace of
corruption and to allow further recovery actions.

The driver defaults to previous behavior (DM_VERITY_MODE_EIO)
and other modes can be enabled with an additional parameter to
the verity table.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

---
 Documentation/device-mapper/verity.txt |   15 ++++-
 drivers/md/dm-verity.c                 |   98 +++++++++++++++++++++++++++++----
 2 files changed, 103 insertions(+), 10 deletions(-)

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index 9884681..470f14c 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -10,7 +10,7 @@ Construction Parameters
     <version> <dev> <hash_dev>
     <data_block_size> <hash_block_size>
     <num_data_blocks> <hash_start_block>
-    <algorithm> <digest> <salt>
+    <algorithm> <digest> <salt> <mode>
 
 <version>
     This is the type of the on-disk hash format.
@@ -62,6 +62,19 @@ Construction Parameters
 <salt>
     The hexadecimal encoding of the salt value.
 
+<mode>
+    Optional. The mode of operation.
+
+    0 is the normal mode of operation where a corrupted block will result in an
+      I/O error.
+
+    1 is logging mode where corrupted blocks are logged, but the read operation
+      will still succeed normally.
+
+    2 is restart mode, where a corrupted block will result in the system being
+      immediately restarted.
+
+
 Theory of operation
 ===================
 
diff --git a/drivers/md/dm-verity.c b/drivers/md/dm-verity.c
index 7a7bab8..ab72062 100644
--- a/drivers/md/dm-verity.c
+++ b/drivers/md/dm-verity.c
@@ -18,20 +18,36 @@
 
 #include <linux/module.h>
 #include <linux/device-mapper.h>
+#include <linux/reboot.h>
 #include <crypto/hash.h>
 
 #define DM_MSG_PREFIX			"verity"
 
+#define DM_VERITY_ENV_LENGTH		42
+#define DM_VERITY_ENV_VAR_NAME		"VERITY_ERR_BLOCK_NR"
+
 #define DM_VERITY_IO_VEC_INLINE		16
 #define DM_VERITY_MEMPOOL_SIZE		4
 #define DM_VERITY_DEFAULT_PREFETCH_SIZE	262144
 
 #define DM_VERITY_MAX_LEVELS		63
+#define DM_VERITY_MAX_CORRUPTED_ERRS	100
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
 module_param_named(prefetch_cluster, dm_verity_prefetch_cluster, uint, S_IRUGO | S_IWUSR);
 
+enum verity_mode {
+	DM_VERITY_MODE_EIO = 0,
+	DM_VERITY_MODE_LOGGING = 1,
+	DM_VERITY_MODE_RESTART = 2
+};
+
+enum verity_block_type {
+	DM_VERITY_BLOCK_TYPE_DATA,
+	DM_VERITY_BLOCK_TYPE_METADATA
+};
+
 struct dm_verity {
 	struct dm_dev *data_dev;
 	struct dm_dev *hash_dev;
@@ -54,6 +70,8 @@ struct dm_verity {
 	unsigned digest_size;	/* digest size for the current hash algorithm */
 	unsigned shash_descsize;/* 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 */
 
 	mempool_t *vec_mempool;	/* mempool of bio vector */
 
@@ -175,6 +193,54 @@ static void verity_hash_at_level(struct dm_verity *v, sector_t block, int level,
 }
 
 /*
+ * Handle verification errors.
+ */
+static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
+				 unsigned long long block)
+{
+	char verity_env[DM_VERITY_ENV_LENGTH];
+	char *envp[] = { verity_env, NULL };
+	const char *type_str = "";
+	struct mapped_device *md = dm_table_get_md(v->ti->table);
+
+	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
+		goto out;
+
+	++v->corrupted_errs;
+
+	switch (type) {
+	case DM_VERITY_BLOCK_TYPE_DATA:
+		type_str = "data";
+		break;
+	case DM_VERITY_BLOCK_TYPE_METADATA:
+		type_str = "metadata";
+		break;
+	default:
+		BUG();
+	}
+
+	DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
+		type_str, block);
+
+	if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
+		DMERR("%s: reached maximum errors", v->data_dev->name);
+
+	snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
+		DM_VERITY_ENV_VAR_NAME, type, block);
+
+	kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE, envp);
+
+out:
+	if (v->mode == DM_VERITY_MODE_LOGGING)
+		return 0;
+
+	if (v->mode == DM_VERITY_MODE_RESTART)
+		kernel_restart("dm-verity device corrupted");
+
+	return 1;
+}
+
+/*
  * Verify hash of a metadata block pertaining to the specified data block
  * ("block" argument) at a specified level ("level" argument).
  *
@@ -251,11 +317,13 @@ static int verity_verify_level(struct dm_verity_io *io, sector_t block,
 			goto release_ret_r;
 		}
 		if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
-			DMERR_LIMIT("metadata block %llu is corrupted",
-				(unsigned long long)hash_block);
 			v->hash_failed = 1;
-			r = -EIO;
-			goto release_ret_r;
+
+			if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_METADATA,
+					      hash_block)) {
+				r = -EIO;
+				goto release_ret_r;
+			}
 		} else
 			aux->hash_verified = 1;
 	}
@@ -367,10 +435,11 @@ test_block_hash:
 			return r;
 		}
 		if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
-			DMERR_LIMIT("data block %llu is corrupted",
-				(unsigned long long)(io->block + b));
 			v->hash_failed = 1;
-			return -EIO;
+
+			if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA,
+					      io->block + b))
+				return -EIO;
 		}
 	}
 
@@ -668,8 +737,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
-	if (argc != 10) {
-		ti->error = "Invalid argument count: exactly 10 arguments required";
+	if (argc < 10 || argc > 11) {
+		ti->error = "Invalid argument count: 10-11 arguments required";
 		r = -EINVAL;
 		goto bad;
 	}
@@ -790,6 +859,17 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		}
 	}
 
+	if (argc > 10) {
+		if (sscanf(argv[10], "%d%c", &num, &dummy) != 1 ||
+			num < DM_VERITY_MODE_EIO ||
+			num > DM_VERITY_MODE_RESTART) {
+			ti->error = "Invalid mode";
+			r = -EINVAL;
+			goto bad;
+		}
+		v->mode = num;
+	}
+
 	v->hash_per_block_bits =
 		__fls((1 << v->hash_dev_block_bits) / v->digest_size);
 

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Will Drewry]

Thanks for posting this!  A few first-pass comments inline:

On Mon, Mar 16, 2015 at 10:55 AM, Sami Tolvanen <samitolvanen@google.com> wrote:
> Add device specific modes to dm-verity to specify how corrupted
> blocks should be handled. The following modes are defined:
>
>   - DM_VERITY_MODE_EIO is the default behavior, where reading a
>     corrupted block results in -EIO.
>
>   - DM_VERITY_MODE_LOGGING only logs corrupted blocks, but does
>     not block the read.
>
>   - DM_VERITY_MODE_RESTART calls kernel_restart when a corrupted
>     block is discovered.
>
> In addition, each mode sends a uevent to notify userspace of
> corruption and to allow further recovery actions.
>
> The driver defaults to previous behavior (DM_VERITY_MODE_EIO)
> and other modes can be enabled with an additional parameter to
> the verity table.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
>
> ---
>  Documentation/device-mapper/verity.txt |   15 ++++-
>  drivers/md/dm-verity.c                 |   98 +++++++++++++++++++++++++++++----
>  2 files changed, 103 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
> index 9884681..470f14c 100644
> --- a/Documentation/device-mapper/verity.txt
> +++ b/Documentation/device-mapper/verity.txt
> @@ -10,7 +10,7 @@ Construction Parameters
>      <version> <dev> <hash_dev>
>      <data_block_size> <hash_block_size>
>      <num_data_blocks> <hash_start_block>
> -    <algorithm> <digest> <salt>
> +    <algorithm> <digest> <salt> <mode>
>
>  <version>
>      This is the type of the on-disk hash format.
> @@ -62,6 +62,19 @@ Construction Parameters
>  <salt>
>      The hexadecimal encoding of the salt value.
>
> +<mode>
> +    Optional. The mode of operation.
> +
> +    0 is the normal mode of operation where a corrupted block will result in an
> +      I/O error.
> +
> +    1 is logging mode where corrupted blocks are logged, but the read operation
> +      will still succeed normally.
> +
> +    2 is restart mode, where a corrupted block will result in the system being
> +      immediately restarted.
> +
> +
>  Theory of operation
>  ===================
>
> diff --git a/drivers/md/dm-verity.c b/drivers/md/dm-verity.c
> index 7a7bab8..ab72062 100644
> --- a/drivers/md/dm-verity.c
> +++ b/drivers/md/dm-verity.c
> @@ -18,20 +18,36 @@
>
>  #include <linux/module.h>
>  #include <linux/device-mapper.h>
> +#include <linux/reboot.h>
>  #include <crypto/hash.h>
>
>  #define DM_MSG_PREFIX                  "verity"
>
> +#define DM_VERITY_ENV_LENGTH           42
> +#define DM_VERITY_ENV_VAR_NAME         "VERITY_ERR_BLOCK_NR"
> +
>  #define DM_VERITY_IO_VEC_INLINE                16
>  #define DM_VERITY_MEMPOOL_SIZE         4
>  #define DM_VERITY_DEFAULT_PREFETCH_SIZE        262144
>
>  #define DM_VERITY_MAX_LEVELS           63
> +#define DM_VERITY_MAX_CORRUPTED_ERRS   100
>
>  static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
>
>  module_param_named(prefetch_cluster, dm_verity_prefetch_cluster, uint, S_IRUGO | S_IWUSR);
>
> +enum verity_mode {
> +       DM_VERITY_MODE_EIO = 0,
> +       DM_VERITY_MODE_LOGGING = 1,
> +       DM_VERITY_MODE_RESTART = 2
> +};
> +
> +enum verity_block_type {
> +       DM_VERITY_BLOCK_TYPE_DATA,
> +       DM_VERITY_BLOCK_TYPE_METADATA
> +};
> +
>  struct dm_verity {
>         struct dm_dev *data_dev;
>         struct dm_dev *hash_dev;
> @@ -54,6 +70,8 @@ struct dm_verity {
>         unsigned digest_size;   /* digest size for the current hash algorithm */
>         unsigned shash_descsize;/* 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 */
>
>         mempool_t *vec_mempool; /* mempool of bio vector */
>
> @@ -175,6 +193,54 @@ static void verity_hash_at_level(struct dm_verity *v, sector_t block, int level,
>  }
>
>  /*
> + * Handle verification errors.
> + */
> +static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
> +                                unsigned long long block)
> +{
> +       char verity_env[DM_VERITY_ENV_LENGTH];
> +       char *envp[] = { verity_env, NULL };
> +       const char *type_str = "";
> +       struct mapped_device *md = dm_table_get_md(v->ti->table);
> +
> +       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
> +               goto out;
> +
> +       ++v->corrupted_errs;
> +

The conditional and increment should be moved below the DMERR_LIMIT().
Otherwise, no logging
will occur in non-logging modes. This would be a change from how the
default "eio" mode behaves today.

> +       switch (type) {
> +       case DM_VERITY_BLOCK_TYPE_DATA:
> +               type_str = "data";
> +               break;
> +       case DM_VERITY_BLOCK_TYPE_METADATA:
> +               type_str = "metadata";
> +               break;
> +       default:
> +               BUG();
> +       }
> +
> +       DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
> +               type_str, block);

Perhaps it'd make sense to consider whether to use DMERR_LIMIT or not
depending on if the mode is logging.  Otherwise you may get weird
interactions from having two different limits.

> +
> +       if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
> +               DMERR("%s: reached maximum errors", v->data_dev->name);
> +
> +       snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
> +               DM_VERITY_ENV_VAR_NAME, type, block);
> +
> +       kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE, envp);
> +
> +out:
> +       if (v->mode == DM_VERITY_MODE_LOGGING)
> +               return 0;
> +
> +       if (v->mode == DM_VERITY_MODE_RESTART)
> +               kernel_restart("dm-verity device corrupted");
> +
> +       return 1;
> +}
> +
> +/*
>   * Verify hash of a metadata block pertaining to the specified data block
>   * ("block" argument) at a specified level ("level" argument).
>   *
> @@ -251,11 +317,13 @@ static int verity_verify_level(struct dm_verity_io *io, sector_t block,
>                         goto release_ret_r;
>                 }
>                 if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
> -                       DMERR_LIMIT("metadata block %llu is corrupted",
> -                               (unsigned long long)hash_block);
>                         v->hash_failed = 1;

Should the dm status reflect the failure even if logging mode isn't
returning EIOs? I think it makes sense still, but it might be good to
note that it is intentionally kept this way.

> -                       r = -EIO;
> -                       goto release_ret_r;
> +
> +                       if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_METADATA,
> +                                             hash_block)) {
> +                               r = -EIO;
> +                               goto release_ret_r;
> +                       }
>                 } else
>                         aux->hash_verified = 1;
>         }
> @@ -367,10 +435,11 @@ test_block_hash:
>                         return r;
>                 }
>                 if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
> -                       DMERR_LIMIT("data block %llu is corrupted",
> -                               (unsigned long long)(io->block + b));
>                         v->hash_failed = 1;

Ditto

> -                       return -EIO;
> +
> +                       if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA,
> +                                             io->block + b))
> +                               return -EIO;
>                 }
>         }
>
> @@ -668,8 +737,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
>                 goto bad;
>         }
>
> -       if (argc != 10) {
> -               ti->error = "Invalid argument count: exactly 10 arguments required";
> +       if (argc < 10 || argc > 11) {
> +               ti->error = "Invalid argument count: 10-11 arguments required";
>                 r = -EINVAL;
>                 goto bad;
>         }
> @@ -790,6 +859,17 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
>                 }
>         }
>
> +       if (argc > 10) {
> +               if (sscanf(argv[10], "%d%c", &num, &dummy) != 1 ||
> +                       num < DM_VERITY_MODE_EIO ||
> +                       num > DM_VERITY_MODE_RESTART) {
> +                       ti->error = "Invalid mode";
> +                       r = -EINVAL;
> +                       goto bad;
> +               }
> +               v->mode = num;
> +       }
> +
>         v->hash_per_block_bits =
>                 __fls((1 << v->hash_dev_block_bits) / v->digest_size);
>

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Alasdair G Kergon]

On Mon, Mar 16, 2015 at 03:55:59PM +0000, Sami Tolvanen wrote:
> +<mode>
> +    Optional. The mode of operation.
> +    0 is the normal mode of operation where a corrupted block will result in an

The usual dm style uses a word rather than hiding behind a numeric code that has
to be learned or looked up.

> +#define DM_VERITY_ENV_VAR_NAME		"VERITY_ERR_BLOCK_NR"

Missing DM_ prefix?

Alasdair

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Alasdair G Kergon]

On Mon, Mar 16, 2015 at 05:13:10PM -0500, Will Drewry wrote:
> Should the dm status reflect the failure even if logging mode isn't
> returning EIOs? I think it makes sense still, but it might be good to
 
The uevents shouldn't be relied upon: 'status' must still tell you
everything you need to know about the internal state of the device.

'table' should show the new parameter and 'status' should probably show
it too if it's needed to interpret the 'status' properly.

Alasdair

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Mon, Mar 16, 2015 at 05:13:10PM -0500, Will Drewry wrote:
> > +       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
> > +               goto out;
> > +
> > +       ++v->corrupted_errs;
> > +
> 
> The conditional and increment should be moved below the DMERR_LIMIT().
> Otherwise, no logging will occur in non-logging modes.

This only limits the maximum number of logged errors, but until it's reached,
it does log them in all modes.

> This would be a change from how the default "eio" mode behaves today.

The only difference is that it will stop logging after reaching the maximum.

> > +       DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
> > +               type_str, block);
> 
> Perhaps it'd make sense to consider whether to use DMERR_LIMIT or not
> depending on if the mode is logging.  Otherwise you may get weird
> interactions from having two different limits.

My intention was initially to use DMERR_LIMIT to handle error bursts, but you
are correct, it's not needed. I'll change this to DMERR in v2.

> >                         v->hash_failed = 1;
> 
> Should the dm status reflect the failure even if logging mode isn't
> returning EIOs? I think it makes sense still, but it might be good to
> note that it is intentionally kept this way.

Yes, I think it makes sense. We should be able to check the device status and
see if there have been corrupted blocks even in logging mode. I will move this
to the error handling function and add a note about it.

Sami

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Tue, Mar 17, 2015 at 12:45:55AM +0000, Alasdair G Kergon wrote:
> 'table' should show the new parameter and 'status' should probably show
> it too if it's needed to interpret the 'status' properly.

Good point, I will add it to 'table' in v2. I doubt it's necessary in 'status'
though, userspace should be able to decide how to handle errors without it.

Sami

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Tue, Mar 17, 2015 at 12:37:47AM +0000, Alasdair G Kergon wrote:
> The usual dm style uses a word rather than hiding behind a numeric code that has
> to be learned or looked up.

> Missing DM_ prefix?

Thanks for the comments, I will fix these in the next version.

Sami

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Vivek Goyal]

On Mon, Mar 16, 2015 at 03:55:59PM +0000, Sami Tolvanen wrote:
> Add device specific modes to dm-verity to specify how corrupted
> blocks should be handled. The following modes are defined:
> 
>   - DM_VERITY_MODE_EIO is the default behavior, where reading a
>     corrupted block results in -EIO.
> 
>   - DM_VERITY_MODE_LOGGING only logs corrupted blocks, but does
>     not block the read.
> 
>   - DM_VERITY_MODE_RESTART calls kernel_restart when a corrupted
>     block is discovered.

What's the idea behind kernel restart mode ? If a corrupted block is in
the path of boot, will we not get into a continuous reboot cycle.

Thanks
Vivek

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Tue, Mar 17, 2015 at 11:27:49AM -0400, Vivek Goyal wrote:
> What's the idea behind kernel restart mode ? If a corrupted block is in
> the path of boot, will we not get into a continuous reboot cycle.

It depends on how dm-verity is used. If we have PSTORE_CONSOLE enabled, for
example, after a restart we can check if the restart was caused by dm-verity
and then use a different mode when setting up the verity device. Most likely
for some systems this is not a reasonable recovery method, but we find it to
be useful on Android at least.

Sami

[PATCHv2] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

Add device specific modes to dm-verity to specify how corrupted
blocks should be handled. The following modes are defined:

  - DM_VERITY_MODE_EIO is the default behavior, where reading a
    corrupted block results in -EIO.

  - DM_VERITY_MODE_LOGGING only logs corrupted blocks, but does
    not block the read.

  - DM_VERITY_MODE_RESTART calls kernel_restart when a corrupted
    block is discovered.

In addition, each mode sends a uevent to notify userspace of
corruption and to allow further recovery actions.

The driver defaults to previous behavior (DM_VERITY_MODE_EIO)
and other modes can be enabled with an additional parameter to
the verity table.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

---
Changes since v1:
  Use words instead of numeric values as the mode parameter
  Use DMERR when logging corrupted blocks
  Add the operating mode to STATUSTYPE_TABLE output
  Add a DM_ prefix to the uevent variable name

---
 Documentation/device-mapper/verity.txt |   17 +++
 drivers/md/dm-verity.c                 |  122 ++++++++++++++++++++---
 2 files changed, 127 insertions(+), 12 deletions(-)

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index 9884681..7fe478b 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -10,7 +10,7 @@ Construction Parameters
     <version> <dev> <hash_dev>
     <data_block_size> <hash_block_size>
     <num_data_blocks> <hash_start_block>
-    <algorithm> <digest> <salt>
+    <algorithm> <digest> <salt> <mode>
 
 <version>
     This is the type of the on-disk hash format.
@@ -62,6 +62,21 @@ Construction Parameters
 <salt>
     The hexadecimal encoding of the salt value.
 
+<mode>
+    Optional. The mode of operation.
+
+    eio
+    - The default mode of operation where a corrupted block will result in an
+      I/O error.
+
+    logging
+    - Corrupted blocks are logged, but the read operation will still succeed
+      normally.
+
+    restart
+    - A corrupted block will result in the system being immediately restarted.
+
+
 Theory of operation
 ===================
 
diff --git a/drivers/md/dm-verity.c b/drivers/md/dm-verity.c
index 7a7bab8..35fc1bc 100644
--- a/drivers/md/dm-verity.c
+++ b/drivers/md/dm-verity.c
@@ -18,20 +18,40 @@
 
 #include <linux/module.h>
 #include <linux/device-mapper.h>
+#include <linux/reboot.h>
 #include <crypto/hash.h>
 
 #define DM_MSG_PREFIX			"verity"
 
+#define DM_VERITY_ENV_LENGTH		42
+#define DM_VERITY_ENV_VAR_NAME		"DM_VERITY_ERR_BLOCK_NR"
+
 #define DM_VERITY_IO_VEC_INLINE		16
 #define DM_VERITY_MEMPOOL_SIZE		4
 #define DM_VERITY_DEFAULT_PREFETCH_SIZE	262144
 
 #define DM_VERITY_MAX_LEVELS		63
+#define DM_VERITY_MAX_CORRUPTED_ERRS	100
+
+#define DM_VERITY_MODE_NAME_EIO		"eio"
+#define DM_VERITY_MODE_NAME_LOGGING	"logging"
+#define DM_VERITY_MODE_NAME_RESTART	"restart"
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
 module_param_named(prefetch_cluster, dm_verity_prefetch_cluster, uint, S_IRUGO | S_IWUSR);
 
+enum verity_mode {
+	DM_VERITY_MODE_EIO,
+	DM_VERITY_MODE_LOGGING,
+	DM_VERITY_MODE_RESTART
+};
+
+enum verity_block_type {
+	DM_VERITY_BLOCK_TYPE_DATA,
+	DM_VERITY_BLOCK_TYPE_METADATA
+};
+
 struct dm_verity {
 	struct dm_dev *data_dev;
 	struct dm_dev *hash_dev;
@@ -54,6 +74,8 @@ struct dm_verity {
 	unsigned digest_size;	/* digest size for the current hash algorithm */
 	unsigned shash_descsize;/* 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 */
 
 	mempool_t *vec_mempool;	/* mempool of bio vector */
 
@@ -175,6 +197,57 @@ static void verity_hash_at_level(struct dm_verity *v, sector_t block, int level,
 }
 
 /*
+ * Handle verification errors.
+ */
+static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
+				 unsigned long long block)
+{
+	char verity_env[DM_VERITY_ENV_LENGTH];
+	char *envp[] = { verity_env, NULL };
+	const char *type_str = "";
+	struct mapped_device *md = dm_table_get_md(v->ti->table);
+
+	/* Corruption should be visible in device status in all modes */
+	v->hash_failed = 1;
+
+	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
+		goto out;
+
+	++v->corrupted_errs;
+
+	switch (type) {
+	case DM_VERITY_BLOCK_TYPE_DATA:
+		type_str = "data";
+		break;
+	case DM_VERITY_BLOCK_TYPE_METADATA:
+		type_str = "metadata";
+		break;
+	default:
+		BUG();
+	}
+
+	DMERR("%s: %s block %llu is corrupted", v->data_dev->name, type_str,
+		block);
+
+	if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
+		DMERR("%s: reached maximum errors", v->data_dev->name);
+
+	snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
+		DM_VERITY_ENV_VAR_NAME, type, block);
+
+	kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE, envp);
+
+out:
+	if (v->mode == DM_VERITY_MODE_LOGGING)
+		return 0;
+
+	if (v->mode == DM_VERITY_MODE_RESTART)
+		kernel_restart("dm-verity device corrupted");
+
+	return 1;
+}
+
+/*
  * Verify hash of a metadata block pertaining to the specified data block
  * ("block" argument) at a specified level ("level" argument).
  *
@@ -251,11 +324,11 @@ static int verity_verify_level(struct dm_verity_io *io, sector_t block,
 			goto release_ret_r;
 		}
 		if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
-			DMERR_LIMIT("metadata block %llu is corrupted",
-				(unsigned long long)hash_block);
-			v->hash_failed = 1;
-			r = -EIO;
-			goto release_ret_r;
+			if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_METADATA,
+					      hash_block)) {
+				r = -EIO;
+				goto release_ret_r;
+			}
 		} else
 			aux->hash_verified = 1;
 	}
@@ -367,10 +440,9 @@ test_block_hash:
 			return r;
 		}
 		if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
-			DMERR_LIMIT("data block %llu is corrupted",
-				(unsigned long long)(io->block + b));
-			v->hash_failed = 1;
-			return -EIO;
+			if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA,
+					      io->block + b))
+				return -EIO;
 		}
 	}
 
@@ -546,6 +618,20 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 		else
 			for (x = 0; x < v->salt_size; x++)
 				DMEMIT("%02x", v->salt[x]);
+		DMEMIT(" ");
+		switch (v->mode) {
+		case DM_VERITY_MODE_EIO:
+			DMEMIT(DM_VERITY_MODE_NAME_EIO);
+			break;
+		case DM_VERITY_MODE_LOGGING:
+			DMEMIT(DM_VERITY_MODE_NAME_LOGGING);
+			break;
+		case DM_VERITY_MODE_RESTART:
+			DMEMIT(DM_VERITY_MODE_NAME_RESTART);
+			break;
+		default:
+			BUG();
+		}
 		break;
 	}
 }
@@ -668,8 +754,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
-	if (argc != 10) {
-		ti->error = "Invalid argument count: exactly 10 arguments required";
+	if (argc < 10 || argc > 11) {
+		ti->error = "Invalid argument count: 10-11 arguments required";
 		r = -EINVAL;
 		goto bad;
 	}
@@ -790,6 +876,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		}
 	}
 
+	if (argc > 10) {
+		if (!strcmp(argv[10], DM_VERITY_MODE_NAME_EIO))
+			v->mode = DM_VERITY_MODE_EIO;
+		else if (!strcmp(argv[10], DM_VERITY_MODE_NAME_LOGGING))
+			v->mode = DM_VERITY_MODE_LOGGING;
+		else if (!strcmp(argv[10], DM_VERITY_MODE_NAME_RESTART))
+			v->mode = DM_VERITY_MODE_RESTART;
+		else {
+			ti->error = "Invalid mode";
+			r = -EINVAL;
+			goto bad;
+		}
+	}
+
 	v->hash_per_block_bits =
 		__fls((1 << v->hash_dev_block_bits) / v->digest_size);
 

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Vivek Goyal]

On Tue, Mar 17, 2015 at 04:06:49PM +0000, Sami Tolvanen wrote:
> On Tue, Mar 17, 2015 at 11:27:49AM -0400, Vivek Goyal wrote:
> > What's the idea behind kernel restart mode ? If a corrupted block is in
> > the path of boot, will we not get into a continuous reboot cycle.
> 
> It depends on how dm-verity is used. If we have PSTORE_CONSOLE enabled, for
> example, after a restart we can check if the restart was caused by dm-verity
> and then use a different mode when setting up the verity device. Most likely
> for some systems this is not a reasonable recovery method, but we find it to
> be useful on Android at least.

Ok.

Without knowing too much of detail, asking kernel to restart because one
block was corrupt sounds little drastic. If you are sending user space
events, why not let user space initiate the start and manage policy in
user space.

Thanks
Vivek

Re: [PATCHv2] dm-verity: Add error handling modes for corrupted blocks

[Alasdair G Kergon]

On Tue, Mar 17, 2015 at 04:37:07PM +0000, Sami Tolvanen wrote:
> +<mode>
> +    Optional. 

The format needs to remain easily extendable.

Two alternatives:

  Document this as a required field going forward, but say that if
  it is missing the default will be assumed.

  eio|logging|restart  (but those names are confusing)


Or (my preference) use a 'features' approach like most other targets do:

<number of feature> <feature arguments>

"error" is the existing default, so doesn't need stating.

The other two could be:
  restart_on_corruption
  ignore_corruption

Logging and events would happen in all cases, but if they did need
controlling independently, additional feature arguments could do that.

Alasdair

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Tue, Mar 17, 2015 at 02:03:58PM -0400, Vivek Goyal wrote:
> Without knowing too much of detail, asking kernel to restart because one
> block was corrupt sounds little drastic.
 
I agree, it's drastic, but in our use case it's necessary, because we have
critical system data on a verified partition. Depending on which blocks are
corrupted, the system may no longer be functional at this point.

> If you are sending user space events, why not let user space initiate the
> start and manage policy in user space.

We already manage policy in user space by determining in which mode dm-verity
will start. Restarting from user space is possible, but it would rely on the
uevent being reliably processed and the daemon responsible for restarting the
device not being blocked by the lack of access to corrupted data. We find
restarting from the dm-verity driver to be more reliable.

Sami

Re: [PATCHv2] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Tue, Mar 17, 2015 at 06:50:53PM +0000, Alasdair G Kergon wrote:
> The format needs to remain easily extendable.

Yes, I agree. I will address this in the next version.

> Or (my preference) use a 'features' approach like most other targets do:

This sounds like a better option to me, I will go with this.

Sami

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Mikulas Patocka]

On Wed, 18 Mar 2015, Sami Tolvanen wrote:

> On Tue, Mar 17, 2015 at 02:03:58PM -0400, Vivek Goyal wrote:
> > Without knowing too much of detail, asking kernel to restart because one
> > block was corrupt sounds little drastic.
>  
> I agree, it's drastic, but in our use case it's necessary, because we have
> critical system data on a verified partition. Depending on which blocks are
> corrupted, the system may no longer be functional at this point.

As a user, I'd rather have half-functioning device where I can at least 
try to recover some data than a non-functioning device that continuously 
reboots.

I hope that you at least turn this feature off after a few reboots and 
give the user a chance to save his data.

Mikulas

Re: [PATCH] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

On Wed, Mar 18, 2015 at 11:42:47AM -0400, Mikulas Patocka wrote:
> I hope that you at least turn this feature off after a few reboots and 
> give the user a chance to save his data.

Of course. We prompt the user after the reboot and ask them if they want
to proceed using the device without verification. We don't want to disable
verification before the user has been notified though.

Sami

[PATCHv3] dm-verity: Add error handling modes for corrupted blocks

[Sami Tolvanen]

Add device specific modes to dm-verity to specify how corrupted
blocks should be handled. The following modes are defined:

  - DM_VERITY_MODE_EIO is the default behavior, where reading a
    corrupted block results in -EIO.

  - DM_VERITY_MODE_LOGGING only logs corrupted blocks, but does
    not block the read.

  - DM_VERITY_MODE_RESTART calls kernel_restart when a corrupted
    block is discovered.

In addition, each mode sends a uevent to notify userspace of
corruption and to allow further recovery actions.

The driver defaults to previous behavior (DM_VERITY_MODE_EIO)
and other modes can be enabled with an additional parameter to
the verity table.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

---
Changes since v2:
  Make feature arguments extendable
  Change argument names to be more understandable

Changes since v1:
  Use words instead of numeric values as the mode parameter
  Use DMERR when logging corrupted blocks
  Add the operating mode to STATUSTYPE_TABLE output
  Add a DM_ prefix to the uevent variable name

---
 Documentation/device-mapper/verity.txt |   17 ++
 drivers/md/dm-verity.c                 |  146 +++++++++++++++++++++--
 2 files changed, 151 insertions(+), 12 deletions(-)

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index 9884681..64ccc5a 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -11,6 +11,7 @@ Construction Parameters
     <data_block_size> <hash_block_size>
     <num_data_blocks> <hash_start_block>
     <algorithm> <digest> <salt>
+    [<#opt_params> <opt_params>]
 
 <version>
     This is the type of the on-disk hash format.
@@ -62,6 +63,22 @@ Construction Parameters
 <salt>
     The hexadecimal encoding of the salt value.
 
+<#opt_params>
+    Number of optional parameters. If there are no optional parameters,
+    the optional paramaters section can be skipped or #opt_params can be zero.
+    Otherwise #opt_params is the number of following arguments.
+
+    Example of optional parameters section:
+        1 ignore_corruption
+
+ignore_corruption
+    Log corrupted blocks, but allow read operations to proceed normally.
+
+restart_on_corruption
+    Restart the system when a corrupted block is discovered. This option is
+    not compatible with ignore_corruption and requires user space support to
+    avoid restart loops.
+
 Theory of operation
 ===================
 
diff --git a/drivers/md/dm-verity.c b/drivers/md/dm-verity.c
index 7a7bab8..6824adc 100644
--- a/drivers/md/dm-verity.c
+++ b/drivers/md/dm-verity.c
@@ -18,20 +18,39 @@
 
 #include <linux/module.h>
 #include <linux/device-mapper.h>
+#include <linux/reboot.h>
 #include <crypto/hash.h>
 
 #define DM_MSG_PREFIX			"verity"
 
+#define DM_VERITY_ENV_LENGTH		42
+#define DM_VERITY_ENV_VAR_NAME		"DM_VERITY_ERR_BLOCK_NR"
+
 #define DM_VERITY_IO_VEC_INLINE		16
 #define DM_VERITY_MEMPOOL_SIZE		4
 #define DM_VERITY_DEFAULT_PREFETCH_SIZE	262144
 
 #define DM_VERITY_MAX_LEVELS		63
+#define DM_VERITY_MAX_CORRUPTED_ERRS	100
+
+#define DM_VERITY_OPT_LOGGING		"ignore_corruption"
+#define DM_VERITY_OPT_RESTART		"restart_on_corruption"
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
 module_param_named(prefetch_cluster, dm_verity_prefetch_cluster, uint, S_IRUGO | S_IWUSR);
 
+enum verity_mode {
+	DM_VERITY_MODE_EIO,
+	DM_VERITY_MODE_LOGGING,
+	DM_VERITY_MODE_RESTART
+};
+
+enum verity_block_type {
+	DM_VERITY_BLOCK_TYPE_DATA,
+	DM_VERITY_BLOCK_TYPE_METADATA
+};
+
 struct dm_verity {
 	struct dm_dev *data_dev;
 	struct dm_dev *hash_dev;
@@ -54,6 +73,8 @@ struct dm_verity {
 	unsigned digest_size;	/* digest size for the current hash algorithm */
 	unsigned shash_descsize;/* 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 */
 
 	mempool_t *vec_mempool;	/* mempool of bio vector */
 
@@ -175,6 +196,57 @@ static void verity_hash_at_level(struct dm_verity *v, sector_t block, int level,
 }
 
 /*
+ * Handle verification errors.
+ */
+static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
+				 unsigned long long block)
+{
+	char verity_env[DM_VERITY_ENV_LENGTH];
+	char *envp[] = { verity_env, NULL };
+	const char *type_str = "";
+	struct mapped_device *md = dm_table_get_md(v->ti->table);
+
+	/* Corruption should be visible in device status in all modes */
+	v->hash_failed = 1;
+
+	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
+		goto out;
+
+	++v->corrupted_errs;
+
+	switch (type) {
+	case DM_VERITY_BLOCK_TYPE_DATA:
+		type_str = "data";
+		break;
+	case DM_VERITY_BLOCK_TYPE_METADATA:
+		type_str = "metadata";
+		break;
+	default:
+		BUG();
+	}
+
+	DMERR("%s: %s block %llu is corrupted", v->data_dev->name, type_str,
+		block);
+
+	if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
+		DMERR("%s: reached maximum errors", v->data_dev->name);
+
+	snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
+		DM_VERITY_ENV_VAR_NAME, type, block);
+
+	kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE, envp);
+
+out:
+	if (v->mode == DM_VERITY_MODE_LOGGING)
+		return 0;
+
+	if (v->mode == DM_VERITY_MODE_RESTART)
+		kernel_restart("dm-verity device corrupted");
+
+	return 1;
+}
+
+/*
  * Verify hash of a metadata block pertaining to the specified data block
  * ("block" argument) at a specified level ("level" argument).
  *
@@ -251,11 +323,11 @@ static int verity_verify_level(struct dm_verity_io *io, sector_t block,
 			goto release_ret_r;
 		}
 		if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
-			DMERR_LIMIT("metadata block %llu is corrupted",
-				(unsigned long long)hash_block);
-			v->hash_failed = 1;
-			r = -EIO;
-			goto release_ret_r;
+			if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_METADATA,
+					      hash_block)) {
+				r = -EIO;
+				goto release_ret_r;
+			}
 		} else
 			aux->hash_verified = 1;
 	}
@@ -367,10 +439,9 @@ test_block_hash:
 			return r;
 		}
 		if (unlikely(memcmp(result, io_want_digest(v, io), v->digest_size))) {
-			DMERR_LIMIT("data block %llu is corrupted",
-				(unsigned long long)(io->block + b));
-			v->hash_failed = 1;
-			return -EIO;
+			if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA,
+					      io->block + b))
+				return -EIO;
 		}
 	}
 
@@ -546,6 +617,19 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 		else
 			for (x = 0; x < v->salt_size; x++)
 				DMEMIT("%02x", v->salt[x]);
+		if (v->mode != DM_VERITY_MODE_EIO) {
+			DMEMIT(" 1");
+			switch (v->mode) {
+			case DM_VERITY_MODE_LOGGING:
+				DMEMIT(DM_VERITY_OPT_LOGGING);
+				break;
+			case DM_VERITY_MODE_RESTART:
+				DMEMIT(DM_VERITY_OPT_RESTART);
+				break;
+			default:
+				BUG();
+			}
+		}
 		break;
 	}
 }
@@ -647,13 +731,19 @@ static void verity_dtr(struct dm_target *ti)
 static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 {
 	struct dm_verity *v;
-	unsigned num;
+	struct dm_arg_set as;
+	const char *opt_string;
+	unsigned int num, opt_params;
 	unsigned long long num_ll;
 	int r;
 	int i;
 	sector_t hash_position;
 	char dummy;
 
+	static struct dm_arg _args[] = {
+		{0, 1, "Invalid number of feature args"},
+	};
+
 	v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
 	if (!v) {
 		ti->error = "Cannot allocate verity structure";
@@ -668,8 +758,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}
 
-	if (argc != 10) {
-		ti->error = "Invalid argument count: exactly 10 arguments required";
+	if (argc < 10) {
+		ti->error = "Not enough arguments";
 		r = -EINVAL;
 		goto bad;
 	}
@@ -790,6 +880,38 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		}
 	}
 
+	argv += 10;
+	argc -= 10;
+
+	/* Optional parameters */
+	if (argc) {
+		as.argc = argc;
+		as.argv = argv;
+
+		r = dm_read_arg_group(_args, &as, &opt_params, &ti->error);
+		if (r)
+			goto bad;
+
+		while (opt_params--) {
+			opt_string = dm_shift_arg(&as);
+			if (!opt_string) {
+				ti->error = "Not enough feature arguments";
+				r = -EINVAL;
+				goto bad;
+			}
+
+			if (!strcasecmp(opt_string, DM_VERITY_OPT_LOGGING))
+				v->mode = DM_VERITY_MODE_LOGGING;
+			else if (!strcasecmp(opt_string, DM_VERITY_OPT_RESTART))
+				v->mode = DM_VERITY_MODE_RESTART;
+			else {
+				ti->error = "Invalid feature arguments";
+				r = -EINVAL;
+				goto bad;
+			}
+		}
+	}
+
 	v->hash_per_block_bits =
 		__fls((1 << v->hash_dev_block_bits) / v->digest_size);
 

Re: [PATCHv3] dm-verity: Add error handling modes for corrupted blocks

[Mike Snitzer]

On Wed, Mar 18, 2015 at 11:52 AM, Sami Tolvanen <samitolvanen@google.com> wrote:
> Add device specific modes to dm-verity to specify how corrupted
> blocks should be handled. The following modes are defined:
>
>   - DM_VERITY_MODE_EIO is the default behavior, where reading a
>     corrupted block results in -EIO.
>
>   - DM_VERITY_MODE_LOGGING only logs corrupted blocks, but does
>     not block the read.
>
>   - DM_VERITY_MODE_RESTART calls kernel_restart when a corrupted
>     block is discovered.
>
> In addition, each mode sends a uevent to notify userspace of
> corruption and to allow further recovery actions.
>
> The driver defaults to previous behavior (DM_VERITY_MODE_EIO)
> and other modes can be enabled with an additional parameter to
> the verity table.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

I staged this v3 with a few additional fixes (mainly whitespace and
nits) but I did need to export dm_disk() otherwise building dm-verity
as a module would fail.

See:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=cf987c1bc5d4c7c6aeed3648118eaa008c1abf8d