bug#15299: [PATCH 1/9] parted: fix EOF and ctrl-c handling

[Brian C. Lane]

On Wed, Sep 11, 2013 at 11:14:50PM -0400, Phillip Susi wrote:
> On 09/11/2013 05:51 PM, Brian C. Lane wrote:
> > Let me see if I understand the problem. The issue is that the disk
> > isn't right, the gpt backup is in the wrong location and currently
> > we will sorta fix that by writing to the correct location if
> > changes are made even if told to ignore it.
> 
> There are two related, but distinct errors.  One problem is that the
> apparent size of the disk does not agree with what the GPT header
> says.  The other is the location of the backup table.  If a disk
> grows, then the size will be wrong, but the backup location will be
> correct, relative to the old size, and incorrect relative to the new
> size of the disk.
> 
> Because parted checks for the backup location first, and the size
> second, after growing a disk, it would complain about the backup
> location, and then also complain about the size.  It didn't make much
> sense to have to fix both and the first message is a bit confusing, so
> now it will not complain about the backup location since it does agree
> with the previous size of the disk, and if you choose to fix the size,
> the backup will then be moved to the correct location as part of the
> size fix.

Ok, that sounds reasonable.

> 
> If you chose not to fix the location ( whether it is wrong entirely or
> only because of a change in size ), parted would go ahead and write it
> to the correct location anyhow, without zeroing the previous copy.
> This is clearly wrong.

Agreed.

> 
> > I'd lean towards not allowing any operations at all until the
> > location is fixed. If it isn't at the end it is in the wrong place
> > and we really have no way to tell why it isn't right.
> 
> If the apparent disk size is wrong and the user chooses to let it be (
> maybe another OS can't see the full disk size and they want to
> maintain compatibility with it ), then the backup should also be left
> where it is, at least if it is in the correct location from the
> perspective of the other OS.  As long as the backup lies outside of
> the usable portion of the disk, not being at the end ( as we see it )
> should not cause any harm, and complaining that it isn't at the end as
> we see it, after they have already said to accept the other OS's view
> of where the end of the disk is, is counter productive.
>
> In the event that the backup is at neither the current or previous end
> of the disk, I lean towards allowing the user to ignore that and
> continue unless we have a compelling reason not to.

Why wouldn't the OS be able to see the full disk size? With a msdos
label, yes, that is possible, but with GPT we cover the whole disk.
There shouldn't be problems with the OS not being able to address the
whole thing.

It seems like allowing this could lead to something (maybe not parted)
adding partitions after the back, or on top of it. I think allowing
operations on something that is clearly not correct can only lead to
other problems in the future.

This would also mean that we support violating the GPT spec, which says
(in part) in seciton 5.3

"The primary GUID Partition Table Header must be located in LBA 1 (i.e.,
the second logical block) and the backup GUID Partition Table Header
must be located in the last LBA of the logical device."

Note that this says 'must' and that they are not relative to each other,
but are absolute positions related to the size of the device. If the
primary is corrupt we must be able to find the backup at the last LBA.

I'm ok with fixing the backup location and the size as a single
warning/operation. But if they choose to leave it alone I think we
should refuse to make any changes to the disk. Anything else encourages
the use of disks with things in the wrong place.

> > Unrelated to that, I also notice that the gpt-head-move.py script
> > isn't taking into account endianness. I just ran into a similar
> > problem with the gpt-header-munge script. You can't just
> > pack/unpack from the header without converting from little-endian.
> 
> I saw that in your patch set today, and yes, since I adapted from
> gpt-head-move.py, it probably needs the same fix.
> 
> > I don't have any better ideas yet. I'm just worried all those
> > changes are going to break something subtle.
> 
> That's why we have a regression test suite ;)
> 
> I'm ok with tabling it for the next release, then apply it after and
> give it some more testing time.

Thanks.

> > Ends up I just had to do something similar to get filesystem
> > probing working all the time. See patch 18 in my set. What I did
> > was keep the kernel 2.6 check and skip the sync on the individual
> > partitions. I think Hans' point about udev, etc. churn while doing
> > this is a good one. parted only operates on offsets inside the
> > parent device anyway so I think it is better to only sync that.
> 
> I don't think that is enough.  Flushing the disk device will clear out
> any stale data in that cache and force it to be re-read from disk when
> we probe the fs type, but if the partition cache still has dirty write
> buffers, the disk is still out of date so re-reading it will still
> return stale data.

Reading the partition nodes will still be stale, but we never do that so
I don't think it is our job to handle that.

> 
> As far as udev churn goes, I have another patch that is now in Debian
> and Ubuntu that I still need to rebase onto upstream parted that
> refactors the BLKPG code to avoid removing and recreating unmodified
> partitions.

Cool, I'd like to see that.


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

pgp37nTnaTh2H.pgp
Description: PGP signature