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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
