Re: [Libguestfs] [libnbd PATCH v3 01/22] block_status: Refactor array storage

[Eric Blake]

On Thu, May 25, 2023 at 06:30:37PM +0200, Laszlo Ersek wrote:
> On 5/25/23 15:00, Eric Blake wrote:
> > For 32-bit block status, we were able to cheat and use an array with
> > an odd number of elements, with array[0] holding the context id, and
> > passing &array[1] to the user's callback.  But once we have 64-bit
> > extents, we can no longer abuse array element 0 like that, for two
> > reasons: 64-bit extents contain uint64_t which might not be
> > alignment-compatible with an array of uint32_t on all architectures,
> > and the new NBD_REPLY_TYPE_BLOCK_STATUS_EXT adds an additional count
> > field before the array.
> >
> > +++ b/generator/states-reply-structured.c
> > @@ -126,19 +126,10 @@  REPLY.STRUCTURED_REPLY.CHECK:
> >          length < 12 || ((length-4) & 7) != 0)
> 
> This is important (the context doesn't show it in full): we're under
> NBD_REPLY_TYPE_BLOCK_STATUS here (nested under
> REPLY.STRUCTURED_REPLY.CHECK), and we enforce that
> 
>   length = be32toh (h->sbuf.sr.structured_reply.length);
> 
> contains the context_id (4 bytes), plus a positive integral number of
> block descriptor structures (8 bytes each).

And when 64-bit replies are added, there's a counterpart that contains
a header (4+4 bytes) and then a positive integral number of block
descriptors (16 bytes each).

> > @@ -445,15 +468,16 @@  REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
> >      assert (h->bs_entries);
> >      assert (length >= 12);
> >      assert (h->meta_valid);
> > +    count = (length - sizeof h->sbuf.sr.payload.bs_hdr) / sizeof 
> > *h->bs_entries;
> 
> I have a slight problem with the pre-patch code here. We keep the
> existent assertions (good), but I think the pre-patch RECV_BS_ENTRIES
> code misses an assertion. Namely, after the size check (i.e., 12+
> bytes), the pre-patch code should have said
> 
>       assert ((length-4) & 7) == 0);
> 
> emphasizing the explicit check under REPLY.STRUCTURED_REPLY.CHECK.
> 
> The pre-patch code relies on this (a) silently by expecting (length/4)
> to be an integer (in the mathematical sense), and (b) very silently by
> expecting (length/4) to be an *odd* integer >= 3.

Likewise, once 64-bit replies are added, we will depend on (h->len/8)
being an *odd* integer >= 3, but additionally that the count in the
header match the (h->len-8)/16 computation.

> 
> Here's what I suggest as an update for this patch (to be squashed):
> 
> diff --git a/generator/states-reply-structured.c 
> b/generator/states-reply-structured.c
> index da1e46929cd0..6cd4a49baa26 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -43,6 +43,20 @@ structured_reply_in_bounds (uint64_t offset, uint32_t 
> length,
>    return true;
>  }
> 
> +static bool
> +bs_reply_length_ok (uint32_t length)
> +{
> +  if (length < (sizeof (struct nbd_structured_reply_block_status_hdr) +
> +                sizeof (struct nbd_block_descriptor)))
> +    return false;
> +
> +  length -= sizeof (struct nbd_structured_reply_block_status_hdr);
> +  if (length % sizeof (struct nbd_block_descriptor) != 0)
> +    return false;
> +
> +  return true;
> +}

This is only valid for the 32-bit reply; but could easily be
generalized to cover the 64-bit reply as well.  I definitely like
wrapping the computation behind a helper function, so that we don't
have to open-code repeat it in multiple spots.

I could even go with doing this sort of cleanup as its own
prerequisite patch instead of squashing it in with this one,
because....

> +
>  STATE_MACHINE {
>   REPLY.STRUCTURED_REPLY.START:
>    /* We've only read the simple_reply.  The structured_reply is longer,
> @@ -123,7 +137,7 @@  REPLY.STRUCTURED_REPLY.CHECK:
> 
>    case NBD_REPLY_TYPE_BLOCK_STATUS:
>      if (cmd->type != NBD_CMD_BLOCK_STATUS ||
> -        length < 12 || ((length-4) & 7) != 0)
> +        !bs_reply_length_ok (length))
>        goto resync;

...that does indeed look easier to read.

> @@ -466,8 +480,11 @@  REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES:
>      assert (cmd->type == NBD_CMD_BLOCK_STATUS);
>      assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
>      assert (h->bs_entries);
> -    assert (length >= 12);
> +    assert (bs_reply_length_ok (length));
>      assert (h->meta_valid);
> +    STATIC_ASSERT ((sizeof (struct nbd_block_descriptor) %
> +                    sizeof *h->bs_entries) == 0,
> +                   _block_desc_is_multiple_of_bs_entry);
>      count = (length - sizeof h->sbuf.sr.payload.bs_hdr) / sizeof 
> *h->bs_entries;

That static assertion also makes sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org