GPT detection regression in efi.c from commit 27a7c64

GPT detection regression in efi.c from commit 27a7c64

[Matt Porter]

The commit, "27a7c64 partitions/efi: account for pmbr size in lba", that
was just merged in 3.12-rc caused a regression on my system with a GPT
formatted eMMC device. In 3.11, the GPT partition table was detected
fine but now a partition table is not detected.

Not being a GPT expert, I did some research and found that the tool used
to create the PMBR on my system shares characteristics with what is
mentioned in an explanation of Microsoft created PMBRs [1]. In short,
the size_in_lba field incorrectly has 0xffffffff even though I have a
<2TiB drive (16GiB eMMC).

I get that this is not compliant with UEFI. I bring this up because
before this commit the is_pmbr_valid() check was less pedantic. In 3.11
a PMBR formatted this way did not fail the check. For my particular
case, I simply dded out LBA 1 and whacked the SizeInLBA field to comply
with the spec and this patch and I'm back in business. We're updating
the tools that we inherited to prepopulate our boards with a GPT to be
compliant. However, I wondered if this would be a problem for all the
people with Windows-generated GPTs as noted in [1].

-Matt

[1] http://thestarman.pcministry.com/asm/mbr/GPT.htm#GPTPT

"The partition table contains only a single "GPT Protective" entry which
in all cases is set to the maximum 32-bit limitation (even though a
drive may have far less than a 2.2 TB capacity). The "GPT Protective MBR
Sector" has exactly the same contents for all GPT disk drives created by
the Windows 7 (or 8) OS. But, note: This does not follow the UEFI
Specification, which states that the "SizeInLBA" should be "set to the
size of the disk minus one" if it's not too large to be represented.[1]
(GPT drives partitioned under various Linux and Apple® Mac OS systems do
follow the UEFI Specification in this regard.)"

Re: GPT detection regression in efi.c from commit 27a7c64

[Davidlohr Bueso]

Cc'ing Linus.

On Fri, 2013-09-13 at 10:50 -0400, Matt Porter wrote:
> The commit, "27a7c64 partitions/efi: account for pmbr size in lba", that
> was just merged in 3.12-rc caused a regression on my system with a GPT
> formatted eMMC device. In 3.11, the GPT partition table was detected
> fine but now a partition table is not detected.
> 
> Not being a GPT expert, I did some research and found that the tool used
> to create the PMBR on my system shares characteristics with what is
> mentioned in an explanation of Microsoft created PMBRs [1]. In short,
> the size_in_lba field incorrectly has 0xffffffff even though I have a
> <2TiB drive (16GiB eMMC).

*sigh*. So Microsoft decided to do its own version of the GPT specs. 

Up until now, Linux was incorrectly enforcing pMBR checks: not
recognizing valid labels and vice versa, as with the case you are
mentioning now. The changes that went in the 3.12 merge window attempt
to address those concerns, enforcing the correct checks - which is also
how Linux partitioning tools do it (fdisk, parted).

> 
> I get that this is not compliant with UEFI. I bring this up because
> before this commit the is_pmbr_valid() check was less pedantic. In 3.11
> a PMBR formatted this way did not fail the check. For my particular
> case, I simply dded out LBA 1 and whacked the SizeInLBA field to comply
> with the spec and this patch and I'm back in business. We're updating
> the tools that we inherited to prepopulate our boards with a GPT to be
> compliant. However, I wondered if this would be a problem for all the
> people with Windows-generated GPTs as noted in [1].

I guess this comes down to choosing whether or not we want Linux to be
more UEFI compliant or not. Should we care if Microsoft decides to go do
things out of the official spec? I don't know the policy here. The fact
is that *they* should update their partitioning tools and create valid
pMBRs. Any way, I'm ok with reverting this commit if deemed necessary.

Thanks,
Davidlohr

> 
> -Matt
> 
> [1] http://thestarman.pcministry.com/asm/mbr/GPT.htm#GPTPT
> 
> "The partition table contains only a single "GPT Protective" entry which
> in all cases is set to the maximum 32-bit limitation (even though a
> drive may have far less than a 2.2 TB capacity). The "GPT Protective MBR
> Sector" has exactly the same contents for all GPT disk drives created by
> the Windows 7 (or 8) OS. But, note: This does not follow the UEFI
> Specification, which states that the "SizeInLBA" should be "set to the
> size of the disk minus one" if it's not too large to be represented.[1]
> (GPT drives partitioned under various Linux and Apple® Mac OS systems do
> follow the UEFI Specification in this regard.)"

Re: GPT detection regression in efi.c from commit 27a7c64

[Matt Porter]

On 09/13/2013 12:28 PM, Davidlohr Bueso wrote:
> Cc'ing Linus.
>
> On Fri, 2013-09-13 at 10:50 -0400, Matt Porter wrote:
>> The commit, "27a7c64 partitions/efi: account for pmbr size in lba", that
>> was just merged in 3.12-rc caused a regression on my system with a GPT
>> formatted eMMC device. In 3.11, the GPT partition table was detected
>> fine but now a partition table is not detected.
>>
>> Not being a GPT expert, I did some research and found that the tool used
>> to create the PMBR on my system shares characteristics with what is
>> mentioned in an explanation of Microsoft created PMBRs [1]. In short,
>> the size_in_lba field incorrectly has 0xffffffff even though I have a
>> <2TiB drive (16GiB eMMC).
>
> *sigh*. So Microsoft decided to do its own version of the GPT specs.

Don't sound so surprised. :)

> Up until now, Linux was incorrectly enforcing pMBR checks: not
> recognizing valid labels and vice versa, as with the case you are
> mentioning now. The changes that went in the 3.12 merge window attempt
> to address those concerns, enforcing the correct checks - which is also
> how Linux partitioning tools do it (fdisk, parted).

Understood, and we are fixing our own manufacturing tool that was used 
to prepopulate the eMMC. I definitely prefer to have this correct for my 
case.

>> I get that this is not compliant with UEFI. I bring this up because
>> before this commit the is_pmbr_valid() check was less pedantic. In 3.11
>> a PMBR formatted this way did not fail the check. For my particular
>> case, I simply dded out LBA 1 and whacked the SizeInLBA field to comply
>> with the spec and this patch and I'm back in business. We're updating
>> the tools that we inherited to prepopulate our boards with a GPT to be
>> compliant. However, I wondered if this would be a problem for all the
>> people with Windows-generated GPTs as noted in [1].
>
> I guess this comes down to choosing whether or not we want Linux to be
> more UEFI compliant or not. Should we care if Microsoft decides to go do
> things out of the official spec? I don't know the policy here. The fact
> is that *they* should update their partitioning tools and create valid
> pMBRs. Any way, I'm ok with reverting this commit if deemed necessary.

I can't say first-hand that Windows 7/8 does what is claimed in this 
description as I simply don't have access to any Windows machines here. 
If it's true, I would have to agree with Linus that meeting reality if 
more important than meeting the spec.

Hopefully somebody can confirm that Windows does indeed produce these 
special PMBRs that need to be handled as an exception to the spec.

-Matt

Re: GPT detection regression in efi.c from commit 27a7c64

[Davidlohr Bueso]

On Fri, 2013-09-13 at 13:01 -0400, Matt Porter wrote:
> On 09/13/2013 12:28 PM, Davidlohr Bueso wrote:
[...]
> 
> >> I get that this is not compliant with UEFI. I bring this up because
> >> before this commit the is_pmbr_valid() check was less pedantic. In 3.11
> >> a PMBR formatted this way did not fail the check. For my particular
> >> case, I simply dded out LBA 1 and whacked the SizeInLBA field to comply
> >> with the spec and this patch and I'm back in business. We're updating
> >> the tools that we inherited to prepopulate our boards with a GPT to be
> >> compliant. However, I wondered if this would be a problem for all the
> >> people with Windows-generated GPTs as noted in [1].
> >
> > I guess this comes down to choosing whether or not we want Linux to be
> > more UEFI compliant or not. Should we care if Microsoft decides to go do
> > things out of the official spec? I don't know the policy here. The fact
> > is that *they* should update their partitioning tools and create valid
> > pMBRs. Any way, I'm ok with reverting this commit if deemed necessary.
> 
> I can't say first-hand that Windows 7/8 does what is claimed in this 
> description as I simply don't have access to any Windows machines here. 
> If it's true, I would have to agree with Linus that meeting reality if 
> more important than meeting the spec.

Yep, me too.

> 
> Hopefully somebody can confirm that Windows does indeed produce these 
> special PMBRs that need to be handled as an exception to the spec.

I've got a partition with Windows 7 and I can take a look during the
weekend. Do you know exactly what tool was used for creating the
partition?

Thanks,
Davidlohr

Re: GPT detection regression in efi.c from commit 27a7c64

[Matt Porter]

On 09/13/2013 01:37 PM, Davidlohr Bueso wrote:
> On Fri, 2013-09-13 at 13:01 -0400, Matt Porter wrote:
>> On 09/13/2013 12:28 PM, Davidlohr Bueso wrote:
> [...]
>>
>>>> I get that this is not compliant with UEFI. I bring this up because
>>>> before this commit the is_pmbr_valid() check was less pedantic. In 3.11
>>>> a PMBR formatted this way did not fail the check. For my particular
>>>> case, I simply dded out LBA 1 and whacked the SizeInLBA field to comply
>>>> with the spec and this patch and I'm back in business. We're updating
>>>> the tools that we inherited to prepopulate our boards with a GPT to be
>>>> compliant. However, I wondered if this would be a problem for all the
>>>> people with Windows-generated GPTs as noted in [1].
>>>
>>> I guess this comes down to choosing whether or not we want Linux to be
>>> more UEFI compliant or not. Should we care if Microsoft decides to go do
>>> things out of the official spec? I don't know the policy here. The fact
>>> is that *they* should update their partitioning tools and create valid
>>> pMBRs. Any way, I'm ok with reverting this commit if deemed necessary.
>>
>> I can't say first-hand that Windows 7/8 does what is claimed in this
>> description as I simply don't have access to any Windows machines here.
>> If it's true, I would have to agree with Linus that meeting reality if
>> more important than meeting the spec.
>
> Yep, me too.
>
>>
>> Hopefully somebody can confirm that Windows does indeed produce these
>> special PMBRs that need to be handled as an exception to the spec.
>
> I've got a partition with Windows 7 and I can take a look during the
> weekend. Do you know exactly what tool was used for creating the
> partition?

Just to be clear, we used an internal Broadcom tool that did this 
Windows-like behavior on our bcm281xx reference board.

In the Windows situation, I really have no idea what tool Windows 7/8 
use to partition disks. Based on that article, I'm assuming it's 
whatever today's version of "FDISK", as shipped on Windows 7/8, is.

Ok, Google claims it is "The Windows 7 Disk Management" tool. I might 
try to stop at somebody's house this weekend, boot Linux from USB and 
extract the PMBR from their drive just as another data point.

-Matt

Re: GPT detection regression in efi.c from commit 27a7c64

[Davidlohr Bueso]

On Fri, 2013-09-13 at 13:01 -0400, Matt Porter wrote:
> On 09/13/2013 12:28 PM, Davidlohr Bueso wrote:
> > Cc'ing Linus.
> >
> > On Fri, 2013-09-13 at 10:50 -0400, Matt Porter wrote:
> >> The commit, "27a7c64 partitions/efi: account for pmbr size in lba", that
> >> was just merged in 3.12-rc caused a regression on my system with a GPT
> >> formatted eMMC device. In 3.11, the GPT partition table was detected
> >> fine but now a partition table is not detected.
> >>
> >> Not being a GPT expert, I did some research and found that the tool used
> >> to create the PMBR on my system shares characteristics with what is
> >> mentioned in an explanation of Microsoft created PMBRs [1]. In short,
> >> the size_in_lba field incorrectly has 0xffffffff even though I have a
> >> <2TiB drive (16GiB eMMC).
> >
> > *sigh*. So Microsoft decided to do its own version of the GPT specs.
> 
> Don't sound so surprised. :)
> 
> > Up until now, Linux was incorrectly enforcing pMBR checks: not
> > recognizing valid labels and vice versa, as with the case you are
> > mentioning now. The changes that went in the 3.12 merge window attempt
> > to address those concerns, enforcing the correct checks - which is also
> > how Linux partitioning tools do it (fdisk, parted).
> 
> Understood, and we are fixing our own manufacturing tool that was used 
> to prepopulate the eMMC. I definitely prefer to have this correct for my 
> case.

Come to think of it, we do have a long existing workaround: the
force_gpt option. Setting it will bypass any MBR checking
(is_pmbr_valid(), specifically).

Thanks,
Davidlohr

Re: GPT detection regression in efi.c from commit 27a7c64

[Matt Porter]

On 09/13/2013 02:07 PM, Davidlohr Bueso wrote:
> On Fri, 2013-09-13 at 13:01 -0400, Matt Porter wrote:
>> On 09/13/2013 12:28 PM, Davidlohr Bueso wrote:
>>> Cc'ing Linus.
>>>
>>> On Fri, 2013-09-13 at 10:50 -0400, Matt Porter wrote:
>>>> The commit, "27a7c64 partitions/efi: account for pmbr size in lba", that
>>>> was just merged in 3.12-rc caused a regression on my system with a GPT
>>>> formatted eMMC device. In 3.11, the GPT partition table was detected
>>>> fine but now a partition table is not detected.
>>>>
>>>> Not being a GPT expert, I did some research and found that the tool used
>>>> to create the PMBR on my system shares characteristics with what is
>>>> mentioned in an explanation of Microsoft created PMBRs [1]. In short,
>>>> the size_in_lba field incorrectly has 0xffffffff even though I have a
>>>> <2TiB drive (16GiB eMMC).
>>>
>>> *sigh*. So Microsoft decided to do its own version of the GPT specs.
>>
>> Don't sound so surprised. :)
>>
>>> Up until now, Linux was incorrectly enforcing pMBR checks: not
>>> recognizing valid labels and vice versa, as with the case you are
>>> mentioning now. The changes that went in the 3.12 merge window attempt
>>> to address those concerns, enforcing the correct checks - which is also
>>> how Linux partitioning tools do it (fdisk, parted).
>>
>> Understood, and we are fixing our own manufacturing tool that was used
>> to prepopulate the eMMC. I definitely prefer to have this correct for my
>> case.
>
> Come to think of it, we do have a long existing workaround: the
> force_gpt option. Setting it will bypass any MBR checking
> (is_pmbr_valid(), specifically).

Yes, that's what I used at first after seeing what the problem was. But 
then I opted to fix my PMBR.

I felt like it was a regression since it required a new option passed on 
the cmdline.

-Matt

Re: GPT detection regression in efi.c from commit 27a7c64

[Karel Zak]

On Fri, Sep 13, 2013 at 02:09:55PM -0400, Matt Porter wrote:
> >Come to think of it, we do have a long existing workaround: the
> >force_gpt option. Setting it will bypass any MBR checking
> >(is_pmbr_valid(), specifically).
> 
> Yes, that's what I used at first after seeing what the problem was. But then
> I opted to fix my PMBR.
> 
> I felt like it was a regression since it required a new option passed on the
> cmdline.

 Yep, it is *regression* and I guess Davidlohr is going to fix it asap :-)

    Karel

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

Re: GPT detection regression in efi.c from commit 27a7c64

[Davidlohr Bueso]

On Fri, 2013-09-13 at 20:17 +0200, Karel Zak wrote:
> On Fri, Sep 13, 2013 at 02:09:55PM -0400, Matt Porter wrote:
> > >Come to think of it, we do have a long existing workaround: the
> > >force_gpt option. Setting it will bypass any MBR checking
> > >(is_pmbr_valid(), specifically).
> > 
> > Yes, that's what I used at first after seeing what the problem was. But then
> > I opted to fix my PMBR.
> > 
> > I felt like it was a regression since it required a new option passed on the
> > cmdline.
> 
>  Yep, it is *regression* and I guess Davidlohr is going to fix it asap :-)

I was doing a git revert, but what would you guys think of keeping the
check but making it more flexible? Instead of enforcing the minimum,
just make sure that the size_in_lba is either the whole disk or 2 TiB,
that should take care of Matt's issue.

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 1a5ec9a..df2fca1 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -220,8 +220,8 @@ check_hybrid:
         * 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))
+               if (le32_to_cpu(mbr->partition_record[part].size_in_lba) != (uint32_t) total_sectors - 1 ||
+                   le32_to_cpu(mbr->partition_record[part].size_in_lba) != 0xFFFFFFFF)
                        ret = 0;
        }
 done:

Karel, I guess any changes that we do here should apply to fdisk :)

Thanks,
Davidlohr

Re: GPT detection regression in efi.c from commit 27a7c64

[Matt Porter]

On 09/13/2013 02:29 PM, Davidlohr Bueso wrote:
> On Fri, 2013-09-13 at 20:17 +0200, Karel Zak wrote:
>> On Fri, Sep 13, 2013 at 02:09:55PM -0400, Matt Porter wrote:
>>>> Come to think of it, we do have a long existing workaround: the
>>>> force_gpt option. Setting it will bypass any MBR checking
>>>> (is_pmbr_valid(), specifically).
>>>
>>> Yes, that's what I used at first after seeing what the problem was. But then
>>> I opted to fix my PMBR.
>>>
>>> I felt like it was a regression since it required a new option passed on the
>>> cmdline.
>>
>>   Yep, it is *regression* and I guess Davidlohr is going to fix it asap :-)
>
> I was doing a git revert, but what would you guys think of keeping the
> check but making it more flexible? Instead of enforcing the minimum,
> just make sure that the size_in_lba is either the whole disk or 2 TiB,
> that should take care of Matt's issue.

That seems to be the way to go given the departure from the spec.

> diff --git a/block/partitions/efi.c b/block/partitions/efi.c
> index 1a5ec9a..df2fca1 100644
> --- a/block/partitions/efi.c
> +++ b/block/partitions/efi.c
> @@ -220,8 +220,8 @@ check_hybrid:
>           * 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))
> +               if (le32_to_cpu(mbr->partition_record[part].size_in_lba) != (uint32_t) total_sectors - 1 ||
> +                   le32_to_cpu(mbr->partition_record[part].size_in_lba) != 0xFFFFFFFF)
>                          ret = 0;
>          }
>   done:
>
> Karel, I guess any changes that we do here should apply to fdisk :)
>
> Thanks,
> Davidlohr
>

Re: GPT detection regression in efi.c from commit 27a7c64

[Davidlohr Bueso]

On Fri, 2013-09-13 at 14:33 -0400, Matt Porter wrote:
> On 09/13/2013 02:29 PM, Davidlohr Bueso wrote:
> > On Fri, 2013-09-13 at 20:17 +0200, Karel Zak wrote:
> >> On Fri, Sep 13, 2013 at 02:09:55PM -0400, Matt Porter wrote:
> >>>> Come to think of it, we do have a long existing workaround: the
> >>>> force_gpt option. Setting it will bypass any MBR checking
> >>>> (is_pmbr_valid(), specifically).
> >>>
> >>> Yes, that's what I used at first after seeing what the problem was. But then
> >>> I opted to fix my PMBR.
> >>>
> >>> I felt like it was a regression since it required a new option passed on the
> >>> cmdline.
> >>
> >>   Yep, it is *regression* and I guess Davidlohr is going to fix it asap :-)
> >
> > I was doing a git revert, but what would you guys think of keeping the
> > check but making it more flexible? Instead of enforcing the minimum,
> > just make sure that the size_in_lba is either the whole disk or 2 TiB,
> > that should take care of Matt's issue.
> 
> That seems to be the way to go given the departure from the spec.

Matt, could you please verify that this patch fixes your problem?

Thanks!

8<-------------------------------------------------

From: Davidlohr Bueso <davidlohr@hp.com>
Subject: [PATCH] partitions/efi: loosen pmbr size in lba check

Matt found that commit 27a7c64 (partitions/efi: account for pmbr size in lba)
caused his GPT formatted eMMC device not to boot. The reason is that this commit
enforced Linux to always check the lesser of the whole disk or 2Tib for the
pMBR size in LBA. While most disk partitioning tools out there create a pMBR with
these characteristics, Microsoft does not, as it always sets the entry to the
maximum 32-bit limitation - even though a drive may be smaller than that[1].

Loosen this check and only verify that the size is either the whole disk or
0xFFFFFFFF. No tool in its right mind would set it to any value other than these.

[1] http://thestarman.pcministry.com/asm/mbr/GPT.htm#GPTPT

Reported-by: Matt Porter <matt.porter@linaro.org>
Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 1a5ec9a..9bae49c 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -186,6 +186,7 @@ invalid:
  */
 static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
 {
+	uint32_t sz = 0;
 	int i, part = 0, ret = 0; /* invalid by default */
 
 	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
@@ -216,12 +217,15 @@ check_hybrid:
 	/*
 	 * Protective MBRs take up the lesser of the whole disk
 	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
+	 * Some partitioning programs, nonetheless, choose to set
+	 * the size to the maximum 32-bit limitation, disregarding
+	 * the disk size.
 	 *
 	 * 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))
+		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
+		if (sz != (uint32_t) total_sectors - 1 || sz != 0xFFFFFFFF)
 			ret = 0;
 	}
 done:
-- 
1.7.11.7

Re: GPT detection regression in efi.c from commit 27a7c64

[Matt Porter]

On Fri, Sep 13, 2013 at 12:26:39PM -0700, Davidlohr Bueso wrote:
> On Fri, 2013-09-13 at 14:33 -0400, Matt Porter wrote:
> > On 09/13/2013 02:29 PM, Davidlohr Bueso wrote:
> > > On Fri, 2013-09-13 at 20:17 +0200, Karel Zak wrote:
> > >> On Fri, Sep 13, 2013 at 02:09:55PM -0400, Matt Porter wrote:
> > >>>> Come to think of it, we do have a long existing workaround: the
> > >>>> force_gpt option. Setting it will bypass any MBR checking
> > >>>> (is_pmbr_valid(), specifically).
> > >>>
> > >>> Yes, that's what I used at first after seeing what the problem was. But then
> > >>> I opted to fix my PMBR.
> > >>>
> > >>> I felt like it was a regression since it required a new option passed on the
> > >>> cmdline.
> > >>
> > >>   Yep, it is *regression* and I guess Davidlohr is going to fix it asap :-)
> > >
> > > I was doing a git revert, but what would you guys think of keeping the
> > > check but making it more flexible? Instead of enforcing the minimum,
> > > just make sure that the size_in_lba is either the whole disk or 2 TiB,
> > > that should take care of Matt's issue.
> > 
> > That seems to be the way to go given the departure from the spec.
> 
> Matt, could you please verify that this patch fixes your problem?
> 
> Thanks!
> 
> 8<-------------------------------------------------
> 
> From: Davidlohr Bueso <davidlohr@hp.com>
> Subject: [PATCH] partitions/efi: loosen pmbr size in lba check
> 
> Matt found that commit 27a7c64 (partitions/efi: account for pmbr size in lba)
> caused his GPT formatted eMMC device not to boot. The reason is that this commit
> enforced Linux to always check the lesser of the whole disk or 2Tib for the
> pMBR size in LBA. While most disk partitioning tools out there create a pMBR with
> these characteristics, Microsoft does not, as it always sets the entry to the
> maximum 32-bit limitation - even though a drive may be smaller than that[1].
> 
> Loosen this check and only verify that the size is either the whole disk or
> 0xFFFFFFFF. No tool in its right mind would set it to any value other than these.
> 
> [1] http://thestarman.pcministry.com/asm/mbr/GPT.htm#GPTPT
> 
> Reported-by: Matt Porter <matt.porter@linaro.org>
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  block/partitions/efi.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/partitions/efi.c b/block/partitions/efi.c
> index 1a5ec9a..9bae49c 100644
> --- a/block/partitions/efi.c
> +++ b/block/partitions/efi.c
> @@ -186,6 +186,7 @@ invalid:
>   */
>  static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
>  {
> +	uint32_t sz = 0;
>  	int i, part = 0, ret = 0; /* invalid by default */
>  
>  	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
> @@ -216,12 +217,15 @@ check_hybrid:
>  	/*
>  	 * Protective MBRs take up the lesser of the whole disk
>  	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
> +	 * Some partitioning programs, nonetheless, choose to set
> +	 * the size to the maximum 32-bit limitation, disregarding
> +	 * the disk size.
>  	 *
>  	 * 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))
> +		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
> +		if (sz != (uint32_t) total_sectors - 1 || sz != 0xFFFFFFFF)

Almost! You'll want to get that brown paper bag out cause I've worn it
for this same type of bug before. Add this fix in and you can have my

Tested-by: Matt Porter <matt.porter@linaro.org>

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 9bae49c..1eb09ee 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -225,7 +225,7 @@ check_hybrid:
         */
        if (ret == GPT_MBR_PROTECTIVE) {
                sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
-               if (sz != (uint32_t) total_sectors - 1 || sz != 0xFFFFFFFF)
+               if (sz != (uint32_t) total_sectors - 1 && sz != 0xFFFFFFFF)
			ret = 0;
        }
 done:

Thanks,
Matt

Re: GPT detection regression in efi.c from commit 27a7c64

[Davidlohr Bueso]

On Fri, 2013-09-13 at 17:36 -0400, Matt Porter wrote:
[...]
> > 
> > diff --git a/block/partitions/efi.c b/block/partitions/efi.c
> > index 1a5ec9a..9bae49c 100644
> > --- a/block/partitions/efi.c
> > +++ b/block/partitions/efi.c
> > @@ -186,6 +186,7 @@ invalid:
> >   */
> >  static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
> >  {
> > +	uint32_t sz = 0;
> >  	int i, part = 0, ret = 0; /* invalid by default */
> >  
> >  	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
> > @@ -216,12 +217,15 @@ check_hybrid:
> >  	/*
> >  	 * Protective MBRs take up the lesser of the whole disk
> >  	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
> > +	 * Some partitioning programs, nonetheless, choose to set
> > +	 * the size to the maximum 32-bit limitation, disregarding
> > +	 * the disk size.
> >  	 *
> >  	 * 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))
> > +		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
> > +		if (sz != (uint32_t) total_sectors - 1 || sz != 0xFFFFFFFF)
> 
> Almost! You'll want to get that brown paper bag out cause I've worn it
> for this same type of bug before. Add this fix in and you can have my
> 
> Tested-by: Matt Porter <matt.porter@linaro.org>
> 
> diff --git a/block/partitions/efi.c b/block/partitions/efi.c
> index 9bae49c..1eb09ee 100644
> --- a/block/partitions/efi.c
> +++ b/block/partitions/efi.c
> @@ -225,7 +225,7 @@ check_hybrid:
>          */
>         if (ret == GPT_MBR_PROTECTIVE) {
>                 sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
> -               if (sz != (uint32_t) total_sectors - 1 || sz != 0xFFFFFFFF)
> +               if (sz != (uint32_t) total_sectors - 1 && sz != 0xFFFFFFFF)
> 			ret = 0;

Oh my, yes, sorry about that. I obviously only built-tested this patch.
Linus, if you're ok with it, please go ahead and pick this up.

Thanks,
Davidlohr

8<-------------------------------------------------

From: Davidlohr Bueso <davidlohr@hp.com>
Subject: [PATCH] partitions/efi: loosen pmbr size in lba check

Matt found that commit 27a7c64 (partitions/efi: account for pmbr size in lba)
caused his GPT formatted eMMC device not to boot. The reason is that this commit
enforced Linux to always check the lesser of the whole disk or 2Tib for the
pMBR size in LBA. While most disk partitioning tools out there create a pMBR with
these characteristics, Microsoft does not, as it always sets the entry to the
maximum 32-bit limitation - even though a drive may be smaller than that[1].

Loosen this check and only verify that the size is either the whole disk or
0xFFFFFFFF. No tool in its right mind would set it to any value other than these.

[1] http://thestarman.pcministry.com/asm/mbr/GPT.htm#GPTPT

Reported-and-tested-by: Matt Porter <matt.porter@linaro.org>
Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 block/partitions/efi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/partitions/efi.c b/block/partitions/efi.c
index 1a5ec9a..1eb09ee 100644
--- a/block/partitions/efi.c
+++ b/block/partitions/efi.c
@@ -186,6 +186,7 @@ invalid:
  */
 static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
 {
+	uint32_t sz = 0;
 	int i, part = 0, ret = 0; /* invalid by default */
 
 	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
@@ -216,12 +217,15 @@ check_hybrid:
 	/*
 	 * Protective MBRs take up the lesser of the whole disk
 	 * or 2 TiB (32bit LBA), ignoring the rest of the disk.
+	 * Some partitioning programs, nonetheless, choose to set
+	 * the size to the maximum 32-bit limitation, disregarding
+	 * the disk size.
 	 *
 	 * 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))
+		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
+		if (sz != (uint32_t) total_sectors - 1 && sz != 0xFFFFFFFF)
 			ret = 0;
 	}
 done:
-- 
1.7.11.7