Libguestfs - Jan 2019 - Re: [PATCH nbdkit v2 2/4] partition filter: Support MBR logical partitions.

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

On Tue, Jan 22, 2019 at 03:04:07PM -0600, Eric Blake
wrote:
> ...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).
Ugh yes, that was all wrong.

[...]
> 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).
I think this would require some rather deeper changes to the way
filters work.
> > +       type-guid=A2A0D0EB-E5B9-3344-87C0-68B6B72699C7 \
> 
> Does type-guid even work with mbr?
No, this is an error -- in existing code.  I'll add an extra patch in
v3 to catch this.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Tue, Jan 22, 2019 at 09:44:07PM +0000, Richard W.M. Jones
wrote:
> > Does type-guid even work with mbr?
> 
> No, this is an error -- in existing code.  I'll add an extra patch in
> v3 to catch this.
... or I would do, except that's quite hard.

I'll fix the problem in this patch in v3 however :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org