Libguestfs - Oct 2023 - [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

On 25.09.23 22:22, Eric Blake wrote:
> Allow a client to request a subset of negotiated meta contexts.  For
> example, a client may ask to use a single connection to learn about
> both block status and dirty bitmaps, but where the dirty bitmap
> queries only need to be performed on a subset of the disk; forcing the
> server to compute that information on block status queries in the rest
> of the disk is wasted effort (both at the server, and on the amount of
> traffic sent over the wire to be parsed and ignored by the client).
> 
> Qemu as an NBD client never requests to use more than one meta
> context, so it has no need to use block status payloads.  Testing this
> instead requires support from libnbd, which CAN access multiple meta
> contexts in parallel from a single NBD connection; an interop test
> submitted to the libnbd project at the same time as this patch
> demonstrates the feature working, as well as testing some corner cases
> (for example, when the payload length is longer than the export
> length), although other corner cases (like passing the same id
> duplicated) requires a protocol fuzzer because libnbd is not wired up
> to break the protocol that badly.
> 
> This also includes tweaks to 'qemu-nbd --list' to show when a
server
> is advertising the capability, and to the testsuite to reflect the
> addition to that output.
> 
> Of note: qemu will always advertise the new feature bit during
> NBD_OPT_INFO if extended headers have alreay been negotiated
> (regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
> occurred); but for NBD_OPT_GO, qemu only advertises the feature if
> block status is also enabled (that is, if the client does not
> negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
> the feature is not advertised).
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
[..]
> 
> +/*
> + * nbd_co_block_status_payload_read
> + * Called when a client wants a subset of negotiated contexts via a
> + * BLOCK_STATUS payload.  Check the payload for valid length and
> + * contents.  On success, return 0 with request updated to effective
> + * length.  If request was invalid but all payload consumed, return 0
> + * with request->len and request->contexts->count set to 0 (which
will
> + * trigger an appropriate NBD_EINVAL response later on).  Return
> + * negative errno if the payload was not fully consumed.
> + */
> +static int
> +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> +                                 Error **errp)
[..]
> +        payload_len > (sizeof(NBDBlockStatusPayload) +
> +                       sizeof(id) * client->contexts.count)) {
> +        goto skip;
> +    }
> +
> +    buf = g_malloc(payload_len);
> +    if (nbd_read(client->ioc, buf, payload_len,
> +                 "CMD_BLOCK_STATUS data", errp) < 0) {
> +        return -EIO;
> +    }
> +    trace_nbd_co_receive_request_payload_received(request->cookie,
> +                                                  payload_len);
> +    request->contexts->bitmaps = g_new0(bool, nr_bitmaps);
> +    count = (payload_len - sizeof(NBDBlockStatusPayload)) / sizeof(id);
> +    payload_len = 0;
> +
> +    for (i = 0; i < count; i++) {
> +        id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) *
i);
> +        if (id == NBD_META_ID_BASE_ALLOCATION) {
> +            if (request->contexts->base_allocation) {
> +                goto skip;
> +            }
should we also check that base_allocation is negotiated?
> +            request->contexts->base_allocation = true;
> +        } else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
> +            if (request->contexts->allocation_depth) {
> +                goto skip;
> +            }
same here
> +            request->contexts->allocation_depth = true;
> +        } else {
> +            int idx = id - NBD_META_ID_DIRTY_BITMAP;
> +
I think, we also should check that idx >=0 after this operation.
> +            if (idx > nr_bitmaps ||
request->contexts->bitmaps[idx]) {
> +                goto skip;
> +            }
> +            request->contexts->bitmaps[idx] = true;
> +        }
> +    }
> +
> +    request->len = ldq_be_p(buf);
> +    request->contexts->count = count;
> +    return 0;
> +
> + skip:
> +    trace_nbd_co_receive_block_status_payload_compliance(request->from,
> +                                                         request->len);
> +    request->len = request->contexts->count = 0;
> +    return nbd_drop(client->ioc, payload_len, errp);
> +}
> +
[..]
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 8f4e20ee9f2..ac186c19ec0 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -70,6 +70,7 @@ nbd_co_send_chunk_read(uint64_t cookie, uint64_t offset,
void *data, uint64_t si
>   nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, uint64_t
size) "Send structured read hole reply: cookie = %" PRIu64 ",
offset = %" PRIu64 ", len = %" PRIu64
>   nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id,
uint64_t length, int last) "Send block status reply: cookie = %"
PRIu64 ", extents = %u, context = %d (extents cover %" PRIu64 "
bytes, last chunk = %d)"
>   nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname,
const char *msg) "Send structured error reply: cookie = %" PRIu64
", error = %d (%s), msg = '%s'"
> +nbd_co_receive_block_status_payload_compliance(uint64_t from, int len)
"client sent unusable block status payload: from=0x%" PRIx64 ",
len=0x%x"
both passed parameters request->from and request->len are uint64_t
actually
>   nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const
char *name) "Decoding type: cookie = %" PRIu64 ", type = %"
PRIu16 " (%s)"
>   nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len)
"Payload received: cookie = %" PRIu64 ", len = %" PRIu64
>   nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len)
"client sent non-compliant write without payload flag: from=0x%"
PRIx64 ", len=0x%" PRIx64
[..]

-- 
Best regards,
Vladimir

On Sat, Sep 30, 2023 at 04:24:02PM +0300, Vladimir Sementsov-Ogievskiy
wrote:
> On 25.09.23 22:22, Eric Blake wrote:
> > Allow a client to request a subset of negotiated meta contexts.  For
> > example, a client may ask to use a single connection to learn about
> > both block status and dirty bitmaps, but where the dirty bitmap
> > queries only need to be performed on a subset of the disk; forcing the
> > server to compute that information on block status queries in the rest
> > of the disk is wasted effort (both at the server, and on the amount of
> > traffic sent over the wire to be parsed and ignored by the client).
> > 
> > Qemu as an NBD client never requests to use more than one meta
> > context, so it has no need to use block status payloads.  Testing this
> > instead requires support from libnbd, which CAN access multiple meta
> > contexts in parallel from a single NBD connection; an interop test
> > submitted to the libnbd project at the same time as this patch
> > demonstrates the feature working, as well as testing some corner cases
> > (for example, when the payload length is longer than the export
> > length), although other corner cases (like passing the same id
> > duplicated) requires a protocol fuzzer because libnbd is not wired up
> > to break the protocol that badly.
> > 
> > This also includes tweaks to 'qemu-nbd --list' to show when a
server
> > is advertising the capability, and to the testsuite to reflect the
> > addition to that output.
> > 
> > Of note: qemu will always advertise the new feature bit during
> > NBD_OPT_INFO if extended headers have alreay been negotiated
> > (regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
> > occurred); but for NBD_OPT_GO, qemu only advertises the feature if
> > block status is also enabled (that is, if the client does not
> > negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
> > the feature is not advertised).
> > 
> > Signed-off-by: Eric Blake <eblake at redhat.com>
> > ---
> > 
> 
> [..]
> 
> > 
> > +/*
> > + * nbd_co_block_status_payload_read
> > + * Called when a client wants a subset of negotiated contexts via a
> > + * BLOCK_STATUS payload.  Check the payload for valid length and
> > + * contents.  On success, return 0 with request updated to effective
> > + * length.  If request was invalid but all payload consumed, return 0
> > + * with request->len and request->contexts->count set to 0
(which will
> > + * trigger an appropriate NBD_EINVAL response later on).  Return
> > + * negative errno if the payload was not fully consumed.
> > + */
> > +static int
> > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest
*request,
> > +                                 Error **errp)
> 
> [..]
> 
> > +        payload_len > (sizeof(NBDBlockStatusPayload) +
> > +                       sizeof(id) * client->contexts.count)) {
> > +        goto skip;
> > +    }
> > +
> > +    buf = g_malloc(payload_len);
> > +    if (nbd_read(client->ioc, buf, payload_len,
> > +                 "CMD_BLOCK_STATUS data", errp) < 0) {
> > +        return -EIO;
> > +    }
> > +    trace_nbd_co_receive_request_payload_received(request->cookie,
> > +                                                  payload_len);
> > +    request->contexts->bitmaps = g_new0(bool, nr_bitmaps);
> > +    count = (payload_len - sizeof(NBDBlockStatusPayload)) /
sizeof(id);
> > +    payload_len = 0;
> > +
> > +    for (i = 0; i < count; i++) {
> > +        id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) +
sizeof(id) * i);
> > +        if (id == NBD_META_ID_BASE_ALLOCATION) {
> > +            if (request->contexts->base_allocation) {
> > +                goto skip;
> > +            }
> 
> should we also check that base_allocation is negotiated?
Oh, good point.  Without that check, the client can pass in random id
numbers that it never negotiated.  I've queued 1-11 and will probably
send a pull request for those this week, while respinning this patch
to fix the remaining issues you pointed out.
> 
> > +            request->contexts->base_allocation = true;
> > +        } else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
> > +            if (request->contexts->allocation_depth) {
> > +                goto skip;
> > +            }
> 
> same here
> 
> > +            request->contexts->allocation_depth = true;
> > +        } else {
> > +            int idx = id - NBD_META_ID_DIRTY_BITMAP;
> > +
> 
> I think, we also should check that idx >=0 after this operation.
> 
> > +            if (idx > nr_bitmaps ||
request->contexts->bitmaps[idx]) {
Or else make idx an unsigned value, instead of signed.  Also a good catch.
> > +                goto skip;
> > +            }
> > +            request->contexts->bitmaps[idx] = true;
> > +        }
> > +    }
> > +
> > +    request->len = ldq_be_p(buf);
> > +    request->contexts->count = count;
> > +    return 0;
> > +
> > + skip:
> > +   
trace_nbd_co_receive_block_status_payload_compliance(request->from,
> > +                                                        
request->len);
> > +    request->len = request->contexts->count = 0;
> > +    return nbd_drop(client->ioc, payload_len, errp);
> > +}
> > +
> 
> [..]
> 
> > diff --git a/nbd/trace-events b/nbd/trace-events
> > index 8f4e20ee9f2..ac186c19ec0 100644
> > --- a/nbd/trace-events
> > +++ b/nbd/trace-events
> > @@ -70,6 +70,7 @@ nbd_co_send_chunk_read(uint64_t cookie, uint64_t
offset, void *data, uint64_t si
> >   nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset,
uint64_t size) "Send structured read hole reply: cookie = %" PRIu64
", offset = %" PRIu64 ", len = %" PRIu64
> >   nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t
id, uint64_t length, int last) "Send block status reply: cookie = %"
PRIu64 ", extents = %u, context = %d (extents cover %" PRIu64 "
bytes, last chunk = %d)"
> >   nbd_co_send_chunk_error(uint64_t cookie, int err, const char
*errname, const char *msg) "Send structured error reply: cookie = %"
PRIu64 ", error = %d (%s), msg = '%s'"
> > +nbd_co_receive_block_status_payload_compliance(uint64_t from, int
len) "client sent unusable block status payload: from=0x%" PRIx64
", len=0x%x"
> 
> both passed parameters request->from and request->len are uint64_t
actually
Also a good catch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org