Fix up GPT partition tables automatically #119
Conversation
|
This fails to build on several distributions due to |
gptfixer/gpt.c
Outdated
| new_gpt = gpt_grow_sectors(used_primary ? sectors - 1 : 1, sector_size, | ||
| new_sector_size, sectors, gpt, &to_write); | ||
| } else { | ||
| new_gpt = gpt_shrink_sectors(used_primary ? sectors - 1 : 1, sector_size, | ||
| new_sector_size, sectors, gpt, &to_write); |
There was a problem hiding this comment.
Take location of the other GPT from the header, instead of hardcoding to sectors - 1 - otherwise it will explode on first boot after resizing disk (if that happens to be also first boot on a disk with different sector size). Same in few other locations below.
(don't try to adjust GPT size to the actual disk size - leave that to the OS)
There was a problem hiding this comment.
The backup GPT is only looked at if the primary GPT is invalid. If the primary GPT is valid, the backup GPT is written before the primary GPT is. Since a GPT cannot be written atomically, this requires that the backup GPT must be written at the end of the disk, so that if there is a problem (such as power failure) either the backup GPT is now valid (at the new sector size) or the old primary GPT (with the old sector size) is still present. Furthermore, writing a GPT will clobber any other GPTs at the same end of the disk, so if the primary GPT was used the backup GPT must be written first and visa versa. Therefore, adjusting the GPT size to the actual disk size is unavoidable, unless I am missing something.
There was a problem hiding this comment.
The backup GPT is only looked at if the primary GPT is invalid.
That's not true. At the very least, fdisk will complain if backup GPT doesn't match the primary GPT even if primary GPT is valid.
Anyway, I'm not going to argue about this too much, it's a very rare case anyway (resizing the disk and changing sector size at the same time). Just ensure you don't resize GPT if you don't change sector size (I think this case is okay currently).
There was a problem hiding this comment.
This may be a corner case, but it is still a case I want to handle correctly. Can you explain, in detail, how this could go wrong?
My understanding is:
- Tools must create a primary and backup GPT.
- Tools must accept a primary GPT that is valid, even if the backup GPT is invalid.
- Tools must use the primary GPT if it is valid.
- If the primary GPT is valid but the backup GPT is invalid or visa versa, tools must not reject the GPT altogether but are allowed to issue a warning.
Do real-world tools behave differently than this?
There was a problem hiding this comment.
Take location of the other GPT from the header, instead of hardcoding to
sectors - 1- otherwise it will explode on first boot after resizing disk (if that happens to be also first boot on a disk with different sector size). Same in few other locations below.
Do you mean that if the primary GPT header is valid, but the primary GPT entries are invalid, the backup GPT header should be read from AlternateLBA in the primary header?
Most of the places where sectors - 1 is written refer to either:
- Where to write the backup GPT. The backup GPT is meant as a backup if the primary GPT is bad, so it needs to be written at the well-known location regardless of where the primary GPT is.
- Where to find the backup GPT if the primary GPT is bad. In this case (such as if e.g. the primary header CRC32 checksum is invalid), there is no point (that I can think of) in using the primary GPT header’s
AlternateLBAat all, as it’s presumably garbage.
There was a problem hiding this comment.
(don't try to adjust GPT size to the actual disk size - leave that to the OS)
Ah, qubes-rootfs-resize.service. That could also use this tool to find the partition to resize, instead of hardcoding 3 (but if you want to hardcode 3, that’s fine too).
|
(the current version fails to build) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
===========================================
- Coverage 79.45% 60.56% -18.89%
===========================================
Files 5 6 +1
Lines 477 951 +474
Branches 0 104 +104
===========================================
+ Hits 379 576 +197
- Misses 98 285 +187
- Partials 0 90 +90 ☔ View full report in Codecov by Sentry. |
DemiMarie
changed the title
|
This is now ready for review, and I recommend backporting to R4.2 if possible. It’s a rather large change, but the bug it fixes is somewhat embarrassing. |
|
Debian 12 HVM fails to start with in-vm kernel (with no sector change needed): |
|
(and one log line got junk prefix) |
I don't see how that would be from this tool, maybe that's still from grub cleaning up terminal settings. |
|
And it looks like dom0-provided initramfs lacks the fixgpt binary: |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024120723-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024111705-4.3&flavor=update
Failed tests5 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/119126#dependencies 3 fixed
Unstable tests
|
|
Another test iteration, this time Debian 12 HVM worked, but Debian 12 PV and Fedora 40 PVH failed: Looking at |
|
And also, even though GPT didn't need changes here, I see reloading partition table, twice. Lets fix it last, as having this issue makes it easier to debug the other one. |
|
Just to not forget - there are a bunch of |
|
@marmarek: can you retest the most recent push? It should fix all of the race conditions, though it loses a couple features (and so is not suitable for merging). |
|
you forgot to update gitlab-ci conf |
|
The test fails due to difference on "Syncing disks." line. And also, if you extend |
|
There is still partition table reload in no-change case. Can this be avoided? |
|
I think it's automatic on device close, if it was opened for writing. Maybe it can be opened read-only for the checking phase? |
DemiMarie
force-pushed
the
gpt-fixer
branch
2 times, most recently
from
6b04d71 to
c8975a7
Compare
|
the CI test still fails |
Now test coverage is properly reported :) The not covered lines are the expected ones - mostly verbose messages and various error handling. Especially, all the code that is expected to be used normally looks to be covered, so all good :) |
This is the only point left. Otherwise all good and tested. |
This includes a C program that adjusts partition tables when the device sector size changes. It is intended to be secure against malicious inputs, although it will not be used with untrusted inputs in Qubes OS. In particular, all calculations are done with sufficiently wide integer types to prevent overflow or wraparound, partition tables read from disk are strictly validated before use, and the total size of the partition table entries is limited to 1MiB. The partition table adjustment does not change the start or end of any partition. If the old sector size is 512 and the new size is 4096, the tool will return an error if any partition does not start or end on a 4096-byte boundary. Furthermore, this tool will refuse to write a partition table if either the primary or backup GPT would overwrite data that is part of any partition. This ensures that user data is not corrupted. All used entries in the original GPT are preserved. However, unused entries will be truncated if necessary. The FirstUsableLBA and LastUsableLBA fields are converted if necessary, and are rounded in the direction of _less_ usable space. This means that FirstUsableLBA is rounded up, while LastUsableLBA is rounded down. If the new primary GPT uses space after FirstUsableLBA, or the new backup GPT uses space before LastUsableLBA, the values of these fields are adjusted accordingly. The new partition tables are written in a crash-safe way. This ensures that in the event of a system crash or power loss, there is always a valid GPT that can be used. The specific algorithm used is as follows: 1. If the GPT being written is the primary GPT, start by writing the pMBR, followed by (new sector size - 512) bytes of zeroes. 2. If the new sector size is 512, overwrite the old 512-byte-sector GPT header with zeroes, then overwrite the old 4096-byte-sector GPT header with zeroes. 3. Write the partition entries. 4. Write the new partition header. All writes are made with O_SYNC, so they cannot be reordered on-disk. Furthermore, overwriting a single sector is atomic. If the primary GPT was valid, the backup GPT is written first; otherwise, the primary GPT is written first. The default CFLAGS enable sanitizers and are intended for local development only. Production builds should set CFLAGS via the 'make' command line or an environment variable, which will override the CFLAGS in the Makefile. The tool will exit with a failure status if file descriptor 0, 1, or 2 is closed when it is invoked. Using these FDs could potentially cause the device to get clobbered or read from accidentally, which would be bad. The block device is opened exclusively to prevent racing with other code that will use the device. It is fsync()d before being used to ensure that the code operates on the most recent on-disk state, not on stale state cached in memory. The whole block device is locked exclusively to avoid racing with udev. If the user passes a partition, the tool exits with an error. A BIOS Parameter Block (BPB) is checked for, to avoid accidentally corrupting a FAT or NTFS file system that happens to have a GPT with valid checksums at the wrong location.
Is it okay to keep |
This adjusts the GPT in the initramfs, and (obviously) requires the previous commit. Fixes: QubesOS/qubes-issues#4974
No functional change. The large number of partitions in the test is to ensure that the code correctly handles such partition tables without a buffer overflow. Previous versions of the code did have a buffer overflow due to incorrectly computing the buffer size.
DemiMarie
force-pushed
the
gpt-fixer
branch
2 times, most recently
from
f3a860d to
01c0506
Compare
Yes, this is fine.
This should be okay too |
This has many problems:
not checked when reading (but are created when writing).are now always checked.NDEBUGis forcibly undefined.Fixes: QubesOS/qubes-issues#4974