Hi Denis,
On 2026-06-05T03:38:59, None <[email protected]> wrote:
> disk: efi: introduce {read,write}_disk_flags
>
> Introduce helper functions for updating top 16 bits of GUID partition
> table entry attributes. The value can be used to store extra information
> about bootflow, such as counters and status.
>
> Signed-off-by: Denis Mukhin <[email protected]>
>
> disk/part_efi.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/part.h | 24 +++++++++++
> 2 files changed, 153 insertions(+)
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -980,6 +980,135 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
> void *buf)
> +int read_disk_flags(struct blk_desc *desc, int partnum, u16 *flags)
> +int write_disk_flags(struct blk_desc *desc, int partnum, u16 flags)
These names are too generic - they are not disk operations, they
read/write the top 16 bits of a GPT PTE in a ChromiumOS-specific
layout. Please rename to gpt_read_flags()/gpt_write_flags() and put
the prototypes alongside the other GPT helpers (next to
part_get_gpt_pte()). As-is, a casual reader would expect them to work
on any partition table.
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -980,6 +980,135 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
> void *buf)
> +int read_disk_flags(struct blk_desc *desc, int partnum, u16 *flags)
> +{
> + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, desc->blksz);
> + gpt_entry *gpt_pte = NULL;
> + u64 attrs;
> +
> + /* 'part' argument must be at least 1 */
> + if (partnum < 1)
> + return -EINVAL;
> +
> + gpt_repair_headers(desc);
A read must not mutate the disk. gpt_repair_headers() performs writes
when one of the two headers is bad - please drop this call from
read_disk_flags(). The return value is also ignored in both functions:
if both GPTs are invalid the caller falls through into
find_valid_gpt()/is_gpt_valid() anyway, so the call adds nothing for
the read path and should be the caller's choice for the write path.
The doxygen says flags cannot be NULL but the function does not check
it - either drop the comment or add a check.
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -980,6 +980,135 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
> void *buf)
> + if (find_valid_gpt(desc, gpt_head, &gpt_pte) != 1)
> + return -EINVAL;
> +
> + if (partnum > le32_to_cpu(gpt_head->num_partition_entries) ||
> + !is_pte_valid(&gpt_pte[partnum - 1])) {
> + free(gpt_pte);
> + return -EPERM;
> + }
...
> + rc = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_h1,
> &gpt_e1);
> + if (!rc) {
> + rc = -EPERM;
> + goto out;
> + }
> + if (partnum > le32_to_cpu(gpt_h1->num_partition_entries) ||
> + !is_pte_valid(&gpt_e1[partnum - 1])) {
> + rc = -EINVAL;
> + goto out;
> + }
The errno choices are odd and inconsistent. -EPERM (operation not
permitted) does not describe 'GPT could not be parsed' or 'partition
does not exist'. read returns -EINVAL for invalid GPT and -EPERM for
bad partnum; write swaps them. I suggest -EIO for the unreadable-GPT
case and -ENOENT for 'partition does not exist', consistently.
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -980,6 +980,135 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
> void *buf)
> + rc = is_gpt_valid(desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_h1,
> &gpt_e1);
...
> + attrs = le64_to_cpu(gpt_e1[partnum - 1].attributes.raw);
> + attrs &= ~(0xFFFFULL << 48);
Please use lower-case hex.
> + attrs |= ((u64)flags << 48);
> + gpt_e1[partnum - 1].attributes.raw = cpu_to_le64(attrs);
> +
> + rc = write_gpt_pte(desc, gpt_h1, gpt_e1);
...
> + rc = is_gpt_valid(desc, desc->lba - 1, gpt_h2, &gpt_e2);
...
> + attrs = le64_to_cpu(gpt_e2[partnum - 1].attributes.raw);
> + attrs &= ~(0xFFFFULL << 48);
> + attrs |= ((u64)flags << 48);
> + gpt_e2[partnum - 1].attributes.raw = cpu_to_le64(attrs);
> +
> + rc = write_gpt_pte(desc, gpt_h2, gpt_e2);
Two copies of the same logic varying only by LBA. Please factor into a
helper that takes the LBA (or a primary/backup flag) and call it
twice. Also please #define a constant for the bit-48 shift
(CROS_FLAGS_SHIFT, or similar in cros_flags.h) rather than open-coding
'<< 48' and '0xFFFFULL << 48' in three places.
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -980,6 +980,135 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
> void *buf)
> + rc = blk_dwrite(desc, le64_to_cpu(gpt_h->my_lba), 1, gpt_h);
> + if (rc != 1) {
> + pr_err("device %d: failed to update GPT header: %d\n",
> + desc->devnum, rc);
> + return rc ?: -EIO;
> + }
> +
> + rc = blk_dwrite(desc, le64_to_cpu(gpt_h->partition_entry_lba), cnt,
> gpt_e);
Please write the PTE array first, then the header - that is what
write_gpt_table() does, and it is the safer ordering: a torn write
leaves the on-disk header pointing at a CRC that does not match the
on-disk PTE if you do header-first.
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -980,6 +980,135 @@ int write_mbr_and_gpt_partitions(struct blk_desc *desc,
> void *buf)
> +static int write_gpt_pte(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry
> *gpt_e)
> +{
> + const int cnt = BLOCK_CNT((le32_to_cpu(gpt_h->num_partition_entries)
> + * sizeof(gpt_entry)), desc);
If primary write succeeds but the backup write fails (bad block at end
of disk, etc.) you leave the two GPTs out of sync and return an error
to the caller, who has no way to know what state the disk is in.
Please mention this in the function comment, and consider attempting
to roll back / retry rather than silently leaving an inconsistent
state. Yes I know this shouldn't happen, but power can fail, device
can reset, etc.
Regards,
Simon
