Re: Possibly Broken GPT from Solaris

[Jim Meyering]

Jim Meyering wrote:

> Richard Laager wrote:
>> Do you have any thoughts on this?
>> http://groups.google.com/a/zfsonlinux.org/group/zfs-devel/browse_thread/thread/04a5b2a71114f66e
>>
>> You can download a compressed sample disk image of a newly created
>> whole-disk zpool from Solaris here:
>> http://coderich.net/solaris-data.img.tgz
>>
>> It's 100 MB disk image, but as a sparse file only takes up about 3 MB.
>> Compressed, it's 144 KB.
>>
>> Is the GPT table really broken, or is this a bug in parted? If it's
>> really broken, is this something worth working around so people don't
>> accidentally destroy their partition tables?
>
> Thanks for the report.
> I fixed parted to handle precisely the case of zfs-related GPT tables.
> Here's the NEWS entry for that change:
>
>     libparted can now read partition tables with a number of partition
>     array entries that is different from the default of 128.  Before,
>     it would fail to recognize them and could even read beyond the end
>     of a heap-allocated buffer.
>
> Here's the commit:
>
>     gpt: don't misbehave with e.g., a 9-entry partition array
>     http://git.sv.gnu.org/cgit/parted.git/commit/?id=ce85c5145ed5e267e
>
> There hasn't been a release since that change, so if you want to
> try it before parted-3.1, you'll have to build from git.
>
> Using the latest built from git against your image, I get this:
>
>   $ ./parted -s /t/solaris-data.img u s p
>   Error: The backup GPT table is not at the end of the disk, as it should be. 
>  This might mean that another operating system believes the disk is smaller.  
> Fix, by moving the backup to the end (and removing the old backup)?
>   Warning: Not all of the space available to /t/solaris-data.img appears to 
> be used, you can fix the GPT to use all of the space (an extra 176 blocks) or 
> continue with the current setting?
>   Model:  (file)
>   Disk /t/solaris-data.img: 204800s
>   Sector size (logical/physical): 512B/512B
>   Partition Table: gpt
>
>   Number  Start    End      Size     File system  Name  Flags
>    1      256s     188206s  187951s               zfs
>    9      188207s  204590s  16384s
>
> If you rerun it without -s, you can actually respond to those prompts.
> Hmm... I have just done that, answered "f" (fix) to the two prompts,
> and it seemed to succeed (exit 0), yet rerunning that same command,
> I see something is not quite right:
>
>   $ ./parted /t/solaris-data.img u s p
>   WARNING: You are not superuser.  Watch out for permissions.
>   Error: Both the primary and backup GPT tables are corrupt.  Try making a 
> fresh
>   table, and using Parted's rescue feature to recover partitions.
>   Model:  (file)
>   Disk /t/solaris-data.img: 204800s
>   Sector size (logical/physical): 512B/512B
>   Partition Table: unknown
>
> I'm debugging that right now...

Notice that while the problem you reported was fixed some time ago,
my little demonstration above exposed a different bug.

I've just diagnosed and fixed that other bug with these two patches, and
will push them as soon as I've written a test to exercise the affected code.

>From b1e4a02c3f905ca566175e3c62cfa0c2a7cb1c1a Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 21 Jan 2012 11:49:08 +0100
Subject: [PATCH 1/2] maint: clean-up preparing for bug fix

* libparted/labels/gpt.c (gpt_write): Rename local:
s/ptes_size/ptes_bytes/; declare as size_t, not "int".
Move decls "down".
---
 libparted/labels/gpt.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
index 3fdd672..c74ff51 100644
--- a/libparted/labels/gpt.c
+++ b/libparted/labels/gpt.c
@@ -1193,12 +1193,10 @@ static int
 gpt_write (const PedDisk *disk)
 {
   GPTDiskData *gpt_disk_data;
-  GuidPartitionEntry_t *ptes;
   uint32_t ptes_crc;
   uint8_t *pth_raw;
   GuidPartitionTableHeader_t *gpt;
   PedPartition *part;
-  int ptes_size;

   PED_ASSERT (disk != NULL);
   PED_ASSERT (disk->dev != NULL);
@@ -1206,11 +1204,12 @@ gpt_write (const PedDisk *disk)

   gpt_disk_data = disk->disk_specific;

-  ptes_size = sizeof (GuidPartitionEntry_t) * gpt_disk_data->entry_count;
-  ptes = (GuidPartitionEntry_t *) ped_malloc (ptes_size);
+  size_t ptes_bytes = (gpt_disk_data->entry_count
+                       * sizeof (GuidPartitionEntry_t));
+  GuidPartitionEntry_t *ptes = malloc (ptes_bytes);
   if (!ptes)
     goto error;
-  memset (ptes, 0, ptes_size);
+  memset (ptes, 0, ptes_bytes);
   for (part = ped_disk_next_partition (disk, NULL); part;
        part = ped_disk_next_partition (disk, part))
     {
@@ -1219,7 +1218,7 @@ gpt_write (const PedDisk *disk)
       _partition_generate_part_entry (part, &ptes[part->num - 1]);
     }

-  ptes_crc = efi_crc32 (ptes, ptes_size);
+  ptes_crc = efi_crc32 (ptes, ptes_bytes);

   /* Write protective MBR */
   if (!_write_pmbr (disk->dev))
@@ -1238,7 +1237,7 @@ gpt_write (const PedDisk *disk)
   if (!write_ok)
     goto error_free_ptes;
   if (!ped_device_write (disk->dev, ptes, 2,
-                         ptes_size / disk->dev->sector_size))
+                         ptes_bytes / disk->dev->sector_size))
     goto error_free_ptes;

   /* Write Alternate PTH & PTEs */
@@ -1255,8 +1254,8 @@ gpt_write (const PedDisk *disk)
     goto error_free_ptes;
   if (!ped_device_write (disk->dev, ptes,
                          disk->dev->length - 1 -
-                         ptes_size / disk->dev->sector_size,
-                         ptes_size / disk->dev->sector_size))
+                         ptes_bytes / disk->dev->sector_size,
+                         ptes_bytes / disk->dev->sector_size))
     goto error_free_ptes;

   free (ptes);
--
1.7.9.rc2.2.g183d6


>From 8511629efd9d9720e6a64c0448b346ba7d02cebf Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 21 Jan 2012 12:50:59 +0100
Subject: [PATCH 2/2] libparted: gpt: rewrite even a 9-partition-entry table
 properly

The vast majority of GPT partition tables have 128 PTEs (partition
table entries.  However, zfs-related ones have only 9, and when
rewriting one (which can happen only in interactive mode), parted
would fail to write the full PTE array whenever the PTE array size
was not a multiple of the sector size.  This fixes the same type
of bug as v3.0-45-gce85c51.
* libparted/labels/gpt.c (gpt_write): When computing how many sectors
to write for the PTE array, round up rather than truncating.  This
matters only when n_PTEs * 128 is not a multiple of the sector size.
For details on how to reproduce see:
http://thread.gmane.org/gmane.comp.gnu.parted.bugs/10691/focus=10695
---
 libparted/labels/gpt.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
index c74ff51..bad9ed4 100644
--- a/libparted/labels/gpt.c
+++ b/libparted/labels/gpt.c
@@ -1236,8 +1236,9 @@ gpt_write (const PedDisk *disk)
   free (pth_raw);
   if (!write_ok)
     goto error_free_ptes;
-  if (!ped_device_write (disk->dev, ptes, 2,
-                         ptes_bytes / disk->dev->sector_size))
+  size_t ss = disk->dev->sector_size;
+  PedSector ptes_sectors = (ptes_bytes + ss - 1) / ss;
+  if (!ped_device_write (disk->dev, ptes, 2, ptes_sectors))
     goto error_free_ptes;

   /* Write Alternate PTH & PTEs */
@@ -1253,9 +1254,7 @@ gpt_write (const PedDisk *disk)
   if (!write_ok)
     goto error_free_ptes;
   if (!ped_device_write (disk->dev, ptes,
-                         disk->dev->length - 1 -
-                         ptes_bytes / disk->dev->sector_size,
-                         ptes_bytes / disk->dev->sector_size))
+                         disk->dev->length - 1 - ptes_sectors, ptes_sectors))
     goto error_free_ptes;

   free (ptes);
--
1.7.9.rc2.2.g183d6