Upcoming additions to support NBD 64-bit effect lengths allow for the possibility to distinguish between payload length (capped at 32M) and effect length (64 bits, although we generally assume 63 bits because of off_t limitations). Without that extension, only the NBD_CMD_WRITE request has a payload; but with the extension, it makes sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length in a future patch (where the payload is a limited-size struct that in turn gives the real effect length as well as a subset of known ids for which status is requested). Other future NBD commands may also have a request payload, so the 64-bit extension introduces a new NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header length is a payload length or an effect length, rather than hard-coding the decision based on the command. According to the spec, a client should never send a command with a payload without the negotiation phase proving such extension is available. So in the unlikely event the bit is set or cleared incorrectly, the client is already at fault; if the client then provides the payload, we can gracefully consume it off the wire and fail the command with NBD_EINVAL (subsequent checks for magic numbers ensure we are still in sync), while if the client fails to send payload we block waiting for it (basically deadlocking our connection to the bad client, but not negatively impacting our ability to service other clients, so not a security risk). Note that we do not support the payload version of BLOCK_STATUS yet. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v7: another try at better logic [Vladimir] v5: retitled from v4 13/24, rewrite on top of previous patch's switch statement [Vladimir] v4: less indentation on several 'if's [Vladimir] --- nbd/server.c | 37 +++++++++++++++++++++++++++++++++---- nbd/trace-events | 1 + 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 7a6f95071f8..1eabcfc908d 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, Error **errp) { NBDClient *client = req->client; + bool extended_with_payload; bool check_length = false; bool check_rofs = false; bool allocate_buffer = false; + bool payload_okay = false; unsigned payload_len = 0; int valid_flags = NBD_CMD_FLAG_FUA; int ret; @@ -2338,6 +2340,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, trace_nbd_co_receive_request_decode_type(request->cookie, request->type, nbd_cmd_lookup(request->type)); + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; + if (extended_with_payload) { + payload_len = request->len; + check_length = true; + } + switch (request->type) { case NBD_CMD_DISC: /* Special case: we're going to disconnect without a reply, @@ -2354,6 +2363,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, break; case NBD_CMD_WRITE: + if (client->mode >= NBD_MODE_EXTENDED) { + if (!extended_with_payload) { + /* The client is noncompliant. Trace it, but proceed. */ + trace_nbd_co_receive_ext_payload_compliance(request->from, + request->len); + } + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } + payload_okay = true; payload_len = request->len; check_length = true; allocate_buffer = true; @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, request->len, NBD_MAX_BUFFER_SIZE); return -EINVAL; } + if (payload_len && !payload_okay) { + /* + * For now, we don't support payloads on other commands; but + * we can keep the connection alive by ignoring the payload. + */ + assert(request->type != NBD_CMD_WRITE); + request->len = 0; + } if (allocate_buffer) { /* READ, WRITE */ req->data = blk_try_blockalign(client->exp->common.blk, @@ -2405,10 +2431,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, } } if (payload_len) { - /* WRITE */ - assert(req->data); - ret = nbd_read(client->ioc, req->data, payload_len, - "CMD_WRITE data", errp); + if (payload_okay) { + assert(req->data); + ret = nbd_read(client->ioc, req->data, payload_len, + "CMD_WRITE data", errp); + } else { + ret = nbd_drop(client->ioc, payload_len, errp); + } if (ret < 0) { return -EIO; } diff --git a/nbd/trace-events b/nbd/trace-events index f9dccfcfb44..c1a3227613f 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t 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_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 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" -- 2.41.0

On Wed, Sep 27, 2023 at 11:55:41AM +0300, Vladimir Sementsov-Ogievskiy wrote: If request->len == 0, then payload_len is also 0 and we do not enter this block, even when !payload_okay. A 0-byte NBD_CMD_READ is already unspecified by the NBD spec (although I think we do permit it as a no-op where we return success). But that is not the whole picture - even if the client passes a payload length 0f 0, it is important to remember that NBD_CMD_READ did not add NBD_CMD_FLAG_PAYLOAD_LEN to valid_flags, so later in this function, whether or not we have payload bytes to slurp off, we will still fail the command with EINVAL for passing an unexpected flag when doing the comparison against valid_flags. But I agree that adding a comment to that effect would help. In that particular case, you argue the client did NOT pass in payload bytes; so our attempt to read payload bytes will block until the client issues enough subsequent commands to actually satisfy the payload_len (or we see early EOF when the client hangs up after noticing we have not responded to commands in a while). We are effectively ignoring all such subsequent commands (having treated them as NBD_CMD_READ payload instead of independent commands), and if we finally do get enough bytes to satisfy the full payload, it is highly likely that we will not land on a clean command boundary and disconnect when we see a magic number mismatch. Less likely is the payload_len ending exactly on a command boundary; but hopefully our return of NBD_EINVAL will explain to the client why we have ignored its subsequent commands. At any rate, the bug is in the client for sending an unsupported flag; anything we do beyond that point is best effort, and the most important part is that if we attempted to keep the connection alive, we are at least not letting the client cause us to do unintended actions. Again, once we consume payload_len bytes (if we did not outright close the connection because the count was larger than 32M and we were unwilling to wade through that many payload bytes, thanks to check_length), we are most likely to hit a magic number mismatch after having ignored all intermediate commands that came in; but the NBD_COMMAND_WRITE_ZEROES will receive an NBD_EINVAL failure, not success, for not satisfying valid_flags. Which do we think is more likely: a client accidentally setting the flag but not passing payload, or a client purposefully setting the flag and including payload? If the former, then it may be nicer to just disconnect any time we see the flag set unexpectedly; trying to keep the connection alive will ignore subsequent client commands, and likely (but not always) eventually result in a magic number mismatch once payload_len bytes are ignored. If the latter, then skipping payload bytes keeps us in sync, so that we can read the next command. We could also try to be a bit more complicated by peeking at the next few bytes: if they look like a magic number of the next request, assume the client set the bit accidentally but didn't send a payload after all; for anything else, assume the client did pass a payload. But adding in machinery to peek at a prefix is more complex than either assuming a payload is always present (as done in this patch) or assuming the bit was in error (and dropping the connection unconditionally). Preferences? At any rate, deciding what to do when dealing with a non-compliant client should NOT hold up reviews of the rest of the series, which only worry about compliant clients that never trigger this issue. Worded differently, the important aspect of the review of this patch is whether we see any scenario where a non-compliant client could cause the server to misbehave, at which point declaring that we would prefer to disconnect immediately rather than attempt to resync may be safer, even though it is more abrupt. But if we are satisfied that the ill-behaved client cannot crash the server, trying to keep the connection alive (at least, until we later hit a magic number mismatch) is probably nicer. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

On 25.09.23 22:22, Eric Blake wrote: So, for sure, after this we go to if (requests->flags & ~valid_flags)... and return -EINVAL. Why we need to set request->len to 0? Just to not return "operation past EOF" instead of "unsupported flags", if len is too big? Maybe, that worth a comment. Anyway, I now see nothing wrong in it, so: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; -- Best regards, Vladimir

On 05.10.23 18:38, Eric Blake wrote: OK, agree -- Best regards, Vladimir

Although extended mode is not yet enabled, once we do turn it on, we need to reply with extended headers to all messages. Update the low level entry points necessary so that all other callers automatically get the right header based on the current mode. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; --- v5: s/iov->iov_len/iov[0].iov_len/ [Vladimir], add R-b v4: new patch, split out from v3 9/14 --- nbd/server.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index e227e470d41..4cd061f9da4 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1938,8 +1938,6 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, size_t niov, uint16_t flags, uint16_t type, NBDRequest *request) { - /* TODO - handle structured vs. extended replies */ - NBDStructuredReplyChunk *chunk = iov->iov_base; size_t i, length = 0; for (i = 1; i < niov; i++) { @@ -1947,12 +1945,26 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, } assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)); - iov[0].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); - stq_be_p(&chunk->cookie, request->cookie); - stl_be_p(&chunk->length, length); + if (client->mode >= NBD_MODE_EXTENDED) { + NBDExtendedReplyChunk *chunk = iov->iov_base; + + iov[0].iov_len = sizeof(*chunk); + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC); + stw_be_p(&chunk->flags, flags); + stw_be_p(&chunk->type, type); + stq_be_p(&chunk->cookie, request->cookie); + stq_be_p(&chunk->offset, request->from); + stq_be_p(&chunk->length, length); + } else { + NBDStructuredReplyChunk *chunk = iov->iov_base; + + iov[0].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); + stq_be_p(&chunk->cookie, request->cookie); + stl_be_p(&chunk->length, length); + } } static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client, @@ -2509,6 +2521,8 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient *client, { if (client->mode >= NBD_MODE_STRUCTURED && ret < 0) { return nbd_co_send_chunk_error(client, request, -ret, error_msg, errp); + } else if (client->mode >= NBD_MODE_EXTENDED) { + return nbd_co_send_chunk_done(client, request, errp); } else { return nbd_co_send_simple_reply(client, request, ret < 0 ? -ret : 0, NULL, 0, errp); -- 2.41.0

Time to start supporting clients that request extended headers. Now we can finally reach the code added across several previous patches. Even though the NBD spec has been altered to allow us to accept NBD_CMD_READ larger than the max payload size (provided our response is a hole or broken up over more than one data chunk), we are not planning to take advantage of that, and continue to cap NBD_CMD_READ to 32M regardless of header size. For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already supports 64-bit operations without any effort on our part. For NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous patch took care of implementing the required NBD_REPLY_TYPE_BLOCK_STATUS_EXT. We do not yet support clients that want to do request payload filtering of NBD_CMD_BLOCK_STATUS; that will be added in later patches, but is not essential for qemu as a client since qemu only requests the single context base:allocation. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; --- v5: add R-b, s/8.1/8.2/ v4: split out parts into earlier patches, rebase to earlier changes, simplify handling of generic replies, retitle (compare to v3 9/14) --- docs/interop/nbd.txt | 1 + nbd/server.c | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index f5ca25174a6..9aae5e1f294 100644 --- a/docs/interop/nbd.txt +++ b/docs/interop/nbd.txt @@ -69,3 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE NBD_CMD_FLAG_FAST_ZERO * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth" * 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports +* 8.2: NBD_OPT_EXTENDED_HEADERS diff --git a/nbd/server.c b/nbd/server.c index 8448167b12a..b09ee44c159 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -482,6 +482,10 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, [10 .. 133] reserved (0) [unless no_zeroes] */ trace_nbd_negotiate_handle_export_name(); + if (client->mode >= NBD_MODE_EXTENDED) { + error_setg(errp, "Extended headers already negotiated"); + return -EINVAL; + } if (client->optlen > NBD_MAX_STRING_SIZE) { error_setg(errp, "Bad length received"); return -EINVAL; @@ -1264,6 +1268,10 @@ 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->mode >= NBD_MODE_EXTENDED) { + ret = nbd_negotiate_send_rep_err( + client, NBD_REP_ERR_EXT_HEADER_REQD, errp, + "extended headers already negotiated"); } else if (client->mode >= NBD_MODE_STRUCTURED) { ret = nbd_negotiate_send_rep_err( client, NBD_REP_ERR_INVALID, errp, @@ -1280,6 +1288,19 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp) errp); break; + case NBD_OPT_EXTENDED_HEADERS: + if (length) { + ret = nbd_reject_length(client, false, errp); + } else if (client->mode >= NBD_MODE_EXTENDED) { + ret = nbd_negotiate_send_rep_err( + client, NBD_REP_ERR_INVALID, errp, + "extended headers already negotiated"); + } else { + ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp); + client->mode = NBD_MODE_EXTENDED; + } + break; + default: ret = nbd_opt_drop(client, NBD_REP_ERR_UNSUP, errp, "Unsupported option %" PRIu32 " (%s)", -- 2.41.0

All the pieces are in place for a client to finally request extended headers. Note that we must not request extended headers when qemu-nbd is used to connect to the kernel module (as nbd.ko does not expect them, but expects us to do the negotiation in userspace before handing the socket over to the kernel), but there is no harm in all other clients requesting them. Extended headers are not essential to the information collected during 'qemu-nbd --list', but probing for it gives us one more piece of information in that output. Update the iotests affected by the new line of output. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; --- v5: add R-b v4: rebase to earlier changes, tweak commit message for why qemu-nbd connection to /dev/nbd cannot use extended mode [Vladimir] --- nbd/client-connection.c | 2 +- nbd/client.c | 20 ++++++++++++++----- qemu-nbd.c | 3 +++ tests/qemu-iotests/223.out | 6 ++++++ tests/qemu-iotests/233.out | 4 ++++ tests/qemu-iotests/241.out | 3 +++ tests/qemu-iotests/307.out | 5 +++++ .../tests/nbd-qemu-allocation.out | 1 + 8 files changed, 38 insertions(+), 6 deletions(-) diff --git a/nbd/client-connection.c b/nbd/client-connection.c index aa0201b7107..f9da67c87e3 100644 --- a/nbd/client-connection.c +++ b/nbd/client-connection.c @@ -93,7 +93,7 @@ NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr, .do_negotiation = do_negotiation, .initial_info.request_sizes = true, - .initial_info.mode = NBD_MODE_STRUCTURED, + .initial_info.mode = NBD_MODE_EXTENDED, .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 a2f253062aa..29ffc609a4b 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -953,15 +953,23 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, if (fixedNewStyle) { int result = 0; + if (max_mode >= NBD_MODE_EXTENDED) { + result = nbd_request_simple_option(ioc, + NBD_OPT_EXTENDED_HEADERS, + false, errp); + if (result) { + return result < 0 ? -EINVAL : NBD_MODE_EXTENDED; + } + } if (max_mode >= NBD_MODE_STRUCTURED) { result = nbd_request_simple_option(ioc, NBD_OPT_STRUCTURED_REPLY, false, errp); - if (result < 0) { - return -EINVAL; + if (result) { + return result < 0 ? -EINVAL : NBD_MODE_STRUCTURED; } } - return result ? NBD_MODE_STRUCTURED : NBD_MODE_SIMPLE; + return NBD_MODE_SIMPLE; } else { return NBD_MODE_EXPORT_NAME; } @@ -1034,6 +1042,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, } switch (info->mode) { + case NBD_MODE_EXTENDED: case NBD_MODE_STRUCTURED: if (base_allocation) { result = nbd_negotiate_simple_meta_context(ioc, info, errp); @@ -1144,7 +1153,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, *info = NULL; result = nbd_start_negotiate(ioc, tlscreds, hostname, &sioc, - NBD_MODE_STRUCTURED, NULL, errp); + NBD_MODE_EXTENDED, NULL, errp); if (tlscreds && sioc) { ioc = sioc; } @@ -1155,6 +1164,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, switch ((NBDMode)result) { case NBD_MODE_SIMPLE: case NBD_MODE_STRUCTURED: + case NBD_MODE_EXTENDED: /* newstyle - use NBD_OPT_LIST to populate array, then try * NBD_OPT_INFO on each array member. If structured replies * are enabled, also try NBD_OPT_LIST_META_CONTEXT. */ @@ -1191,7 +1201,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, break; } - if (result == NBD_MODE_STRUCTURED && + if (result >= NBD_MODE_STRUCTURED && nbd_list_meta_contexts(ioc, &array[i], errp) < 0) { goto out; } diff --git a/qemu-nbd.c b/qemu-nbd.c index 70aa3c487a0..e6681450cfe 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -235,6 +235,9 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls, printf(" opt block: %u\n", list[i].opt_block); printf(" max block: %u\n", list[i].max_block); } + printf(" transaction size: %s\n", + list[i].mode >= NBD_MODE_EXTENDED ? + "64-bit" : "32-bit"); if (list[i].n_contexts) { printf(" available meta contexts: %d\n", list[i].n_contexts); for (j = 0; j < list[i].n_contexts; j++) { diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 86a37014d0f..32f05f1c9af 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -87,6 +87,7 @@ exports available: 3 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:dirty-bitmap:b @@ -97,6 +98,7 @@ exports available: 3 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:dirty-bitmap:b2 @@ -106,6 +108,7 @@ exports available: 3 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:dirty-bitmap:b3 @@ -206,6 +209,7 @@ exports available: 3 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:dirty-bitmap:b @@ -216,6 +220,7 @@ exports available: 3 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:dirty-bitmap:b2 @@ -225,6 +230,7 @@ exports available: 3 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:dirty-bitmap:b3 diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out index 237c82767ea..1910f7df20f 100644 --- a/tests/qemu-iotests/233.out +++ b/tests/qemu-iotests/233.out @@ -39,6 +39,7 @@ exports available: 1 export: '' size: 67108864 min block: 1 + transaction size: 64-bit == check TLS fail over TCP with mismatched hostname == qemu-img: Could not open 'driver=nbd,host=localhost,port=PORT,tls-creds=tls0': Certificate does not match the hostname localhost @@ -53,6 +54,7 @@ exports available: 1 export: '' size: 67108864 min block: 1 + transaction size: 64-bit == check TLS with different CA fails == qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer @@ -83,6 +85,7 @@ exports available: 1 export: '' size: 67108864 min block: 1 + transaction size: 64-bit == check TLS works over UNIX with PSK == image: nbd+unix://?socket=SOCK_DIR/qemu-nbd.sock @@ -93,6 +96,7 @@ exports available: 1 export: '' size: 67108864 min block: 1 + transaction size: 64-bit == check TLS fails over UNIX with mismatch PSK == qemu-img: Could not open 'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': TLS handshake failed: The TLS connection was non-properly terminated. diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out index 7946c286d51..7267cd2997e 100644 --- a/tests/qemu-iotests/241.out +++ b/tests/qemu-iotests/241.out @@ -6,6 +6,7 @@ exports available: 1 export: '' size: 1024 min block: 1 + transaction size: 64-bit [{ "start": 0, "length": 1000, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET}, { "start": 1000, "length": 24, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false, "offset": OFFSET}] 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0) @@ -16,6 +17,7 @@ exports available: 1 export: '' size: 1024 min block: 512 + transaction size: 64-bit [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET}] 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0) WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw. @@ -28,6 +30,7 @@ exports available: 1 export: '' size: 1024 min block: 1 + transaction size: 64-bit [{ "start": 0, "length": 1000, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET}, { "start": 1000, "length": 24, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false, "offset": OFFSET}] 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0) diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out index 390f05d1b78..2b9a6a67a1a 100644 --- a/tests/qemu-iotests/307.out +++ b/tests/qemu-iotests/307.out @@ -19,6 +19,7 @@ exports available: 1 min block: XXX opt block: XXX max block: XXX + transaction size: 64-bit available meta contexts: 1 base:allocation @@ -47,6 +48,7 @@ exports available: 1 min block: XXX opt block: XXX max block: XXX + transaction size: 64-bit available meta contexts: 1 base:allocation @@ -78,6 +80,7 @@ exports available: 2 min block: XXX opt block: XXX max block: XXX + transaction size: 64-bit available meta contexts: 1 base:allocation export: 'export1' @@ -87,6 +90,7 @@ exports available: 2 min block: XXX opt block: XXX max block: XXX + transaction size: 64-bit available meta contexts: 1 base:allocation @@ -113,6 +117,7 @@ exports available: 1 min block: XXX opt block: XXX max block: XXX + transaction size: 64-bit available meta contexts: 1 base:allocation diff --git a/tests/qemu-iotests/tests/nbd-qemu-allocation.out b/tests/qemu-iotests/tests/nbd-qemu-allocation.out index 138eb09c6d0..1db16926ed4 100644 --- a/tests/qemu-iotests/tests/nbd-qemu-allocation.out +++ b/tests/qemu-iotests/tests/nbd-qemu-allocation.out @@ -21,6 +21,7 @@ exports available: 1 min block: 1 opt block: 4096 max block: 33554432 + transaction size: 64-bit available meta contexts: 2 base:allocation qemu:allocation-depth -- 2.41.0

The next commit will add support for the optional extension NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can request that the server only return a subset of negotiated contexts, rather than all contexts. To make that task easier, this patch populates the list of contexts to return on a per-command basis (for now, identical to the full set of negotiated contexts). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v5: fix null dereference on early error [Vladimir], hoist in assertion from v4 24/24 v4: split out NBDMetaContexts refactoring to its own patch, track NBDRequests.contexts as a pointer rather than inline --- include/block/nbd.h | 1 + nbd/server.c | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index 2006497f987..4e7bd6342f9 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -77,6 +77,7 @@ typedef struct NBDRequest { uint16_t flags; /* NBD_CMD_FLAG_* */ uint16_t type; /* NBD_CMD_* */ NBDMode mode; /* Determines which network representation to use */ + NBDMetaContexts *contexts; /* Used by NBD_CMD_BLOCK_STATUS */ } NBDRequest; typedef struct NBDSimpleReply { diff --git a/nbd/server.c b/nbd/server.c index 44ebbd139b2..2d4cec75a49 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2505,6 +2505,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, break; case NBD_CMD_BLOCK_STATUS: + request->contexts = &client->contexts; valid_flags |= NBD_CMD_FLAG_REQ_ONE; break; @@ -2745,17 +2746,18 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, "discard failed", errp); case NBD_CMD_BLOCK_STATUS: + assert(request->contexts); if (!request->len) { return nbd_send_generic_reply(client, request, -EINVAL, "need non-zero length", errp); } assert(client->mode >= NBD_MODE_EXTENDED || request->len <= UINT32_MAX); - if (client->contexts.count) { + if (request->contexts->count) { bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE; - int contexts_remaining = client->contexts.count; + int contexts_remaining = request->contexts->count; - if (client->contexts.base_allocation) { + if (request->contexts->base_allocation) { ret = nbd_co_send_block_status(client, request, exp->common.blk, request->from, @@ -2768,7 +2770,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } } - if (client->contexts.allocation_depth) { + if (request->contexts->allocation_depth) { ret = nbd_co_send_block_status(client, request, exp->common.blk, request->from, request->len, @@ -2781,8 +2783,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } } + assert(request->contexts->exp == client->exp); for (i = 0; i < client->exp->nr_export_bitmaps; i++) { - if (!client->contexts.bitmaps[i]) { + if (!request->contexts->bitmaps[i]) { continue; } ret = nbd_co_send_bitmap(client, request, @@ -2798,6 +2801,10 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, assert(!contexts_remaining); return 0; + } else if (client->contexts.count) { + return nbd_send_generic_reply(client, request, -EINVAL, + "CMD_BLOCK_STATUS payload not valid", + errp); } else { return nbd_send_generic_reply(client, request, -EINVAL, "CMD_BLOCK_STATUS not negotiated", @@ -2876,6 +2883,11 @@ static coroutine_fn void nbd_trip(void *opaque) } else { ret = nbd_handle_request(client, &request, req->data, &local_err); } + if (request.contexts && request.contexts != &client->contexts) { + assert(request.type == NBD_CMD_BLOCK_STATUS); + g_free(request.contexts->bitmaps); + g_free(request.contexts); + } if (ret < 0) { error_prepend(&local_err, "Failed to send reply: "); goto disconnect; -- 2.41.0

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(a)redhat.com&gt; --- v5: factor out 'id - NBD_MTA_ID_DIRTY_BITMAP' [Vladimir], rework logic on zero-length requests to be clearer [Vladimir], rebase to earlier changes --- docs/interop/nbd.txt | 2 +- nbd/server.c | 114 ++++++++++++++++-- qemu-nbd.c | 1 + nbd/trace-events | 1 + tests/qemu-iotests/223.out | 12 +- tests/qemu-iotests/307.out | 10 +- .../tests/nbd-qemu-allocation.out | 2 +- 7 files changed, 122 insertions(+), 20 deletions(-) diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index 9aae5e1f294..18efb251de9 100644 --- a/docs/interop/nbd.txt +++ b/docs/interop/nbd.txt @@ -69,4 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE NBD_CMD_FLAG_FAST_ZERO * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth" * 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports -* 8.2: NBD_OPT_EXTENDED_HEADERS +* 8.2: NBD_OPT_EXTENDED_HEADERS, NBD_FLAG_BLOCK_STATUS_PAYLOAD diff --git a/nbd/server.c b/nbd/server.c index 2d4cec75a49..898580a9b0b 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -512,6 +512,9 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, if (client->mode >= NBD_MODE_STRUCTURED) { myflags |= NBD_FLAG_SEND_DF; } + if (client->mode >= NBD_MODE_EXTENDED && client->contexts.count) { + myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD; + } trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags); stq_be_p(buf, client->exp->size); stw_be_p(buf + 8, myflags); @@ -699,6 +702,10 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) if (client->mode >= NBD_MODE_STRUCTURED) { myflags |= NBD_FLAG_SEND_DF; } + if (client->mode >= NBD_MODE_EXTENDED && + (client->contexts.count || client->opt == NBD_OPT_INFO)) { + myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD; + } trace_nbd_negotiate_new_style_size_flags(exp->size, myflags); stq_be_p(buf, exp->size); stw_be_p(buf + 8, myflags); @@ -2420,6 +2427,87 @@ static int coroutine_fn nbd_co_send_bitmap(NBDClient *client, return nbd_co_send_extents(client, request, ea, last, context_id, errp); } +/* + * 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) +{ + int payload_len = request->len; + g_autofree char *buf = NULL; + size_t count, i, nr_bitmaps; + uint32_t id; + + if (payload_len > NBD_MAX_BUFFER_SIZE) { + error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)", + request->len, NBD_MAX_BUFFER_SIZE); + return -EINVAL; + } + + assert(client->contexts.exp == client->exp); + nr_bitmaps = client->exp->nr_export_bitmaps; + request->contexts = g_new0(NBDMetaContexts, 1); + request->contexts->exp = client->exp; + + if (payload_len % sizeof(uint32_t) || + payload_len < sizeof(NBDBlockStatusPayload) || + 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; + } + request->contexts->base_allocation = true; + } else if (id == NBD_META_ID_ALLOCATION_DEPTH) { + if (request->contexts->allocation_depth) { + goto skip; + } + request->contexts->allocation_depth = true; + } else { + int idx = id - NBD_META_ID_DIRTY_BITMAP; + + 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); +} + /* nbd_co_receive_request * Collect a client request. Return 0 if request looks valid, -EIO to drop * connection right away, -EAGAIN to indicate we were interrupted and the @@ -2505,7 +2593,18 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, break; case NBD_CMD_BLOCK_STATUS: - request->contexts = &client->contexts; + if (extended_with_payload) { + ret = nbd_co_block_status_payload_read(client, request, errp); + if (ret < 0) { + return ret; + } + /* payload now consumed */ + check_length = extended_with_payload = false; + payload_len = 0; + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } else { + request->contexts = &client->contexts; + } valid_flags |= NBD_CMD_FLAG_REQ_ONE; break; @@ -2747,16 +2846,16 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, case NBD_CMD_BLOCK_STATUS: assert(request->contexts); - if (!request->len) { - return nbd_send_generic_reply(client, request, -EINVAL, - "need non-zero length", errp); - } assert(client->mode >= NBD_MODE_EXTENDED || request->len <= UINT32_MAX); if (request->contexts->count) { bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE; int contexts_remaining = request->contexts->count; + if (!request->len) { + return nbd_send_generic_reply(client, request, -EINVAL, + "need non-zero length", errp); + } if (request->contexts->base_allocation) { ret = nbd_co_send_block_status(client, request, exp->common.blk, @@ -2893,8 +2992,9 @@ static coroutine_fn void nbd_trip(void *opaque) goto disconnect; } - /* We must disconnect after NBD_CMD_WRITE if we did not - * read the payload. + /* + * We must disconnect after NBD_CMD_WRITE or BLOCK_STATUS with + * payload if we did not read the payload. */ if (!req->complete) { error_setg(&local_err, "Request handling failed in intermediate state"); diff --git a/qemu-nbd.c b/qemu-nbd.c index e6681450cfe..7f1734cfccc 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -219,6 +219,7 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls, [NBD_FLAG_SEND_RESIZE_BIT] = "resize", [NBD_FLAG_SEND_CACHE_BIT] = "cache", [NBD_FLAG_SEND_FAST_ZERO_BIT] = "fast-zero", + [NBD_FLAG_BLOCK_STAT_PAYLOAD_BIT] = "block-status-payload", }; printf(" size: %" PRIu64 "\n", list[i].size); 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" 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 diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 32f05f1c9af..e5e7f42caac 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -83,7 +83,7 @@ exports available: 0 exports available: 3 export: 'n' size: 4194304 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 @@ -94,7 +94,7 @@ exports available: 3 export: 'n2' description: some text size: 4194304 - flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) + flags: 0x1ded ( flush fua trim zeroes df multi cache fast-zero block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 @@ -104,7 +104,7 @@ exports available: 3 qemu:dirty-bitmap:b2 export: 'n3' size: 4194304 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 @@ -205,7 +205,7 @@ exports available: 0 exports available: 3 export: 'n' size: 4194304 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 @@ -216,7 +216,7 @@ exports available: 3 export: 'n2' description: some text size: 4194304 - flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) + flags: 0x1ded ( flush fua trim zeroes df multi cache fast-zero block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 @@ -226,7 +226,7 @@ exports available: 3 qemu:dirty-bitmap:b2 export: 'n3' size: 4194304 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out index 2b9a6a67a1a..f645f3315f8 100644 --- a/tests/qemu-iotests/307.out +++ b/tests/qemu-iotests/307.out @@ -15,7 +15,7 @@ wrote 4096/4096 bytes at offset 0 exports available: 1 export: 'fmt' size: 67108864 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: XXX opt block: XXX max block: XXX @@ -44,7 +44,7 @@ exports available: 1 exports available: 1 export: 'fmt' size: 67108864 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: XXX opt block: XXX max block: XXX @@ -76,7 +76,7 @@ exports available: 1 exports available: 2 export: 'fmt' size: 67108864 - flags: 0x58f ( readonly flush fua df multi cache ) + flags: 0x158f ( readonly flush fua df multi cache block-status-payload ) min block: XXX opt block: XXX max block: XXX @@ -86,7 +86,7 @@ exports available: 2 export: 'export1' description: This is the writable second export size: 67108864 - flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) + flags: 0x1ded ( flush fua trim zeroes df multi cache fast-zero block-status-payload ) min block: XXX opt block: XXX max block: XXX @@ -113,7 +113,7 @@ exports available: 1 export: 'export1' description: This is the writable second export size: 67108864 - flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) + flags: 0x1ded ( flush fua trim zeroes df multi cache fast-zero block-status-payload ) min block: XXX opt block: XXX max block: XXX diff --git a/tests/qemu-iotests/tests/nbd-qemu-allocation.out b/tests/qemu-iotests/tests/nbd-qemu-allocation.out index 1db16926ed4..56b57c69ed8 100644 --- a/tests/qemu-iotests/tests/nbd-qemu-allocation.out +++ b/tests/qemu-iotests/tests/nbd-qemu-allocation.out @@ -17,7 +17,7 @@ wrote 2097152/2097152 bytes at offset 1048576 exports available: 1 export: '' size: 4194304 - flags: 0x48f ( readonly flush fua df cache ) + flags: 0x148f ( readonly flush fua df cache block-status-payload ) min block: 1 opt block: 4096 max block: 33554432 -- 2.41.0

On Sat, Sep 30, 2023 at 04:24:02PM +0300, Vladimir Sementsov-Ogievskiy wrote: 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. Or else make idx an unsigned value, instead of signed. Also a good catch. Also a good catch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

On 05.10.23 16:49, Eric Blake wrote: Oops, I didn't notice: s/>/>=/, as nr_bitmaps is length of array. -- Best regards, Vladimir

On Thu, Oct 05, 2023 at 05:33:26PM +0300, Vladimir Sementsov-Ogievskiy wrote: Oh, it looks like I introduced that same type mismatch in commit 8db7e2d6 as well, although it appears to have a latent effect until this series enables the ability for request->length to actually exceed 32 bits. I'll reply on 1/12 with another squash I'm making there. Indeed; it doesn't make any different to later in the function. Will drop. Thanks for the careful review. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org