[PATCH 0/8] partitions/efi: detect hybrid mbrs

[PATCH 0/8] partitions/efi: detect hybrid mbrs

[Davidlohr Bueso]

This patchset teaches the kernel about hybrid master boot records (MBRs), one of
the most common issues with GUID partition tables, as a workaround to layout
disk partitions to be compatible with both EFI and legacy MBR based systems.
Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
functionality is left unchanged - we only inform (through debug) the user about
the used MBR scheme. While it is true that these restrictions can be bypassed when
forcing gpt, this is not the correct or default way of doing things, complicating
users furthermore. More details are in the individual patches.

Patches 1-5 enables the kernel to inform the user about the mbr scheme being used.
They also include more protective mbr checks to be more UEFI compliant - we currently
have a very open and generic gpt implementation that can cause non-GPT disks to be
recognized/probed as GPT.

Patch 6 adds a missing check when verifying the header integrity.

Patches 7 & 8 are trivial cleanups.

All changes were tested on a macbook pro containing a hybrid mbr and a large EFI based 
HP server with a standard protective mbr.

Thanks!

Davidlohr Bueso (8):
  partitions/efi: use lba-aware partition records
  partitions/efi: check pmbr record's starting lba
  partitions/efi: do not require gpt partition to begin at sector 1
  partitions/efi: detect hybrid MBRs
  partitions/efi: account for pmbr size in lba
  partitions/efi: compare first and last usable LBAs
  partitions/efi: delete annoying emacs style comments
  partitions/efi: some style cleanups

 block/partitions/efi.c | 128 ++++++++++++++++++++++++++++++++++---------------
 block/partitions/efi.h |  38 +++++++--------
 2 files changed, 108 insertions(+), 58 deletions(-)

-- 
1.7.11.7

[PATCH 1/8] partitions/efi: use lba-aware partition records

[Davidlohr Bueso]

The kernel's GPT implementation currently uses the generic
'struct partition' type for dealing with legacy MBR partition
records. While this is is useful for disklabels that we designed
for CHS addressing, such as msdos, it doesn't adapt well to newer
standards that use LBA instead, such as GUID partition tables.
Furthermore, these generic partition structures do not have all the
required fields to properly follow the UEFI specs.

While a CHS address can be translated to LBA, it's much simpler and
cleaner to just replace the partition type. This patch adds a new
'gpt_record' type that is fully compliant with EFI and will allow,
in the next patches, to add more checks to properly verify a protective
MBR, which is paramount to probing a device that makes use of GPT.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c |  7 +++----
 block/partitions/efi.h | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index c85fc89..3ebd3d8 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -149,11 +149,10 @@ static u64 last_lba(struct block_device *bdev)
 		       bdev_logical_block_size(bdev)) - 1ULL;
 }
 
-static inline int
-pmbr_part_valid(struct partition *part)
+static inline int pmbr_part_valid(gpt_record *part)
 {
-        if (part->sys_ind == EFI_PMBR_OSTYPE_EFI_GPT &&
-            le32_to_cpu(part->start_sect) == 1UL)
+        if (part->os_type == EFI_PMBR_OSTYPE_EFI_GPT &&
+            le32_to_cpu(part->start_sector) == 1UL)
                 return 1;
         return 0;
 }
diff --git a/block/partitions/efi.h b/block/partitions/efi.h
index b69ab72..46cf1a4 100644
--- a/block/partitions/efi.h
+++ b/block/partitions/efi.h
@@ -101,11 +101,25 @@ typedef struct _gpt_entry {
 	efi_char16_t partition_name[72 / sizeof (efi_char16_t)];
 } __attribute__ ((packed)) gpt_entry;
 
+typedef struct _gpt_record {
+        u8      boot_indicator; /* unused by EFI, set to 0x80 for bootable */
+        u8      start_head;     /* unused by EFI, pt start in CHS */
+        u8      start_sector;   /* unused by EFI, pt start in CHS */
+        u8      start_track;
+        u8      os_type;        /* EFI and legacy non-EFI OS types */
+        u8      end_head;       /* unused by EFI, pt end in CHS */
+        u8      end_sector;     /* unused by EFI, pt end in CHS */
+        u8      end_track;      /* unused by EFI, pt end in CHS */
+        __le32  starting_lba;   /* used by EFI - start addr of the on disk pt */
+        __le32  size_in_lba;    /* used by EFI - size of pt in LBA */
+} __attribute__ ((packed)) gpt_record;
+
+
 typedef struct _legacy_mbr {
 	u8 boot_code[440];
 	__le32 unique_mbr_signature;
 	__le16 unknown;
-	struct partition partition_record[4];
+	gpt_record partition_record[4];
 	__le16 signature;
 } __attribute__ ((packed)) legacy_mbr;
 
-- 
1.7.11.7

[PATCH 2/8] partitions/efi: check pmbr record's starting lba

[Davidlohr Bueso]

Per the UEFI Specs 2.4, June 2013, the starting lba of the partition
that has the EFI GPT (0xEE) must be set to 0x00000001 - this is obviously
the LBA of the GPT Partition Header.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 3ebd3d8..6a997b1 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -151,9 +151,18 @@ static u64 last_lba(struct block_device *bdev)
 
 static inline int pmbr_part_valid(gpt_record *part)
 {
-        if (part->os_type == EFI_PMBR_OSTYPE_EFI_GPT &&
-            le32_to_cpu(part->start_sector) == 1UL)
-                return 1;
+        if (part->os_type != EFI_PMBR_OSTYPE_EFI_GPT)
+                goto invalid;
+
+        /* set to 0x00000001 (i.e., the LBA of the GPT Partition Header) */
+        if (le32_to_cpu(part->starting_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
+                goto invalid;
+
+        if (le32_to_cpu(part->start_sector) != 1UL)
+                goto invalid;
+
+        return 1;
+invalid:
         return 0;
 }
 
-- 
1.7.11.7

[PATCH 3/8] partitions/efi: do not require gpt partition to begin at sector 1

[Davidlohr Bueso]

When detecting a valid protective MBR, the Linux kernel isn't picky about
the partition (1-4) the 0xEE is at, but, unlike other operating systems,
it does require it to begin at the second sector (sector 1). This check, apart
from it not being enforced by UEFI, and causing Linux to potentially fail to detect
any *valid* partitions on the disk, can present problems when dealing with hybrid
MBRs[1].

For compatibility reasons, if the first partition is hybridized, the 0xEE
partition must be small enough to ensure that it only protects the GPT data
structures - as opposed to the the whole disk in a protective MBR.
This problem is very well described by Rod Smith[1]: where MBR-only partitioning
programs (such as older versions of fdisk) can see some of the disk space as
unallocated, thus loosing the purpose of the 0xEE partition's protection of GPT
data structures.

By dropping this check, this patch enables Linux to be more flexible when probing
for GPT disklabels.

[1] http://www.rodsbooks.com/gdisk/hybrid.html#reactions

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 6a997b1..331cd1c 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -158,12 +158,9 @@ static inline int pmbr_part_valid(gpt_record *part)
         if (le32_to_cpu(part->starting_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
                 goto invalid;
 
-        if (le32_to_cpu(part->start_sector) != 1UL)
-                goto invalid;
-
-        return 1;
+	return 1;
 invalid:
-        return 0;
+	return 0;
 }
 
 /**
-- 
1.7.11.7

[PATCH 4/8] partitions/efi: detect hybrid MBRs

[Davidlohr Bueso]

One of the biggest problems with GPT is compatibility with older,
non-GPT systems. The problem is addressed by creating hybrid mbrs,
an extension, or variant, of the traditional protective mbr. This
contains, apart from the 0xEE partition, up three additional
primary partitions that point to the same space marked by up to
three GPT partitions. The result is that legacy OSs can see the
three required MBR partitions and at the same time ignore the
GPT-aware partitions that protect the GPT structures.

While hybrid MBRs are hacks, workarounds and simply not part of the
GPT standard, they do exist and we have no way around them. For instance,
by default, OSX creates a hybrid scheme when using multi-OS booting.

In order for Linux to properly discover protective MBRs, it must be
made aware of devices that have hybrid MBRs. No functionality is
changed by this patch, just a debug message informing the user of the
MBR scheme that is being used.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 72 +++++++++++++++++++++++++++++++++++---------------
 block/partitions/efi.h |  3 +++
 2 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 331cd1c..4bf8165 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -158,7 +158,7 @@ static inline int pmbr_part_valid(gpt_record *part)
         if (le32_to_cpu(part->starting_lba) != GPT_PRIMARY_PARTITION_TABLE_LBA)
                 goto invalid;
 
-	return 1;
+	return GPT_MBR_PROTECTIVE;
 invalid:
 	return 0;
 }
@@ -167,21 +167,47 @@ invalid:
  * is_pmbr_valid(): test Protective MBR for validity
  * @mbr: pointer to a legacy mbr structure
  *
- * Description: Returns 1 if PMBR is valid, 0 otherwise.
- * Validity depends on two things:
+ * Description: Checks for a valid protective or hybrid
+ * master boot record (MBR). The validity of a pMBR depends
+ * on all of the following properties:
  *  1) MSDOS signature is in the last two bytes of the MBR
  *  2) One partition of type 0xEE is found
+ *
+ * In addition, a hybrid MBR will have up to three additional
+ * primary partitions, which point to the same space that's
+ * marked out by up to three GPT partitions.
+ *
+ * Returns 0 upon invalid MBR, or GPT_MBR_PROTECTIVE or
+ * GPT_MBR_HYBRID depending on the device layout.
  */
-static int
-is_pmbr_valid(legacy_mbr *mbr)
+static int is_pmbr_valid(legacy_mbr *mbr)
 {
-	int i;
+	int i, ret = 0; /* invalid by default */
+
 	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
-                return 0;
+		goto done;
+
+	for (i = 0; i < 4; i++) {
+		ret = pmbr_part_valid(&mbr->partition_record[i]);
+		if (ret == GPT_MBR_PROTECTIVE) {
+			/*
+			 * Ok, we at least know that there's a protective MBR,
+			 * now check if there are other partition types for
+			 * hybrid MBR.
+			 */
+			goto check_hybrid;
+		}
+	}
+
+	if (ret != GPT_MBR_PROTECTIVE)
+		goto done;
+check_hybrid:
 	for (i = 0; i < 4; i++)
-		if (pmbr_part_valid(&mbr->partition_record[i]))
-                        return 1;
-	return 0;
+		if ((mbr->partition_record[i].os_type != EFI_PMBR_OSTYPE_EFI_GPT) &&
+		    (mbr->partition_record[i].os_type != 0x00))
+			ret = GPT_MBR_HYBRID;
+done:
+	return ret;
 }
 
 /**
@@ -548,17 +574,21 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 
 	lastlba = last_lba(state->bdev);
         if (!force_gpt) {
-                /* This will be added to the EFI Spec. per Intel after v1.02. */
-                legacymbr = kzalloc(sizeof (*legacymbr), GFP_KERNEL);
-                if (legacymbr) {
-                        read_lba(state, 0, (u8 *) legacymbr,
-				 sizeof (*legacymbr));
-                        good_pmbr = is_pmbr_valid(legacymbr);
-                        kfree(legacymbr);
-                }
-                if (!good_pmbr)
-                        goto fail;
-        }
+		/* This will be added to the EFI Spec. per Intel after v1.02. */
+		legacymbr = kzalloc(sizeof (*legacymbr), GFP_KERNEL);
+		if (!legacymbr)
+			goto fail;
+
+		read_lba(state, 0, (u8 *) legacymbr, sizeof (*legacymbr));
+		good_pmbr = is_pmbr_valid(legacymbr);
+		kfree(legacymbr);
+
+		if (!good_pmbr)
+			goto fail;
+
+		pr_debug("Device has a %s MBR\n",
+			 good_pmbr == GPT_MBR_PROTECTIVE ? "protective" : "hybrid");
+	}
 
 	good_pgpt = is_gpt_valid(state, GPT_PRIMARY_PARTITION_TABLE_LBA,
 				 &pgpt, &pptes);
diff --git a/block/partitions/efi.h b/block/partitions/efi.h
index 46cf1a4..e9741de 100644
--- a/block/partitions/efi.h
+++ b/block/partitions/efi.h
@@ -37,6 +37,9 @@
 #define EFI_PMBR_OSTYPE_EFI 0xEF
 #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
 
+#define GPT_MBR_PROTECTIVE  1
+#define GPT_MBR_HYBRID      2
+
 #define GPT_HEADER_SIGNATURE 0x5452415020494645ULL
 #define GPT_HEADER_REVISION_V1 0x00010000
 #define GPT_PRIMARY_PARTITION_TABLE_LBA 1
-- 
1.7.11.7

[PATCH 5/8] partitions/efi: account for pmbr size in lba

[Davidlohr Bueso]

The partition that has the 0xEE (GPT protective), must
have the size in lba field set to the lesser of the size
of the disk minus one or 0xFFFFFFFF for larger disks.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 4bf8165..ab6cd08 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -166,6 +166,7 @@ invalid:
 /**
  * is_pmbr_valid(): test Protective MBR for validity
  * @mbr: pointer to a legacy mbr structure
+ * @total_sectors: amount of sectors in the device
  *
  * Description: Checks for a valid protective or hybrid
  * master boot record (MBR). The validity of a pMBR depends
@@ -180,9 +181,9 @@ invalid:
  * Returns 0 upon invalid MBR, or GPT_MBR_PROTECTIVE or
  * GPT_MBR_HYBRID depending on the device layout.
  */
-static int is_pmbr_valid(legacy_mbr *mbr)
+static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
 {
-	int i, ret = 0; /* invalid by default */
+	int i, part = 0, ret = 0; /* invalid by default */
 
 	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
 		goto done;
@@ -190,6 +191,7 @@ static int is_pmbr_valid(legacy_mbr *mbr)
 	for (i = 0; i < 4; i++) {
 		ret = pmbr_part_valid(&mbr->partition_record[i]);
 		if (ret == GPT_MBR_PROTECTIVE) {
+			part = i;
 			/*
 			 * Ok, we at least know that there's a protective MBR,
 			 * now check if there are other partition types for
@@ -206,6 +208,18 @@ check_hybrid:
 		if ((mbr->partition_record[i].os_type != EFI_PMBR_OSTYPE_EFI_GPT) &&
 		    (mbr->partition_record[i].os_type != 0x00))
 			ret = GPT_MBR_HYBRID;
+
+	/*
+	 * Protective MBRs take up the lesser of the whole disk
+	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
+	 *
+	 * Hybrid MBRs do not necessarily comply with this.
+	 */
+	if (ret == GPT_MBR_PROTECTIVE) {
+		if (le32_to_cpu(mbr->partition_record[part].size_in_lba) !=
+		    min((uint32_t) total_sectors - 1, 0xFFFFFFFF))
+			ret = 0;
+	}
 done:
 	return ret;
 }
@@ -567,6 +581,7 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 	gpt_header *pgpt = NULL, *agpt = NULL;
 	gpt_entry *pptes = NULL, *aptes = NULL;
 	legacy_mbr *legacymbr;
+	sector_t total_sectors = i_size_read(state->bdev->bd_inode) >> 9;
 	u64 lastlba;
 
 	if (!ptes)
@@ -580,7 +595,7 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 			goto fail;
 
 		read_lba(state, 0, (u8 *) legacymbr, sizeof (*legacymbr));
-		good_pmbr = is_pmbr_valid(legacymbr);
+		good_pmbr = is_pmbr_valid(legacymbr, total_sectors);
 		kfree(legacymbr);
 
 		if (!good_pmbr)
-- 
1.7.11.7

[PATCH 6/8] partitions/efi: compare first and last usable LBAs

[Davidlohr Bueso]

When verifying GPT header integrity, make sure that
first usable LBA is smaller than last usable LBA.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index ab6cd08..9a81c3b 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -409,7 +409,12 @@ static int is_gpt_valid(struct parsed_partitions *state, u64 lba,
 			 (unsigned long long)lastlba);
 		goto fail;
 	}
-
+	if (le64_to_cpu((*gpt)->last_usable_lba) < le64_to_cpu((*gpt)->first_usable_lba)) {
+		pr_debug("GPT: last_usable_lba incorrect: %lld > %lld\n",
+			 (unsigned long long)le64_to_cpu((*gpt)->last_usable_lba),
+			 (unsigned long long)le64_to_cpu((*gpt)->first_usable_lba));
+		goto fail;
+	}
 	/* Check that sizeof_partition_entry has the correct value */
 	if (le32_to_cpu((*gpt)->sizeof_partition_entry) != sizeof(gpt_entry)) {
 		pr_debug("GUID Partitition Entry Size check failed.\n");
-- 
1.7.11.7

[PATCH 7/8] partitions/efi: delete annoying emacs style comments

[Davidlohr Bueso]

I love emacs, but these settings for coding style are
annoying when trying to open the efi.h file. More important,
we already have checkpatch for that.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.h | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/block/partitions/efi.h b/block/partitions/efi.h
index e9741de..9ab8ee9 100644
--- a/block/partitions/efi.h
+++ b/block/partitions/efi.h
@@ -130,22 +130,3 @@ typedef struct _legacy_mbr {
 extern int efi_partition(struct parsed_partitions *state);
 
 #endif
-
-/*
- * Overrides for Emacs so that we follow Linus's tabbing style.
- * Emacs will notice this stuff at the end of the file and automatically
- * adjust the settings for this buffer only.  This must remain at the end
- * of the file.
- * --------------------------------------------------------------------------
- * Local variables:
- * c-indent-level: 4 
- * c-brace-imaginary-offset: 0
- * c-brace-offset: -4
- * c-argdecl-indent: 4
- * c-label-offset: -4
- * c-continued-statement-offset: 4
- * c-continued-brace-offset: 0
- * indent-tabs-mode: nil
- * tab-width: 8
- * End:
- */
-- 
1.7.11.7

[PATCH 8/8] partitions/efi: some style cleanups

[Davidlohr Bueso]

Trivial coding style cleanups - still plenty left.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 9a81c3b..8e6d77e 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -25,6 +25,9 @@
  * TODO:
  *
  * Changelog:
+ * Mon August 5th, 2013 Davidlohr Bueso <davidlohr@hp.com>
+ * - detect hybrid MBRs, tighter pMBR checking & cleanups.
+ *
  * Mon Nov 09 2004 Matt Domsch <Matt_Domsch@dell.com>
  * - test for valid PMBR and valid PGPT before ever reading
  *   AGPT, allow override with 'gpt' kernel command line option.
@@ -288,8 +291,7 @@ static gpt_entry *alloc_read_gpt_entries(struct parsed_partitions *state,
 		return NULL;
 
 	if (read_lba(state, le64_to_cpu(gpt->partition_entry_lba),
-                     (u8 *) pte,
-		     count) < count) {
+                     (u8 *) pte, count) < count) {
 		kfree(pte);
                 pte=NULL;
 		return NULL;
@@ -613,8 +615,7 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
 	good_pgpt = is_gpt_valid(state, GPT_PRIMARY_PARTITION_TABLE_LBA,
 				 &pgpt, &pptes);
         if (good_pgpt)
-		good_agpt = is_gpt_valid(state,
-					 le64_to_cpu(pgpt->alternate_lba),
+		good_agpt = is_gpt_valid(state, le64_to_cpu(pgpt->alternate_lba),
 					 &agpt, &aptes);
         if (!good_agpt && force_gpt)
                 good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes);
@@ -632,9 +633,7 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
                 kfree(agpt);
                 kfree(aptes);
                 if (!good_agpt) {
-                        printk(KERN_WARNING 
-			       "Alternate GPT is invalid, "
-                               "using primary GPT.\n");
+                        printk(KERN_WARNING "Alternate GPT is invalid, using primary GPT.\n");
                 }
                 return 1;
         }
@@ -643,8 +642,7 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
                 *ptes = aptes;
                 kfree(pgpt);
                 kfree(pptes);
-                printk(KERN_WARNING 
-                       "Primary GPT is invalid, using alternate GPT.\n");
+                printk(KERN_WARNING "Primary GPT is invalid, using alternate GPT.\n");
                 return 1;
         }
 
@@ -706,8 +704,7 @@ int efi_partition(struct parsed_partitions *state)
 		put_partition(state, i+1, start * ssz, size * ssz);
 
 		/* If this is a RAID volume, tell md */
-		if (!efi_guidcmp(ptes[i].partition_type_guid,
-				 PARTITION_LINUX_RAID_GUID))
+		if (!efi_guidcmp(ptes[i].partition_type_guid, PARTITION_LINUX_RAID_GUID))
 			state->parts[i + 1].flags = ADDPART_FLAG_RAID;
 
 		info = &state->parts[i + 1].info;
-- 
1.7.11.7

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Andrew Morton]

On Mon,  5 Aug 2013 22:21:08 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:

> This patchset teaches the kernel about hybrid master boot records (MBRs), one of
> the most common issues with GUID partition tables, as a workaround to layout
> disk partitions to be compatible with both EFI and legacy MBR based systems.
> Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
> functionality is left unchanged - we only inform (through debug) the user about
> the used MBR scheme. While it is true that these restrictions can be bypassed when
> forcing gpt, this is not the correct or default way of doing things, complicating
> users furthermore. More details are in the individual patches.

Patches look nice, although I'll cheerily admit to not having a clue
what they do.  What is a "hybrid MBR" anyway?

Someone's editor seems to replace tabs with spaces so the patches
generate quite a checkpatch storm.  Please use checkpatch.

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Davidlohr Bueso]

On Tue, 2013-08-06 at 14:16 -0700, Andrew Morton wrote:
> On Mon,  5 Aug 2013 22:21:08 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> 
> > This patchset teaches the kernel about hybrid master boot records (MBRs), one of
> > the most common issues with GUID partition tables, as a workaround to layout
> > disk partitions to be compatible with both EFI and legacy MBR based systems.
> > Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
> > functionality is left unchanged - we only inform (through debug) the user about
> > the used MBR scheme. While it is true that these restrictions can be bypassed when
> > forcing gpt, this is not the correct or default way of doing things, complicating
> > users furthermore. More details are in the individual patches.
> 
> Patches look nice, although I'll cheerily admit to not having a clue
> what they do.  What is a "hybrid MBR" anyway?

Not having to know about hybrid MBRs is actually a good thing, they can
be quite a pain :)

I tried to explain more carefully what this particular MBR scheme is,
why it exists and why we cannot do anything about it in patch 5, but
perhaps it wasn't left clear and should have been in the cover letter as
well. Here's a brief summary:

EFI's GPT disklabels present a number of benefits to the traditional MBR
such as not having to deal with CHS addressing, better data integrity
(including a backup header) and 64bit LBA addressing (which allows
partitions to go beyond the 2Tb limit all the way up to 9.4 Zb), among
others. These nice features don't come free, however, having to deal
with older legacy systems (normally BIOS-based) that only use MBR, and
do not know about GPT. For example, users (like myself), who have an EFI
system (say a mac), dual booting with an older, non-EFI version of
Windows. While OSX knows GPT and uses the GPT partition(s), Windows
doesn't, so I cannot dual boot without creating a hybrid MBR - the
standard protective MBR (pMBR) won't allow Windows to boot. This hybrid
MBR will extend the regular pMBR (containing a 0xEE GPT partition) so
that it contains up to three primary partitions that point to the same
disk locations that the GPT partitions point to.

Hybrid MBRs are *unofficial* workarounds to the GPT specs, but necessary
for backward compatibility. Furthermore, most bootloaders are now
acknowledging this kind of scheme.

These patches only detect these configurations, and it's important that
the kernel can differentiate between hybrid and protective MBRs to
better detect correctly built pMBRs.

> Someone's editor seems to replace tabs with spaces so the patches
> generate quite a checkpatch storm.  Please use checkpatch.

Sorry, will do so in the future. I guess all patches from me have this
problem.

Thanks,
Davidlohr

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Davidlohr Bueso]

On Tue, 2013-08-06 at 14:16 -0700, Andrew Morton wrote:
> On Mon,  5 Aug 2013 22:21:08 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> 
> > This patchset teaches the kernel about hybrid master boot records (MBRs), one of
> > the most common issues with GUID partition tables, as a workaround to layout
> > disk partitions to be compatible with both EFI and legacy MBR based systems.
> > Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
> > functionality is left unchanged - we only inform (through debug) the user about
> > the used MBR scheme. While it is true that these restrictions can be bypassed when
> > forcing gpt, this is not the correct or default way of doing things, complicating
> > users furthermore. More details are in the individual patches.
> 
> Patches look nice, although I'll cheerily admit to not having a clue
> what they do.  What is a "hybrid MBR" anyway?
> 
> Someone's editor seems to replace tabs with spaces so the patches
> generate quite a checkpatch storm.  Please use checkpatch.
> 

Andrew, any chance of getting this in for 3.12?

Thanks,
Davidlohr

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Andrew Morton]

On Thu, 15 Aug 2013 09:59:42 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:

> On Tue, 2013-08-06 at 14:16 -0700, Andrew Morton wrote:
> > On Mon,  5 Aug 2013 22:21:08 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> > 
> > > This patchset teaches the kernel about hybrid master boot records (MBRs), one of
> > > the most common issues with GUID partition tables, as a workaround to layout
> > > disk partitions to be compatible with both EFI and legacy MBR based systems.
> > > Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
> > > functionality is left unchanged - we only inform (through debug) the user about
> > > the used MBR scheme. While it is true that these restrictions can be bypassed when
> > > forcing gpt, this is not the correct or default way of doing things, complicating
> > > users furthermore. More details are in the individual patches.
> > 
> > Patches look nice, although I'll cheerily admit to not having a clue
> > what they do.  What is a "hybrid MBR" anyway?
> > 
> > Someone's editor seems to replace tabs with spaces so the patches
> > generate quite a checkpatch storm.  Please use checkpatch.
> > 
> 
> Andrew, any chance of getting this in for 3.12?

gee, I didn't review the patches because I simply have no useful
knowledge in the area.  I can check the whitespace and code comments,
but that has the downside of creating the false impression that someone
actually reviewed the code :(

I'm struggling to think who might be better situated.  Matt Fleming or
Matt Domsch, maybe?

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Davidlohr Bueso]

On Thu, 2013-08-15 at 12:29 -0700, Andrew Morton wrote:
> On Thu, 15 Aug 2013 09:59:42 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> 
> > On Tue, 2013-08-06 at 14:16 -0700, Andrew Morton wrote:
> > > On Mon,  5 Aug 2013 22:21:08 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> > > 
> > > > This patchset teaches the kernel about hybrid master boot records (MBRs), one of
> > > > the most common issues with GUID partition tables, as a workaround to layout
> > > > disk partitions to be compatible with both EFI and legacy MBR based systems.
> > > > Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
> > > > functionality is left unchanged - we only inform (through debug) the user about
> > > > the used MBR scheme. While it is true that these restrictions can be bypassed when
> > > > forcing gpt, this is not the correct or default way of doing things, complicating
> > > > users furthermore. More details are in the individual patches.
> > > 
> > > Patches look nice, although I'll cheerily admit to not having a clue
> > > what they do.  What is a "hybrid MBR" anyway?
> > > 
> > > Someone's editor seems to replace tabs with spaces so the patches
> > > generate quite a checkpatch storm.  Please use checkpatch.
> > > 
> > 
> > Andrew, any chance of getting this in for 3.12?
> 
> gee, I didn't review the patches because I simply have no useful
> knowledge in the area.  I can check the whitespace and code comments,
> but that has the downside of creating the false impression that someone
> actually reviewed the code :(
> 
> I'm struggling to think who might be better situated.  Matt Fleming or
> Matt Domsch, maybe?
> 

Cc'ing Matt Fleming.

Karel, I believe you took a look at these, any thoughts?

Thanks,
Davidlohr

Re: [PATCH 1/8] partitions/efi: use lba-aware partition records

[Karel Zak]

On Mon, Aug 05, 2013 at 10:21:09PM -0700, Davidlohr Bueso wrote:
>  
> +typedef struct _gpt_record {
> +        u8      boot_indicator; /* unused by EFI, set to 0x80 for bootable */
> +        u8      start_head;     /* unused by EFI, pt start in CHS */
> +        u8      start_sector;   /* unused by EFI, pt start in CHS */
> +        u8      start_track;
> +        u8      os_type;        /* EFI and legacy non-EFI OS types */
> +        u8      end_head;       /* unused by EFI, pt end in CHS */
> +        u8      end_sector;     /* unused by EFI, pt end in CHS */
> +        u8      end_track;      /* unused by EFI, pt end in CHS */
> +        __le32  starting_lba;   /* used by EFI - start addr of the on disk pt */
> +        __le32  size_in_lba;    /* used by EFI - size of pt in LBA */
> +} __attribute__ ((packed)) gpt_record;
> +

 Maybe it would be better to rename this struct to "gpt_mbr_record" to
 make it more obvious.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Karel Zak]

On Thu, Aug 15, 2013 at 06:37:19PM -0700, Davidlohr Bueso wrote:
> On Thu, 2013-08-15 at 12:29 -0700, Andrew Morton wrote:
> > On Thu, 15 Aug 2013 09:59:42 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> > 
> > > On Tue, 2013-08-06 at 14:16 -0700, Andrew Morton wrote:
> > > > On Mon,  5 Aug 2013 22:21:08 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> > > > 
> > > > > This patchset teaches the kernel about hybrid master boot records (MBRs), one of
> > > > > the most common issues with GUID partition tables, as a workaround to layout
> > > > > disk partitions to be compatible with both EFI and legacy MBR based systems.
> > > > > Except for adding more pmbr checks, to better comply with the UEFI/GPT specs, the
> > > > > functionality is left unchanged - we only inform (through debug) the user about
> > > > > the used MBR scheme. While it is true that these restrictions can be bypassed when
> > > > > forcing gpt, this is not the correct or default way of doing things, complicating
> > > > > users furthermore. More details are in the individual patches.
> > > > 
> > > > Patches look nice, although I'll cheerily admit to not having a clue
> > > > what they do.  What is a "hybrid MBR" anyway?
> > > > 
> > > > Someone's editor seems to replace tabs with spaces so the patches
> > > > generate quite a checkpatch storm.  Please use checkpatch.
> > > > 
> > > 
> > > Andrew, any chance of getting this in for 3.12?
> > 
> > gee, I didn't review the patches because I simply have no useful
> > knowledge in the area.  I can check the whitespace and code comments,
> > but that has the downside of creating the false impression that someone
> > actually reviewed the code :(
> > 
> > I'm struggling to think who might be better situated.  Matt Fleming or
> > Matt Domsch, maybe?
> > 
> 
> Cc'ing Matt Fleming.
> 
> Karel, I believe you took a look at these, any thoughts?

 Reviewed-by: Karel Zak <kzak@redhat.com>

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/8] partitions/efi: use lba-aware partition records

[Davidlohr Bueso]

On Mon, 2013-09-02 at 12:10 +0200, Karel Zak wrote:
> On Mon, Aug 05, 2013 at 10:21:09PM -0700, Davidlohr Bueso wrote:
> >  
> > +typedef struct _gpt_record {
> > +        u8      boot_indicator; /* unused by EFI, set to 0x80 for bootable */
> > +        u8      start_head;     /* unused by EFI, pt start in CHS */
> > +        u8      start_sector;   /* unused by EFI, pt start in CHS */
> > +        u8      start_track;
> > +        u8      os_type;        /* EFI and legacy non-EFI OS types */
> > +        u8      end_head;       /* unused by EFI, pt end in CHS */
> > +        u8      end_sector;     /* unused by EFI, pt end in CHS */
> > +        u8      end_track;      /* unused by EFI, pt end in CHS */
> > +        __le32  starting_lba;   /* used by EFI - start addr of the on disk pt */
> > +        __le32  size_in_lba;    /* used by EFI - size of pt in LBA */
> > +} __attribute__ ((packed)) gpt_record;
> > +
> 
>  Maybe it would be better to rename this struct to "gpt_mbr_record" to
>  make it more obvious.

Yes, good idea. I've added the patch below.

8<------------------------------------------
From: Davidlohr Bueso <davidlohr@hp.com>
Subject: [PATCH] partitions/efi: rename gpt_record structure

Since the gpt_record structure is an MBR-specific
type, rename it to gpt_mbr_record for obvious
reading.

Suggested-by: Karel Zak <kzak@redhat.com>
Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 2 +-
 block/partitions/efi.h | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 8e6d77e..9a4eba7 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -152,7 +152,7 @@ static u64 last_lba(struct block_device *bdev)
 		       bdev_logical_block_size(bdev)) - 1ULL;
 }
 
-static inline int pmbr_part_valid(gpt_record *part)
+static inline int pmbr_part_valid(gpt_mbr_record *part)
 {
         if (part->os_type != EFI_PMBR_OSTYPE_EFI_GPT)
                 goto invalid;
diff --git a/block/partitions/efi.h b/block/partitions/efi.h
index 9ab8ee9..54b2687 100644
--- a/block/partitions/efi.h
+++ b/block/partitions/efi.h
@@ -104,7 +104,7 @@ typedef struct _gpt_entry {
 	efi_char16_t partition_name[72 / sizeof (efi_char16_t)];
 } __attribute__ ((packed)) gpt_entry;
 
-typedef struct _gpt_record {
+typedef struct _gpt_mbr_record {
         u8      boot_indicator; /* unused by EFI, set to 0x80 for bootable */
         u8      start_head;     /* unused by EFI, pt start in CHS */
         u8      start_sector;   /* unused by EFI, pt start in CHS */
@@ -115,14 +115,14 @@ typedef struct _gpt_record {
         u8      end_track;      /* unused by EFI, pt end in CHS */
         __le32  starting_lba;   /* used by EFI - start addr of the on disk pt */
         __le32  size_in_lba;    /* used by EFI - size of pt in LBA */
-} __attribute__ ((packed)) gpt_record;
+} __attribute__ ((packed)) gpt_mbr_record;
 
 
 typedef struct _legacy_mbr {
 	u8 boot_code[440];
 	__le32 unique_mbr_signature;
 	__le16 unknown;
-	gpt_record partition_record[4];
+	gpt_mbr_record partition_record[4];
 	__le16 signature;
 } __attribute__ ((packed)) legacy_mbr;
 
-- 
1.7.11.7

Re: [PATCH 0/8] partitions/efi: detect hybrid mbrs

[Matt Fleming]

On Thu, 15 Aug, at 06:37:19PM, Davidlohr Bueso wrote:
> Cc'ing Matt Fleming.
> 
> Karel, I believe you took a look at these, any thoughts?

While I haven't tested these patches, because none of my systems use
this MBR scheme, they seem fairly straight forward.

Acked-by: Matt Fleming <matt.fleming@intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center