[Libguestfs] [PATCH v3 11/14] nbd/client: Accept 64-bit block status chunks
Vladimir Sementsov-Ogievskiy
vsementsov at yandex-team.ru
Wed May 31 17:00:43 UTC 2023
On 15.05.23 22:53, Eric Blake wrote:
> Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a
> client in narrow mode should not be able to provoke a server into
> sending a block status result larger than the client's 32-bit request.
> But in extended mode, a 64-bit status request must be able to handle a
> 64-bit status result, once a future patch enables the client
> requesting extended mode. We can also tolerate a non-compliant server
> sending the new chunk even when it should not.
>
> In normal execution, we are only requesting "base:allocation" which
> never exceeds 32 bits. But during testing with x-dirty-bitmap, we can
> force qemu to connect to some other context that might have 64-bit
> status bit; however, we ignore those upper bits (other than mapping
> qemu:allocation-depth into something that 'qemu-img map --output=json'
> can expose), and since it is only testing, we really don't bother with
> checking whether more than the two least-significant bits are set.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> block/nbd.c | 39 ++++++++++++++++++++++++++++-----------
> block/trace-events | 1 +
> 2 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index d6caea44928..150dfe7170c 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -610,13 +610,16 @@ static int nbd_parse_offset_hole_payload(BDRVNBDState *s,
> */
> static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
> NBDStructuredReplyChunk *chunk,
> - uint8_t *payload, uint64_t orig_length,
> - NBDExtent *extent, Error **errp)
> + uint8_t *payload, bool wide,
> + uint64_t orig_length,
> + NBDExtentExt *extent, Error **errp)
> {
> uint32_t context_id;
> + uint32_t count = 0;
> + size_t len = wide ? sizeof(*extent) : sizeof(NBDExtent);
>
> /* The server succeeded, so it must have sent [at least] one extent */
> - if (chunk->length < sizeof(context_id) + sizeof(*extent)) {
> + if (chunk->length < sizeof(context_id) + wide * sizeof(count) + len) {
> error_setg(errp, "Protocol error: invalid payload for "
> "NBD_REPLY_TYPE_BLOCK_STATUS");
> return -EINVAL;
> @@ -631,8 +634,14 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
> return -EINVAL;
> }
>
> - extent->length = payload_advance32(&payload);
> - extent->flags = payload_advance32(&payload);
> + if (wide) {
> + count = payload_advance32(&payload);
> + extent->length = payload_advance64(&payload);
> + extent->flags = payload_advance64(&payload);
> + } else {
> + extent->length = payload_advance32(&payload);
> + extent->flags = payload_advance32(&payload);
> + }
>
> if (extent->length == 0) {
> error_setg(errp, "Protocol error: server sent status chunk with "
> @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
> * connection; just ignore trailing extents, and clamp things to
> * the length of our request.
> */
> - if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
> + if (count != wide ||
hard to read for me. Could it be simply "count > 1 ||" ?
> + chunk->length > sizeof(context_id) + wide * sizeof(count) + len) {
> trace_nbd_parse_blockstatus_compliance("more than one extent");
> }
> if (extent->length > orig_length) {
> @@ -1117,7 +1127,7 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h
>
> static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> uint64_t handle, uint64_t length,
> - NBDExtent *extent,
> + NBDExtentExt *extent,
> int *request_ret, Error **errp)
> {
> NBDReplyChunkIter iter;
> @@ -1125,6 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> void *payload = NULL;
> Error *local_err = NULL;
> bool received = false;
> + bool wide = false;
>
> assert(!extent->length);
> NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) {
> @@ -1134,7 +1145,13 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> assert(nbd_reply_is_structured(&reply));
>
> switch (chunk->type) {
> + case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
> + wide = true;
> + /* fallthrough */
> case NBD_REPLY_TYPE_BLOCK_STATUS:
> + if (s->info.extended_headers != wide) {
> + trace_nbd_extended_headers_compliance("block_status");
You set wide to true once, on first "NBD_REPLY_TYPE_BLOCK_STATUS_EXT", and then parse following "NBD_REPLY_TYPE_BLOCK_STATUS" if the come with wide=true.
Should it be:
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1135,7 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
void *payload = NULL;
Error *local_err = NULL;
bool received = false;
- bool wide = false;
+ bool wide;
assert(!extent->length);
NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) {
@@ -1146,9 +1146,8 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
switch (chunk->type) {
case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
- wide = true;
- /* fallthrough */
case NBD_REPLY_TYPE_BLOCK_STATUS:
+ wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT;
if (s->info.extended_headers != wide) {
trace_nbd_extended_headers_compliance("block_status");
}
> + }
> if (received) {
> nbd_channel_error(s, -EINVAL);
> error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
> @@ -1142,9 +1159,9 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> }
> received = true;
>
> - ret = nbd_parse_blockstatus_payload(s, &reply.structured,
> - payload, length, extent,
> - &local_err);
> + ret = nbd_parse_blockstatus_payload(
> + s, &reply.structured, payload, wide,
> + length, extent, &local_err);
> if (ret < 0) {
> nbd_channel_error(s, ret);
> nbd_iter_channel_error(&iter, ret, &local_err);
> @@ -1374,7 +1391,7 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status(
> int64_t *pnum, int64_t *map, BlockDriverState **file)
> {
> int ret, request_ret;
> - NBDExtent extent = { 0 };
> + NBDExtentExt extent = { 0 };
> BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> Error *local_err = NULL;
>
> diff --git a/block/trace-events b/block/trace-events
> index 48dbf10c66f..afb32fcce5b 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -168,6 +168,7 @@ iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, ui
> # nbd.c
> nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from non-compliant server: %s"
> nbd_structured_read_compliance(const char *type) "server sent non-compliant unaligned read %s chunk"
> +nbd_extended_headers_compliance(const char *type) "server sent non-compliant %s chunk not matching choice of extended headers"
> nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
> nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) "Request failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"
> nbd_client_handshake(const char *export_name) "export '%s'"
--
Best regards,
Vladimir
More information about the Libguestfs
mailing list