[parted-devel] [PATCH 1/2] gpt_read: reintroduce restoring the primary/backup GPT when corruption

[Hatayama, Daisuke]

Hi Phillip,

> From: Phillip Susi [mailto:phill at thesusis.net]
> Sent: Wednesday, November 14, 2018 4:09 AM

> On 11/7/2018 2:11 AM, Hatayama, Daisuke wrote:
> > Currently, no primary/backup GPT is restored when one of them is
> > corrupted due to the commit 432a33115c50005bbe96a09d55edc7d034715ec8.
> >
> > According to the description of the commit, the purpose of the commit
> > is to deal with corruption of pMBR due to another issue during
> > clobbering, but there seems no such issue because now
> > ped_disk_clobber() avoids pMBR to clobber.
> 
> I'm confused; what is it that you are trying to do?  I'm certain that
> there are a number of errors with GPT that parted detects, and offers to
> fix.
> 

The purpose of this patch set as a whole is to deal with the regression of
recovery of primary/backup GPTs when their corruption is detected, specifically,
in the following path:

  else if (primary_gpt && !backup_gpt)
    {
      /* The primary header is ok, but backup is corrupt.  */
      if (ped_exception_throw
          (PED_EXCEPTION_ERROR, PED_EXCEPTION_OK_CANCEL,
           _("The backup GPT table is corrupt, but the "
             "primary appears OK, so that will be used."))
          == PED_EXCEPTION_CANCEL)
        goto error_free_gpt;

      gpt = primary_gpt;
    }
  else /* !primary_gpt && backup_gpt */
    {
      /* primary GPT corrupt, backup is ok.  */
      if (ped_exception_throw
          (PED_EXCEPTION_ERROR, PED_EXCEPTION_OK_CANCEL,
           _("The primary GPT table is corrupt, but the "
             "backup appears OK, so that will be used."))
          == PED_EXCEPTION_CANCEL)
        goto error_free_gpt;

      gpt = backup_gpt;
    }

Currently, there is a question in this path but no recovery is performed even if
we responds as yes.

On the other hand, there is no issue in another point of recovery below, where
recovery is performed when and only when user responds as yes:

      if (primary_gpt && backup_gpt)
        {
          /* Both are valid.  */
    #ifndef DISCOVER_ONLY
          /* The backup header must be at the end of the disk, or at what the primary
           * header thinks is the end of the disk.
           */
          gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU (primary_gpt->AlternateLBA);
          PedSector pri_disk_end = _hdr_disk_end(disk, primary_gpt);

          if (gpt_disk_data->AlternateLBA != disk->dev->length -1 &&
              gpt_disk_data->AlternateLBA != pri_disk_end)
            {
              if (ped_exception_throw
                      (PED_EXCEPTION_ERROR,
                       (PED_EXCEPTION_FIX | PED_EXCEPTION_IGNORE),
                       _("The backup GPT table is not at the end of the disk, as it "
                         "should be.  Fix, by moving the backup to the end "
                         "(and removing the old backup)?")) == PED_EXCEPTION_FIX)
                {
                  ptt_clear_sectors (disk->dev,
                                     PED_LE64_TO_CPU (primary_gpt->AlternateLBA), 1);
                  gpt_disk_data->AlternateLBA = disk->dev->length -1;
                  write_back = 1;
                }
            }
    #endif /* !DISCOVER_ONLY */
          pth_free (backup_gpt);
          gpt = primary_gpt;
        }

This patch 1/2 is essentially a revert of the commit 432a33115c50005bbe96a09d55edc7d034715ec8.

Only the concern of this patch I think is that whether or not the issue mentioned
in the commit about unintentionally clobbering pMBR has been fixed or not. I guess
the clobbering would be done in ped_disk_clobber() in the following path:

    gpt_read
      ped_disk_commit_to_dev
        ped_disk_clobber

and it looks to me that ped_disk_clobber() now avoids pMBR to clobber if it exists:

    int
    ped_disk_clobber (PedDevice* dev)
    {
            PED_ASSERT (dev != NULL);

            if (!ped_device_open (dev))
                    goto error;

            PedDiskType const *gpt = find_disk_type ("gpt");
            PED_ASSERT (gpt != NULL);

            /* If there is a GPT table, don't clobber the protective MBR.  */
            bool is_gpt = gpt->ops->probe (dev);
            PedSector first_sector = (is_gpt ? 1 : 0);

            /* How many sectors to zero out at each end.
               This must be large enough to zero out the magic bytes
               starting at offset 8KiB on a DASD partition table.
               Doing the same from the end of the disk is probably
               overkill, but at least on GPT, we do need to zero out
               the final sector.  */
            const PedSector n_sectors = 9 * 1024 / dev->sector_size + 1;

            /* Clear the first few.  */
            PedSector n = n_sectors;
            if (dev->length < first_sector + n_sectors)
              n = dev->length - first_sector;
            if (!ptt_clear_sectors (dev, first_sector, n))
              goto error_close_dev;

            /* Clear the last few.  */
            PedSector t = (dev->length -
                           (n_sectors < dev->length ? n_sectors : 1));

            /* Don't clobber the pMBR if we have a pathologically small disk.  */
            if (t < first_sector)
              t = first_sector;
            if (!ptt_clear_sectors (dev, t, dev->length - t))
              goto error_close_dev;

            ped_device_close (dev);
            return 1;

    error_close_dev:
            ped_device_close (dev);
    error:
            return 0;
    }

Thanks.
HATAYAMA, Daisuke