GNU bug report logs - #17108
[PATCH 1/4] libparted: Check AlternateLBA against LBA-1

Previous Next

To reply to this bug, email your comments to 17108 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: bug-parted <at> gnu.org
Subject: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Wed, 26 Mar 2014 10:56:19 -0700

commit 9e9588c71e introduced a problem with how the backup gpt
partition's location was checked, and where it was re-written. It was
using LastUsableLBA plus an offset based on the number of partition
entries. This will not always match up with the end of the disk. eg.
during t0210 where the number of partitions is set to 9 instead of 128.

The only way to check the AlternateLBA is against the size of the disk.
If it isn't in LBA - 1 then it is in the wrong place.

The outcome was that t0210 and t0211 were failing becase the location
that it tried to move the backup to was also invalid.

* libparted/labels/gpt.c (gpt_read): Fix check for backup gpt location
---
 libparted/labels/gpt.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
index 42b0360..3e8e603 100644
--- a/libparted/labels/gpt.c
+++ b/libparted/labels/gpt.c
@@ -985,13 +985,7 @@ gpt_read (PedDisk *disk)
     {
       /* Both are valid.  */
 #ifndef DISCOVER_ONLY
-      PedSector gpt_disk_end = PED_LE64_TO_CPU (primary_gpt->LastUsableLBA) + 1;
-      gpt_disk_end += ((PedSector) (PED_LE32_TO_CPU (primary_gpt->NumberOfPartitionEntries)) *
-                       (PedSector) (PED_LE32_TO_CPU (primary_gpt->SizeOfPartitionEntry)) /
-                       disk->dev->sector_size);
-
-      gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU (primary_gpt->AlternateLBA);
-      if (PED_LE64_TO_CPU (primary_gpt->AlternateLBA) != gpt_disk_end)
+	if (PED_LE64_TO_CPU (primary_gpt->AlternateLBA) < disk->dev->length - 1)
         {
           if (ped_exception_throw
                   (PED_EXCEPTION_ERROR,
@@ -1002,7 +996,7 @@ gpt_read (PedDisk *disk)
             {
               ptt_clear_sectors (disk->dev,
                                  PED_LE64_TO_CPU (primary_gpt->AlternateLBA), 1);
-              gpt_disk_data->AlternateLBA = gpt_disk_end;
+              gpt_disk_data->AlternateLBA = disk->dev->length - 1;
               write_back = 1;
             }
         }
-- 
1.8.5.3

Message #8 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>, 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Wed, 26 Mar 2014 22:25:03 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 03/26/2014 01:56 PM, Brian C. Lane wrote:
> commit 9e9588c71e introduced a problem with how the backup gpt 
> partition's location was checked, and where it was re-written. It
> was using LastUsableLBA plus an offset based on the number of
> partition entries. This will not always match up with the end of
> the disk. eg. during t0210 where the number of partitions is set to
> 9 instead of 128.

I mentioned recently that t0210 was failing, but when I looked it it,
it looked to me like the script was broken and was underflowing a
variable, causing it to write the backup to a location near 2 TiB,
rather than at the end of the much smaller "disk".  In other words,
the script initially creates a 2M loop file, and after running
gpt-header-munge on it, it is now a 2 TiB file.  This is clearly an
error in gpt-heade-munge, rather than parted.

> The only way to check the AlternateLBA is against the size of the
> disk. If it isn't in LBA - 1 then it is in the wrong place.

While this is true, the patch below regresses what I fixed before,
which is that if the AlternateLBA says it does not go at the end of
the disk, and we don't fix that, then we write it to the end of the
disk anyhow.  This is incorrect.  If we don't fix AlternateLBA to
point to the right location relative to the end of the disk, then we
need to actually write the backup to where AlternateLBA says it is,
even if that isn't where it should be.  With this patch, we write the
backup to where it should be, but the header now claims it is in
another location.

> The outcome was that t0210 and t0211 were failing becase the
> location that it tried to move the backup to was also invalid.
> 
> * libparted/labels/gpt.c (gpt_read): Fix check for backup gpt
> location --- libparted/labels/gpt.c | 10 ++-------- 1 file changed,
> 2 insertions(+), 8 deletions(-)
> 
> diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c index
> 42b0360..3e8e603 100644 --- a/libparted/labels/gpt.c +++
> b/libparted/labels/gpt.c @@ -985,13 +985,7 @@ gpt_read (PedDisk
> *disk) { /* Both are valid.  */ #ifndef DISCOVER_ONLY -
> PedSector gpt_disk_end = PED_LE64_TO_CPU
> (primary_gpt->LastUsableLBA) + 1; -      gpt_disk_end +=
> ((PedSector) (PED_LE32_TO_CPU
> (primary_gpt->NumberOfPartitionEntries)) * -
> (PedSector) (PED_LE32_TO_CPU (primary_gpt->SizeOfPartitionEntry))
> / -                       disk->dev->sector_size); - -
> gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU
> (primary_gpt->AlternateLBA); -      if (PED_LE64_TO_CPU
> (primary_gpt->AlternateLBA) != gpt_disk_end) +	if (PED_LE64_TO_CPU
> (primary_gpt->AlternateLBA) < disk->dev->length - 1) { if
> (ped_exception_throw (PED_EXCEPTION_ERROR, @@ -1002,7 +996,7 @@
> gpt_read (PedDisk *disk) { ptt_clear_sectors (disk->dev, 
> PED_LE64_TO_CPU (primary_gpt->AlternateLBA), 1); -
> gpt_disk_data->AlternateLBA = gpt_disk_end; +
> gpt_disk_data->AlternateLBA = disk->dev->length - 1; write_back =
> 1; } }

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJTM4v/AAoJEI5FoCIzSKrwB0AIAJ2SrlsrQmhlBmAWgbKUW6k4
g+XK0UG7Kfyc1dDrerS66Dxt55H6gCW5e65Z9Ul/L06+OCTmdL6sYLNXHQH8joWz
H6NEqmQlHvBwxR0XYb7VCNv8C75HM9stiA5aMLftT26H8EqmCqyVjXPfvYCqvpBq
Y/oES3Ht0EXE1A+2Leg7cqoI420rK1sj2VyuXq6l82lp9My1TGw9FcrUYmQBG2Rz
MaiOIO+BO6/1Ygl6LyToctTb0OHJqu9kj8Asx9aTgEXshfIWvjZI1bBwDJ6xG44V
drAIXatbAk2CQM/V1tE+JmZK2ElAqp0RafZhqUxvrBwt7VQpLyr6te8iKRhASpM=
=VKTU
-----END PGP SIGNATURE-----

Message #11 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Phillip Susi <psusi <at> ubuntu.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 08:00:30 -0700

On Wed, Mar 26, 2014 at 10:25:03PM -0400, Phillip Susi wrote:
> On 03/26/2014 01:56 PM, Brian C. Lane wrote:
> > commit 9e9588c71e introduced a problem with how the backup gpt 
> > partition's location was checked, and where it was re-written. It
> > was using LastUsableLBA plus an offset based on the number of
> > partition entries. This will not always match up with the end of
> > the disk. eg. during t0210 where the number of partitions is set to
> > 9 instead of 128.
> 
> I mentioned recently that t0210 was failing, but when I looked it it,
> it looked to me like the script was broken and was underflowing a
> variable, causing it to write the backup to a location near 2 TiB,
> rather than at the end of the much smaller "disk".  In other words,
> the script initially creates a 2M loop file, and after running
> gpt-header-munge on it, it is now a 2 TiB file.  This is clearly an
> error in gpt-heade-munge, rather than parted.

I haven't seen any evidence of that, and that isn't what's happening
here.

> 
> > The only way to check the AlternateLBA is against the size of the
> > disk. If it isn't in LBA - 1 then it is in the wrong place.
> 
> While this is true, the patch below regresses what I fixed before,
> which is that if the AlternateLBA says it does not go at the end of
> the disk, and we don't fix that, then we write it to the end of the
> disk anyhow.  This is incorrect.  If we don't fix AlternateLBA to
> point to the right location relative to the end of the disk, then we
> need to actually write the backup to where AlternateLBA says it is,
> even if that isn't where it should be.  With this patch, we write the
> backup to where it should be, but the header now claims it is in
> another location.

Ok, I'll check to make sure that isn't the case -- I thought the
location was updated when it was rewritten, but I'll double check.

But the basic problem here is that you are making calculations that are
invalid, as revealed by the failing test. You cannot count on the
AlternateLBA always being at the block after LastUsableLBA+Partition
entries. This *may* be true, but it isn't guaranteed to be true.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)

Message #14 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 12:23:48 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/27/2014 11:00 AM, Brian C. Lane wrote:
> I haven't seen any evidence of that, and that isn't what's
> happening here.

Try running the test steps manually so you can see what is going on in
the middle before the test suite cleans it up.  Last time I did that,
the loop-file size went from 20m to 2g after running gpt-header-munge.
 That makes parted complain that the backup is in the wrong place.

> Ok, I'll check to make sure that isn't the case -- I thought the 
> location was updated when it was rewritten, but I'll double check.
> 
> But the basic problem here is that you are making calculations that
> are invalid, as revealed by the failing test. You cannot count on
> the AlternateLBA always being at the block after
> LastUsableLBA+Partition entries. This *may* be true, but it isn't
> guaranteed to be true.

How would it not be true?  The reason the calculation is that way
rather than last sector of the disk - 1 is because if you have a disk
that has been extended, then the backup is no longer at last sector -
1.  By changing it back to last sector - 1, you will be looking for it
in the wrong place after the disk has grown.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTNFCUAAoJEI5FoCIzSKrwqTkH/1ggCHfISIXSrCGaqYp9GX7V
09AEFulpwpV9ShJ04QbfBS7s9ShQQBJMds++2QyJpcXCVVG9TARynCAhZ9vSg/Ov
tNSDGlA/6DJQ+Bzs/mQNX3WyG0bEkrOlv9P+X+YBw+hT0f7ARRpsbSurnEHS565p
rVnNkjgvlZuIx/tVE75JZhB8aAHw3JVMjQ7W2XZ77gziMQmn+QB9Ox0/Qluxe8yv
hSk6lXFthvGZ0XHhl9zcyRjXBF7cFkfe6yISxSX3WEMYvx8MMbdeZms3k9XYRqWQ
C2DRBjVHX15T2vptL1uD4IhHUT4yi0P1hHe660i8tBhmRwp9uRbsmNgzHE9c0eg=
=bDhE
-----END PGP SIGNATURE-----

Message #17 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 12:55:31 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Having thought about it over lunch, I think I see what is going on
here now.  gpt-header-munge is broken and writes the backup to an
offset of 2gb rather than 20mb, extending the "disk".  But
LastUsableLba still says it's 20mb.  This caused parted to complain
that the backup was in the wrong place.

Parted is correct here: the backup *is* in the wrong place, since it
is at the end of where *we* think the disk is rather than where the
header thinks it is.  I changed the code to be this way in the first
place so that this complaint would not be triggered by growing a disk,
leaving both the LastUableLba and backup locations unchanged.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTNFgDAAoJEI5FoCIzSKrwFmwH/1cXVkFUlJzQTEYnKKraaq90
9x8uhUpvfTRHwBVoOBTqBxawNZ0WrzDaMqUwE6k1S25JltpuVLarAOYSNXWj1WCd
tHDpzgo6XwYmpTF+TqHbiBxnFCIBOC+ncUEMa9isFKgx2SsGdnoBbNxSN9bmZebB
l00bK7vHKI8NaPt7mWcOR4KcgVo4dNMpbSRXtCwCZMyzonh//d6Qs1kR8wTWMHYd
uM12baAUkMizKskmA2ucLe0jvwCgRnUqI3elEEUumTICG57EP17vNElka8MfF3RC
JNcT9YzI/OE8oBZa3KwJzaO2VPSxDR9m5cmjaAL95oOtCkqaQoCARj2INdjvROo=
=BPT4
-----END PGP SIGNATURE-----

Message #20 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Phillip Susi <psusi <at> ubuntu.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 15:48:36 -0700

On Thu, Mar 27, 2014 at 12:23:48PM -0400, Phillip Susi wrote:
> On 3/27/2014 11:00 AM, Brian C. Lane wrote:
> > I haven't seen any evidence of that, and that isn't what's
> > happening here.
> 
> Try running the test steps manually so you can see what is going on in
> the middle before the test suite cleans it up.  Last time I did that,
> the loop-file size went from 20m to 2g after running gpt-header-munge.
>  That makes parted complain that the backup is in the wrong place.

Tried that (with my modified code) and it works fine. The header munge
is changing the PE count to 9 from 128 and re-calculating the checksums.

> 
> > Ok, I'll check to make sure that isn't the case -- I thought the 
> > location was updated when it was rewritten, but I'll double check.

I went through the code -- when we rewrite the headers all the sizes and
references are updated, in the pmbr and in both headers.


-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)

Message #23 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Phillip Susi <psusi <at> ubuntu.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 16:10:11 -0700

On Thu, Mar 27, 2014 at 12:55:31PM -0400, Phillip Susi wrote:
> Parted is correct here: the backup *is* in the wrong place, since it
> is at the end of where *we* think the disk is rather than where the
> header thinks it is.  I changed the code to be this way in the first
> place so that this complaint would not be triggered by growing a disk,
> leaving both the LastUableLba and backup locations unchanged.

I think we should remove the second prompt and only check for header at
end of disk. When we fix the header location we should also be adjusting
LastUsableLBA based on the disk size anyway.

Also note that the existing calculations for the backup partition table
entries are relative to AlternateLBA, not LastUsableLBA. In practice
when parted operates on things there is no difference, but according to
the comments in t0210 others may not do the same thing and there is
nothing in the UEFI spec that guarantees that the PTE will start at
LastUsableLBA+1 -- that's why the header has a pointer to the start of
the PTE instead of just calculating it.

In summary:
 * I am pretty confident that my patches are correct
 * We may need additional changes to remove the 'do you want to grow'
   prompt. I think we probably do.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)

Message #26 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 20:04:28 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 03/27/2014 06:48 PM, Brian C. Lane wrote:
> Tried that (with my modified code) and it works fine. The header
> munge is changing the PE count to 9 from 128 and re-calculating the
> checksums.
> 
> I went through the code -- when we rewrite the headers all the
> sizes and references are updated, in the pmbr and in both headers.

Ok, now I'm not seeing that odd 2g problem.  Maybe before I was
running it on a 32 bit machine?  Anyhow, the problem is that
gpt-header-munge shrinks the table but doesn't update LastUsableLba to
agree.  It should do that, but I suppose we could put the two tests
together.  If the backup is *either* at the last sector, *or* the
right location relative to LastUsableLba, then it's in the right place.

Following up with a patch to do this.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJTNLyMAAoJEI5FoCIzSKrw7DAH/j8v8CxDVlRNGsOVYEs7v9hp
LfXsiKEvV/shIa89IOjOZCRYiBESNxNBoRqhlgujrlCKhlB0GCHTsZwdWxQy3nst
/goTdl4gFwKZ/oCZ44SGM9jNpy2eTpwodgPZOIejerRq0a+mzbc5g5PL7xdpreai
jZMKNWTHzxm6ruQt75Img8YyZO7rrxPyOljAB8L6PV2Rf7M4DE6hTkWzZ7sM1PFu
GpGAwHfk7jFBmGEPylg9uC78eP60/swp+aXeSHmOcnylT/ZaPQEkvi1nK2uEdWa0
r5wRkIu976djJ2QTAfgC81SdMJjf4ZFIu6kL6zLPxlAS7xkkgrD6pNEuk+A91VE=
=FfJR
-----END PGP SIGNATURE-----

Message #29 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: bcl <at> redhat.com
Cc: 17108 <at> debbugs.gnu.org
Subject: [PATCH] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 20:05:14 -0400

t0210-gpt-resized-partition-entry-array failed because gpt-header-munge
did not relocate the LastUsableLBA to agree with the smaller table size.
This caused parted to complain that the backup was not at the end of the
disk ( as indicated by LastUsableLBA ), when in fact, it was.  Accept
the current AlternateLBA if it is *either* in the right place relative
to LastUsableLBA, or the last sector of the disk.
---
 libparted/labels/gpt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
index 42b0360..971c1c0 100644
--- a/libparted/labels/gpt.c
+++ b/libparted/labels/gpt.c
@@ -991,7 +991,8 @@ gpt_read (PedDisk *disk)
                        disk->dev->sector_size);
 
       gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU (primary_gpt->AlternateLBA);
-      if (PED_LE64_TO_CPU (primary_gpt->AlternateLBA) != gpt_disk_end)
+      if (gpt_disk_data->AlternateLBA != gpt_disk_end &&
+	  gpt_disk_data->AlternateLBA != disk->dev->length - 1)
         {
           if (ped_exception_throw
                   (PED_EXCEPTION_ERROR,
-- 
1.8.3.2

Message #32 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: bug#17108: [PATCH 1/4] libparted: Check AlternateLBA against LBA-1
Date: Thu, 27 Mar 2014 20:12:08 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 03/27/2014 07:10 PM, Brian C. Lane wrote:
> I think we should remove the second prompt and only check for
> header at end of disk. When we fix the header location we should
> also be adjusting LastUsableLBA based on the disk size anyway.

We only adjust if the user says to.

> Also note that the existing calculations for the backup partition
> table entries are relative to AlternateLBA, not LastUsableLBA. In
> practice when parted operates on things there is no difference, but
> according to the comments in t0210 others may not do the same thing
> and there is nothing in the UEFI spec that guarantees that the PTE
> will start at LastUsableLBA+1 -- that's why the header has a
> pointer to the start of the PTE instead of just calculating it.

The goal is that if it isn't at LastUableLBA+1, then we offer to grow
the table to use the extra space, rather than complain first that the
backup is in the wrong location, and *then* offer to use the extra space.

> In summary: * I am pretty confident that my patches are correct

They completely revert the behavioral fixes of my previously applied
patches, which is why you needed the second patch to fix the other
test to check for *both* errors.

> * We may need additional changes to remove the 'do you want to
> grow' prompt. I think we probably do.

It is the wrong location prompt that we don't want to get when the
disk has grown.  See the patch I just posted; it fixes the table
resize test without breaking anything else.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJTNL5YAAoJEI5FoCIzSKrwY6cH/2X5+fwqFS92dxTlzgskKZ0+
pcto5eREMwa33+HlmBFDYTGNqJE06Q4aQsy7dQLB6YQgKAwPuXK4mARE1xBl3a6B
BV6okzjsjmb9pzginM+d4rmZWfyyoCCOA4zrxycrrK5lsNA/GIz5eCHfOwLS22F2
G77tvFe0nxqgX8A/1+LYXhZqjJ6ARgftErJjIZlpV2jMyo44uJfOTA10BOOsFt5Z
dXoXkhuPFYbxozYn1rraj8RUjXnyV031l5X0C8XgOuPAjT6b9PcjucSTF4w3oNJY
9/shJc2L+yO9wJYBxO8PfWmQXrqBkyg2KWxrsAUAk7HmE6eRitNrNPiCUbAsVhg=
=ljRF
-----END PGP SIGNATURE-----

Message #35 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Phillip Susi <psusi <at> ubuntu.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: [PATCH] libparted: Check AlternateLBA against LBA-1
Date: Fri, 28 Mar 2014 10:02:39 -0700

On Thu, Mar 27, 2014 at 08:05:14PM -0400, Phillip Susi wrote:
> t0210-gpt-resized-partition-entry-array failed because gpt-header-munge
> did not relocate the LastUsableLBA to agree with the smaller table size.
> This caused parted to complain that the backup was not at the end of the
> disk ( as indicated by LastUsableLBA ), when in fact, it was.  Accept
> the current AlternateLBA if it is *either* in the right place relative
> to LastUsableLBA, or the last sector of the disk.
> ---
>  libparted/labels/gpt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
> index 42b0360..971c1c0 100644
> --- a/libparted/labels/gpt.c
> +++ b/libparted/labels/gpt.c
> @@ -991,7 +991,8 @@ gpt_read (PedDisk *disk)
>                         disk->dev->sector_size);
>  
>        gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU (primary_gpt->AlternateLBA);
> -      if (PED_LE64_TO_CPU (primary_gpt->AlternateLBA) != gpt_disk_end)
> +      if (gpt_disk_data->AlternateLBA != gpt_disk_end &&
> +	  gpt_disk_data->AlternateLBA != disk->dev->length - 1)
>          {
>            if (ped_exception_throw
>                    (PED_EXCEPTION_ERROR,
> -- 
> 1.8.3.2
> 

No, this would still set the new end to gpt_disk_end which is the
calculation I was complaining about. You cannot assume that the PTE will
start right after LastUsableLBA, you have to use the backup's
PartitionEntryLBA for any calculations related to that.

When the new backup is written it *must* be written to disk->dev->length
- 1, there is no flexibility there. And because the
  gpt_disk_data->data_area is initialized to the disk size it will
  automatically update the LastUsableLBA when it rewrites the headers.


With these changes are you trying to keep the backup at the wrong place
AND change the LastUsableLBA? If so, that is incorrect behavior.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)

Message #38 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: [PATCH] libparted: Check AlternateLBA against LBA-1
Date: Fri, 28 Mar 2014 13:31:18 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/28/2014 1:02 PM, Brian C. Lane wrote:
> No, this would still set the new end to gpt_disk_end which is the 
> calculation I was complaining about. You cannot assume that the PTE
> will start right after LastUsableLBA, you have to use the backup's 
> PartitionEntryLBA for any calculations related to that.

It doesn't assume the pte will start right after LastUsableLBA.  What
it does is to use LastUsableLBA to infer that AlternateLBA was
perfectly correct, before the disk was grown.

> When the new backup is written it *must* be written to
> disk->dev->length - 1, there is no flexibility there. And because
> the gpt_disk_data->data_area is initialized to the disk size it
> will automatically update the LastUsableLBA when it rewrites the
> headers.

There is flexibility here just as there is for LastUsableLBA.  If the
user chooses not to fix the table so things are where they should be,
then we need to respect that.

> With these changes are you trying to keep the backup at the wrong
> place AND change the LastUsableLBA? If so, that is incorrect
> behavior.

The purpose is to keep the backup where it is if the user says to
ignore the "error" and if the "error" is that the disk has grown, then
just say the disk has grown, without first complaining that the backup
is in the "wrong" place ( since it is in fact, in the right place if
you are going by the old size of the disk ).

In other words, if we know that the backup being in the wrong place is
purely a side effect of the disk growing, then don't treat it as a
separate error; it will be handled as part of the disk growing error.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTNbHmAAoJEI5FoCIzSKrwYEkIAIMxb7P5U97aw3PbemMDOhwg
GuOlE7/6FHZRbEtAzXJM6VGiKB/td+WnGoijbjiR+LcC6qEMJdhoFHWvLE9GNMy3
F4sFZKSglQmQhKexRl0m6MDVH6F15GLruDjBCwAJz92C3V6PCMQ59OKNECf/WgrR
JwwcH2F9Z00cLTOQ0JrCRKI25HiIaNBP8ytHue14fauIWEfNyFVKlpFUeAHcaQWT
nuMF+sFn5HuNsrOcwUjjn4HGlQ/tI1jiz+0ym1wB0E9dSM5/2oTEU8C3l5wq53FI
HZfb8XfwA6FLqA49Nezc2xxr0B8ZUiDrUM6pPRVqE8F2h+avBdq22kXpc2u34HU=
=NDOt
-----END PGP SIGNATURE-----

Message #41 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Phillip Susi <psusi <at> ubuntu.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: [PATCH] libparted: Check AlternateLBA against LBA-1
Date: Mon, 7 Apr 2014 17:38:12 -0700

On Thu, Mar 27, 2014 at 08:05:14PM -0400, Phillip Susi wrote:
> t0210-gpt-resized-partition-entry-array failed because gpt-header-munge
> did not relocate the LastUsableLBA to agree with the smaller table size.
> This caused parted to complain that the backup was not at the end of the
> disk ( as indicated by LastUsableLBA ), when in fact, it was.  Accept
> the current AlternateLBA if it is *either* in the right place relative
> to LastUsableLBA, or the last sector of the disk.
> ---
>  libparted/labels/gpt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
> index 42b0360..971c1c0 100644
> --- a/libparted/labels/gpt.c
> +++ b/libparted/labels/gpt.c
> @@ -991,7 +991,8 @@ gpt_read (PedDisk *disk)
>                         disk->dev->sector_size);
>  
>        gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU (primary_gpt->AlternateLBA);
> -      if (PED_LE64_TO_CPU (primary_gpt->AlternateLBA) != gpt_disk_end)
> +      if (gpt_disk_data->AlternateLBA != gpt_disk_end &&
> +	  gpt_disk_data->AlternateLBA != disk->dev->length - 1)
>          {
>            if (ped_exception_throw
>                    (PED_EXCEPTION_ERROR,
> -- 
> 1.8.3.2
> 

That can't work either:

1 - you dropped PED_LE64_TO_CPU
2 - if the PE count doesn't match the amount of space between the start
and the backup.
3 - when we fix the backup we *must* write it to length-1, not to the
possibly incorrect gpt_disk_end.

I think we're better off dropping one or the other of these prompts.
I've accepted the inevitability of leaving the header in place and using
it as is, I'm fine with those bits of the patches.

The check for extra space will catch a out of position backup, there's
no need to have two different ways to check. Either the backup is at
length-1 or there is extra space.

I'll revisit your comments about last_usable_min_default tomorrow.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)

Message #44 received at 17108 <at> debbugs.gnu.org (full text, mbox):

From: Phillip Susi <psusi <at> ubuntu.com>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 17108 <at> debbugs.gnu.org
Subject: Re: [PATCH] libparted: Check AlternateLBA against LBA-1
Date: Mon, 07 Apr 2014 20:59:47 -0400

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 04/07/2014 08:38 PM, Brian C. Lane wrote:
>> diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c 
>> index 42b0360..971c1c0 100644 --- a/libparted/labels/gpt.c +++
>> b/libparted/labels/gpt.c @@ -991,7 +991,8 @@ gpt_read (PedDisk
>> *disk) disk->dev->sector_size);
>> 
>> gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU
>> (primary_gpt->AlternateLBA); -      if (PED_LE64_TO_CPU
>> (primary_gpt->AlternateLBA) != gpt_disk_end) +      if
>> (gpt_disk_data->AlternateLBA != gpt_disk_end && +
>> gpt_disk_data->AlternateLBA != disk->dev->length - 1) { if
>> (ped_exception_throw (PED_EXCEPTION_ERROR, -- 1.8.3.2
>> 
> 
> That can't work either:
> 
> 1 - you dropped PED_LE64_TO_CPU

Look again; I switched from primary_gpt->AlternateLBA to
gpt_disk_data->AlternateLBA, which already has been converted on the
previous line.

> 2 - if the PE count doesn't match the amount of space between the
> start and the backup.

I don't understand what you mean by this.

> 3 - when we fix the backup we *must* write it to length-1, not to
> the possibly incorrect gpt_disk_end.

We do.  This code is in the read path, not the write path.  It only
avoids throwing an incorrect error in the event that the disk has grown.

> I think we're better off dropping one or the other of these
> prompts. I've accepted the inevitability of leaving the header in
> place and using it as is, I'm fine with those bits of the patches.

As I said before, we don't want to drop this error if the backup
really is in the wrong place; we just don't want to have duplicate
errors that are both caused by the disk growing.

> The check for extra space will catch a out of position backup,
> there's no need to have two different ways to check. Either the
> backup is at length-1 or there is extra space.

No, it won't.  The extra space check only looks to see if there is
room to increase LastUsableLBA.  That may be fine, but AlternateLBA is
not the last sector of the disk.

There are two distinct errors here that have some overlap.  Picture a
venn diagram of the two errors.  The region where they overlap is when
the disk has grown, in which case, we only want to show the extra
space error, but they also have distinct, non overlapping regions.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJTQ0oDAAoJEI5FoCIzSKrw4MUH/ReJGAB1wo2aSYIb7maAY3IG
YSekltL7NzGSPenGF5a5pi0mRerMojHdP/3iaXHKKtgurQ6+PBWIfg/H4nUAc9m+
QcBzewjvwiIHso6Oj26tE3o73cM2DqdpeSfSQ5pLVeW9LUWyS+wETS67Io6fqAsi
w4NPHHavAOAc7p3tbXBXa3bFHV3CawjLwcoKNJu/UNZN1wPoMc1YF/1QXG/VnZUC
HVk092PYPSExRsf6v8vIDZU1RK166+LrjFHhl6jwHYyngdLBCLBvacLZMZ9RGuXG
Vcn+MQuxuoSWbWRXjpHrzQGsS+KdhtW2iWBRwGTuU+utm9j7lDxLPEpgffupV08=
=wuwS
-----END PGP SIGNATURE-----

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.