From 80c7c2b00d607221bb43815d2c1951d54229b3ee Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 27 Sep 2018 17:42:00 +0100 Subject: [PATCH 1/6] nbd: Don't take address of fields in packed structs Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. This patch was produced with the following spatch script: @@ expression E; @@ -be16_to_cpus(&E); +E = be16_to_cpu(E); @@ expression E; @@ -be32_to_cpus(&E); +E = be32_to_cpu(E); @@ expression E; @@ -be64_to_cpus(&E); +E = be64_to_cpu(E); @@ expression E; @@ -cpu_to_be16s(&E); +E = cpu_to_be16(E); @@ expression E; @@ -cpu_to_be32s(&E); +E = cpu_to_be32(E); @@ expression E; @@ -cpu_to_be64s(&E); +E = cpu_to_be64(E); Signed-off-by: Peter Maydell Message-Id: <20180927164200.15097-1-peter.maydell@linaro.org> Reviewed-by: Eric Blake [eblake: rebase, and squash in missed changes] Signed-off-by: Eric Blake --- nbd/client.c | 44 ++++++++++++++++++++++---------------------- nbd/server.c | 24 ++++++++++++------------ 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index 40b74d9761..b4d457a19a 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -117,10 +117,10 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt, nbd_send_opt_abort(ioc); return -1; } - be64_to_cpus(&reply->magic); - be32_to_cpus(&reply->option); - be32_to_cpus(&reply->type); - be32_to_cpus(&reply->length); + reply->magic = be64_to_cpu(reply->magic); + reply->option = be32_to_cpu(reply->option); + reply->type = be32_to_cpu(reply->type); + reply->length = be32_to_cpu(reply->length); trace_nbd_receive_option_reply(reply->option, nbd_opt_lookup(reply->option), reply->type, nbd_rep_lookup(reply->type), @@ -396,7 +396,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname, return -1; } len -= sizeof(type); - be16_to_cpus(&type); + type = be16_to_cpu(type); switch (type) { case NBD_INFO_EXPORT: if (len != sizeof(info->size) + sizeof(info->flags)) { @@ -410,13 +410,13 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname, nbd_send_opt_abort(ioc); return -1; } - be64_to_cpus(&info->size); + info->size = be64_to_cpu(info->size); if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) { error_prepend(errp, "failed to read info flags: "); nbd_send_opt_abort(ioc); return -1; } - be16_to_cpus(&info->flags); + info->flags = be16_to_cpu(info->flags); trace_nbd_receive_negotiate_size_flags(info->size, info->flags); break; @@ -433,7 +433,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname, nbd_send_opt_abort(ioc); return -1; } - be32_to_cpus(&info->min_block); + info->min_block = be32_to_cpu(info->min_block); if (!is_power_of_2(info->min_block)) { error_setg(errp, "server minimum block size %" PRIu32 " is not a power of two", info->min_block); @@ -447,7 +447,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname, nbd_send_opt_abort(ioc); return -1; } - be32_to_cpus(&info->opt_block); + info->opt_block = be32_to_cpu(info->opt_block); if (!is_power_of_2(info->opt_block) || info->opt_block < info->min_block) { error_setg(errp, "server preferred block size %" PRIu32 @@ -461,7 +461,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname, nbd_send_opt_abort(ioc); return -1; } - be32_to_cpus(&info->max_block); + info->max_block = be32_to_cpu(info->max_block); if (info->max_block < info->min_block) { error_setg(errp, "server maximum block size %" PRIu32 " is not valid", info->max_block); @@ -668,7 +668,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) { return -1; } - be32_to_cpus(&received_id); + received_id = be32_to_cpu(received_id); reply.length -= sizeof(received_id); name = g_malloc(reply.length + 1); @@ -872,13 +872,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, error_prepend(errp, "Failed to read export length: "); goto fail; } - be64_to_cpus(&info->size); + info->size = be64_to_cpu(info->size); if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) { error_prepend(errp, "Failed to read export flags: "); goto fail; } - be16_to_cpus(&info->flags); + info->flags = be16_to_cpu(info->flags); } else if (magic == NBD_CLIENT_MAGIC) { uint32_t oldflags; @@ -895,13 +895,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, error_prepend(errp, "Failed to read export length: "); goto fail; } - be64_to_cpus(&info->size); + info->size = be64_to_cpu(info->size); if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) { error_prepend(errp, "Failed to read export flags: "); goto fail; } - be32_to_cpus(&oldflags); + oldflags = be32_to_cpu(oldflags); if (oldflags & ~0xffff) { error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); goto fail; @@ -1080,8 +1080,8 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply, return ret; } - be32_to_cpus(&reply->error); - be64_to_cpus(&reply->handle); + reply->error = be32_to_cpu(reply->error); + reply->handle = be64_to_cpu(reply->handle); return 0; } @@ -1105,10 +1105,10 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, return ret; } - be16_to_cpus(&chunk->flags); - be16_to_cpus(&chunk->type); - be64_to_cpus(&chunk->handle); - be32_to_cpus(&chunk->length); + chunk->flags = be16_to_cpu(chunk->flags); + chunk->type = be16_to_cpu(chunk->type); + chunk->handle = be64_to_cpu(chunk->handle); + chunk->length = be32_to_cpu(chunk->length); return 0; } @@ -1128,7 +1128,7 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) return ret; } - be32_to_cpus(&reply->magic); + reply->magic = be32_to_cpu(reply->magic); switch (reply->magic) { case NBD_SIMPLE_REPLY_MAGIC: diff --git a/nbd/server.c b/nbd/server.c index c3dd402b45..98d0fa2515 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -333,7 +333,7 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length, if (ret <= 0) { return ret; } - cpu_to_be32s(&len); + len = cpu_to_be32(len); if (len > NBD_MAX_NAME_SIZE) { return nbd_opt_invalid(client, errp, @@ -486,7 +486,7 @@ static int nbd_negotiate_send_info(NBDClient *client, if (rc < 0) { return rc; } - cpu_to_be16s(&info); + info = cpu_to_be16(info); if (nbd_write(client->ioc, &info, sizeof(info), errp) < 0) { return -EIO; } @@ -551,14 +551,14 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, if (rc <= 0) { return rc; } - be16_to_cpus(&requests); + requests = be16_to_cpu(requests); trace_nbd_negotiate_handle_info_requests(requests); while (requests--) { rc = nbd_opt_read(client, &request, sizeof(request), errp); if (rc <= 0) { return rc; } - be16_to_cpus(&request); + request = be16_to_cpu(request); trace_nbd_negotiate_handle_info_request(request, nbd_info_lookup(request)); /* We care about NBD_INFO_NAME and NBD_INFO_BLOCK_SIZE; @@ -618,9 +618,9 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, /* maximum - At most 32M, but smaller as appropriate. */ sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE); trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]); - cpu_to_be32s(&sizes[0]); - cpu_to_be32s(&sizes[1]); - cpu_to_be32s(&sizes[2]); + sizes[0] = cpu_to_be32(sizes[0]); + sizes[1] = cpu_to_be32(sizes[1]); + sizes[2] = cpu_to_be32(sizes[2]); rc = nbd_negotiate_send_info(client, NBD_INFO_BLOCK_SIZE, sizeof(sizes), sizes, errp); if (rc < 0) { @@ -904,7 +904,7 @@ static int nbd_negotiate_meta_query(NBDClient *client, if (ret <= 0) { return ret; } - cpu_to_be32s(&len); + len = cpu_to_be32(len); if (len < ns_len) { trace_nbd_negotiate_meta_query_skip("length too short"); @@ -971,7 +971,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client, if (ret <= 0) { return ret; } - cpu_to_be32s(&nb_queries); + nb_queries = cpu_to_be32(nb_queries); trace_nbd_negotiate_meta_context(nbd_opt_lookup(client->opt), export_name, nb_queries); @@ -1049,7 +1049,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, error_prepend(errp, "read failed: "); return -EIO; } - be32_to_cpus(&flags); + flags = be32_to_cpu(flags); trace_nbd_negotiate_options_flags(flags); if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) { fixedNewstyle = true; @@ -1900,8 +1900,8 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset, extents_end = extent + 1; for (extent = extents; extent < extents_end; extent++) { - cpu_to_be32s(&extent->flags); - cpu_to_be32s(&extent->length); + extent->flags = cpu_to_be32(extent->flags); + extent->length = cpu_to_be32(extent->length); } *bytes -= remaining_bytes; From 2f454defc23e1be78f2a96bad2877ce7829f61b4 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 3 Oct 2018 17:47:38 +0300 Subject: [PATCH 2/6] nbd/server: fix NBD_CMD_CACHE We should not go to structured-read branch on CACHE command, fix that. Bug introduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE" with the whole feature and affects 3.0.0 release. Signed-off-by: Vladimir Sementsov-Ogievskiy CC: qemu-stable@nongnu.org Message-Id: <20181003144738.70670-1-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake [eblake: commit message typo fix] Signed-off-by: Eric Blake --- nbd/server.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nbd/server.c b/nbd/server.c index 98d0fa2515..4fb247b116 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2177,7 +2177,8 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, } if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) && - request->len) { + request->len && request->type != NBD_CMD_CACHE) + { return nbd_co_send_sparse_read(client, request->handle, request->from, data, request->len, errp); } From f7812df77d7830c6b375066a4e656f3b79232c13 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 3 Oct 2018 13:04:26 -0500 Subject: [PATCH 3/6] qemu-nbd: Document --tls-creds Commit 145614a1 introduced --tls-creds and documented it in qemu-nbd.texi, but forgot to document it in 'qemu-nbd --help'. Signed-off-by: Eric Blake Message-Id: <20181003180426.602765-1-eblake@redhat.com> Reviewed-by: John Snow --- qemu-nbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qemu-nbd.c b/qemu-nbd.c index 51b9d38c72..66e023f7fa 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -94,6 +94,7 @@ static void usage(const char *name) "General purpose options:\n" " --object type,id=ID,... define an object such as 'secret' for providing\n" " passwords and/or encryption keys\n" +" --tls-creds=ID use id of an earlier --object to provide TLS\n" " -T, --trace [[enable=]][,events=][,file=]\n" " specify tracing options\n" " --fork fork off the server process and exit the parent\n" From f5cd0bb5174dcd6e8c160d7992fb89f09f264ef0 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 3 Oct 2018 20:02:27 +0300 Subject: [PATCH 4/6] qemu-nbd: drop old-style negotiation Use new-style negotiation always, with default "" (empty) export name if it is not specified with '-x' option. qemu as client can manage either style since 2.6.0, commit 69b49502d8 For comparison: nbd 3.10 dropped oldstyle long ago (Mar 2015): https://github.com/NetworkBlockDevice/nbd/commit/36940193 nbdkit 1.3 switched its default to newstyle (Jan 2018): https://github.com/libguestfs/nbdkit/commit/b2a8aecc https://github.com/libguestfs/nbdkit/commit/8158e773 Furthermore, if a client that only speaks oldstyle still needs to communicate to qemu, nbdkit remains available to perform the translation between the two protocols. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181003170228.95973-2-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake [eblake: enhance commit message] Signed-off-by: Eric Blake --- qemu-nbd.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index 66e023f7fa..6aaebe7d93 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -56,7 +56,6 @@ #define MBR_SIZE 512 static NBDExport *exp; -static bool newproto; static int verbose; static char *srcpath; static SocketAddress *saddr; @@ -84,8 +83,8 @@ static void usage(const char *name) " -e, --shared=NUM device can be shared by NUM clients (default '1')\n" " -t, --persistent don't exit on the last connection\n" " -v, --verbose display extra debugging information\n" -" -x, --export-name=NAME expose export by name\n" -" -D, --description=TEXT with -x, also export a human-readable description\n" +" -x, --export-name=NAME expose export by name (default is empty string)\n" +" -D, --description=TEXT export a human-readable description\n" "\n" "Exposing part of the image:\n" " -o, --offset=OFFSET offset into the image\n" @@ -355,8 +354,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, nb_fds++; nbd_update_server_watch(); - nbd_client_new(newproto ? NULL : exp, cioc, - tlscreds, NULL, nbd_client_closed); + nbd_client_new(NULL, cioc, tlscreds, NULL, nbd_client_closed); } static void nbd_update_server_watch(void) @@ -550,7 +548,7 @@ int main(int argc, char **argv) Error *local_err = NULL; BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; QDict *options = NULL; - const char *export_name = NULL; + const char *export_name = ""; /* Default export name */ const char *export_description = NULL; const char *tlscredsid = NULL; bool imageOpts = false; @@ -809,11 +807,6 @@ int main(int argc, char **argv) error_report("TLS is not supported with a host device"); exit(EXIT_FAILURE); } - if (!export_name) { - /* Set the default NBD protocol export name, since - * we *must* use new style protocol for TLS */ - export_name = ""; - } tlscreds = nbd_get_tls_creds(tlscredsid, &local_err); if (local_err) { error_report("Failed to get TLS creds %s", @@ -1014,14 +1007,8 @@ int main(int argc, char **argv) error_report_err(local_err); exit(EXIT_FAILURE); } - if (export_name) { - nbd_export_set_name(exp, export_name); - nbd_export_set_description(exp, export_description); - newproto = true; - } else if (export_description) { - error_report("Export description requires an export name"); - exit(EXIT_FAILURE); - } + nbd_export_set_name(exp, export_name); + nbd_export_set_description(exp, export_description); if (device) { int ret; From 7f7dfe2a53446072c136d349e3150c84d322b2bc Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 3 Oct 2018 20:02:28 +0300 Subject: [PATCH 5/6] nbd/server: drop old-style negotiation After the previous commit, nbd_client_new's first parameter is always NULL. Let's drop it with all corresponding old-style negotiation code path which is unreachable now. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181003170228.95973-3-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake [eblake: re-wrap short line] Signed-off-by: Eric Blake --- blockdev-nbd.c | 3 +-- include/block/nbd.h | 3 +-- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 2 +- 4 files changed, 18 insertions(+), 43 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 1ef11041a7..1d170c80b8 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -36,8 +36,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, gpointer opaque) { qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server"); - nbd_client_new(NULL, cioc, - nbd_server->tlscreds, NULL, + nbd_client_new(cioc, nbd_server->tlscreds, NULL, nbd_blockdev_client_closed); } diff --git a/include/block/nbd.h b/include/block/nbd.h index 4638c839f5..0129d1a4b4 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -308,8 +308,7 @@ void nbd_export_set_name(NBDExport *exp, const char *name); void nbd_export_set_description(NBDExport *exp, const char *description); void nbd_export_close_all(void); -void nbd_client_new(NBDExport *exp, - QIOChannelSocket *sioc, +void nbd_client_new(QIOChannelSocket *sioc, QCryptoTLSCreds *tlscreds, const char *tlsaclname, void (*close_fn)(NBDClient *, bool)); diff --git a/nbd/server.c b/nbd/server.c index 4fb247b116..a1eda0114f 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1253,7 +1253,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_WRITE_ZEROES | NBD_FLAG_SEND_CACHE); - bool oldStyle; /* Old style negotiation header, no room for options [ 0 .. 7] passwd ("NBDMAGIC") @@ -1274,33 +1273,19 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) trace_nbd_negotiate_begin(); memcpy(buf, "NBDMAGIC", 8); - oldStyle = client->exp != NULL && !client->tlscreds; - if (oldStyle) { - trace_nbd_negotiate_old_style(client->exp->size, - client->exp->nbdflags | myflags); - stq_be_p(buf + 8, NBD_CLIENT_MAGIC); - stq_be_p(buf + 16, client->exp->size); - stl_be_p(buf + 24, client->exp->nbdflags | myflags); + stq_be_p(buf + 8, NBD_OPTS_MAGIC); + stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES); - if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) { - error_prepend(errp, "write failed: "); - return -EINVAL; - } - } else { - stq_be_p(buf + 8, NBD_OPTS_MAGIC); - stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES); - - if (nbd_write(client->ioc, buf, 18, errp) < 0) { - error_prepend(errp, "write failed: "); - return -EINVAL; - } - ret = nbd_negotiate_options(client, myflags, errp); - if (ret != 0) { - if (ret < 0) { - error_prepend(errp, "option negotiation failed: "); - } - return ret; + if (nbd_write(client->ioc, buf, 18, errp) < 0) { + error_prepend(errp, "write failed: "); + return -EINVAL; + } + ret = nbd_negotiate_options(client, myflags, errp); + if (ret != 0) { + if (ret < 0) { + error_prepend(errp, "option negotiation failed: "); } + return ret; } assert(!client->optlen); @@ -2396,13 +2381,8 @@ static void nbd_client_receive_next_request(NBDClient *client) static coroutine_fn void nbd_co_client_start(void *opaque) { NBDClient *client = opaque; - NBDExport *exp = client->exp; Error *local_err = NULL; - if (exp) { - nbd_export_get(exp); - QTAILQ_INSERT_TAIL(&exp->clients, client, next); - } qemu_co_mutex_init(&client->send_lock); if (nbd_negotiate(client, &local_err)) { @@ -2417,13 +2397,11 @@ static coroutine_fn void nbd_co_client_start(void *opaque) } /* - * Create a new client listener on the given export @exp, using the - * given channel @sioc. Begin servicing it in a coroutine. When the - * connection closes, call @close_fn with an indication of whether the - * client completed negotiation. + * Create a new client listener using the given channel @sioc. + * Begin servicing it in a coroutine. When the connection closes, call + * @close_fn with an indication of whether the client completed negotiation. */ -void nbd_client_new(NBDExport *exp, - QIOChannelSocket *sioc, +void nbd_client_new(QIOChannelSocket *sioc, QCryptoTLSCreds *tlscreds, const char *tlsaclname, void (*close_fn)(NBDClient *, bool)) @@ -2433,7 +2411,6 @@ void nbd_client_new(NBDExport *exp, client = g_new0(NBDClient, 1); client->refcount = 1; - client->exp = exp; client->tlscreds = tlscreds; if (tlscreds) { object_ref(OBJECT(client->tlscreds)); diff --git a/qemu-nbd.c b/qemu-nbd.c index 6aaebe7d93..e76fe3082a 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -354,7 +354,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, nb_fds++; nbd_update_server_watch(); - nbd_client_new(NULL, cioc, tlscreds, NULL, nbd_client_closed); + nbd_client_new(cioc, tlscreds, NULL, nbd_client_closed); } static void nbd_update_server_watch(void) From df91328adab8490367776d2b21b35d790a606120 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 4 Oct 2018 13:03:13 +0300 Subject: [PATCH 6/6] nbd: fix NBD_FLAG_SEND_CACHE value Commit bc37b06a5 added NBD_CMD_CACHE support, but used the wrong value for NBD_FLAG_SEND_CACHE flag for negotiation. That commit picked bit 8, which had already been assigned by the NBD specification to mean NBD_FLAG_CAN_MULTI_CONN, and which was already implemented in the Linux kernel as a part of stable userspace-kernel API since 4.10: "bit 8, NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates entirely without cache, or that the cache it uses is shared among all connections to the given device. In particular, if this flag is present, then the effects of NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA MUST be visible across all connections when the server sends its reply to that command to the client. In the absense of this flag, clients SHOULD NOT multiplex their commands over more than one connection to the export. ... bit 10, NBD_FLAG_SEND_CACHE: documents that the server understands NBD_CMD_CACHE; however, note that server implementations exist which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact." Consequences: - a client trying to use NBD_CMD_CACHE per the NBD spec will not see the feature as available from a qemu 3.0 server (not fatal, clients already have to be prepared for caching to not exist) - a client accidentally coded to the qemu 3.0 bit value instead of following the spec may interpret NBD_CMD_CACHE as being available when it is not (probably not fatal, the spec says the server should gracefully fail unknown commands, and that clients of NBD_CMD_CACHE should be prepared for failure even when the feature is advertised); such clients are unlikely (perhaps only in unreleased Virtuozzo code), and will disappear over time - a client prepared to use multiple connections based on NBD_FLAG_CAN_MULTI_CONN may cause data corruption when it assumes that caching is consistent when in reality qemu 3.0 did not have a consistent cache. Partially mitigated by using read-only connections (where nothing needs to be flushed, so caching is indeed consistent) or when using qemu-nbd with the default -e 1 (at most one client at a time); visible only when using -e 2 or more for a writable export. Thus the commit fixes negotiation flag in QEMU according to the specification. Signed-off-by: Denis V. Lunev CC: Vladimir Sementsov-Ogievskiy CC: Valery Vdovin CC: Eric Blake CC: Paolo Bonzini CC: qemu-stable@nongnu.org Message-Id: <20181004100313.4253-1-den@openvz.org> Reviewed-by: Eric Blake [eblake: enhance commit message, add defines for unimplemented flags] Signed-off-by: Eric Blake --- include/block/nbd.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index 0129d1a4b4..6a5bfe5d55 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -135,7 +135,9 @@ typedef struct NBDExtent { #define NBD_FLAG_SEND_TRIM (1 << 5) /* Send TRIM (discard) */ #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */ #define NBD_FLAG_SEND_DF (1 << 7) /* Send DF (Do not Fragment) */ -#define NBD_FLAG_SEND_CACHE (1 << 8) /* Send CACHE (prefetch) */ +#define NBD_FLAG_CAN_MULTI_CONN (1 << 8) /* Multi-client cache consistent */ +#define NBD_FLAG_SEND_RESIZE (1 << 9) /* Send resize */ +#define NBD_FLAG_SEND_CACHE (1 << 10) /* Send CACHE (prefetch) */ /* New-style handshake (global) flags, sent from server to client, and control what will happen during handshake phase. */