[Libguestfs] [PATCH nbdkit v2 2/4] partition filter: Support MBR logical partitions.
Eric Blake
eblake at redhat.com
Tue Jan 22 21:04:07 UTC 2019
On 1/21/19 12:15 PM, Richard W.M. Jones wrote:
> ---
> filters/partition/nbdkit-partition-filter.pod | 5 -
> filters/partition/partition-mbr.c | 105 +++++++++++++++++-
> tests/test-partitioning1.sh | 22 +++-
> 3 files changed, 121 insertions(+), 11 deletions(-)
>
> @@ -69,16 +75,16 @@ find_mbr_partition (struct nbdkit_next_ops *next_ops, void *nxdata,
> {
> int i;
> struct mbr_partition partition;
> + uint32_t ep_start_sector, ep_nr_sectors;
> + uint64_t ebr, next_ebr;
> + uint8_t sector[SECTOR_SIZE];
>
> - if (partnum > 4) {
> - nbdkit_error ("MBR logical partitions are not supported");
> - return -1;
> - }
> -
> + /* Primary partition. */
> for (i = 0; i < 4; ++i) {
> get_mbr_partition (mbr, i, &partition);
> if (partition.nr_sectors > 0 &&
> partition.part_type_byte != 0 &&
> + !is_extended (partition.part_type_byte) &&
> partnum == i+1) {
Given a disk with only 2 partitions, but requesting access to partition
3, fails this loop, and falls into:
> *offset_r = partition.start_sector * SECTOR_SIZE;
> *range_r = partition.nr_sectors * SECTOR_SIZE;
> @@ -86,6 +92,95 @@ find_mbr_partition (struct nbdkit_next_ops *next_ops, void *nxdata,
> }
> }
>
> + /* Logical partition. */
> +
> + /* Find the extended partition in the primary partition table. */
> + for (i = 0; i < 4; ++i) {
> + get_mbr_partition (mbr, i, &partition);
> + if (partition.nr_sectors > 0 &&
> + is_extended (partition.part_type_byte)) {
> + goto found_extended;
> + }
> + }
> + nbdkit_error ("MBR logical partition selected, "
> + "but there is no extended partition in the partition table");
> + return -1;
...this code, which returns the wrong error message (partition 3 is not
logical, but the real problem is that there is no partition 3 in the
master table, not that there is no extended partition).
For example:
term1$ ./nbdkit --unix nbd.sock -r -fv partitioning README README
term2$ ./nbdkit -p 10810 -r -fv --filter=partition nbd socket=nbd.sock
partition=3
term3$ ./qemu-nbd --list -p 10810
term2> ...
nbdkit: nbd[1]: error: MBR logical partition selected, but there is no
extended partition in the partition table
nbdkit: nbd[1]: debug: partition: close
nbdkit: nbd[1]: debug: close
nbdkit: nbd[1]: debug: sending request type 2 (NBD_CMD_DISC), flags 0,
offset 0, count 0, cookie 0
nbdkit: debug: permanent failure while talking to server
/home/eblake/nbdkit/nbd.sock: Bad message
term3>
qemu-nbd: Failed to read initial magic: Unexpected end-of-file before
all bytes were read
It's also a bit awkward that we don't detect the invalid partition
number until a client first connects - I don't know if that can be
improved to detect it earlier before even allowing clients, especially
if we are going to hand up on the client before even giving them the
magic number. (I also don't know if the "Bad message" claim from the
nbd plugin is worth improving, but that's unrelated to this series).
Similarly, if requesting partition 4, when partition 4 happens to be the
extended partition container for logical partitions, the code goes to
found_extended...
> +
> + found_extended:
> + ep_start_sector = partition.start_sector;
> + ep_nr_sectors = partition.nr_sectors;
> + ebr = ep_start_sector * (uint64_t)SECTOR_SIZE;
> +
> + /* This loop will terminate eventually because we only accept
> + * links which strictly increase the EBR pointer.
The statement is a fair limitation on what we are willing to read, and
matches what the partitioning plugin generates, but I think there are
disks in the wild where you can have logical partitions out of order.
If we DO decide to support such disks, we'll need to make sure a bad
disk can't make us loop forever when chasing backwards.
> + */
> + for (i = 5; ; ++i) {
...performs the loop but never finds partition 4 (because it started
looking for partition 5)...
> + /* Check that the ebr is aligned and pointing inside the disk
> + * and doesn't point to the MBR.
> + */
> + if (!IS_ALIGNED (ebr, SECTOR_SIZE) ||
> + ebr < SECTOR_SIZE || ebr >= size-SECTOR_SIZE) {
> + nbdkit_error ("invalid EBR chain: "
> + "next EBR boot sector is located outside disk boundary");
> + return -1;
> + }
> +
> + /* Read the EBR sector. */
> + if (next_ops->pread (nxdata, sector, sizeof sector, ebr, 0,
> + &errno) == -1)
> + return -1;
> +
> + if (i == partnum) {
> + uint64_t offset, range;
> +
> + /* First entry in EBR points to the logical partition. */
> + get_mbr_partition (sector, 0, &partition);
> +
> + /* The first entry start sector is relative to the EBR. */
> + offset = ebr + partition.start_sector * (uint64_t)SECTOR_SIZE;
> + range = partition.nr_sectors * (uint64_t)SECTOR_SIZE;
> +
> + /* Logical partition cannot be before the corresponding EBR,
> + * and it cannot extend beyond the enclosing extended
> + * partition.
> + */
> + if (offset <= ebr ||
> + offset + range >
> + ((uint64_t)ep_start_sector + ep_nr_sectors) * SECTOR_SIZE) {
> + nbdkit_error ("logical partition start or size out of range "
> + "(offset=%" PRIu64 ", range=%" PRIu64 ", "
> + "ep:startsect=%" PRIu32 ", ep:nrsects=%" PRIu32 ")",
> + offset, range, ep_start_sector, ep_nr_sectors);
> + return -1;
> + }
> + *offset_r = offset;
> + *range_r = range;
> + return 0;
> + }
> +
> + /* Second entry in EBR links to the next EBR. */
> + get_mbr_partition (sector, 1, &partition);
> +
> + /* All zeroes means the end of the chain. */
> + if (partition.start_sector == 0 && partition.nr_sectors == 0)
> + break;
> +
> + /* The second entry start sector is relative to the start to the
> + * extended partition.
> + */
> + next_ebr =
> + ((uint64_t)ep_start_sector + partition.start_sector) * SECTOR_SIZE;
> +
> + /* Make sure the next EBR > current EBR. */
> + if (next_ebr <= ebr) {
> + nbdkit_error ("invalid EBR chain: "
> + "next EBR %" PRIu64 " <= current EBR %" PRIu64,
> + next_ebr, ebr);
In other words, this safety check matches the comment above that you
insist on making progress, but I think actual hardware can boot logical
partitions that are listed in non-ascending order.
> + return -1;
> + }
> + ebr = next_ebr;
> + }
> +
> nbdkit_error ("MBR partition %d not found", partnum);
...and eventually gives the misleading message that the partition was
not found (when it was found, but was not usable because it is extended
rather than primary).
> return -1;
> }
> diff --git a/tests/test-partitioning1.sh b/tests/test-partitioning1.sh
> index 76ab43b..8aa45b9 100755
> --- a/tests/test-partitioning1.sh
> +++ b/tests/test-partitioning1.sh
> @@ -77,7 +77,27 @@ nbdkit -f -v -D partitioning.regions=1 -U - \
> # Contents of partitioning1.out should be identical to file-data.
> cmp file-data partitioning1.out
>
> -# Same test with GPT and more partitions.
> +# Same test with > 4 MBR partitions.
> +# Note we select partition 6 because partition 4 is the extended partition.
> +nbdkit -f -v -D partitioning.regions=1 -U - \
> + --filter=partition \
> + partitioning \
> + partitioning1-p1 \
> + partitioning1-p2 \
> + partitioning1-p3 \
> + partitioning1-p4 \
> + type-guid=A2A0D0EB-E5B9-3344-87C0-68B6B72699C7 \
Does type-guid even work with mbr?
> + file-data \
> + type-guid=AF3DC60F-8384-7247-8E79-3D69D8477DE4 \
> + partitioning1-p5 \
> + partitioning1-p6 \
> + partition-type=mbr \
> + partition=6 \
> + --run 'qemu-img convert $nbd partitioning1.out'
> +
> +cmp file-data partitioning1.out
> +
> +# Same test with GPT.
> nbdkit -f -v -D partitioning.regions=1 -U - \
> --filter=partition \
> partitioning \
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190122/a58cdb36/attachment.sig>
More information about the Libguestfs
mailing list