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

Hi,

On Fri, 2 Jun 2017 19:22:33 -0700 alison@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