[U-Boot] [PATCH v3 4/5] rename GPT partitions to detect boot failure

["Lothar Waßmann" <LW@KARO-electronics.de>]

Hi,

On Fri,  2 Jun 2017 19:22:33 -0700 alison at peloton-tech.com wrote:
> From: Alison Chaiken <alison@peloton-tech.com>
> 
> This patch provides support in u-boot for renaming GPT
> partitions.  The renaming is accomplished via a new 'gpt flip'
> command.
> 
> The concept for the bootloader state machine is the following:
> 
> -- u-boot renames ‘primary’ partitions as ‘candidate’ and tries
>    to boot them.
> -- Linux, at boot, will rename ‘candidate’ partitions as
>    ‘primary’.
> -- If u-boot sees a ‘candidate’ partition after a boot attempt,
>    it tries to boot the ‘backup’ partition.
> 
> Rewriting the partition table has the side-effect that all partitions
> end up with "msftdata" flag set.  The reason is that partition type
> PARTITION_BASIC_DATA_GUID is hard-coded in the gpt_fill_pte()
> function.  This does not appear to cause any harm.
> 
> Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
> ---
>  cmd/Kconfig    |   7 ++
>  cmd/gpt.c      | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  doc/README.gpt |  13 ++++
>  3 files changed, 215 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 6f75b86..8b925e5 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -593,6 +593,13 @@ config CMD_GPT
>  	  Enable the 'gpt' command to ready and write GPT style partition
>  	  tables.
>  
> +config CMD_GPT_FLIP
> +	bool "GPT flip-partitions command"
> +	depends on CMD_GPT
> +	help
> +	  Enables the 'gpt' command to write modified GPT partition
> +	  tables via the 'gpt flip' command.
> +
>  config CMD_ARMFLASH
>  	#depends on FLASH_CFI_DRIVER
>  	bool "armflash"
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 5c2651f..f6968de 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -20,6 +20,7 @@
>  #include <div64.h>
>  #include <memalign.h>
>  #include <linux/compat.h>
> +#include <linux/sizes.h>
>  
>  static LIST_HEAD(disk_partitions);
>  
> @@ -190,16 +191,33 @@ static struct disk_part *allocate_disk_part(disk_partition_t *info, int partnum)
>  	return newpart;
>  }
>  
> +static void prettyprint_part_size(char *sizestr, unsigned long partsize,
> +				  unsigned long blksize)
> +{
> +	unsigned long long partbytes;
> +	unsigned long partmegabytes;
> +
> +	partbytes = partsize * blksize;
> +	partmegabytes = lldiv(partbytes, SZ_1M);
> +	snprintf(sizestr, 16, "%luMiB", partmegabytes);
> +}
> +
>  static void print_gpt_info(void)
>  {
>  	struct list_head *pos;
>  	struct disk_part *curr;
> +	char partstartstr[16];
> +	char partsizestr[16];
>  
>  	list_for_each(pos, &disk_partitions) {
>  		curr = list_entry(pos, struct disk_part, list);
> +		prettyprint_part_size(partstartstr, (unsigned long)curr->gpt_part_info.start,
> +				      (unsigned long) curr->gpt_part_info.blksz);
> +		prettyprint_part_size(partsizestr, (unsigned long)curr->gpt_part_info.size,
> +				      (unsigned long) curr->gpt_part_info.blksz);
> +
>  		printf("Partition %d:\n", curr->partnum);
> -		printf("1st block %x, size %x\n", (unsigned)curr->gpt_part_info.start,
> -		       (unsigned)curr->gpt_part_info.size);
> +		printf("Start %s, size %s\n", partstartstr, partsizestr);
>  		printf("Block size %lu, name %s\n", curr->gpt_part_info.blksz,
>  		       curr->gpt_part_info.name);
>  		printf("Type %s, bootable %d\n", curr->gpt_part_info.type,
> @@ -211,6 +229,85 @@ static void print_gpt_info(void)
>  	}
>  }
>  
> +#ifdef CONFIG_CMD_GPT_FLIP
> +static int calc_parts_list_len(int numparts)
> +{
> +	/*
> +	 * prefatory string:
> +	 * doc/README.GPT, suggests that
> +	 * int partlistlen = UUID_STR_LEN + 1 + strlen("partitions=uuid_disk=");
> +	 * is correct, but extract_val() expects "uuid_disk" first.
> +	 */
> +	int partlistlen = UUID_STR_LEN + 1 + strlen("uuid_disk=");
> +	/* for the comma */
> +	partlistlen++;
> +
> +	/* per-partition additions; numparts starts at 1, so this should be correct */
> +	partlistlen += numparts * (strlen("name=,") + PART_NAME_LEN + 1);
> +	/* 17 because partstr in create_gpt_partitions_list() is 16 chars */
>
NO! The size of partstr in create_gpt_partitions_list() is:
|	char partstr[PART_NAME_LEN + 1];
which happens to be defined as:
--- a/include/part.h
+++ b/include/part.h
@@ -49,6 +49,7 @@ struct block_drvr {
 
 #define PART_NAME_LEN 32
                       ^^
 #define PART_TYPE_LEN 32

> +	partlistlen += numparts * (strlen("start=MiB,") + 17);
> +	partlistlen += numparts * (strlen("size=MiB,") + 17);
>
Never use magic numbers in code, but use appropriate #define's!

> @@ -523,6 +625,86 @@ static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_CMD_GPT_FLIP
> +static int do_flip_gpt_parts(struct blk_desc *dev_desc)
> +{
> +	struct list_head *pos;
> +	struct disk_part *curr;
> +	disk_partition_t *new_partitions = NULL;
> +	char disk_guid[UUID_STR_LEN + 1];
> +	char *partitions_list, *str_disk_guid;
> +	u8 part_count = 0;
> +	int partlistlen, ret, numparts = 0;
> +
> +	ret = get_disk_guid(dev_desc, disk_guid);
> +	if (ret < 0)
> +		return ret;
AGAIN: You are passing the return value of this function back to the
caller of the CMD handler do_gpt() which expects a return value of
CMD_RET_FAILURE or CMD_RET_SUCCESS, not an arbitrary negative value.

> +
> +	numparts = get_gpt_info(dev_desc);
> +	if (numparts <  0)
> +		return numparts;
> +	printf("Current partition table with %d partitions is:\n", numparts);
> +	print_gpt_info();
> +
> +	partlistlen = calc_parts_list_len(numparts);
> +	partitions_list = (char *)malloc(partlistlen);
> +	memset(partitions_list, '\0', partlistlen);
> +
> +	ret = create_gpt_partitions_list(numparts, (const char *) disk_guid,
> +					 partitions_list);
> +	if (ret < 0)
> +		return ret;
> +	debug("OLD partitions_list is %s with %d chars\n", partitions_list, strlen(partitions_list));
> +
> +	ret = set_gpt_info(dev_desc, (const char *)partitions_list, &str_disk_guid,
> +			   &new_partitions, &part_count);
> +	if (ret < 0)
> +		return ret;
> +
> +	list_for_each(pos, &disk_partitions) {
> +		curr = list_entry(pos, struct disk_part, list);
> +		if (!strcmp((char *)curr->gpt_part_info.name, "backup_kernel"))
> +			strcpy((char *)curr->gpt_part_info.name, "candidate_kernel");
> +		if (!strcmp((char *)curr->gpt_part_info.name, "primary_kernel"))
> +			strcpy((char *)curr->gpt_part_info.name, "backup_kernel");
> +		if (!strcmp((char *)curr->gpt_part_info.name, "backup_rootfs"))
> +			strcpy((char *)curr->gpt_part_info.name, "candidate_rootfs");
> +		if (!strcmp((char *)curr->gpt_part_info.name, "primary_rootfs"))
> +			strcpy((char *)curr->gpt_part_info.name, "backup_rootfs");
> +	}
> +
> +	ret = create_gpt_partitions_list(numparts, (const char *) disk_guid, partitions_list);
> +	if (ret < 0)
> +		return ret;
> +	debug("NEW partitions_list is %s with %d chars\n", partitions_list, strlen(partitions_list));
> +
> +	ret = set_gpt_info(dev_desc, (const char *)partitions_list, &str_disk_guid,
> +			   &new_partitions, &part_count);
> +	if (ret < 0)
> +		return ret;
> +
> +	debug("Writing new partition table\n");
> +	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
> +	if (ret < 0) {
> +		printf("Writing new partition table failed\n");
> +		return ret;
> +	}
> +
> +	debug("Reading back new partition table\n");
> +	numparts = get_gpt_info(dev_desc);
> +	if (numparts <  0)
> +		return numparts;
> +	printf("new partition table with %d partitions is:\n", numparts);
> +	print_gpt_info();
> +
> +	del_gpt_info();
> +	free(partitions_list);
> +	free(str_disk_guid);
> +	free(new_partitions);
> +	return ret;
> +}
> +#endif
> +
>  /**
>   * do_gpt(): Perform GPT operations
>   *
> @@ -565,6 +747,10 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		return do_disk_guid(blk_dev_desc, argv[4]);
>  	} else if (strcmp(argv[1], "read") == 0) {
>  		return do_get_gpt_info(blk_dev_desc);
> +#ifdef CONFIG_CMD_GPT_FLIP
> +	} else if (strcmp(argv[1], "flip") == 0) {
> +		return do_flip_gpt_parts(blk_dev_desc);
> +#endif
>
See my comment to your patch 1/5.


Lothar Waßmann