| Message ID | 20150316155559.GA32397@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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); > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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);
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(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel