Assigning strlen() to a uint32_t and then asserting that it isn't too large doesn't catch the case of an input string 4G in length. Thankfully, the incoming strings can never be that large: if the export name or query is reflecting a string the client got from the server, we already guarantee that we dropped the NBD connection if the server sent more than 32M in a single reply to our NBD_OPT_* request; if the export name is coming from qemu, nbd_receive_negotiate() asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and similarly, a query string via x->dirty_bitmap coming from the user was bounds-checked in either qemu-nbd or by the limitations of QMP. Still, it doesn't hurt to be more explicit in how we write our assertions to not have to analyze whether inadvertent wraparound is possible. Fixes: 93676c88 ("nbd: Don't send oversize strings", v4.2.0) Reported-by: Dr. David Alan Gilbert <dave(a)treblig.org&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- Looking through older branches, I came across this one that was never applied at the time, but which also had a useful review comment from Vladimir that invalidates the R-b it had back then. v2 was here: https://lists.gnu.org/archive/html/qemu-devel/2022-10/msg02733.html since then - update David's email, use strnlen before strlen --- nbd/client.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index 30d5383cb19..ff75722e487 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -650,19 +650,20 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt, Error **errp) { int ret; - uint32_t export_len = strlen(export); + uint32_t export_len; uint32_t queries = !!query; uint32_t query_len = 0; uint32_t data_len; char *data; char *p; + assert(strnlen(export, NBD_MAX_STRING_SIZE + 1) <= NBD_MAX_STRING_SIZE); + export_len = strlen(export); data_len = sizeof(export_len) + export_len + sizeof(queries); - assert(export_len <= NBD_MAX_STRING_SIZE); if (query) { + assert(strnlen(query, NBD_MAX_STRING_SIZE + 1) <= NBD_MAX_STRING_SIZE); query_len = strlen(query); data_len += sizeof(query_len) + query_len; - assert(query_len <= NBD_MAX_STRING_SIZE); } else { assert(opt == NBD_OPT_LIST_META_CONTEXT); } -- 2.40.1

Our existing use of structured replies either reads into a qiov capped at 32M (NBD_CMD_READ) or caps allocation to 1000 bytes (see NBD_MAX_MALLOC_PAYLOAD in block/nbd.c). But the existing length checks are rather late; if we encounter a buggy (or malicious) server that sends a super-large payload length, we should drop the connection right then rather than assuming the layer on top will be careful. This becomes more important when we permit 64-bit lengths which are even more likely to have the potential for attempted denial of service abuse. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- nbd/client.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/nbd/client.c b/nbd/client.c index ff75722e487..46f476400ab 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1413,6 +1413,18 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, chunk->handle = be64_to_cpu(chunk->handle); chunk->length = be32_to_cpu(chunk->length); + /* + * Because we use BLOCK_STATUS with REQ_ONE, and cap READ requests + * at 32M, no valid server should send us payload larger than + * this. Even if we stopped using REQ_ONE, sane servers will cap + * the number of extents they return for block status. + */ + if (chunk->length > NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)) { + error_setg(errp, "server chunk %" PRIu32 " (%s) payload is too long", + chunk->type, nbd_rep_lookup(chunk->type)); + return -EINVAL; + } + return 0; } -- 2.40.1

Upstream NBD now documents[1] an extension that supports 64-bit effect lengths in requests. As part of that extension, the size of the reply headers will change in order to permit a 64-bit length in the reply for symmetry[2]. Additionally, where the reply header is currently 16 bytes for simple reply, and 20 bytes for structured reply; with the extension enabled, there will only be one structured reply type, of 32 bytes. Since we are already wired up to use iovecs, it is easiest to allow for this change in header size by splitting each structured reply across two iovecs, one for the header (which will become variable-length in a future patch according to client negotiation), and the other for the payload, and removing the header from the payload struct definitions. Interestingly, the client side code never utilized the packed types, so only the server code needs to be updated. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/p... as of NBD commit e6f3b94a934 [2] Note that on the surface, this is because some future server might permit a 4G+ NBD_CMD_READ and need to reply with that much data in one transaction. But even though the extended reply length is widened to 64 bits, for now the NBD spec is clear that servers will not reply with more than a maximum payload bounded by the 32-bit NBD_INFO_BLOCK_SIZE field; allowing a client and server to mutually agree to transactions larger than 4G would require yet another extension. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- include/block/nbd.h | 8 +++--- nbd/server.c | 64 ++++++++++++++++++++++++++++----------------- 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index a4c98169c39..f1d838d24f5 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -96,28 +96,28 @@ typedef union NBDReply { /* Header of chunk for NBD_REPLY_TYPE_OFFSET_DATA */ typedef struct NBDStructuredReadData { - NBDStructuredReplyChunk h; /* h.length >= 9 */ + /* header's .length >= 9 */ uint64_t offset; /* At least one byte of data payload follows, calculated from h.length */ } QEMU_PACKED NBDStructuredReadData; /* Complete chunk for NBD_REPLY_TYPE_OFFSET_HOLE */ typedef struct NBDStructuredReadHole { - NBDStructuredReplyChunk h; /* h.length == 12 */ + /* header's length == 12 */ uint64_t offset; uint32_t length; } QEMU_PACKED NBDStructuredReadHole; /* Header of all NBD_REPLY_TYPE_ERROR* errors */ typedef struct NBDStructuredError { - NBDStructuredReplyChunk h; /* h.length >= 6 */ + /* header's length >= 6 */ uint32_t error; uint16_t message_length; } QEMU_PACKED NBDStructuredError; /* Header of NBD_REPLY_TYPE_BLOCK_STATUS */ typedef struct NBDStructuredMeta { - NBDStructuredReplyChunk h; /* h.length >= 12 (at least one extent) */ + /* header's length >= 12 (at least one extent) */ uint32_t context_id; /* extents follows */ } QEMU_PACKED NBDStructuredMeta; diff --git a/nbd/server.c b/nbd/server.c index e239c2890fa..eefe3401560 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1885,9 +1885,12 @@ static int coroutine_fn nbd_co_send_iov(NBDClient *client, struct iovec *iov, return ret; } -static inline void set_be_simple_reply(NBDSimpleReply *reply, uint64_t error, - uint64_t handle) +static inline void set_be_simple_reply(NBDClient *client, struct iovec *iov, + uint64_t error, uint64_t handle) { + NBDSimpleReply *reply = iov->iov_base; + + iov->iov_len = sizeof(*reply); stl_be_p(&reply->magic, NBD_SIMPLE_REPLY_MAGIC); stl_be_p(&reply->error, error); stq_be_p(&reply->handle, handle); @@ -1900,23 +1903,27 @@ static int coroutine_fn nbd_co_send_simple_reply(NBDClient *client, size_t len, Error **errp) { - NBDSimpleReply reply; + NBDReply hdr; int nbd_err = system_errno_to_nbd_errno(error); struct iovec iov[] = { - {.iov_base = &reply, .iov_len = sizeof(reply)}, + {.iov_base = &hdr}, {.iov_base = data, .iov_len = len} }; trace_nbd_co_send_simple_reply(handle, nbd_err, nbd_err_lookup(nbd_err), len); - set_be_simple_reply(&reply, nbd_err, handle); + set_be_simple_reply(client, &iov[0], nbd_err, handle); return nbd_co_send_iov(client, iov, len ? 2 : 1, errp); } -static inline void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags, - uint16_t type, uint64_t handle, uint32_t length) +static inline void set_be_chunk(NBDClient *client, struct iovec *iov, + uint16_t flags, uint16_t type, + uint64_t handle, uint32_t length) { + NBDStructuredReplyChunk *chunk = iov->iov_base; + + iov->iov_len = sizeof(*chunk); stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC); stw_be_p(&chunk->flags, flags); stw_be_p(&chunk->type, type); @@ -1928,13 +1935,14 @@ static int coroutine_fn nbd_co_send_structured_done(NBDClient *client, uint64_t handle, Error **errp) { - NBDStructuredReplyChunk chunk; + NBDReply hdr; struct iovec iov[] = { - {.iov_base = &chunk, .iov_len = sizeof(chunk)}, + {.iov_base = &hdr}, }; trace_nbd_co_send_structured_done(handle); - set_be_chunk(&chunk, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_NONE, handle, 0); + set_be_chunk(client, &iov[0], NBD_REPLY_FLAG_DONE, + NBD_REPLY_TYPE_NONE, handle, 0); return nbd_co_send_iov(client, iov, 1, errp); } @@ -1947,20 +1955,21 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client, bool final, Error **errp) { + NBDReply hdr; NBDStructuredReadData chunk; struct iovec iov[] = { + {.iov_base = &hdr}, {.iov_base = &chunk, .iov_len = sizeof(chunk)}, {.iov_base = data, .iov_len = size} }; assert(size); trace_nbd_co_send_structured_read(handle, offset, data, size); - set_be_chunk(&chunk.h, final ? NBD_REPLY_FLAG_DONE : 0, - NBD_REPLY_TYPE_OFFSET_DATA, handle, - sizeof(chunk) - sizeof(chunk.h) + size); + set_be_chunk(client, &iov[0], final ? NBD_REPLY_FLAG_DONE : 0, + NBD_REPLY_TYPE_OFFSET_DATA, handle, iov[1].iov_len + size); stq_be_p(&chunk.offset, offset); - return nbd_co_send_iov(client, iov, 2, errp); + return nbd_co_send_iov(client, iov, 3, errp); } static int coroutine_fn nbd_co_send_structured_error(NBDClient *client, @@ -1969,9 +1978,11 @@ static int coroutine_fn nbd_co_send_structured_error(NBDClient *client, const char *msg, Error **errp) { + NBDReply hdr; NBDStructuredError chunk; int nbd_err = system_errno_to_nbd_errno(error); struct iovec iov[] = { + {.iov_base = &hdr}, {.iov_base = &chunk, .iov_len = sizeof(chunk)}, {.iov_base = (char *)msg, .iov_len = msg ? strlen(msg) : 0}, }; @@ -1979,12 +1990,12 @@ static int coroutine_fn nbd_co_send_structured_error(NBDClient *client, assert(nbd_err); trace_nbd_co_send_structured_error(handle, nbd_err, nbd_err_lookup(nbd_err), msg ? msg : ""); - set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle, - sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len); + set_be_chunk(client, &iov[0], NBD_REPLY_FLAG_DONE, + NBD_REPLY_TYPE_ERROR, handle, iov[1].iov_len + iov[2].iov_len); stl_be_p(&chunk.error, nbd_err); - stw_be_p(&chunk.message_length, iov[1].iov_len); + stw_be_p(&chunk.message_length, iov[2].iov_len); - return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp); + return nbd_co_send_iov(client, iov, 2 + !!iov[2].iov_len, errp); } /* Do a sparse read and send the structured reply to the client. @@ -2022,19 +2033,22 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client, assert(pnum && pnum <= size - progress); final = progress + pnum == size; if (status & BDRV_BLOCK_ZERO) { + NBDReply hdr; NBDStructuredReadHole chunk; struct iovec iov[] = { + {.iov_base = &hdr}, {.iov_base = &chunk, .iov_len = sizeof(chunk)}, }; trace_nbd_co_send_structured_read_hole(handle, offset + progress, pnum); - set_be_chunk(&chunk.h, final ? NBD_REPLY_FLAG_DONE : 0, + set_be_chunk(client, &iov[0], + final ? NBD_REPLY_FLAG_DONE : 0, NBD_REPLY_TYPE_OFFSET_HOLE, - handle, sizeof(chunk) - sizeof(chunk.h)); + handle, iov[1].iov_len); stq_be_p(&chunk.offset, offset + progress); stl_be_p(&chunk.length, pnum); - ret = nbd_co_send_iov(client, iov, 1, errp); + ret = nbd_co_send_iov(client, iov, 2, errp); } else { ret = blk_co_pread(exp->common.blk, offset + progress, pnum, data + progress, 0); @@ -2200,8 +2214,10 @@ static int coroutine_fn nbd_co_send_extents(NBDClient *client, uint64_t handle, NBDExtentArray *ea, bool last, uint32_t context_id, Error **errp) { + NBDReply hdr; NBDStructuredMeta chunk; struct iovec iov[] = { + {.iov_base = &hdr}, {.iov_base = &chunk, .iov_len = sizeof(chunk)}, {.iov_base = ea->extents, .iov_len = ea->count * sizeof(ea->extents[0])} }; @@ -2210,12 +2226,12 @@ nbd_co_send_extents(NBDClient *client, uint64_t handle, NBDExtentArray *ea, trace_nbd_co_send_extents(handle, ea->count, context_id, ea->total_length, last); - set_be_chunk(&chunk.h, last ? NBD_REPLY_FLAG_DONE : 0, + set_be_chunk(client, &iov[0], last ? NBD_REPLY_FLAG_DONE : 0, NBD_REPLY_TYPE_BLOCK_STATUS, - handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len); + handle, iov[1].iov_len + iov[2].iov_len); stl_be_p(&chunk.context_id, context_id); - return nbd_co_send_iov(client, iov, 2, errp); + return nbd_co_send_iov(client, iov, 3, errp); } /* Get block status from the exported device and send it to the client */ -- 2.40.1

Widen the length field of NBDRequest to 64-bits, although we can assert that all current uses are still under 32 bits. Move the request magic number to nbd.h, to live alongside the reply magic number. Convert 'bool structured_reply' into a tri-state enum that will eventually track whether the client successfully negotiated extended headers with the server, allowing the nbd driver to pass larger requests along where possible; although in this patch the enum never surpasses structured replies, for no semantic change yet. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- include/block/nbd.h | 36 +++++++++++++++++++++------------ nbd/nbd-internal.h | 3 +-- block/nbd.c | 45 +++++++++++++++++++++++++++-------------- nbd/client-connection.c | 4 ++-- nbd/client.c | 18 ++++++++++------- nbd/server.c | 37 +++++++++++++++++++-------------- nbd/trace-events | 8 ++++---- 7 files changed, 93 insertions(+), 58 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index f1d838d24f5..50626ab2744 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016-2022 Red Hat, Inc. + * Copyright Red Hat * Copyright (C) 2005 Anthony Liguori <anthony(a)codemonkey.ws&gt; * * Network Block Device @@ -51,19 +51,26 @@ typedef struct NBDOptionReplyMetaContext { /* metadata context name follows */ } QEMU_PACKED NBDOptionReplyMetaContext; -/* Transmission phase structs - * - * Note: these are _NOT_ the same as the network representation of an NBD - * request and reply! +/* Transmission phase structs */ + +/* Header style in use */ +typedef enum NBDHeaderStyle { + NBD_HEADER_SIMPLE, /* default; simple replies only */ + NBD_HEADER_STRUCTURED, /* NBD_OPT_STRUCTURED_REPLY negotiated */ + NBD_HEADER_EXTENDED, /* NBD_OPT_EXTENDED_HEADERS negotiated */ +} NBDHeaderStyle; + +/* + * Note: NBDRequest is _NOT_ the same as the network representation of an NBD + * request! */ -struct NBDRequest { +typedef struct NBDRequest { uint64_t handle; - uint64_t from; - uint32_t len; + uint64_t from; /* Offset touched by the command */ + uint64_t len; /* Effect length; 32 bit limit without extended headers */ uint16_t flags; /* NBD_CMD_FLAG_* */ - uint16_t type; /* NBD_CMD_* */ -}; -typedef struct NBDRequest NBDRequest; + uint16_t type; /* NBD_CMD_* */ +} NBDRequest; typedef struct NBDSimpleReply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC */ @@ -236,6 +243,9 @@ enum { */ #define NBD_MAX_STRING_SIZE 4096 +/* Transmission request structure */ +#define NBD_REQUEST_MAGIC 0x25609513 + /* Two types of reply structures */ #define NBD_SIMPLE_REPLY_MAGIC 0x67446698 #define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef @@ -293,7 +303,7 @@ struct NBDExportInfo { /* In-out fields, set by client before nbd_receive_negotiate() and * updated by server results during nbd_receive_negotiate() */ - bool structured_reply; + NBDHeaderStyle header_style; bool base_allocation; /* base:allocation context for NBD_CMD_BLOCK_STATUS */ /* Set by server results during nbd_receive_negotiate() and @@ -323,7 +333,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, Error **errp); int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info, Error **errp); -int nbd_send_request(QIOChannel *ioc, NBDRequest *request); +int nbd_send_request(QIOChannel *ioc, NBDRequest *request, NBDHeaderStyle hdr); int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, NBDReply *reply, Error **errp); int nbd_client(int fd); diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index df42fef7066..133b1d94b50 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -1,7 +1,7 @@ /* * NBD Internal Declarations * - * Copyright (C) 2016 Red Hat, Inc. + * Copyright Red Hat * * This work is licensed under the terms of the GNU GPL, version 2 or later. * See the COPYING file in the top-level directory. @@ -44,7 +44,6 @@ #define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124) #define NBD_INIT_MAGIC 0x4e42444d41474943LL /* ASCII "NBDMAGIC" */ -#define NBD_REQUEST_MAGIC 0x25609513 #define NBD_OPTS_MAGIC 0x49484156454F5054LL /* ASCII "IHAVEOPT" */ #define NBD_CLIENT_MAGIC 0x0000420281861253LL #define NBD_REP_MAGIC 0x0003e889045565a9LL diff --git a/block/nbd.c b/block/nbd.c index a3f8f8a9d5e..6ad6a4f5ecd 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1,8 +1,8 @@ /* - * QEMU Block driver for NBD + * QEMU Block driver for NBD * * Copyright (c) 2019 Virtuozzo International GmbH. - * Copyright (C) 2016 Red Hat, Inc. + * Copyright Red Hat * Copyright (C) 2008 Bull S.A.S. * Author: Laurent Vivier <Laurent.Vivier(a)bull.net&gt; * @@ -341,7 +341,7 @@ int coroutine_fn nbd_co_do_establish_connection(BlockDriverState *bs, */ NBDRequest request = { .type = NBD_CMD_DISC }; - nbd_send_request(s->ioc, &request); + nbd_send_request(s->ioc, &request, s->info.header_style); yank_unregister_function(BLOCKDEV_YANK_INSTANCE(s->bs->node_name), nbd_yank, bs); @@ -464,7 +464,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) nbd_channel_error(s, ret); return ret; } - if (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply) { + if (nbd_reply_is_structured(&s->reply) && + s->info.header_style < NBD_HEADER_STRUCTURED) { nbd_channel_error(s, -EINVAL); return -EINVAL; } @@ -525,14 +526,14 @@ nbd_co_send_request(BlockDriverState *bs, NBDRequest *request, if (qiov) { qio_channel_set_cork(s->ioc, true); - rc = nbd_send_request(s->ioc, request); + rc = nbd_send_request(s->ioc, request, s->info.header_style); if (rc >= 0 && qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov, NULL) < 0) { rc = -EIO; } qio_channel_set_cork(s->ioc, false); } else { - rc = nbd_send_request(s->ioc, request); + rc = nbd_send_request(s->ioc, request, s->info.header_style); } qemu_co_mutex_unlock(&s->send_mutex); @@ -867,7 +868,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( } /* handle structured reply chunk */ - assert(s->info.structured_reply); + assert(s->info.header_style >= NBD_HEADER_STRUCTURED); chunk = &s->reply.structured; if (chunk->type == NBD_REPLY_TYPE_NONE) { @@ -1069,7 +1070,8 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h void *payload = NULL; Error *local_err = NULL; - NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply, + NBD_FOREACH_REPLY_CHUNK(s, iter, handle, + s->info.header_style >= NBD_HEADER_STRUCTURED, qiov, &reply, &payload) { int ret; @@ -1301,10 +1303,11 @@ nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes, NBDRequest request = { .type = NBD_CMD_WRITE_ZEROES, .from = offset, - .len = bytes, /* .len is uint32_t actually */ + .len = bytes, }; - assert(bytes <= UINT32_MAX); /* rely on max_pwrite_zeroes */ + /* rely on max_pwrite_zeroes */ + assert(bytes <= UINT32_MAX || s->info.header_style >= NBD_HEADER_EXTENDED); assert(!(s->info.flags & NBD_FLAG_READ_ONLY)); if (!(s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES)) { @@ -1351,10 +1354,11 @@ nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes) NBDRequest request = { .type = NBD_CMD_TRIM, .from = offset, - .len = bytes, /* len is uint32_t */ + .len = bytes, }; - assert(bytes <= UINT32_MAX); /* rely on max_pdiscard */ + /* rely on max_pdiscard */ + assert(bytes <= UINT32_MAX || s->info.header_style >= NBD_HEADER_EXTENDED); assert(!(s->info.flags & NBD_FLAG_READ_ONLY)); if (!(s->info.flags & NBD_FLAG_SEND_TRIM) || !bytes) { @@ -1376,8 +1380,7 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status( NBDRequest request = { .type = NBD_CMD_BLOCK_STATUS, .from = offset, - .len = MIN(QEMU_ALIGN_DOWN(INT_MAX, bs->bl.request_alignment), - MIN(bytes, s->info.size - offset)), + .len = MIN(bytes, s->info.size - offset), .flags = NBD_CMD_FLAG_REQ_ONE, }; @@ -1387,6 +1390,10 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status( *file = bs; return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; } + if (s->info.header_style < NBD_HEADER_EXTENDED) { + request.len = MIN(QEMU_ALIGN_DOWN(INT_MAX, bs->bl.request_alignment), + request.len); + } /* * Work around the fact that the block layer doesn't do @@ -1465,7 +1472,7 @@ static void nbd_client_close(BlockDriverState *bs) NBDRequest request = { .type = NBD_CMD_DISC }; if (s->ioc) { - nbd_send_request(s->ioc, &request); + nbd_send_request(s->ioc, &request, s->info.header_style); } nbd_teardown_connection(bs); @@ -1951,6 +1958,14 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) bs->bl.max_pwrite_zeroes = max; bs->bl.max_transfer = max; + /* + * Assume that if the server supports extended headers, it also + * supports unlimited size zero and trim commands. + */ + if (s->info.header_style >= NBD_HEADER_EXTENDED) { + bs->bl.max_pdiscard = bs->bl.max_pwrite_zeroes = 0; + } + if (s->info.opt_block && s->info.opt_block > bs->bl.opt_transfer) { bs->bl.opt_transfer = s->info.opt_block; diff --git a/nbd/client-connection.c b/nbd/client-connection.c index e5b1046a1c7..62d75af0bb3 100644 --- a/nbd/client-connection.c +++ b/nbd/client-connection.c @@ -1,5 +1,5 @@ /* - * QEMU Block driver for NBD + * QEMU Block driver for NBD * * Copyright (c) 2021 Virtuozzo International GmbH. * @@ -93,7 +93,7 @@ NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr, .do_negotiation = do_negotiation, .initial_info.request_sizes = true, - .initial_info.structured_reply = true, + .initial_info.header_style = NBD_HEADER_STRUCTURED, .initial_info.base_allocation = true, .initial_info.x_dirty_bitmap = g_strdup(x_dirty_bitmap), .initial_info.name = g_strdup(export_name ?: "") diff --git a/nbd/client.c b/nbd/client.c index 46f476400ab..17d1f57da60 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016-2019 Red Hat, Inc. + * Copyright Red Hat * Copyright (C) 2005 Anthony Liguori <anthony(a)codemonkey.ws&gt; * * Network Block Device Client Side @@ -1031,9 +1031,10 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, trace_nbd_receive_negotiate_name(info->name); result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc, - info->structured_reply, &zeroes, errp); + info->header_style >= NBD_HEADER_STRUCTURED, + &zeroes, errp); - info->structured_reply = false; + info->header_style = NBD_HEADER_SIMPLE; info->base_allocation = false; if (tlscreds && *outioc) { ioc = *outioc; @@ -1041,7 +1042,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, switch (result) { case 3: /* newstyle, with structured replies */ - info->structured_reply = true; + info->header_style = NBD_HEADER_STRUCTURED; if (base_allocation) { result = nbd_negotiate_simple_meta_context(ioc, info, errp); if (result < 0) { @@ -1179,7 +1180,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, memset(&array[count - 1], 0, sizeof(*array)); array[count - 1].name = name; array[count - 1].description = desc; - array[count - 1].structured_reply = result == 3; + array[count - 1].header_style = result == 3 ? + NBD_HEADER_STRUCTURED : NBD_HEADER_SIMPLE; } for (i = 0; i < count; i++) { @@ -1222,7 +1224,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, if (nbd_drop(ioc, 124, NULL) == 0) { NBDRequest request = { .type = NBD_CMD_DISC }; - nbd_send_request(ioc, &request); + nbd_send_request(ioc, &request, NBD_HEADER_SIMPLE); } break; default: @@ -1346,10 +1348,12 @@ int nbd_disconnect(int fd) #endif /* __linux__ */ -int nbd_send_request(QIOChannel *ioc, NBDRequest *request) +int nbd_send_request(QIOChannel *ioc, NBDRequest *request, NBDHeaderStyle hdr) { uint8_t buf[NBD_REQUEST_SIZE]; + assert(hdr < NBD_HEADER_EXTENDED); + assert(request->len <= UINT32_MAX); trace_nbd_send_request(request->from, request->len, request->handle, request->flags, request->type, nbd_cmd_lookup(request->type)); diff --git a/nbd/server.c b/nbd/server.c index eefe3401560..cf38a104d9a 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016-2022 Red Hat, Inc. + * Copyright Red Hat * Copyright (C) 2005 Anthony Liguori <anthony(a)codemonkey.ws&gt; * * Network Block Device Server Side @@ -143,7 +143,7 @@ struct NBDClient { uint32_t check_align; /* If non-zero, check for aligned client requests */ - bool structured_reply; + NBDHeaderStyle header_style; NBDExportMetaContexts export_meta; uint32_t opt; /* Current option being negotiated */ @@ -502,7 +502,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, } myflags = client->exp->nbdflags; - if (client->structured_reply) { + if (client->header_style >= NBD_HEADER_STRUCTURED) { myflags |= NBD_FLAG_SEND_DF; } trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags); @@ -687,7 +687,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) /* Send NBD_INFO_EXPORT always */ myflags = exp->nbdflags; - if (client->structured_reply) { + if (client->header_style >= NBD_HEADER_STRUCTURED) { myflags |= NBD_FLAG_SEND_DF; } trace_nbd_negotiate_new_style_size_flags(exp->size, myflags); @@ -985,7 +985,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client, size_t i; size_t count = 0; - if (client->opt == NBD_OPT_SET_META_CONTEXT && !client->structured_reply) { + if (client->opt == NBD_OPT_SET_META_CONTEXT && + client->header_style < NBD_HEADER_STRUCTURED) { return nbd_opt_invalid(client, errp, "request option '%s' when structured reply " "is not negotiated", @@ -1261,13 +1262,13 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp) case NBD_OPT_STRUCTURED_REPLY: if (length) { ret = nbd_reject_length(client, false, errp); - } else if (client->structured_reply) { + } else if (client->header_style >= NBD_HEADER_STRUCTURED) { ret = nbd_negotiate_send_rep_err( client, NBD_REP_ERR_INVALID, errp, "structured reply already negotiated"); } else { ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp); - client->structured_reply = true; + client->header_style = NBD_HEADER_STRUCTURED; } break; @@ -1438,7 +1439,7 @@ static int coroutine_fn nbd_receive_request(NBDClient *client, NBDRequest *reque request->type = lduw_be_p(buf + 6); request->handle = ldq_be_p(buf + 8); request->from = ldq_be_p(buf + 16); - request->len = ldl_be_p(buf + 24); + request->len = ldl_be_p(buf + 24); /* widen 32 to 64 bits */ trace_nbd_receive_request(magic, request->flags, request->type, request->from, request->len); @@ -2343,7 +2344,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * request->type == NBD_CMD_CACHE) { if (request->len > NBD_MAX_BUFFER_SIZE) { - error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)", + error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", request->len, NBD_MAX_BUFFER_SIZE); return -EINVAL; } @@ -2359,6 +2360,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * } if (request->type == NBD_CMD_WRITE) { + assert(request->len <= NBD_MAX_BUFFER_SIZE); if (nbd_read(client->ioc, req->data, request->len, "CMD_WRITE data", errp) < 0) { @@ -2380,7 +2382,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * } if (request->from > client->exp->size || request->len > client->exp->size - request->from) { - error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32 + error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu64 ", Size: %" PRIu64, request->from, request->len, client->exp->size); return (request->type == NBD_CMD_WRITE || @@ -2398,7 +2400,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * client->check_align); } valid_flags = NBD_CMD_FLAG_FUA; - if (request->type == NBD_CMD_READ && client->structured_reply) { + if (request->type == NBD_CMD_READ && + client->header_style >= NBD_HEADER_STRUCTURED) { valid_flags |= NBD_CMD_FLAG_DF; } else if (request->type == NBD_CMD_WRITE_ZEROES) { valid_flags |= NBD_CMD_FLAG_NO_HOLE | NBD_CMD_FLAG_FAST_ZERO; @@ -2424,7 +2427,7 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient *client, const char *error_msg, Error **errp) { - if (client->structured_reply && ret < 0) { + if (client->header_style >= NBD_HEADER_STRUCTURED && ret < 0) { return nbd_co_send_structured_error(client, handle, -ret, error_msg, errp); } else { @@ -2443,6 +2446,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, NBDExport *exp = client->exp; assert(request->type == NBD_CMD_READ); + assert(request->len <= NBD_MAX_BUFFER_SIZE); /* XXX: NBD Protocol only documents use of FUA with WRITE */ if (request->flags & NBD_CMD_FLAG_FUA) { @@ -2453,8 +2457,8 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, } } - if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) && - request->len) + if (client->header_style >= NBD_HEADER_STRUCTURED && + !(request->flags & NBD_CMD_FLAG_DF) && request->len) { return nbd_co_send_sparse_read(client, request->handle, request->from, data, request->len, errp); @@ -2466,7 +2470,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, "reading from file failed", errp); } - if (client->structured_reply) { + if (client->header_style >= NBD_HEADER_STRUCTURED) { if (request->len) { return nbd_co_send_structured_read(client, request->handle, request->from, data, @@ -2494,6 +2498,7 @@ static coroutine_fn int nbd_do_cmd_cache(NBDClient *client, NBDRequest *request, NBDExport *exp = client->exp; assert(request->type == NBD_CMD_CACHE); + assert(request->len <= NBD_MAX_BUFFER_SIZE); ret = blk_co_preadv(exp->common.blk, request->from, request->len, NULL, BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH); @@ -2527,6 +2532,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (request->flags & NBD_CMD_FLAG_FUA) { flags |= BDRV_REQ_FUA; } + assert(request->len <= NBD_MAX_BUFFER_SIZE); ret = blk_co_pwrite(exp->common.blk, request->from, request->len, data, flags); return nbd_send_generic_reply(client, request->handle, ret, @@ -2570,6 +2576,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, return nbd_send_generic_reply(client, request->handle, -EINVAL, "need non-zero length", errp); } + assert(request->len <= UINT32_MAX); if (client->export_meta.count) { bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE; int contexts_remaining = client->export_meta.count; diff --git a/nbd/trace-events b/nbd/trace-events index b7032ca2778..e2c1d68688d 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -31,7 +31,7 @@ nbd_client_loop(void) "Doing NBD loop" nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s" nbd_client_clear_queue(void) "Clearing NBD queue" nbd_client_clear_socket(void) "Clearing NBD socket" -nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }" +nbd_send_request(uint64_t from, uint64_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu64 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }" nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t handle) "Got simple reply: { .error = %" PRId32 " (%s), handle = %" PRIu64" }" nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char *name, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d (%s), handle = %" PRIu64 ", length = %" PRIu32 " }" @@ -60,7 +60,7 @@ nbd_negotiate_options_check_option(uint32_t option, const char *name) "Checking nbd_negotiate_begin(void) "Beginning negotiation" nbd_negotiate_new_style_size_flags(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags 0x%x" nbd_negotiate_success(void) "Negotiation succeeded" -nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16 ", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }" +nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from, uint64_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16 ", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu64 " }" nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p" nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p" nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d" @@ -70,8 +70,8 @@ nbd_co_send_structured_read_hole(uint64_t handle, uint64_t offset, size_t size) nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t length, int last) "Send block status reply: handle = %" PRIu64 ", extents = %u, context = %d (extents cover %" PRIu64 " bytes, last chunk = %d)" nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" -nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 -nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx32 ", align=0x%" PRIx32 +nbd_co_receive_request_payload_received(uint64_t handle, uint64_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu64 +nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32 nbd_trip(void) "Reading request" # client-connection.c -- 2.40.1

On 15.05.23 22:53, Eric Blake wrote: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; [..] Why? NBDExtent is one extent, not extent array. -- Best regards, Vladimir

On 30.05.23 21:22, Eric Blake wrote: Yes, sounds good -- Best regards, Vladimir

On 15.05.23 22:53, Eric Blake wrote: hmm pre-patch, here req->complete is set to true, except for WRITE request the whole big if with sub-ifs becomes really hard to read. At least, I think, no reason to keep the following two ifs inside the bigger if, as small ifs just check the same conditions. -- Best regards, Vladimir

On 15.05.23 22:53, Eric Blake wrote: Hmm. Maybe just pass unitialized iov, and set bases here? It would be more clear interface. IMHO, this union-magic significantly increases the complexity of the code.. For example, if simply swap these two lines: ea->narrow[i].flags = cpu_to_be32(ea->extents[i].flags); ea->narrow[i].length = cpu_to_be32(ea->extents[i].length); that would be a bug, which's not simple to find. I have an idea: 1. rewrite the common logic to work with new extended structures 2. add a separate function, which produces old style array, allocating it in flight. let me try. Something like this (applied on top of this patch). This way we can avoid both unions and passing half-initialized iovs: diff --git a/nbd/server.c b/nbd/server.c index 70cc1808c4..b0075dd1ee 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2073,14 +2073,7 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client, typedef struct NBDExtentArray { NBDHeaderStyle style; /* 32- or 64-bit extent descriptions */ - union { - NBDStructuredMeta id; /* style == NBD_HEADER_STRUCTURED */ - NBDStructuredMetaExt meta; /* style == NBD_HEADER_EXTENDED */ - }; - union { - NBDExtent *narrow; /* style == NBD_HEADER_STRUCTURED */ - NBDExtentExt *extents; /* style == NBD_HEADER_EXTENDED */ - }; + NBDExtentExt *extents; unsigned int nb_alloc; unsigned int count; uint64_t total_length; @@ -2110,38 +2103,35 @@ static void nbd_extent_array_free(NBDExtentArray *ea) G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free) /* Further modifications of the array after conversion are abandoned */ -static void nbd_extent_array_convert_to_be(NBDExtentArray *ea, - uint32_t context_id, - struct iovec *iov) +static void nbd_extent_array_convert_to_be(NBDExtentArray *ea) { int i; assert(!ea->converted_to_be); - assert(iov[0].iov_base == &ea->meta); - assert(iov[1].iov_base == ea->extents); ea->can_add = false; ea->converted_to_be = true; - stl_be_p(&ea->meta.context_id, context_id); - if (ea->style >= NBD_HEADER_EXTENDED) { - stl_be_p(&ea->meta.count, ea->count); - for (i = 0; i < ea->count; i++) { - ea->extents[i].length = cpu_to_be64(ea->extents[i].length); - ea->extents[i].flags = cpu_to_be64(ea->extents[i].flags); - } - iov[0].iov_len = sizeof(ea->meta); - iov[1].iov_len = ea->count * sizeof(ea->extents[0]); - } else { - /* Conversion reduces memory usage, order of iteration matters */ - for (i = 0; i < ea->count; i++) { - assert(ea->extents[i].length <= UINT32_MAX); - assert((uint32_t) ea->extents[i].flags == ea->extents[i].flags); - ea->narrow[i].length = cpu_to_be32(ea->extents[i].length); - ea->narrow[i].flags = cpu_to_be32(ea->extents[i].flags); - } - iov[0].iov_len = sizeof(ea->id); - iov[1].iov_len = ea->count * sizeof(ea->narrow[0]); + for (i = 0; i < ea->count; i++) { + ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags); + ea->extents[i].length = cpu_to_be32(ea->extents[i].length); + } +} + +static NBDExtent *nbd_extent_array_make_old_style_extents(NBDExtentArray *ea) +{ + int i; + NBDExtent *extents = g_new(NBDExtent, ea->count); + + assert(!ea->converted_to_be); + + for (i = 0; i < ea->count; i++) { + assert(ea->extents[i].length <= UINT32_MAX); + assert((uint32_t) ea->extents[i].flags == ea->extents[i].flags); + ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags); + ea->extents[i].length = cpu_to_be32(ea->extents[i].length); } + + return extents; } /* @@ -2244,7 +2234,7 @@ static int coroutine_fn blockalloc_to_extents(BlockBackend *blk, /* * nbd_co_send_extents * - * @ea is converted to BE by the function + * @ea may be converted to BE by the function * @last controls whether NBD_REPLY_FLAG_DONE is sent. */ static int coroutine_fn @@ -2252,15 +2242,35 @@ nbd_co_send_extents(NBDClient *client, NBDRequest *request, NBDExtentArray *ea, bool last, uint32_t context_id, Error **errp) { NBDReply hdr; - struct iovec iov[] = { - {.iov_base = &hdr}, - {.iov_base = &ea->meta}, - {.iov_base = ea->extents} - }; - uint16_t type = client->header_style == NBD_HEADER_EXTENDED ? - NBD_REPLY_TYPE_BLOCK_STATUS_EXT : NBD_REPLY_TYPE_BLOCK_STATUS; + NBDStructuredMeta meta; + NBDStructuredMetaExt meta_ext; + g_autofree NBDExtent *extents = NULL; + uint16_t type; + struct iovec iov[] = { {.iov_base = &hdr}, {0}, {0} }; + + if (client->header_style == NBD_HEADER_EXTENDED) { + type = NBD_REPLY_TYPE_BLOCK_STATUS_EXT; + + iov[1].iov_base = &meta_ext; + iov[1].iov_len = sizeof(meta_ext); + stl_be_p(&meta_ext.context_id, context_id); + stl_be_p(&meta_ext.count, ea->count); + + nbd_extent_array_convert_to_be(ea); + iov[2].iov_base = ea->extents; + iov[2].iov_len = ea->count * sizeof(ea->extents[0]); + } else { + type = NBD_REPLY_TYPE_BLOCK_STATUS; + + iov[1].iov_base = &meta; + iov[1].iov_len = sizeof(meta); + stl_be_p(&meta.context_id, context_id); + + extents = nbd_extent_array_make_old_style_extents(ea); + iov[2].iov_base = extents; + iov[2].iov_len = ea->count * sizeof(extents[0]); + } - nbd_extent_array_convert_to_be(ea, context_id, &iov[1]); trace_nbd_co_send_extents(request->handle, ea->count, context_id, ea->total_length, last); -- Best regards, Vladimir

On 15.05.23 22:53, Eric Blake wrote: [..] So, that's not _simple_ now.. The function should be renamed. As well as set_be_simple_reply(). _simple_or_extended_ ? a bit too long. But continuing to use "simple" is in bad relation with use of "simple" word in specification. Probably better to update callers? The only caller isi nbd_send_generic_reply(). So, could we just add nbd_co_send_single_extended_reply() to call from nbd_send_generic_reply() in case of EXTENDED? Also, transformation of set_be_simple_reply() do look like it should be two separate functions. -- Best regards, Vladimir

Update the client code to be able to send an extended request, and parse an extended header from the server. Note that since we reject any structured reply with a too-large payload, we can always normalize a valid header back into the compact form, so that the caller need not deal with two branches of a union. Still, until a later patch lets the client negotiate extended headers, the code added here should not be reached. Note that because of the different magic numbers, it is just as easy to trace and then tolerate a non-compliant server sending the wrong header reply as it would be to insist that the server is compliant. The only caller to nbd_receive_reply() always passed NULL for errp; since we are changing the signature anyways, I decided to sink the decision to ignore errors one layer lower. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- include/block/nbd.h | 2 +- block/nbd.c | 3 +- nbd/client.c | 86 +++++++++++++++++++++++++++++++-------------- nbd/trace-events | 1 + 4 files changed, 63 insertions(+), 29 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index d753fb8006f..865bb4ee2e1 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -371,7 +371,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info, Error **errp); int nbd_send_request(QIOChannel *ioc, NBDRequest *request, NBDHeaderStyle hdr); int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, - NBDReply *reply, Error **errp); + NBDReply *reply, NBDHeaderStyle hdr); int nbd_client(int fd); int nbd_disconnect(int fd); int nbd_errno_to_system_errno(int err); diff --git a/block/nbd.c b/block/nbd.c index 6ad6a4f5ecd..d6caea44928 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -458,7 +458,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) /* We are under mutex and handle is 0. We have to do the dirty work. */ assert(s->reply.handle == 0); - ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, NULL); + ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, + s->info.header_style); if (ret <= 0) { ret = ret ? ret : -EIO; nbd_channel_error(s, ret); diff --git a/nbd/client.c b/nbd/client.c index 17d1f57da60..e5db3c8b79d 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1350,22 +1350,29 @@ int nbd_disconnect(int fd) int nbd_send_request(QIOChannel *ioc, NBDRequest *request, NBDHeaderStyle hdr) { - uint8_t buf[NBD_REQUEST_SIZE]; + uint8_t buf[NBD_EXTENDED_REQUEST_SIZE]; + size_t len; - assert(hdr < NBD_HEADER_EXTENDED); - assert(request->len <= UINT32_MAX); trace_nbd_send_request(request->from, request->len, request->handle, request->flags, request->type, nbd_cmd_lookup(request->type)); - stl_be_p(buf, NBD_REQUEST_MAGIC); stw_be_p(buf + 4, request->flags); stw_be_p(buf + 6, request->type); stq_be_p(buf + 8, request->handle); stq_be_p(buf + 16, request->from); - stl_be_p(buf + 24, request->len); + if (hdr >= NBD_HEADER_EXTENDED) { + stl_be_p(buf, NBD_EXTENDED_REQUEST_MAGIC); + stq_be_p(buf + 24, request->len); + len = NBD_EXTENDED_REQUEST_SIZE; + } else { + assert(request->len <= UINT32_MAX); + stl_be_p(buf, NBD_REQUEST_MAGIC); + stl_be_p(buf + 24, request->len); + len = NBD_REQUEST_SIZE; + } - return nbd_write(ioc, buf, sizeof(buf), NULL); + return nbd_write(ioc, buf, len, NULL); } /* nbd_receive_simple_reply @@ -1394,28 +1401,34 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply, /* nbd_receive_structured_reply_chunk * Read structured reply chunk except magic field (which should be already - * read). + * read). Normalize into the compact form. * Payload is not read. */ -static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, - NBDStructuredReplyChunk *chunk, +static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, NBDReply *chunk, Error **errp) { int ret; + size_t len; + uint64_t payload_len; - assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC); + if (chunk->magic == NBD_STRUCTURED_REPLY_MAGIC) { + len = sizeof(chunk->structured); + } else { + assert(chunk->magic == NBD_EXTENDED_REPLY_MAGIC); + len = sizeof(chunk->extended); + } ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic), - sizeof(*chunk) - sizeof(chunk->magic), "structured chunk", + len - sizeof(chunk->magic), "structured chunk", errp); if (ret < 0) { return ret; } - chunk->flags = be16_to_cpu(chunk->flags); - chunk->type = be16_to_cpu(chunk->type); - chunk->handle = be64_to_cpu(chunk->handle); - chunk->length = be32_to_cpu(chunk->length); + /* flags, type, and handle occupy same space between forms */ + chunk->structured.flags = be16_to_cpu(chunk->structured.flags); + chunk->structured.type = be16_to_cpu(chunk->structured.type); + chunk->structured.handle = be64_to_cpu(chunk->structured.handle); /* * Because we use BLOCK_STATUS with REQ_ONE, and cap READ requests @@ -1423,11 +1436,20 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, * this. Even if we stopped using REQ_ONE, sane servers will cap * the number of extents they return for block status. */ - if (chunk->length > NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)) { + if (chunk->magic == NBD_STRUCTURED_REPLY_MAGIC) { + payload_len = be32_to_cpu(chunk->structured.length); + } else { + /* For now, we are ignoring the extended header offset. */ + payload_len = be64_to_cpu(chunk->extended.length); + chunk->magic = NBD_STRUCTURED_REPLY_MAGIC; + } + if (payload_len > NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)) { error_setg(errp, "server chunk %" PRIu32 " (%s) payload is too long", - chunk->type, nbd_rep_lookup(chunk->type)); + chunk->structured.type, + nbd_rep_lookup(chunk->structured.type)); return -EINVAL; } + chunk->structured.length = payload_len; return 0; } @@ -1474,30 +1496,35 @@ nbd_read_eof(BlockDriverState *bs, QIOChannel *ioc, void *buffer, size_t size, /* nbd_receive_reply * - * Decreases bs->in_flight while waiting for a new reply. This yield is where - * we wait indefinitely and the coroutine must be able to be safely reentered - * for nbd_client_attach_aio_context(). + * Wait for a new reply. If this yields, the coroutine must be able to be + * safely reentered for nbd_client_attach_aio_context(). @hdr determines + * which reply magic we are expecting, although this normalizes the result + * so that the caller only has to work with compact headers. * * Returns 1 on success - * 0 on eof, when no data was read (errp is not set) - * negative errno on failure (errp is set) + * 0 on eof, when no data was read + * negative errno on failure */ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, - NBDReply *reply, Error **errp) + NBDReply *reply, NBDHeaderStyle hdr) { int ret; const char *type; - ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), errp); + ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), NULL); if (ret <= 0) { return ret; } reply->magic = be32_to_cpu(reply->magic); + /* Diagnose but accept wrong-width header */ switch (reply->magic) { case NBD_SIMPLE_REPLY_MAGIC: - ret = nbd_receive_simple_reply(ioc, &reply->simple, errp); + if (hdr >= NBD_HEADER_EXTENDED) { + trace_nbd_receive_wrong_header(reply->magic); + } + ret = nbd_receive_simple_reply(ioc, &reply->simple, NULL); if (ret < 0) { break; } @@ -1506,7 +1533,12 @@ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, reply->handle); break; case NBD_STRUCTURED_REPLY_MAGIC: - ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured, errp); + case NBD_EXTENDED_REPLY_MAGIC: + if ((hdr >= NBD_HEADER_EXTENDED) != + (reply->magic == NBD_EXTENDED_REPLY_MAGIC)) { + trace_nbd_receive_wrong_header(reply->magic); + } + ret = nbd_receive_structured_reply_chunk(ioc, reply, NULL); if (ret < 0) { break; } @@ -1517,7 +1549,7 @@ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, reply->structured.length); break; default: - error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", reply->magic); + trace_nbd_receive_wrong_header(reply->magic); return -EINVAL; } if (ret < 0) { diff --git a/nbd/trace-events b/nbd/trace-events index adf5666e207..c20df33a431 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -34,6 +34,7 @@ nbd_client_clear_socket(void) "Clearing NBD socket" nbd_send_request(uint64_t from, uint64_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu64 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }" nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t handle) "Got simple reply: { .error = %" PRId32 " (%s), handle = %" PRIu64" }" nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char *name, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d (%s), handle = %" PRIu64 ", length = %" PRIu32 " }" +nbd_receive_wrong_header(uint32_t magic) "Server sent unexpected magic 0x%" PRIx32 # common.c nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL" -- 2.40.1

The subject lines are confusing: 9/14 enables extended headers in the server, while this one does not yet enable the headers but is merely a preliminary step. I'll probably retitle one or the other in v4. On Wed, May 31, 2023 at 06:26:17PM +0300, Vladimir Sementsov-Ogievskiy wrote: I can explore plumbing errp back through for v4. I'm going with 'nbd_receive_reply_chunk', since both structured and extended modes receive chunks. Or even just "chunk header", which covers both modes. Sure, I can give that a shot. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 15.05.23 22:53, Eric Blake wrote: hard to read for me. Could it be simply "count > 1 ||" ? 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"); } -- Best regards, Vladimir

On 31.05.23 20:40, Eric Blake wrote: The only case you add to the check is when count = 0 for extended. But in this case "more than one extent" message is counterintuitive. -- Best regards, Vladimir

On 15.05.23 22:53, Eric Blake wrote: why must not? It should gracefully report ENOTSUP? Or not? O, that's a part of previous commit. Also, initialization of wide to false becomes unneeded. I'd prefer explicit info->header_style = (result == 4 ? NBD_HEADER_EXTENDED : NBD_HEADER_STRUCTURED); with no comment Personally, I don't like enum-based arithmetics.. It's something to be very careful with and check enum definition every time. Maybe, add enum NBDConnectionStyle : NBD_STYLE_OLD, NBD_STYLE_EXPORT_NAME, NBD_STYLE_SIMPLE, NBD_STYLE_STRUCTURED, NBD_STYLE_EXTENDED, and a simple function nbd_con_style_to_hdr_style: NBDConnectionStyle -> NBDHeaderStyle Or, better, nbd_start_negotiate() may return only 0/1/2 as success values, and additionally set OUT argument *header_style? And anyway, 0/1/2/3/4 - sounds like too much magic numeric logic (I feel, that's all a kind of taste and don't want to insist anyway) -- Best regards, Vladimir

On 15.05.23 22:53, Eric Blake wrote: sometimes you change s/meta->exp->nr_export_bitmaps/meta->nr_bitmaps/ and sometimes s/meta->exp->nr_export_bitmaps/exp->nr_export_bitmaps/ I don't like the change: we iterate through exp->export_bitmaps. So seems more correct to use exp->nr_export_bitmaps as a limit. I understand why you want to duplicated nr_export_bitmaps in NBDMetaContexts, but the result doesn't seem better than just use exp->nr_export_bitmaps. Probably, if we want to duplicate nr_export_bitmaps, we should duplicate export_bitmaps as well. But this leads actually to keeping meta->exp as is. Hm, may be just do size_t nr_bitmaps = meta->nr_bitmaps; assert(nr_bitmaps == exp->nr_export_bitmaps) at top of the function? and use nr_bitmaps everywhere. But this again make me doubt that meta.nr_bitmaps is good idea: we have to pass exp together and check meta.nr_bitmaps == exp->nr_export_bitmaps every time. So, I understand, that bool *bitmaps; /* * export qemu:dirty-bitmap:<export bitmap name>, * sized by exp->nr_export_bitmaps */ sounds like not good design - we have an array, sized by field of another object. But, we have a link that that object (meta->exp). But with your way we fall into bad design too: we have a duplicated field, and have to control that they not diverse. And we iterate things in exp, using field of meta. I looked through, seems in all places where we use nr_bitmaps, we also have access to nr_export_bitmaps. So, maybe, drop this nr_bitmaps? And why not to keep meta.exp? With it, meta is self-sufficient object, which fully describe some selected contexts. For per-request filtering, meta.exp just must always point to current export. And anyway would be good to split refactoring of meta structure (if we need it) and new logic of per-command filtering. -- Best regards, Vladimir

On 15.05.23 22:53, Eric Blake wrote: Hmm. So, it leads to case NBD_CMD_BLOCK_STATUS: if (!request->len) { return nbd_send_generic_reply(client, request, -EINVAL, "need non-zero length", errp); } EINVAL is OK, but "need non-zero length" message is not appropriate.. I think we need separate reply for the case of invalid payload. In doc we have MUST: "The payload form MUST occupy 8 + n*4 bytes", do we really want to forgive and ignore unaligned tail? May be better to "goto skip" in this case, to avoid ambiguity. better I think: request->context.bitmaps = g_steal_pointer(bitmaps); - as a pair to g_autofree. Seems we can handle all payload in one "switch" block, instead of handling BLOCK_STATUS here and postponing WRITE payload (and dropping) up to the end of the block with help of payload_len variable. -- Best regards, Vladimir