[PATCH] mtd-utils: add possibility to flag a static volume to skip CRC check when opening

[PATCH] mtd-utils: add possibility to flag a static volume to skip CRC check when opening

[Quentin Schulz]

From: Boris Brezillon <boris.brezillon@bootlin.com>

Some users of static UBI volumes implement their own integrity check,
thus making the volume CRC check done at open time by the kernel
useless. For instance, this is the case when one use the ubiblock +
dm-verity + squashfs combination, where dm-verity already checks
integrity of the block device but this time at the block granularity
instead of verifying the whole volume.

Skipping this test drastically improves the boot-time.

Let's add this feature to ubimkvol and ubinize so that we can flag a
static volume as where the CRC check should be skipped.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 include/libubi.h        |  2 ++
 include/mtd/ubi-media.h |  6 ++++++
 include/mtd/ubi-user.h  | 16 ++++++++++++++--
 lib/libubi.c            |  1 +
 ubi-utils/ubimkvol.c    | 19 ++++++++++++++++---
 ubi-utils/ubinize.c     |  7 +++++++
 6 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/include/libubi.h b/include/libubi.h
index 4d6a7ee..46596a3 100644
--- a/include/libubi.h
+++ b/include/libubi.h
@@ -69,6 +69,7 @@ struct ubi_attach_request
  * @bytes: volume size in bytes
  * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME)
  * @name: volume name
+ * @flags: volume flags
  */
 struct ubi_mkvol_request
 {
@@ -77,6 +78,7 @@ struct ubi_mkvol_request
 	long long bytes;
 	int vol_type;
 	const char *name;
+	uint8_t flags;
 };
 
 /**
diff --git a/include/mtd/ubi-media.h b/include/mtd/ubi-media.h
index 08bec3e..4db252f 100644
--- a/include/mtd/ubi-media.h
+++ b/include/mtd/ubi-media.h
@@ -61,6 +61,11 @@ enum {
  * Volume flags used in the volume table record.
  *
  * @UBI_VTBL_AUTORESIZE_FLG: auto-resize this volume
+ * @UBI_VTBL_SKIP_CRC_CHECK_FLG: skip the CRC check done on static volume at
+ *				 open time. Should only be set on volumes that
+ *				 are used by upper layers doing this kind of
+ *				 check. Main use-case for this flag is
+ *				 boot-time reduction
  *
  * %UBI_VTBL_AUTORESIZE_FLG flag can be set only for one volume in the volume
  * table. UBI automatically re-sizes the volume which has this flag and makes
@@ -92,6 +97,7 @@ enum {
  */
 enum {
 	UBI_VTBL_AUTORESIZE_FLG = 0x01,
+	UBI_VTBL_SKIP_CRC_CHECK_FLG = 0x02,
 };
 
 /*
diff --git a/include/mtd/ubi-user.h b/include/mtd/ubi-user.h
index 2b50dad..443f2e1 100644
--- a/include/mtd/ubi-user.h
+++ b/include/mtd/ubi-user.h
@@ -279,6 +279,18 @@ struct ubi_attach_req {
 	int8_t  padding[10];
 };
 
+/*
+ * UBI volume flags.
+ *
+ * @UBI_VOL_SKIP_CRC_CHECK_FLG: skip the CRC check done on static volumes at
+ *				open time. Only valid for static volumes and
+ *				should only be used if the volume user has a
+ *				way to verify data integrity
+ */
+enum {
+	UBI_VOL_SKIP_CRC_CHECK_FLG = 0x1,
+};
+
 /**
  * struct ubi_mkvol_req - volume description data structure used in
  *                        volume creation requests.
@@ -286,7 +298,7 @@ struct ubi_attach_req {
  * @alignment: volume alignment
  * @bytes: volume size in bytes
  * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME)
- * @padding1: reserved for future, not used, has to be zeroed
+ * @flags: volume flags (%UBI_VOL_SKIP_CRC_CHECK_FLG)
  * @name_len: volume name length
  * @padding2: reserved for future, not used, has to be zeroed
  * @name: volume name
@@ -315,7 +327,7 @@ struct ubi_mkvol_req {
 	int32_t alignment;
 	int64_t bytes;
 	int8_t vol_type;
-	int8_t padding1;
+	uint8_t flags;
 	int16_t name_len;
 	int8_t padding2[4];
 	char name[UBI_MAX_VOLUME_NAME + 1];
diff --git a/lib/libubi.c b/lib/libubi.c
index 978b433..4322a19 100644
--- a/lib/libubi.c
+++ b/lib/libubi.c
@@ -1000,6 +1000,7 @@ int ubi_mkvol(libubi_t desc, const char *node, struct ubi_mkvol_request *req)
 	r.alignment = req->alignment;
 	r.bytes = req->bytes;
 	r.vol_type = req->vol_type;
+	r.flags = req->flags;
 
 	n = strlen(req->name);
 	if (n > UBI_MAX_VOLUME_NAME)
diff --git a/ubi-utils/ubimkvol.c b/ubi-utils/ubimkvol.c
index b81fc99..6c641e5 100644
--- a/ubi-utils/ubimkvol.c
+++ b/ubi-utils/ubimkvol.c
@@ -44,6 +44,7 @@ struct args {
 	const char *name;
 	const char *node;
 	int maxavs;
+	int skipcheck;
 };
 
 static struct args args = {
@@ -68,16 +69,17 @@ static const char optionsstr[] =
 "                              eraseblocks\n"
 "-m, --maxavsize               set volume size to maximum available size\n"
 "-t, --type=<static|dynamic>   volume type (dynamic, static), default is dynamic\n"
+"-k, --skipcheck               skip the CRC check done at volume open time\n"
 "-h, -?, --help                print help message\n"
 "-V, --version                 print program version";
 
 
 static const char usage[] =
 "Usage: " PROGRAM_NAME " <UBI device node file name> [-h] [-a <alignment>] [-n <volume ID>] [-N <name>]\n"
-"\t\t\t[-s <bytes>] [-S <LEBs>] [-t <static|dynamic>] [-V] [-m]\n"
+"\t\t\t[-s <bytes>] [-S <LEBs>] [-t <static|dynamic>] [-V] [-m] [-k]\n"
 "\t\t\t[--alignment=<alignment>][--vol_id=<volume ID>] [--name=<name>]\n"
 "\t\t\t[--size=<bytes>] [--lebs=<LEBs>] [--type=<static|dynamic>] [--help]\n"
-"\t\t\t[--version] [--maxavsize]\n\n"
+"\t\t\t[--version] [--maxavsize] --[skipcheck]\n\n"
 "Example: " PROGRAM_NAME " /dev/ubi0 -s 20MiB -N config_data - create a 20 Megabytes volume\n"
 "         named \"config_data\" on UBI device /dev/ubi0.";
 
@@ -91,6 +93,7 @@ static const struct option long_options[] = {
 	{ .name = "help",      .has_arg = 0, .flag = NULL, .val = 'h' },
 	{ .name = "version",   .has_arg = 0, .flag = NULL, .val = 'V' },
 	{ .name = "maxavsize", .has_arg = 0, .flag = NULL, .val = 'm' },
+	{ .name = "skipcheck", .has_arg = 0, .flag = NULL, .val = 'k' },
 	{ NULL, 0, NULL, 0},
 };
 
@@ -113,6 +116,9 @@ static int param_sanity_check(void)
 	if (len > UBI_MAX_VOLUME_NAME)
 		return errmsg("too long name (%d symbols), max is %d", len, UBI_MAX_VOLUME_NAME);
 
+	if (args.skipcheck && args.vol_type != UBI_STATIC_VOLUME)
+		return errmsg("skipcheck is only valid for static volumes");
+
 	return 0;
 }
 
@@ -121,7 +127,7 @@ static int parse_opt(int argc, char * const argv[])
 	while (1) {
 		int key, error = 0;
 
-		key = getopt_long(argc, argv, "a:n:N:s:S:t:h?Vm", long_options, NULL);
+		key = getopt_long(argc, argv, "a:n:N:s:S:t:h?Vmk", long_options, NULL);
 		if (key == -1)
 			break;
 
@@ -183,6 +189,10 @@ static int parse_opt(int argc, char * const argv[])
 			args.maxavs = 1;
 			break;
 
+		case 'k':
+			args.skipcheck = 1;
+			break;
+
 		case ':':
 			return errmsg("parameter is missing");
 
@@ -266,6 +276,9 @@ int main(int argc, char * const argv[])
 	req.vol_type = args.vol_type;
 	req.name = args.name;
 
+	if (args.skipcheck)
+		req.flags |= UBI_VOL_SKIP_CRC_CHECK_FLG;
+
 	err = ubi_mkvol(libubi, args.node, &req);
 	if (err < 0) {
 		sys_errmsg("cannot UBI create volume");
diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
index c85ff9b..bb2cbc6 100644
--- a/ubi-utils/ubinize.c
+++ b/ubi-utils/ubinize.c
@@ -388,6 +388,9 @@ static int read_section(const struct ubigen_info *ui, const char *sname,
 		if (!strcmp(p, "autoresize")) {
 			verbose(args.verbose, "autoresize flags found");
 			vi->flags |= UBI_VTBL_AUTORESIZE_FLG;
+		} else if (!strcmp(p, "skip-check")) {
+			verbose(args.verbose, "skip-check flag found");
+			vi->flags |= UBI_VTBL_SKIP_CRC_CHECK_FLG;
 		} else {
 			return errmsg("unknown flags \"%s\" in section \"%s\"",
 				      p, sname);
@@ -523,6 +526,10 @@ int main(int argc, char * const argv[])
 			}
 		}
 
+		if (vi[i].flags & UBI_VTBL_SKIP_CRC_CHECK_FLG &&
+		    vi[i].type != UBI_VID_STATIC)
+			return errmsg("skip-check is only valid for static volumes");
+
 		if (vi[i].flags & UBI_VTBL_AUTORESIZE_FLG) {
 			if (autoresize_was_already)
 				return errmsg("only one volume is allowed "
-- 
2.14.1

Re: [PATCH] mtd-utils: add possibility to flag a static volume to skip CRC check when opening

[Boris Brezillon]

On Wed, 27 Jun 2018 13:38:23 +0200
Quentin Schulz <quentin.schulz@bootlin.com> wrote:

> From: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Some users of static UBI volumes implement their own integrity check,
> thus making the volume CRC check done at open time by the kernel
> useless. For instance, this is the case when one use the ubiblock +
> dm-verity + squashfs combination, where dm-verity already checks
> integrity of the block device but this time at the block granularity
> instead of verifying the whole volume.
> 
> Skipping this test drastically improves the boot-time.
> 
> Let's add this feature to ubimkvol and ubinize so that we can flag a
> static volume as where the CRC check should be skipped.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
>  include/libubi.h        |  2 ++
>  include/mtd/ubi-media.h |  6 ++++++
>  include/mtd/ubi-user.h  | 16 ++++++++++++++--
>  lib/libubi.c            |  1 +
>  ubi-utils/ubimkvol.c    | 19 ++++++++++++++++---
>  ubi-utils/ubinize.c     |  7 +++++++

Hm, I think you should split the changes in several patches.

1/ sync ubi-media.h and ubi-user.h with the (future) kernel version
2/ add support for this feature to libubi.{c,h}
3/ patch ubimkvol to support the skip-check feature
4/ patch ubinize to support the skip-check feature

>  6 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libubi.h b/include/libubi.h
> index 4d6a7ee..46596a3 100644
> --- a/include/libubi.h
> +++ b/include/libubi.h
> @@ -69,6 +69,7 @@ struct ubi_attach_request
>   * @bytes: volume size in bytes
>   * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME)
>   * @name: volume name
> + * @flags: volume flags
>   */
>  struct ubi_mkvol_request
>  {
> @@ -77,6 +78,7 @@ struct ubi_mkvol_request
>  	long long bytes;
>  	int vol_type;
>  	const char *name;
> +	uint8_t flags;
>  };
>  
>  /**
> diff --git a/include/mtd/ubi-media.h b/include/mtd/ubi-media.h
> index 08bec3e..4db252f 100644
> --- a/include/mtd/ubi-media.h
> +++ b/include/mtd/ubi-media.h
> @@ -61,6 +61,11 @@ enum {
>   * Volume flags used in the volume table record.
>   *
>   * @UBI_VTBL_AUTORESIZE_FLG: auto-resize this volume
> + * @UBI_VTBL_SKIP_CRC_CHECK_FLG: skip the CRC check done on static volume at
> + *				 open time. Should only be set on volumes that
> + *				 are used by upper layers doing this kind of
> + *				 check. Main use-case for this flag is
> + *				 boot-time reduction
>   *
>   * %UBI_VTBL_AUTORESIZE_FLG flag can be set only for one volume in the volume
>   * table. UBI automatically re-sizes the volume which has this flag and makes
> @@ -92,6 +97,7 @@ enum {
>   */
>  enum {
>  	UBI_VTBL_AUTORESIZE_FLG = 0x01,
> +	UBI_VTBL_SKIP_CRC_CHECK_FLG = 0x02,
>  };
>  
>  /*
> diff --git a/include/mtd/ubi-user.h b/include/mtd/ubi-user.h
> index 2b50dad..443f2e1 100644
> --- a/include/mtd/ubi-user.h
> +++ b/include/mtd/ubi-user.h
> @@ -279,6 +279,18 @@ struct ubi_attach_req {
>  	int8_t  padding[10];
>  };
>  
> +/*
> + * UBI volume flags.
> + *
> + * @UBI_VOL_SKIP_CRC_CHECK_FLG: skip the CRC check done on static volumes at
> + *				open time. Only valid for static volumes and
> + *				should only be used if the volume user has a
> + *				way to verify data integrity
> + */
> +enum {
> +	UBI_VOL_SKIP_CRC_CHECK_FLG = 0x1,
> +};
> +
>  /**
>   * struct ubi_mkvol_req - volume description data structure used in
>   *                        volume creation requests.
> @@ -286,7 +298,7 @@ struct ubi_attach_req {
>   * @alignment: volume alignment
>   * @bytes: volume size in bytes
>   * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME)
> - * @padding1: reserved for future, not used, has to be zeroed
> + * @flags: volume flags (%UBI_VOL_SKIP_CRC_CHECK_FLG)
>   * @name_len: volume name length
>   * @padding2: reserved for future, not used, has to be zeroed
>   * @name: volume name
> @@ -315,7 +327,7 @@ struct ubi_mkvol_req {
>  	int32_t alignment;
>  	int64_t bytes;
>  	int8_t vol_type;
> -	int8_t padding1;
> +	uint8_t flags;
>  	int16_t name_len;
>  	int8_t padding2[4];
>  	char name[UBI_MAX_VOLUME_NAME + 1];
> diff --git a/lib/libubi.c b/lib/libubi.c
> index 978b433..4322a19 100644
> --- a/lib/libubi.c
> +++ b/lib/libubi.c
> @@ -1000,6 +1000,7 @@ int ubi_mkvol(libubi_t desc, const char *node, struct ubi_mkvol_request *req)
>  	r.alignment = req->alignment;
>  	r.bytes = req->bytes;
>  	r.vol_type = req->vol_type;
> +	r.flags = req->flags;
>  
>  	n = strlen(req->name);
>  	if (n > UBI_MAX_VOLUME_NAME)
> diff --git a/ubi-utils/ubimkvol.c b/ubi-utils/ubimkvol.c
> index b81fc99..6c641e5 100644
> --- a/ubi-utils/ubimkvol.c
> +++ b/ubi-utils/ubimkvol.c
> @@ -44,6 +44,7 @@ struct args {
>  	const char *name;
>  	const char *node;
>  	int maxavs;
> +	int skipcheck;
>  };
>  
>  static struct args args = {
> @@ -68,16 +69,17 @@ static const char optionsstr[] =
>  "                              eraseblocks\n"
>  "-m, --maxavsize               set volume size to maximum available size\n"
>  "-t, --type=<static|dynamic>   volume type (dynamic, static), default is dynamic\n"
> +"-k, --skipcheck               skip the CRC check done at volume open time\n"
>  "-h, -?, --help                print help message\n"
>  "-V, --version                 print program version";
>  
>  
>  static const char usage[] =
>  "Usage: " PROGRAM_NAME " <UBI device node file name> [-h] [-a <alignment>] [-n <volume ID>] [-N <name>]\n"
> -"\t\t\t[-s <bytes>] [-S <LEBs>] [-t <static|dynamic>] [-V] [-m]\n"
> +"\t\t\t[-s <bytes>] [-S <LEBs>] [-t <static|dynamic>] [-V] [-m] [-k]\n"
>  "\t\t\t[--alignment=<alignment>][--vol_id=<volume ID>] [--name=<name>]\n"
>  "\t\t\t[--size=<bytes>] [--lebs=<LEBs>] [--type=<static|dynamic>] [--help]\n"
> -"\t\t\t[--version] [--maxavsize]\n\n"
> +"\t\t\t[--version] [--maxavsize] --[skipcheck]\n\n"
>  "Example: " PROGRAM_NAME " /dev/ubi0 -s 20MiB -N config_data - create a 20 Megabytes volume\n"
>  "         named \"config_data\" on UBI device /dev/ubi0.";
>  
> @@ -91,6 +93,7 @@ static const struct option long_options[] = {
>  	{ .name = "help",      .has_arg = 0, .flag = NULL, .val = 'h' },
>  	{ .name = "version",   .has_arg = 0, .flag = NULL, .val = 'V' },
>  	{ .name = "maxavsize", .has_arg = 0, .flag = NULL, .val = 'm' },
> +	{ .name = "skipcheck", .has_arg = 0, .flag = NULL, .val = 'k' },
>  	{ NULL, 0, NULL, 0},
>  };
>  
> @@ -113,6 +116,9 @@ static int param_sanity_check(void)
>  	if (len > UBI_MAX_VOLUME_NAME)
>  		return errmsg("too long name (%d symbols), max is %d", len, UBI_MAX_VOLUME_NAME);
>  
> +	if (args.skipcheck && args.vol_type != UBI_STATIC_VOLUME)
> +		return errmsg("skipcheck is only valid for static volumes");
> +
>  	return 0;
>  }
>  
> @@ -121,7 +127,7 @@ static int parse_opt(int argc, char * const argv[])
>  	while (1) {
>  		int key, error = 0;
>  
> -		key = getopt_long(argc, argv, "a:n:N:s:S:t:h?Vm", long_options, NULL);
> +		key = getopt_long(argc, argv, "a:n:N:s:S:t:h?Vmk", long_options, NULL);
>  		if (key == -1)
>  			break;
>  
> @@ -183,6 +189,10 @@ static int parse_opt(int argc, char * const argv[])
>  			args.maxavs = 1;
>  			break;
>  
> +		case 'k':
> +			args.skipcheck = 1;
> +			break;
> +
>  		case ':':
>  			return errmsg("parameter is missing");
>  
> @@ -266,6 +276,9 @@ int main(int argc, char * const argv[])
>  	req.vol_type = args.vol_type;
>  	req.name = args.name;
>  
> +	if (args.skipcheck)
> +		req.flags |= UBI_VOL_SKIP_CRC_CHECK_FLG;
> +
>  	err = ubi_mkvol(libubi, args.node, &req);
>  	if (err < 0) {
>  		sys_errmsg("cannot UBI create volume");
> diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
> index c85ff9b..bb2cbc6 100644
> --- a/ubi-utils/ubinize.c
> +++ b/ubi-utils/ubinize.c
> @@ -388,6 +388,9 @@ static int read_section(const struct ubigen_info *ui, const char *sname,
>  		if (!strcmp(p, "autoresize")) {
>  			verbose(args.verbose, "autoresize flags found");
>  			vi->flags |= UBI_VTBL_AUTORESIZE_FLG;
> +		} else if (!strcmp(p, "skip-check")) {
> +			verbose(args.verbose, "skip-check flag found");
> +			vi->flags |= UBI_VTBL_SKIP_CRC_CHECK_FLG;

This works because autoresize and skip-check do not apply to the same
volume types (autoresize only applies to dynamic volumes and
skip-check is only valid for static ones), hence you can't have both of
them set at the same time. Still, I think it would be better to
implement a real parsing of the vol_flags prop (use ',' as a separator
between flags for example).

I'm not necessarily asking that you implement that parsing now, but you
should at least add a big fat comment explaining why this is valid for
now and what should be done if we have to support new flags.

>  		} else {
>  			return errmsg("unknown flags \"%s\" in section \"%s\"",
>  				      p, sname);
> @@ -523,6 +526,10 @@ int main(int argc, char * const argv[])
>  			}
>  		}
>  
> +		if (vi[i].flags & UBI_VTBL_SKIP_CRC_CHECK_FLG &&
> +		    vi[i].type != UBI_VID_STATIC)
> +			return errmsg("skip-check is only valid for static volumes");
> +
>  		if (vi[i].flags & UBI_VTBL_AUTORESIZE_FLG) {
>  			if (autoresize_was_already)
>  				return errmsg("only one volume is allowed "