[parted-devel] [PATCH 1/2] gpt_read: reintroduce restoring the primary/backup GPT when corruption
Hatayama, Daisuke
d.hatayama at jp.fujitsu.com
Thu Nov 15 02:56:11 GMT 2018
Hi Phillip,
> From: Phillip Susi [mailto:phill at thesusis.net]
> Sent: Thursday, November 15, 2018 12:51 AM
...<snip>...
> On 11/13/2018 9:22 PM, Hatayama, Daisuke wrote:
> > 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.
>
> You mean if no other modification is made then the gpt is not written
> back out later?
I don't understand what 'if no other modification is made' means.
Here is demonstration for in case. I attached python script foobar.py
that creates 4 files prGPTcorrupt.bin, backupGPTcorrupt.bin,
prbackupGPTcorrupt.bin and GPTexpanded.bin such that:
# LANG=C parted ./prGPTcorrupt.bin print
Error: The primary GPT table is corrupt, but the backup appears OK, so that will be used.
OK/Cancel?
# LANG=C parted ./backupGPTcorrupt.bin print
Error: The backup GPT table is corrupt, but the primary appears OK, so that will be used.
OK/Cancel?
# LANG=C parted ./prbackupGPTcorrupt.bin print
Error: Both the primary and backup GPT tables are corrupt. Try making a fresh table, and using Parted's rescue feature to recover partitions.
# LANG=C parted ./GPTexpanded.bin print
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)?
Fix/Ignore/Cancel?
For the first and the second case, the corruption is not restored even
if I respond OK:
# LANG=C parted ./prGPTcorrupt.bin print
Error: The primary GPT table is corrupt, but the backup appears OK, so that will be used.
OK/Cancel? OK
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
# LANG=C parted ./prGPTcorrupt.bin print
Error: The primary GPT table is corrupt, but the backup appears OK, so that will be used.
OK/Cancel?
# LANG=C parted ./backupGPTcorrupt.bin print
Error: The backup GPT table is corrupt, but the primary appears OK, so that will be used.
OK/Cancel? OK
Model: (file)
Disk /root/work/parted_work/backupGPTcorrupt.bin: 524kB
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:
Number Start End Size File system Name Flags
# LANG=C parted ./backupGPTcorrupt.bin print
Error: The backup GPT table is corrupt, but the primary appears OK, so that will be used.
OK/Cancel?
For the last case, the corruption is restored if I respond FIX twice:
# LANG=C parted ./GPTexpanded.bin print
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)?
Fix/Ignore/Cancel? Fix
Warning: Not all of the space available to /root/work/parted_work/GPTexpanded.bin appears to be used, you can fix the GPT to use all of the space (an extra 1024 blocks) or continue with the current setting?
Fix/Ignore? Fix
Model: (file)
Disk /root/work/parted_work/GPTexpanded.bin: 1049kB
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:
Number Start End Size File system Name Flags
# LANG=C parted ./GPTexpanded.bin print
Model: (file)
Disk /root/work/parted_work/GPTexpanded.bin: 1049kB
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:
Number Start End Size File system Name Flags
What the patch set is trying to fix is the first and the second
behaviors so that recovery is performed when and only when users
respond OK.
> I'm not sure that's a bad thing.
The reason why I think it problem is it's regression. The UEFI spec
says whether to perform recovery of the primary/backup GPTs is SHOULD,
not MUST so parted command originally didn't have to implement it, but
once provided, needs to keep it.
> You don't expect a parted print operation to modify the disk.
Yes, I don't expect parted read operation to modify the disk.
I think such thought is also common in parted-devel according to the
commit description 432a33115c50005bbe96a09d55edc7d034715ec8, where
print operation is said to be an example of a read-only operation in
parted command:
http://git.savannah.gnu.org/cgit/parted.git/commit/?id=432a33115c50005bbe96a09d55edc7d034715ec8
gpt: "read-only" operation could clobber the pMBR in another way
gpt: "read-only" operation could clobber the pMBR in another way
A read-only operation like "parted $dev print" would overwrite $dev's
pMBR when exactly one of the primary and backup tables was corrupt.
* libparted/labels/gpt.c (gpt_read): Clear "write_back" in those
two cases. Hans De Goede spotted this bug by inspection.
* NEWS (Bug fixes): Mention it.
* tests/t0206-gpt-print-with-corrupt-primary-clobbers-pmbr.sh: New test.
* tests/Makefile.am (TESTS): Add
t0206-gpt-print-with-corrupt-primary-clobbers-pmbr.sh.
I also guess that is assumed in the real world. There is a program
that executes parted print command while expecting it to be
read-only. An example here is sosreport command that is used to
collect a variety of diagnostic data on the system, which doesn't
expect to modify data on the system.
https://github.com/sosreport/sos/blob/master/sos/plugins/block.py#L66
66 "parted -s %s unit s print" % (disk_path),
> > 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:
This is about the last, fourth case in the above example.
>
> An important difference here is that the user must respond yes to cause
> the disk to be modified.
Yes.
But note for in case that UEFI spec says need of user confirmation in
advance is SHOULD, not MUST. The need of user confirmation for parted
command comes from the fact that print command is read-only.
> In the case above, in scripting mode the lack
> of any response will still use the backup.
>
There is no chance for users to make any response in scripting mode
because ped_exception_throw() unconditionally returns PED_EXCEPTION_UNHANDLED
in scripting mode:
static PedExceptionOption
exception_handler (PedException* ex)
{
PedExceptionOption opt;
_print_exception_text (ex);
/* only one choice? Take it ;-) */
opt = option_get_next (ex->options, 0);
if (!option_get_next (ex->options, opt))
return opt;
/* script-mode: don't handle the exception */
if (opt_script_mode || (!isatty (0) && !pretend_input_tty))
return PED_EXCEPTION_UNHANDLED;
got_ctrl_c = 0;
opt = command_line_get_ex_opt ("", ex->options);
return opt;
}
> > 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
>
> I thought ped_disk_clobber was called when you mklabel, not every time
> the table is written.
>
> > 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);
>
> That definitely looks like a dirty hack.
>
> I seem to remember there being a patch a while back to have gpt save and
> restore the existing pbmr when it writes out instead of generating a new
> one each time.
Anyway the issue about the clobbering pMBR mentioned in the commit has
already fixed, right?
Thanks.
HATAYAMA, Daisuke
-------------- next part --------------
A non-text attachment was scrubbed...
Name: foobar.py
Type: application/octet-stream
Size: 1312 bytes
Desc: foobar.py
URL: <http://alioth-lists.debian.net/pipermail/parted-devel/attachments/20181115/452856b6/attachment-0001.obj>
More information about the parted-devel
mailing list