[parted-devel] [PATCH 1/2] gpt_read: reintroduce restoring the primary/backup GPT when corruption
Hatayama, Daisuke
d.hatayama at jp.fujitsu.com
Mon Nov 19 13:15:50 GMT 2018
Hi Brian, John and Phillip,
> From: parted-devel
> [mailto:parted-devel-bounces+d.hatayama=jp.fujitsu.com at alioth-lists.debian
> .net] On Behalf Of Hatayama, Daisuke
> Sent: Monday, November 19, 2018 10:27 AM
> > From: parted-devel
> >
> [mailto:parted-devel-bounces+d.hatayama=jp.fujitsu.com at alioth-lists.debian
> > .net] On Behalf Of Brian C. Lane
> > Sent: Saturday, November 17, 2018 3:45 AM
> >
> > On Wed, Nov 07, 2018 at 07:11:05AM +0000, 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 am opposed to this change. I think the current behavior is safer, and
> > makes sense. Doing a print should not make changes to the disk, and when
>
> I also think print should not make any changes to the disk.
>
> But I also think it important to fix regression of restoring the
> primary/backup GPTs headers.
>
> The design of this patch set is not always to allow restoring the
> primary/backup GPT headers for print command but when and only when
> there is explicit confirmation from users.
> Don't perform the restoring when there is no confirmation from users
> such as in the script mode.
>
> There is the 2/2 patch in this patch set and please also read it here:
>
> [parted-devel] [PATCH 2/2] gpt_read: don't restore the primary/backup GPT
> in script mode
>
> https://alioth-lists.debian.net/pipermail/parted-devel/2018-November/00533
> 1.html
>
> If you still think print command should not modify data on the disk even in
> the
> interactive mode with explicit user confirmation, I'll make next patch set that
> tries to allow restoring the primary/backup GPT headers only for the commands
> that are not read-only.
> But then I will need suggestion on how to modify parted data structure to
> pass information of the currently executed command to gpt_read().
>
> > one or the other header is corrupt (but not both) things still work
> > correctly.
>
> Currently, no restoring of the primary/backup GPT headers is performed
> in the interactive mode even when there is explicit confirmation from users
> and the command being executed is the one that is not read-only.
>
> For example:
>
> # LANG=C parted ./prGPTcorrupt.bin "rescue 0 1"
> Error: The primary GPT table is corrupt, but the backup appears OK, so that
> will be used.
> parted: invalid token: 0
> OK/Cancel? OK
> Start? 0
> End? 1%
> Error: Can't have a partition outside the disk!
> Error: Can't have a partition outside the disk!
> Error: Can't have a partition outside the disk!
> ...<snip>...
>
> # LANG=C parted -s ./prGPTcorrupt.bin print
> Error: The primary GPT table is corrupt, but the backup appears OK, so that
> will be used.
> Model: (file)
> Disk /root/work/parted_work/prGPTcorrupt.bin: 524kB
> Sector size (logical/physical): 512B/512B
> Partition Table: gpt
> Disk Flags:
>
> Number Start End Size File system Name Flags
>
> Do you say this behavior is correct?
> >
> > The header *will* be fixed when you make an explicit change to the disk.
>
> What I think problematic is a regression of the feature.
>
I figured out what you guys are saying now.
The following commands call ped_disk_commit() meaning that the GPT
header is ultimately updated in each of the following commands.
- cp
- mkfs
- mkpart
- mkpartfs
- move
- name
- resize
- mklabel
- rm
- set
So, there is no explicit recovery for these commands in gpt_read() but
restoring is done in a whole part of each command. OTOH, read-only sub
commands such as print don't call ped_disk_commit() thus naturally GPT
headers are never restored.
I agree to this design but there are still the remaining two issues:
1) There is still possibility of retoring the GPT headers is performed
via read-only sub-commands here:
957static int
958gpt_read (PedDisk *disk)
959{
...<snip>...
994 if (primary_gpt && backup_gpt)
995 {
996 /* Both are valid. */
997#ifndef DISCOVER_ONLY
998 /* The backup header must be at the end of the disk, or at what the primary
999 * header thinks is the end of the disk.
1000 */
1001 gpt_disk_data->AlternateLBA = PED_LE64_TO_CPU (primary_gpt->AlternateLBA);
1002 PedSector pri_disk_end = _hdr_disk_end(disk, primary_gpt);
1003
1004 if (gpt_disk_data->AlternateLBA != disk->dev->length -1 &&
1005 gpt_disk_data->AlternateLBA != pri_disk_end)
1006 {
1007 if (ped_exception_throw
1008 (PED_EXCEPTION_ERROR,
1009 (PED_EXCEPTION_FIX | PED_EXCEPTION_IGNORE),
1010 _("The backup GPT table is not at the end of the disk, as it "
1011 "should be. Fix, by moving the backup to the end "
1012 "(and removing the old backup)?")) == PED_EXCEPTION_FIX)
1013 {
1014 ptt_clear_sectors (disk->dev,
1015 PED_LE64_TO_CPU (primary_gpt->AlternateLBA), 1);
1016 gpt_disk_data->AlternateLBA = disk->dev->length -1;
1017 write_back = 1;
1018 }
1019 }
1020#endif /* !DISCOVER_ONLY */
1021 pth_free (backup_gpt);
1022 gpt = primary_gpt;
1023 }
This seems inconsistent with the current design.
2) The UEFI spec says that software MUST report whenever it restores a
GPT:
If the primary GPT is corrupt, software must check the last LBA
of the device to see if it has a valid GPT Header and point to a
valid GPT Partition Entry Array. If it points to a valid GPT
Partition Entry Array, then software should restore the primary
GPT if allowed by platform policy settings (e.g. a platform may
require a user to provide confirmation before restoring the
table, or may allow the table to be restored
automatically). Software must report whenever it restores a GPT.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Software should ask a user for confirmation before restoring the
primary GPT and must report whenever it does modify the media to
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
restore a GPT. If a GPT formatted disk is reformatted to the
^^^^^^^^^^^^^^
legacy MBR format by legacy software, the last logical block
might not be overwritten and might still contain a stale GPT. If
GPT-cognizant software then accesses the disk and honors the
stale GPT, it will misinterpret the contents of the
disk. Software may detect this scenario if the legacy MBR
contains valid partitions rather than a protective MBR (see
Section 5.2.1).
However, there is now no explicit report message indicating that
parted command has performed restoring the primary/backup GPT
headers.
I thought the following messages are used for both user confirmation
and report:
Error: The primary GPT table is corrupt, but the backup appears OK, so that will be used.
Error: The backup GPT table is corrupt, but the primary appears OK, so that will be used.
but they are output also via print command where restoring GPTs is
not performed. Users cannot use these messages as report message.
parted command need to output report message for that purpose when
ped_disk_commit() is successfully done in each of the above 10
commands.
I want to fix the 1) above, but I don't come up with good design to do that.
I guess it's enough to pass information of currently running command to
gpt_read() (or at least just the information that the command
currently being executed is read-only). But I'm unfamiliar to parted
data structures so not trivial about how to pass it in the following
path:
command_run (Command* cmd, PedDevice** dev, PedDisk** diskp)
do_mkpart (PedDevice** dev, PedDisk** diskp)
ped_disk_new (PedDevice* dev)
gpt_read (PedDisk *disk)
Any suggestions?
Thanks.
HATAYAMA, Daisuke
More information about the parted-devel
mailing list