We had a mix of struct declarataions followed by typedefs, and direct struct definitions as part of a typedef. Pick a single style. Also float a couple of opaque typedefs earlier in the file, as a later patch wants to refer NBDExport* in NBDRequest. No semantic impact. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: new patch --- include/block/nbd.h | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index a4c98169c39..9c3ceae5ba5 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 @@ -26,24 +26,25 @@ #include "qapi/error.h" #include "qemu/bswap.h" +typedef struct NBDExport NBDExport; +typedef struct NBDClient NBDClient; + extern const BlockExportDriver blk_exp_nbd; /* Handshake phase structs - this struct is passed on the wire */ -struct NBDOption { +typedef struct NBDOption { uint64_t magic; /* NBD_OPTS_MAGIC */ uint32_t option; /* NBD_OPT_* */ uint32_t length; -} QEMU_PACKED; -typedef struct NBDOption NBDOption; +} QEMU_PACKED NBDOption; -struct NBDOptionReply { +typedef struct NBDOptionReply { uint64_t magic; /* NBD_REP_MAGIC */ uint32_t option; /* NBD_OPT_* */ uint32_t type; /* NBD_REP_* */ uint32_t length; -} QEMU_PACKED; -typedef struct NBDOptionReply NBDOptionReply; +} QEMU_PACKED NBDOptionReply; typedef struct NBDOptionReplyMetaContext { NBDOptionReply h; /* h.type = NBD_REP_META_CONTEXT, h.length > 4 */ @@ -56,14 +57,13 @@ typedef struct NBDOptionReplyMetaContext { * Note: these are _NOT_ the same as the network representation of an NBD * request and reply! */ -struct NBDRequest { +typedef struct NBDRequest { uint64_t handle; uint64_t from; uint32_t len; uint16_t flags; /* NBD_CMD_FLAG_* */ uint16_t type; /* NBD_CMD_* */ -}; -typedef struct NBDRequest NBDRequest; +} NBDRequest; typedef struct NBDSimpleReply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC */ @@ -282,7 +282,7 @@ static inline bool nbd_reply_type_is_error(int type) #define NBD_ESHUTDOWN 108 /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */ -struct NBDExportInfo { +typedef struct NBDExportInfo { /* Set by client before nbd_receive_negotiate() */ bool request_sizes; char *x_dirty_bitmap; @@ -310,8 +310,7 @@ struct NBDExportInfo { char *description; int n_contexts; char **contexts; -}; -typedef struct NBDExportInfo NBDExportInfo; +} NBDExportInfo; int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, QCryptoTLSCreds *tlscreds, @@ -330,9 +329,6 @@ int nbd_client(int fd); int nbd_disconnect(int fd); int nbd_errno_to_system_errno(int err); -typedef struct NBDExport NBDExport; -typedef struct NBDClient NBDClient; - void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk); AioContext *nbd_export_aio_context(NBDExport *exp); -- 2.40.1

On 08.06.23 17:17, Eric Blake wrote: Sounds good to me. Anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; -- Best regards, Vladimir

Externally, libnbd exposed the 64-bit opaque marker for each client NBD packet as the "cookie", because it was less confusing when contrasted with 'struct nbd_handle *' holding all libnbd state. It also avoids confusion between the nown 'handle' as a way to identify a packet and the verb 'handle' for reacting to things like signals. Upstream NBD changed their spec to favor the name "cookie" based on libnbd's recommendations[1], so we can do likewise. [1] https://github.com/NetworkBlockDevice/nbd/commit/ca4392eb2b Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: new patch --- include/block/nbd.h | 11 +++--- block/nbd.c | 96 +++++++++++++++++++++++---------------------- nbd/client.c | 14 +++---- nbd/server.c | 29 +++++++------- nbd/trace-events | 22 +++++------ 5 files changed, 87 insertions(+), 85 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index e563f1774b0..59db69bafa5 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -58,7 +58,7 @@ typedef struct NBDOptionReplyMetaContext { * request and reply! */ typedef struct NBDRequest { - uint64_t handle; + uint64_t cookie; uint64_t from; uint32_t len; uint16_t flags; /* NBD_CMD_FLAG_* */ @@ -68,7 +68,7 @@ typedef struct NBDRequest { typedef struct NBDSimpleReply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC */ uint32_t error; - uint64_t handle; + uint64_t cookie; } QEMU_PACKED NBDSimpleReply; /* Header of all structured replies */ @@ -76,7 +76,7 @@ typedef struct NBDStructuredReplyChunk { uint32_t magic; /* NBD_STRUCTURED_REPLY_MAGIC */ uint16_t flags; /* combination of NBD_REPLY_FLAG_* */ uint16_t type; /* NBD_REPLY_TYPE_* */ - uint64_t handle; /* request handle */ + uint64_t cookie; /* request handle */ uint32_t length; /* length of payload */ } QEMU_PACKED NBDStructuredReplyChunk; @@ -84,13 +84,14 @@ typedef union NBDReply { NBDSimpleReply simple; NBDStructuredReplyChunk structured; struct { - /* @magic and @handle fields have the same offset and size both in + /* + * @magic and @cookie fields have the same offset and size both in * simple reply and structured reply chunk, so let them be accessible * without ".simple." or ".structured." specification */ uint32_t magic; uint32_t _skip; - uint64_t handle; + uint64_t cookie; } QEMU_PACKED; } NBDReply; diff --git a/block/nbd.c b/block/nbd.c index 5aef5cb6bd5..be3c46c6fee 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; * @@ -50,8 +50,8 @@ #define EN_OPTSTR ":exportname=" #define MAX_NBD_REQUESTS 16 -#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs)) -#define INDEX_TO_HANDLE(bs, index) ((index) ^ (uint64_t)(intptr_t)(bs)) +#define COOKIE_TO_INDEX(bs, cookie) ((cookie) ^ (uint64_t)(intptr_t)(bs)) +#define INDEX_TO_COOKIE(bs, index) ((index) ^ (uint64_t)(intptr_t)(bs)) typedef struct { Coroutine *coroutine; @@ -417,25 +417,25 @@ static void coroutine_fn GRAPH_RDLOCK nbd_reconnect_attempt(BDRVNBDState *s) reconnect_delay_timer_del(s); } -static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) +static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie) { int ret; - uint64_t ind = HANDLE_TO_INDEX(s, handle), ind2; + uint64_t ind = COOKIE_TO_INDEX(s, cookie), ind2; QEMU_LOCK_GUARD(&s->receive_mutex); while (true) { - if (s->reply.handle == handle) { + if (s->reply.cookie == cookie) { /* We are done */ return 0; } - if (s->reply.handle != 0) { + if (s->reply.cookie != 0) { /* * Some other request is being handled now. It should already be - * woken by whoever set s->reply.handle (or never wait in this + * woken by whoever set s->reply.cookie (or never wait in this * yield). So, we should not wake it here. */ - ind2 = HANDLE_TO_INDEX(s, s->reply.handle); + ind2 = COOKIE_TO_INDEX(s, s->reply.cookie); assert(!s->requests[ind2].receiving); s->requests[ind].receiving = true; @@ -445,9 +445,9 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) /* * We may be woken for 2 reasons: * 1. From this function, executing in parallel coroutine, when our - * handle is received. + * cookie is received. * 2. From nbd_co_receive_one_chunk(), when previous request is - * finished and s->reply.handle set to 0. + * finished and s->reply.cookie set to 0. * Anyway, it's OK to lock the mutex and go to the next iteration. */ @@ -456,8 +456,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) continue; } - /* We are under mutex and handle is 0. We have to do the dirty work. */ - assert(s->reply.handle == 0); + /* We are under mutex and cookie is 0. We have to do the dirty work. */ + assert(s->reply.cookie == 0); ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, NULL); if (ret <= 0) { ret = ret ? ret : -EIO; @@ -468,12 +468,12 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) nbd_channel_error(s, -EINVAL); return -EINVAL; } - ind2 = HANDLE_TO_INDEX(s, s->reply.handle); + ind2 = COOKIE_TO_INDEX(s, s->reply.cookie); if (ind2 >= MAX_NBD_REQUESTS || !s->requests[ind2].coroutine) { nbd_channel_error(s, -EINVAL); return -EINVAL; } - if (s->reply.handle == handle) { + if (s->reply.cookie == cookie) { /* We are done */ return 0; } @@ -519,7 +519,7 @@ nbd_co_send_request(BlockDriverState *bs, NBDRequest *request, qemu_mutex_unlock(&s->requests_lock); qemu_co_mutex_lock(&s->send_mutex); - request->handle = INDEX_TO_HANDLE(s, i); + request->cookie = INDEX_TO_COOKIE(s, i); assert(s->ioc); @@ -828,11 +828,11 @@ static coroutine_fn int nbd_co_receive_structured_payload( * corresponding to the server's error reply), and errp is unchanged. */ static coroutine_fn int nbd_co_do_receive_one_chunk( - BDRVNBDState *s, uint64_t handle, bool only_structured, + BDRVNBDState *s, uint64_t cookie, bool only_structured, int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp) { int ret; - int i = HANDLE_TO_INDEX(s, handle); + int i = COOKIE_TO_INDEX(s, cookie); void *local_payload = NULL; NBDStructuredReplyChunk *chunk; @@ -841,14 +841,14 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( } *request_ret = 0; - ret = nbd_receive_replies(s, handle); + ret = nbd_receive_replies(s, cookie); if (ret < 0) { error_setg(errp, "Connection closed"); return -EIO; } assert(s->ioc); - assert(s->reply.handle == handle); + assert(s->reply.cookie == cookie); if (nbd_reply_is_simple(&s->reply)) { if (only_structured) { @@ -918,11 +918,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( * Return value is a fatal error code or normal nbd reply error code */ static coroutine_fn int nbd_co_receive_one_chunk( - BDRVNBDState *s, uint64_t handle, bool only_structured, + BDRVNBDState *s, uint64_t cookie, bool only_structured, int *request_ret, QEMUIOVector *qiov, NBDReply *reply, void **payload, Error **errp) { - int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured, + int ret = nbd_co_do_receive_one_chunk(s, cookie, only_structured, request_ret, qiov, payload, errp); if (ret < 0) { @@ -932,7 +932,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( /* For assert at loop start in nbd_connection_entry */ *reply = s->reply; } - s->reply.handle = 0; + s->reply.cookie = 0; nbd_recv_coroutines_wake(s); @@ -975,10 +975,10 @@ static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret) * NBD_FOREACH_REPLY_CHUNK * The pointer stored in @payload requires g_free() to free it. */ -#define NBD_FOREACH_REPLY_CHUNK(s, iter, handle, structured, \ +#define NBD_FOREACH_REPLY_CHUNK(s, iter, cookie, structured, \ qiov, reply, payload) \ for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \ - nbd_reply_chunk_iter_receive(s, &iter, handle, qiov, reply, payload);) + nbd_reply_chunk_iter_receive(s, &iter, cookie, qiov, reply, payload);) /* * nbd_reply_chunk_iter_receive @@ -986,7 +986,7 @@ static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret) */ static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s, NBDReplyChunkIter *iter, - uint64_t handle, + uint64_t cookie, QEMUIOVector *qiov, NBDReply *reply, void **payload) @@ -1005,7 +1005,7 @@ static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s, reply = &local_reply; } - ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured, + ret = nbd_co_receive_one_chunk(s, cookie, iter->only_structured, &request_ret, qiov, reply, payload, &local_err); if (ret < 0) { @@ -1038,7 +1038,7 @@ static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s, break_loop: qemu_mutex_lock(&s->requests_lock); - s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL; + s->requests[COOKIE_TO_INDEX(s, cookie)].coroutine = NULL; s->in_flight--; qemu_co_queue_next(&s->free_sema); qemu_mutex_unlock(&s->requests_lock); @@ -1046,12 +1046,13 @@ break_loop: return false; } -static int coroutine_fn nbd_co_receive_return_code(BDRVNBDState *s, uint64_t handle, - int *request_ret, Error **errp) +static int coroutine_fn +nbd_co_receive_return_code(BDRVNBDState *s, uint64_t cookie, + int *request_ret, Error **errp) { NBDReplyChunkIter iter; - NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, NULL, NULL) { + NBD_FOREACH_REPLY_CHUNK(s, iter, cookie, false, NULL, NULL, NULL) { /* nbd_reply_chunk_iter_receive does all the work */ } @@ -1060,16 +1061,17 @@ static int coroutine_fn nbd_co_receive_return_code(BDRVNBDState *s, uint64_t han return iter.ret; } -static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle, - uint64_t offset, QEMUIOVector *qiov, - int *request_ret, Error **errp) +static int coroutine_fn +nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t cookie, + uint64_t offset, QEMUIOVector *qiov, + int *request_ret, Error **errp) { NBDReplyChunkIter iter; NBDReply reply; void *payload = NULL; Error *local_err = NULL; - NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply, + NBD_FOREACH_REPLY_CHUNK(s, iter, cookie, s->info.structured_reply, qiov, &reply, &payload) { int ret; @@ -1112,10 +1114,10 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h return iter.ret; } -static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, - uint64_t handle, uint64_t length, - NBDExtent *extent, - int *request_ret, Error **errp) +static int coroutine_fn +nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t cookie, + uint64_t length, NBDExtent *extent, + int *request_ret, Error **errp) { NBDReplyChunkIter iter; NBDReply reply; @@ -1124,7 +1126,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, bool received = false; assert(!extent->length); - NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { + NBD_FOREACH_REPLY_CHUNK(s, iter, cookie, false, NULL, &reply, &payload) { int ret; NBDStructuredReplyChunk *chunk = &reply.structured; @@ -1194,11 +1196,11 @@ nbd_co_request(BlockDriverState *bs, NBDRequest *request, continue; } - ret = nbd_co_receive_return_code(s, request->handle, + ret = nbd_co_receive_return_code(s, request->cookie, &request_ret, &local_err); if (local_err) { trace_nbd_co_request_fail(request->from, request->len, - request->handle, request->flags, + request->cookie, request->flags, request->type, nbd_cmd_lookup(request->type), ret, error_get_pretty(local_err)); @@ -1253,10 +1255,10 @@ nbd_client_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, continue; } - ret = nbd_co_receive_cmdread_reply(s, request.handle, offset, qiov, + ret = nbd_co_receive_cmdread_reply(s, request.cookie, offset, qiov, &request_ret, &local_err); if (local_err) { - trace_nbd_co_request_fail(request.from, request.len, request.handle, + trace_nbd_co_request_fail(request.from, request.len, request.cookie, request.flags, request.type, nbd_cmd_lookup(request.type), ret, error_get_pretty(local_err)); @@ -1411,11 +1413,11 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status( continue; } - ret = nbd_co_receive_blockstatus_reply(s, request.handle, bytes, + ret = nbd_co_receive_blockstatus_reply(s, request.cookie, bytes, &extent, &request_ret, &local_err); if (local_err) { - trace_nbd_co_request_fail(request.from, request.len, request.handle, + trace_nbd_co_request_fail(request.from, request.len, request.cookie, request.flags, request.type, nbd_cmd_lookup(request.type), ret, error_get_pretty(local_err)); diff --git a/nbd/client.c b/nbd/client.c index ff75722e487..ea3590ca3d0 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 @@ -1350,14 +1350,14 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request) { uint8_t buf[NBD_REQUEST_SIZE]; - trace_nbd_send_request(request->from, request->len, request->handle, + trace_nbd_send_request(request->from, request->len, request->cookie, 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 + 8, request->cookie); stq_be_p(buf + 16, request->from); stl_be_p(buf + 24, request->len); @@ -1383,7 +1383,7 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply, } reply->error = be32_to_cpu(reply->error); - reply->handle = be64_to_cpu(reply->handle); + reply->cookie = be64_to_cpu(reply->cookie); return 0; } @@ -1410,7 +1410,7 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, chunk->flags = be16_to_cpu(chunk->flags); chunk->type = be16_to_cpu(chunk->type); - chunk->handle = be64_to_cpu(chunk->handle); + chunk->cookie = be64_to_cpu(chunk->cookie); chunk->length = be32_to_cpu(chunk->length); return 0; @@ -1487,7 +1487,7 @@ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, } trace_nbd_receive_simple_reply(reply->simple.error, nbd_err_lookup(reply->simple.error), - reply->handle); + reply->cookie); break; case NBD_STRUCTURED_REPLY_MAGIC: ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured, errp); @@ -1497,7 +1497,7 @@ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, type = nbd_reply_type_lookup(reply->structured.type); trace_nbd_receive_structured_reply_chunk(reply->structured.flags, reply->structured.type, type, - reply->structured.handle, + reply->structured.cookie, reply->structured.length); break; default: diff --git a/nbd/server.c b/nbd/server.c index 26b27d69202..8486b64b15d 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1428,7 +1428,7 @@ static int coroutine_fn nbd_receive_request(NBDClient *client, NBDRequest *reque [ 0 .. 3] magic (NBD_REQUEST_MAGIC) [ 4 .. 5] flags (NBD_CMD_FLAG_FUA, ...) [ 6 .. 7] type (NBD_CMD_READ, ...) - [ 8 .. 15] handle + [ 8 .. 15] cookie [16 .. 23] from [24 .. 27] len */ @@ -1436,7 +1436,7 @@ static int coroutine_fn nbd_receive_request(NBDClient *client, NBDRequest *reque magic = ldl_be_p(buf); request->flags = lduw_be_p(buf + 4); request->type = lduw_be_p(buf + 6); - request->handle = ldq_be_p(buf + 8); + request->cookie = ldq_be_p(buf + 8); request->from = ldq_be_p(buf + 16); request->len = ldl_be_p(buf + 24); @@ -1885,11 +1885,11 @@ static int coroutine_fn nbd_co_send_iov(NBDClient *client, struct iovec *iov, } static inline void set_be_simple_reply(NBDSimpleReply *reply, uint64_t error, - uint64_t handle) + uint64_t cookie) { stl_be_p(&reply->magic, NBD_SIMPLE_REPLY_MAGIC); stl_be_p(&reply->error, error); - stq_be_p(&reply->handle, handle); + stq_be_p(&reply->cookie, cookie); } static int coroutine_fn nbd_co_send_simple_reply(NBDClient *client, @@ -1908,9 +1908,9 @@ static int coroutine_fn nbd_co_send_simple_reply(NBDClient *client, assert(!len || !nbd_err); assert(!client->structured_reply || request->type != NBD_CMD_READ); - trace_nbd_co_send_simple_reply(request->handle, nbd_err, + trace_nbd_co_send_simple_reply(request->cookie, nbd_err, nbd_err_lookup(nbd_err), len); - set_be_simple_reply(&reply, nbd_err, request->handle); + set_be_simple_reply(&reply, nbd_err, request->cookie); return nbd_co_send_iov(client, iov, 2, errp); } @@ -1940,7 +1940,7 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, 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->handle, request->handle); + stq_be_p(&chunk->cookie, request->cookie); stl_be_p(&chunk->length, length); } @@ -1953,10 +1953,9 @@ static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client, {.iov_base = &hdr}, }; - trace_nbd_co_send_chunk_done(request->handle); + trace_nbd_co_send_chunk_done(request->cookie); set_be_chunk(client, iov, 1, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_NONE, request); - return nbd_co_send_iov(client, iov, 1, errp); } @@ -1977,7 +1976,7 @@ static int coroutine_fn nbd_co_send_chunk_read(NBDClient *client, }; assert(size); - trace_nbd_co_send_chunk_read(request->handle, offset, data, size); + trace_nbd_co_send_chunk_read(request->cookie, offset, data, size); set_be_chunk(client, iov, 3, final ? NBD_REPLY_FLAG_DONE : 0, NBD_REPLY_TYPE_OFFSET_DATA, request); stq_be_p(&chunk.offset, offset); @@ -2001,7 +2000,7 @@ static int coroutine_fn nbd_co_send_chunk_error(NBDClient *client, }; assert(nbd_err); - trace_nbd_co_send_chunk_error(request->handle, nbd_err, + trace_nbd_co_send_chunk_error(request->cookie, nbd_err, nbd_err_lookup(nbd_err), msg ? msg : ""); set_be_chunk(client, iov, 3, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, request); @@ -2052,7 +2051,7 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client, {.iov_base = &chunk, .iov_len = sizeof(chunk)}, }; - trace_nbd_co_send_chunk_read_hole(request->handle, + trace_nbd_co_send_chunk_read_hole(request->cookie, offset + progress, pnum); set_be_chunk(client, iov, 2, final ? NBD_REPLY_FLAG_DONE : 0, @@ -2234,7 +2233,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest *request, NBDExtentArray *ea, nbd_extent_array_convert_to_be(ea); - trace_nbd_co_send_extents(request->handle, ea->count, context_id, + trace_nbd_co_send_extents(request->cookie, ea->count, context_id, ea->total_length, last); set_be_chunk(client, iov, 3, last ? NBD_REPLY_FLAG_DONE : 0, NBD_REPLY_TYPE_BLOCK_STATUS, request); @@ -2337,7 +2336,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * return ret; } - trace_nbd_co_receive_request_decode_type(request->handle, request->type, + trace_nbd_co_receive_request_decode_type(request->cookie, request->type, nbd_cmd_lookup(request->type)); if (request->type != NBD_CMD_WRITE) { @@ -2378,7 +2377,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * } req->complete = true; - trace_nbd_co_receive_request_payload_received(request->handle, + trace_nbd_co_receive_request_payload_received(request->cookie, request->len); } diff --git a/nbd/trace-events b/nbd/trace-events index 50ca05a9e22..f19a4d0db39 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -31,9 +31,9 @@ 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_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_send_request(uint64_t from, uint32_t len, uint64_t cookie, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .cookie = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }" +nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t cookie) "Got simple reply: { .error = %" PRId32 " (%s), cookie = %" PRIu64" }" +nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char *name, uint64_t cookie, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d (%s), cookie = %" PRIu64 ", length = %" PRIu32 " }" # common.c nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL" @@ -63,14 +63,14 @@ 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_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" -nbd_co_send_chunk_done(uint64_t handle) "Send structured reply done: handle = %" PRIu64 -nbd_co_send_chunk_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu" -nbd_co_send_chunk_read_hole(uint64_t handle, uint64_t offset, size_t size) "Send structured read hole reply: handle = %" PRIu64 ", offset = %" PRIu64 ", len = %zu" -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_chunk_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_send_simple_reply(uint64_t cookie, uint32_t error, const char *errname, int len) "Send simple reply: cookie = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d" +nbd_co_send_chunk_done(uint64_t cookie) "Send structured reply done: cookie = %" PRIu64 +nbd_co_send_chunk_read(uint64_t cookie, uint64_t offset, void *data, size_t size) "Send structured read data reply: cookie = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu" +nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, size_t size) "Send structured read hole reply: cookie = %" PRIu64 ", offset = %" PRIu64 ", len = %zu" +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_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, uint32_t len) "Payload received: cookie = %" 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_trip(void) "Reading request" -- 2.40.1

On 08.06.23 16:56, Eric Blake wrote: Should we go further and use (uint64)-1 as a sentinel cookie value, and just use index as a cookie? Or, using zero cookie in a wire looks too asymmetric? Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; That looked like some security trick to hide real indices. But I don't think that index of request in a list is a secret information. [..] -- Best regards, Vladimir

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; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; --- v4: sink this later in series [Vladimir] --- nbd/client.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/nbd/client.c b/nbd/client.c index ea3590ca3d0..1b5569556fe 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1413,6 +1413,18 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, chunk->cookie = be64_to_cpu(chunk->cookie); 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

On 08.06.23 16:56, Eric Blake wrote: [..] Hmm. in all other cases in server code client.mode remains zero = OLDSTYLE, which is not quite correct. [..] -- Best regards, Vladimir

On Mon, Jun 12, 2023 at 02:24:52PM -0500, Eric Blake wrote: To move things along, I have now staged 1-8 in my NBD queue for a pull request, and will then repost this patch and the remainder of the series as v5, to make it easier to pick up the final needed R-b. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Upcoming additions to support NBD 64-bit effect lengths allow for the possibility to distinguish between payload length (capped at 32M) and effect length (up to 63 bits). 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 (where the payload is a limited-size struct that in turns 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. Note that we do not support the payload version of BLOCK_STATUS yet. For this patch, no semantic change is intended for a compliant client. For a non-compliant client, it is possible that the error behavior changes (a different message, a change on whether the connection is killed or remains alive for the next command, or so forth), in part because req->complete is set later on some paths, but all errors should still be handled gracefully. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: less indentation on several 'if's [Vladimir] --- nbd/server.c | 76 ++++++++++++++++++++++++++++++------------------ nbd/trace-events | 1 + 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 4ac05d0cd7b..d7dc29f0445 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2329,6 +2329,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * Error **errp) { NBDClient *client = req->client; + bool extended_with_payload; + unsigned payload_len = 0; int valid_flags; int ret; @@ -2342,48 +2344,63 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * trace_nbd_co_receive_request_decode_type(request->cookie, request->type, nbd_cmd_lookup(request->type)); - if (request->type != NBD_CMD_WRITE) { - /* No payload, we are ready to read the next request. */ - req->complete = true; - } - if (request->type == NBD_CMD_DISC) { /* Special case: we're going to disconnect without a reply, * whether or not flags, from, or len are bogus */ + req->complete = true; return -EIO; } - if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || - request->type == NBD_CMD_CACHE) - { - if (request->len > NBD_MAX_BUFFER_SIZE) { - error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", - request->len, NBD_MAX_BUFFER_SIZE); - return -EINVAL; + /* Payload and buffer handling. */ + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && + (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN); + if ((request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || + request->type == NBD_CMD_CACHE || extended_with_payload) && + request->len > NBD_MAX_BUFFER_SIZE) { + error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", + request->len, NBD_MAX_BUFFER_SIZE); + return -EINVAL; + } + + if (request->type == NBD_CMD_WRITE || extended_with_payload) { + payload_len = request->len; + if (request->type != NBD_CMD_WRITE) { + /* + * For now, we don't support payloads on other + * commands; but we can keep the connection alive. + */ + request->len = 0; + } else if (client->mode >= NBD_MODE_EXTENDED && + !extended_with_payload) { + /* The client is noncompliant. Trace it, but proceed. */ + trace_nbd_co_receive_ext_payload_compliance(request->from, + request->len); } + } - if (request->type != NBD_CMD_CACHE) { - req->data = blk_try_blockalign(client->exp->common.blk, - request->len); - if (req->data == NULL) { - error_setg(errp, "No memory"); - return -ENOMEM; - } + if (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ) { + req->data = blk_try_blockalign(client->exp->common.blk, + request->len); + if (req->data == NULL) { + error_setg(errp, "No memory"); + return -ENOMEM; } } - 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) - { + if (payload_len) { + if (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; } - req->complete = true; - trace_nbd_co_receive_request_payload_received(request->cookie, - request->len); + payload_len); } + req->complete = true; /* Sanity checks. */ if (client->exp->nbdflags & NBD_FLAG_READ_ONLY && @@ -2413,7 +2430,10 @@ 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->mode >= NBD_MODE_STRUCTURED) { + if (request->type == NBD_CMD_WRITE && client->mode >= NBD_MODE_EXTENDED) { + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } else if (request->type == NBD_CMD_READ && + client->mode >= NBD_MODE_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; diff --git a/nbd/trace-events b/nbd/trace-events index 3338da2be2a..6a34d7f027a 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.40.1

On 08.06.23 21:29, Eric Blake wrote: I vote for switch!) Really, seems it would be a lot simpler to read. -- Best regards, Vladimir

On 08.06.23 16:56, Eric Blake wrote: I'd prefer to keep iov[0].iov_len notation, to stress that iov is an array anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; Hm. Not about this patch: we now moved to simple cookies. And it seems that actually, 64bit is too much for number of request. -- Best regards, Vladimir

On 04.08.23 22:28, Eric Blake wrote: OK Agree -- Best regards, Vladimir

On 08.06.23 16:56, Eric Blake wrote: [..] that "if" and uint64_t sum was to avoid overflow. I think, we can't just assert, instead include the check into if: if (sum >= length && (sum <= UINT32_MAX || ea->extended) { ... } with this: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; -- Best regards, Vladimir

On 04.08.23 22:36, Eric Blake wrote: OK. Small comment would be good. Keep my r-b. The only my point is that you make this small "extent API" block-layer dependent. But I'm not sure that is the only dependency, and I don't insist anyway. -- 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. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: split off errp handling to separate patch [Vladimir], better function naming [Vladimir] --- include/block/nbd.h | 3 +- block/nbd.c | 2 +- nbd/client.c | 100 +++++++++++++++++++++++++++++--------------- nbd/trace-events | 3 +- 4 files changed, 72 insertions(+), 36 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index dc05f5981fb..af80087e2cd 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -389,7 +389,8 @@ int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info, Error **errp); int nbd_send_request(QIOChannel *ioc, NBDRequest *request); int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, - NBDReply *reply, Error **errp); + NBDReply *reply, NBDMode mode, + Error **errp); 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 c17ce935f17..e281fac43d1 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -459,7 +459,7 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie, /* We are under mutex and cookie is 0. We have to do the dirty work. */ assert(s->reply.cookie == 0); - ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, errp); + ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, s->info.mode, errp); if (ret == 0) { ret = -EIO; error_setg(errp, "server dropped connection"); diff --git a/nbd/client.c b/nbd/client.c index 1495a9b0ab1..a4598a95427 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1352,22 +1352,29 @@ int nbd_disconnect(int fd) int nbd_send_request(QIOChannel *ioc, NBDRequest *request) { - uint8_t buf[NBD_REQUEST_SIZE]; + uint8_t buf[NBD_EXTENDED_REQUEST_SIZE]; + size_t len; - assert(request->mode <= NBD_MODE_STRUCTURED); /* TODO handle extended */ - assert(request->len <= UINT32_MAX); trace_nbd_send_request(request->from, request->len, request->cookie, 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->cookie); stq_be_p(buf + 16, request->from); - stl_be_p(buf + 24, request->len); + if (request->mode >= NBD_MODE_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,30 +1401,36 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply, return 0; } -/* nbd_receive_structured_reply_chunk +/* nbd_receive_reply_chunk_header * 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, - Error **errp) +static int nbd_receive_reply_chunk_header(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->cookie = be64_to_cpu(chunk->cookie); - chunk->length = be32_to_cpu(chunk->length); + /* flags, type, and cookie 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.cookie = be64_to_cpu(chunk->structured.cookie); /* * Because we use BLOCK_STATUS with REQ_ONE, and cap READ requests @@ -1425,11 +1438,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; } @@ -1476,19 +1498,21 @@ 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(). @mode 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, NBDMode mode, Error **errp) { int ret; const char *type; + uint32_t expected; ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), errp); if (ret <= 0) { @@ -1497,8 +1521,13 @@ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, reply->magic = be32_to_cpu(reply->magic); + /* Diagnose but accept wrong-width header */ switch (reply->magic) { case NBD_SIMPLE_REPLY_MAGIC: + if (mode >= NBD_MODE_EXTENDED) { + trace_nbd_receive_wrong_header(reply->magic, + nbd_mode_lookup(mode)); + } ret = nbd_receive_simple_reply(ioc, &reply->simple, errp); if (ret < 0) { break; @@ -1508,23 +1537,28 @@ int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, reply->cookie); break; case NBD_STRUCTURED_REPLY_MAGIC: - ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured, errp); + case NBD_EXTENDED_REPLY_MAGIC: + expected = mode >= NBD_MODE_EXTENDED ? NBD_EXTENDED_REPLY_MAGIC + : NBD_STRUCTURED_REPLY_MAGIC; + if (reply->magic != expected) { + trace_nbd_receive_wrong_header(reply->magic, + nbd_mode_lookup(mode)); + } + ret = nbd_receive_reply_chunk_header(ioc, reply, errp); if (ret < 0) { break; } type = nbd_reply_type_lookup(reply->structured.type); - trace_nbd_receive_structured_reply_chunk(reply->structured.flags, - reply->structured.type, type, - reply->structured.cookie, - reply->structured.length); + trace_nbd_receive_reply_chunk_header(reply->structured.flags, + reply->structured.type, type, + reply->structured.cookie, + reply->structured.length); break; default: + trace_nbd_receive_wrong_header(reply->magic, nbd_mode_lookup(mode)); error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", reply->magic); return -EINVAL; } - if (ret < 0) { - return ret; - } return 1; } diff --git a/nbd/trace-events b/nbd/trace-events index 6a34d7f027a..51bfb129c95 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -33,7 +33,8 @@ nbd_client_clear_queue(void) "Clearing NBD queue" nbd_client_clear_socket(void) "Clearing NBD socket" nbd_send_request(uint64_t from, uint64_t len, uint64_t cookie, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu64 ", .cookie = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }" nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t cookie) "Got simple reply: { .error = %" PRId32 " (%s), cookie = %" PRIu64" }" -nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char *name, uint64_t cookie, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d (%s), cookie = %" PRIu64 ", length = %" PRIu32 " }" +nbd_receive_reply_chunk_header(uint16_t flags, uint16_t type, const char *name, uint64_t cookie, uint32_t length) "Got reply chunk header: { flags = 0x%" PRIx16 ", type = %d (%s), cookie = %" PRIu64 ", length = %" PRIu32 " }" +nbd_receive_wrong_header(uint32_t magic, const char *mode) "Server sent unexpected magic 0x%" PRIx32 " for negotiated mode %s" # common.c nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL" -- 2.40.1

On Tue, Jun 27, 2023 at 05:22:09PM +0300, Vladimir Sementsov-Ogievskiy wrote: case NBD_SIMPLE_REPLY_MAGIC: if (mode >= NBD_MODE_EXTENDED) { trace_nbd_receive_wrong_header(reply->magic, nbd_mode_lookup(mode)); } ret = nbd_receive_simple_reply(ioc, &reply->simple, errp); if (ret < 0) { break; } trace_nbd_receive_simple_reply(reply->simple.error, nbd_err_lookup(reply->simple.error), reply->cookie); No, not even as a temporary assertion. You are correct that as of this patch, a compliant server will only be sending structured replies, not extended (because we haven't turned on a request for extended headers yet); but we should never assert something that a non-compliant server can send over the wire. So the best we can do is trace the server's unusual response. Indeed, I botched that logic (either keep the 'ret < 0' filter after the switch, or change 'break' to 'return ret' within the switch). Will fix in v5. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

On 08.06.23 16:56, Eric Blake wrote: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru&gt; [..] this calculation is done twice. I'd put it into expected_chunk_length variable. [..] -- Best regards, Vladimir

On Thu, Jun 08, 2023 at 08:56:52AM -0500, Eric Blake wrote: THere are paths where request->contexts is left NULL but request->type was set... so I think this also needs to be tweaked to check that request.contexts is non-NULL before dereferencing to free bitmaps. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

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. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/interop/nbd.txt | 2 +- nbd/server.c | 99 ++++++++++++++++++- 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, 111 insertions(+), 16 deletions(-) diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index abaf4c28a96..83d85ce8d13 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.1: NBD_OPT_EXTENDED_HEADERS +* 8.1: NBD_OPT_EXTENDED_HEADERS, NBD_FLAG_BLOCK_STATUS_PAYLOAD diff --git a/nbd/server.c b/nbd/server.c index 308846fe46b..696afcf5c46 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); @@ -2424,6 +2431,81 @@ 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 payload consumed, return 0 with + * request->len and request->contexts->count set to 0 (which will + * trigger an appropriate NBD_EINVAL response later on). On I/O + * error, return -EIO. + */ +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; + + assert(request->len <= NBD_MAX_BUFFER_SIZE); + 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 { + if (id - NBD_META_ID_DIRTY_BITMAP > nr_bitmaps || + request->contexts->bitmaps[id - NBD_META_ID_DIRTY_BITMAP]) { + goto skip; + } + request->contexts->bitmaps[id - NBD_META_ID_DIRTY_BITMAP] = 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 @@ -2470,7 +2552,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * if (request->type == NBD_CMD_WRITE || extended_with_payload) { payload_len = request->len; - if (request->type != NBD_CMD_WRITE) { + if (request->type == NBD_CMD_BLOCK_STATUS) { + ret = nbd_co_block_status_payload_read(client, request, errp); + if (ret < 0) { + return ret; + } + payload_len = 0; + } else if (request->type != NBD_CMD_WRITE) { /* * For now, we don't support payloads on other * commands; but we can keep the connection alive. @@ -2491,7 +2579,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * error_setg(errp, "No memory"); return -ENOMEM; } - } else if (request->type == NBD_CMD_BLOCK_STATUS) { + } else if (request->type == NBD_CMD_BLOCK_STATUS && + !extended_with_payload) { request->contexts = &client->contexts; } @@ -2547,6 +2636,9 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * valid_flags |= NBD_CMD_FLAG_NO_HOLE | NBD_CMD_FLAG_FAST_ZERO; } else if (request->type == NBD_CMD_BLOCK_STATUS) { valid_flags |= NBD_CMD_FLAG_REQ_ONE; + if (client->mode >= NBD_MODE_EXTENDED && client->contexts.count) { + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } } if (request->flags & ~valid_flags) { error_setg(errp, "unsupported flags for command %s (got 0x%x)", @@ -2712,7 +2804,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, "discard failed", errp); case NBD_CMD_BLOCK_STATUS: - if (!request->len) { + assert(request->contexts); + if (!request->len && !(request->flags & NBD_CMD_FLAG_PAYLOAD_LEN)) { return nbd_send_generic_reply(client, request, -EINVAL, "need non-zero length", errp); } diff --git a/qemu-nbd.c b/qemu-nbd.c index 1d155fc2c66..cbca0eeee62 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -222,6 +222,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 51bfb129c95..a1af6d003b4 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, size_t size nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, size_t size) "Send structured read hole reply: cookie = %" PRIu64 ", offset = %" PRIu64 ", len = %zu" 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 b98582c38ea..b38f0b7963b 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 659276032b0..794d1bfce62 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.40.1

On 08.06.23 16:56, Eric Blake wrote: [..] maybe, int ind = id - NBD_META_ID_DIRTY_BITMAP; [..] why not error-out for len=0 in case of payload? -- Best regards, Vladimir