* [PATCH v2 0/2] Better NBD string length handling @ 2019-10-10 21:00 Eric Blake 2019-10-10 21:00 ` [PATCH v2 1/2] nbd: Don't send oversize strings Eric Blake 2019-10-10 21:00 ` [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev Eric Blake 0 siblings, 2 replies; 10+ messages in thread From: Eric Blake @ 2019-10-10 21:00 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, vsementsov, qemu-block, mlevitsk Patch 1 is a revised version after Maxim's comments - it moves the length checks earlier into the system (for cleaner error messages as soon as possible) and adds asserts at the later points that are now guaranteed by the earlier checks. It also covers more string handling, both in the client and in the server, by ensuring that outgoing strings are properly constrained and incoming strings are checked for validity before blind use. Patch 2 is a new patch, written to make testing of description strings in patch 1 easier. Eric Blake (2): nbd: Don't send oversize strings nbd: Allow description when creating NBD blockdev qapi/block.json | 8 +++++--- include/block/nbd.h | 1 + block/nbd.c | 9 +++++++++ blockdev-nbd.c | 14 +++++++++++++- monitor/hmp-cmds.c | 4 ++-- nbd/client.c | 16 +++++++++++++--- nbd/server.c | 14 ++++++++++++-- qemu-nbd.c | 9 +++++++++ tests/qemu-iotests/223 | 2 +- tests/qemu-iotests/223.out | 1 + 10 files changed, 66 insertions(+), 12 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] nbd: Don't send oversize strings 2019-10-10 21:00 [PATCH v2 0/2] Better NBD string length handling Eric Blake @ 2019-10-10 21:00 ` Eric Blake 2019-10-11 7:32 ` Vladimir Sementsov-Ogievskiy 2019-10-10 21:00 ` [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev Eric Blake 1 sibling, 1 reply; 10+ messages in thread From: Eric Blake @ 2019-10-10 21:00 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, vsementsov, Max Reitz, qemu-block, mlevitsk Qemu as server currently won't accept export names larger than 256 bytes, nor create dirty bitmap names longer than 1023 bytes, so most uses of qemu as client or server have no reason to get anywhere near the NBD spec maximum of a 4k limit per string. However, we weren't actually enforcing things, ignoring when the remote side violates the protocol on input, and also having several code paths where we send oversize strings on output (for example, qemu-nbd --description could easily send more than 4k). Tighten things up as follows: client: - Perform bounds check on export name and dirty bitmap request prior to handing it to server - Validate that copied server replies are not too long (ignoring NBD_INFO_* replies that are not copied is not too bad) server: - Perform bounds check on export name and description prior to advertising it to client - Reject client name or metadata query that is too long Signed-off-by: Eric Blake <eblake@redhat.com> --- include/block/nbd.h | 1 + block/nbd.c | 9 +++++++++ blockdev-nbd.c | 5 +++++ nbd/client.c | 16 +++++++++++++--- nbd/server.c | 14 ++++++++++++-- qemu-nbd.c | 9 +++++++++ 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index 316fd705a9e4..fcabdf0f37c3 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -232,6 +232,7 @@ enum { * going larger would require an audit of more code to make sure we * aren't overflowing some other buffer. */ #define NBD_MAX_NAME_SIZE 256 +#define NBD_MAX_STRING_SIZE 4096 /* Two types of reply structures */ #define NBD_SIMPLE_REPLY_MAGIC 0x67446698 diff --git a/block/nbd.c b/block/nbd.c index 813c40d8f067..76eb1dbe04df 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1621,6 +1621,10 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, } s->export = g_strdup(qemu_opt_get(opts, "export")); + if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) { + error_setg(errp, "export name too long to send to server"); + goto error; + } s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds")); if (s->tlscredsid) { @@ -1638,6 +1642,11 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, } s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap")); + if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > NBD_MAX_STRING_SIZE) { + error_setg(errp, "x_dirty_bitmap query too long to send to server"); + goto error; + } + s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0); ret = 0; diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 6a8b206e1d74..8c20baa4a4b9 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, name = device; } + if (strlen(name) > NBD_MAX_STRING_SIZE) { + error_setg(errp, "export name '%s' too long", name); + return; + } + if (nbd_export_find(name)) { error_setg(errp, "NBD server already has export named '%s'", name); return; diff --git a/nbd/client.c b/nbd/client.c index f6733962b49b..d6e29daced63 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, return -1; } len -= sizeof(namelen); - if (len < namelen) { - error_setg(errp, "incorrect option name length"); + if (len < namelen || namelen > NBD_MAX_STRING_SIZE) { + error_setg(errp, "incorrect list name length"); nbd_send_opt_abort(ioc); return -1; } @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, local_name[namelen] = '\0'; len -= namelen; if (len) { + if (len > NBD_MAX_STRING_SIZE) { + error_setg(errp, "incorrect list description length"); + nbd_send_opt_abort(ioc); + return -1; + } local_desc = g_malloc(len + 1); if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) { nbd_send_opt_abort(ioc); @@ -479,6 +484,10 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt, break; default: + /* + * Not worth the bother to check if NBD_INFO_NAME or + * NBD_INFO_DESCRIPTION exceed NBD_MAX_STRING_SIZE. + */ trace_nbd_opt_info_unknown(type, nbd_info_lookup(type)); if (nbd_drop(ioc, len, errp) < 0) { error_prepend(errp, "Failed to read info payload: "); @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt, if (query) { query_len = strlen(query); data_len += sizeof(query_len) + query_len; + assert(query_len <= NBD_MAX_STRING_SIZE); } else { assert(opt == NBD_OPT_LIST_META_CONTEXT); } @@ -1009,7 +1019,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, bool zeroes; bool base_allocation = info->base_allocation; - assert(info->name); + assert(info->name && strlen(info->name) <= NBD_MAX_STRING_SIZE); trace_nbd_receive_negotiate_name(info->name); result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc, diff --git a/nbd/server.c b/nbd/server.c index d8d1e6245532..dfbefd5a1ebc 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -375,6 +375,7 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, trace_nbd_negotiate_send_rep_list(name, desc); name_len = strlen(name); desc_len = strlen(desc); + assert(name_len <= NBD_MAX_STRING_SIZE && desc_len <= NBD_MAX_STRING_SIZE); len = name_len + desc_len + sizeof(len); ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp); if (ret < 0) { @@ -608,6 +609,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) if (exp->description) { size_t len = strlen(exp->description); + assert(len <= NBD_MAX_STRING_SIZE); rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION, len, exp->description, errp); if (rc < 0) { @@ -752,6 +754,7 @@ static int nbd_negotiate_send_meta_context(NBDClient *client, {.iov_base = (void *)context, .iov_len = strlen(context)} }; + assert(iov[1].iov_len <= NBD_MAX_STRING_SIZE); if (client->opt == NBD_OPT_LIST_META_CONTEXT) { context_id = 0; } @@ -900,7 +903,7 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, * Parse namespace name and call corresponding function to parse body of the * query. * - * The only supported namespace now is 'base'. + * The only supported namespaces are 'base' and 'qemu'. * * The function aims not wasting time and memory to read long unknown namespace * names. @@ -926,6 +929,10 @@ static int nbd_negotiate_meta_query(NBDClient *client, } len = cpu_to_be32(len); + if (len > NBD_MAX_STRING_SIZE) { + trace_nbd_negotiate_meta_query_skip("length too long"); + return nbd_opt_skip(client, len, errp); + } if (len < ns_len) { trace_nbd_negotiate_meta_query_skip("length too short"); return nbd_opt_skip(client, len, errp); @@ -1487,7 +1494,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, * access since the export could be available before migration handover. * ctx was acquired in the caller. */ - assert(name); + assert(name && strlen(name) <= NBD_MAX_STRING_SIZE); ctx = bdrv_get_aio_context(bs); bdrv_invalidate_cache(bs, NULL); @@ -1513,6 +1520,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, assert(dev_offset <= INT64_MAX); exp->dev_offset = dev_offset; exp->name = g_strdup(name); + assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE); exp->description = g_strdup(desc); exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE); @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, exp->export_bitmap = bm; exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", bitmap); + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */ + assert(strlen(exp->export_bitmap_context) <= NBD_MAX_STRING_SIZE); } exp->close = close; diff --git a/qemu-nbd.c b/qemu-nbd.c index 9032b6de2ace..55ce69b141f0 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -826,9 +826,18 @@ int main(int argc, char **argv) break; case 'x': export_name = optarg; + if (strlen(export_name) > NBD_MAX_STRING_SIZE) { + error_report("export name '%s' too long", export_name); + exit(EXIT_FAILURE); + } break; case 'D': export_description = optarg; + if (strlen(export_description) > NBD_MAX_STRING_SIZE) { + error_report("export description '%s' too long", + export_description); + exit(EXIT_FAILURE); + } break; case 'v': verbose = 1; -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] nbd: Don't send oversize strings 2019-10-10 21:00 ` [PATCH v2 1/2] nbd: Don't send oversize strings Eric Blake @ 2019-10-11 7:32 ` Vladimir Sementsov-Ogievskiy 2019-10-15 15:07 ` Eric Blake 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-11 7:32 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: kwolf, Max Reitz, qemu-block, mlevitsk 11.10.2019 0:00, Eric Blake wrote: > Qemu as server currently won't accept export names larger than 256 > bytes, nor create dirty bitmap names longer than 1023 bytes, so most > uses of qemu as client or server have no reason to get anywhere near > the NBD spec maximum of a 4k limit per string. > > However, we weren't actually enforcing things, ignoring when the > remote side violates the protocol on input, and also having several > code paths where we send oversize strings on output (for example, > qemu-nbd --description could easily send more than 4k). Tighten > things up as follows: > > client: > - Perform bounds check on export name and dirty bitmap request prior > to handing it to server > - Validate that copied server replies are not too long (ignoring > NBD_INFO_* replies that are not copied is not too bad) > server: > - Perform bounds check on export name and description prior to > advertising it to client > - Reject client name or metadata query that is too long > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/block/nbd.h | 1 + > block/nbd.c | 9 +++++++++ > blockdev-nbd.c | 5 +++++ > nbd/client.c | 16 +++++++++++++--- > nbd/server.c | 14 ++++++++++++-- > qemu-nbd.c | 9 +++++++++ > 6 files changed, 49 insertions(+), 5 deletions(-) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 316fd705a9e4..fcabdf0f37c3 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -232,6 +232,7 @@ enum { > * going larger would require an audit of more code to make sure we > * aren't overflowing some other buffer. */ This comment says, that we restrict export name to 256... > #define NBD_MAX_NAME_SIZE 256 > +#define NBD_MAX_STRING_SIZE 4096 > > /* Two types of reply structures */ > #define NBD_SIMPLE_REPLY_MAGIC 0x67446698 > diff --git a/block/nbd.c b/block/nbd.c > index 813c40d8f067..76eb1dbe04df 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -1621,6 +1621,10 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, > } > > s->export = g_strdup(qemu_opt_get(opts, "export")); > + if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) { > + error_setg(errp, "export name too long to send to server"); > + goto error; > + } > > s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds")); > if (s->tlscredsid) { > @@ -1638,6 +1642,11 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, > } > > s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap")); > + if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > NBD_MAX_STRING_SIZE) { > + error_setg(errp, "x_dirty_bitmap query too long to send to server"); > + goto error; this is new latest "goto error", you should add g_free(s->x_dirty_bitmap) in following "if (ret < 0)" > + } > + > s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0); > > ret = 0; > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 6a8b206e1d74..8c20baa4a4b9 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > name = device; > } > > + if (strlen(name) > NBD_MAX_STRING_SIZE) { > + error_setg(errp, "export name '%s' too long", name); > + return; > + } Hmmm, no, so here we restrict to 4096, but, we will not allow client to request more than 256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE and do the audit mentioned in the comment above its definition. > + > if (nbd_export_find(name)) { > error_setg(errp, "NBD server already has export named '%s'", name); > return; > diff --git a/nbd/client.c b/nbd/client.c > index f6733962b49b..d6e29daced63 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, > return -1; > } > len -= sizeof(namelen); > - if (len < namelen) { > - error_setg(errp, "incorrect option name length"); > + if (len < namelen || namelen > NBD_MAX_STRING_SIZE) { > + error_setg(errp, "incorrect list name length"); New wording made me go above and read the comment, what functions does. Comment is good, but without it, it sounds like name of the list for me... > nbd_send_opt_abort(ioc); > return -1; > } > @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, > local_name[namelen] = '\0'; > len -= namelen; > if (len) { > + if (len > NBD_MAX_STRING_SIZE) { > + error_setg(errp, "incorrect list description length"); > + nbd_send_opt_abort(ioc); > + return -1; > + } > local_desc = g_malloc(len + 1); > if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) { > nbd_send_opt_abort(ioc); > @@ -479,6 +484,10 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt, > break; > > default: > + /* > + * Not worth the bother to check if NBD_INFO_NAME or > + * NBD_INFO_DESCRIPTION exceed NBD_MAX_STRING_SIZE. > + */ > trace_nbd_opt_info_unknown(type, nbd_info_lookup(type)); > if (nbd_drop(ioc, len, errp) < 0) { > error_prepend(errp, "Failed to read info payload: "); > @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt, > if (query) { > query_len = strlen(query); > data_len += sizeof(query_len) + query_len; > + assert(query_len <= NBD_MAX_STRING_SIZE); > } else { > assert(opt == NBD_OPT_LIST_META_CONTEXT); > } you may assert export_len as well.. > @@ -1009,7 +1019,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, > bool zeroes; > bool base_allocation = info->base_allocation; > > - assert(info->name); > + assert(info->name && strlen(info->name) <= NBD_MAX_STRING_SIZE); > trace_nbd_receive_negotiate_name(info->name); > > result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc, > diff --git a/nbd/server.c b/nbd/server.c > index d8d1e6245532..dfbefd5a1ebc 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -375,6 +375,7 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, > trace_nbd_negotiate_send_rep_list(name, desc); > name_len = strlen(name); > desc_len = strlen(desc); > + assert(name_len <= NBD_MAX_STRING_SIZE && desc_len <= NBD_MAX_STRING_SIZE); > len = name_len + desc_len + sizeof(len); > ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp); > if (ret < 0) { > @@ -608,6 +609,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) > if (exp->description) { > size_t len = strlen(exp->description); > > + assert(len <= NBD_MAX_STRING_SIZE); > rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION, > len, exp->description, errp); > if (rc < 0) { > @@ -752,6 +754,7 @@ static int nbd_negotiate_send_meta_context(NBDClient *client, > {.iov_base = (void *)context, .iov_len = strlen(context)} > }; > > + assert(iov[1].iov_len <= NBD_MAX_STRING_SIZE); > if (client->opt == NBD_OPT_LIST_META_CONTEXT) { > context_id = 0; > } > @@ -900,7 +903,7 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta, > * Parse namespace name and call corresponding function to parse body of the > * query. > * > - * The only supported namespace now is 'base'. > + * The only supported namespaces are 'base' and 'qemu'. > * > * The function aims not wasting time and memory to read long unknown namespace > * names. > @@ -926,6 +929,10 @@ static int nbd_negotiate_meta_query(NBDClient *client, > } > len = cpu_to_be32(len); > > + if (len > NBD_MAX_STRING_SIZE) { > + trace_nbd_negotiate_meta_query_skip("length too long"); > + return nbd_opt_skip(client, len, errp); > + } > if (len < ns_len) { > trace_nbd_negotiate_meta_query_skip("length too short"); > return nbd_opt_skip(client, len, errp); > @@ -1487,7 +1494,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, > * access since the export could be available before migration handover. > * ctx was acquired in the caller. > */ > - assert(name); > + assert(name && strlen(name) <= NBD_MAX_STRING_SIZE); > ctx = bdrv_get_aio_context(bs); > bdrv_invalidate_cache(bs, NULL); > > @@ -1513,6 +1520,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, > assert(dev_offset <= INT64_MAX); > exp->dev_offset = dev_offset; > exp->name = g_strdup(name); > + assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE); > exp->description = g_strdup(desc); > exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH | > NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE); > @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, > exp->export_bitmap = bm; > exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", > bitmap); > + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */ Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But for non-persistent name length is actually unlimited. So, we should either limit all bitmap names to 1023 (hope, this will not break existing scenarios) or error out here (or earlier) instead of assertion. We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1) > + assert(strlen(exp->export_bitmap_context) <= NBD_MAX_STRING_SIZE); > } > > exp->close = close; > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 9032b6de2ace..55ce69b141f0 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -826,9 +826,18 @@ int main(int argc, char **argv) > break; > case 'x': > export_name = optarg; > + if (strlen(export_name) > NBD_MAX_STRING_SIZE) { > + error_report("export name '%s' too long", export_name); > + exit(EXIT_FAILURE); > + } > break; > case 'D': > export_description = optarg; > + if (strlen(export_description) > NBD_MAX_STRING_SIZE) { > + error_report("export description '%s' too long", > + export_description); > + exit(EXIT_FAILURE); > + } > break; > case 'v': > verbose = 1; > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] nbd: Don't send oversize strings 2019-10-11 7:32 ` Vladimir Sementsov-Ogievskiy @ 2019-10-15 15:07 ` Eric Blake 2019-10-15 16:16 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 10+ messages in thread From: Eric Blake @ 2019-10-15 15:07 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel Cc: kwolf, Max Reitz, qemu-block, mlevitsk On 10/11/19 2:32 AM, Vladimir Sementsov-Ogievskiy wrote: > 11.10.2019 0:00, Eric Blake wrote: >> Qemu as server currently won't accept export names larger than 256 >> bytes, nor create dirty bitmap names longer than 1023 bytes, so most >> uses of qemu as client or server have no reason to get anywhere near >> the NBD spec maximum of a 4k limit per string. >> >> However, we weren't actually enforcing things, ignoring when the >> remote side violates the protocol on input, and also having several >> code paths where we send oversize strings on output (for example, >> qemu-nbd --description could easily send more than 4k). Tighten >> things up as follows: >> >> client: >> - Perform bounds check on export name and dirty bitmap request prior >> to handing it to server >> - Validate that copied server replies are not too long (ignoring >> NBD_INFO_* replies that are not copied is not too bad) >> server: >> - Perform bounds check on export name and description prior to >> advertising it to client >> - Reject client name or metadata query that is too long >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> +++ b/include/block/nbd.h >> @@ -232,6 +232,7 @@ enum { >> * going larger would require an audit of more code to make sure we >> * aren't overflowing some other buffer. */ > > This comment says, that we restrict export name to 256... Yes, because we still stack-allocate the name in places, but 4k is too large for stack allocation. But we're inconsistent on where we use the smaller 256-limit; the server won't serve an image that large, but doesn't prevent a client from requesting a 4k name export (even though that export will not be present). >> +++ b/blockdev-nbd.c >> @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, >> name = device; >> } >> >> + if (strlen(name) > NBD_MAX_STRING_SIZE) { >> + error_setg(errp, "export name '%s' too long", name); >> + return; >> + } > > Hmmm, no, so here we restrict to 4096, but, we will not allow client to request more than > 256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE and do the > audit mentioned in the comment above its definition. Yeah, I guess it's time to just get rid of NBD_MAX_NAME_SIZE, and move away from stack allocations. Should I do that as a followup to this patch, or spin a v3? >> +++ b/nbd/client.c >> @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, >> return -1; >> } >> len -= sizeof(namelen); >> - if (len < namelen) { >> - error_setg(errp, "incorrect option name length"); >> + if (len < namelen || namelen > NBD_MAX_STRING_SIZE) { >> + error_setg(errp, "incorrect list name length"); > > New wording made me go above and read the comment, what functions does. Comment is good, but without > it, it sounds like name of the list for me... Maybe: incorrect name length in server's list response > >> nbd_send_opt_abort(ioc); >> return -1; >> } >> @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, >> local_name[namelen] = '\0'; >> len -= namelen; >> if (len) { >> + if (len > NBD_MAX_STRING_SIZE) { >> + error_setg(errp, "incorrect list description length"); and incorrect description length in server's list response >> @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt, >> if (query) { >> query_len = strlen(query); >> data_len += sizeof(query_len) + query_len; >> + assert(query_len <= NBD_MAX_STRING_SIZE); >> } else { >> assert(opt == NBD_OPT_LIST_META_CONTEXT); >> } > > you may assert export_len as well.. It was asserted earlier, but doing it again might not hurt, especially if I do the followup patch getting rid of NBD_MAX_NAME_SIZE >> @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, >> exp->export_bitmap = bm; >> exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", >> bitmap); >> + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */ > > Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But for non-persistent > name length is actually unlimited. So, we should either limit all bitmap names to 1023 (hope, > this will not break existing scenarios) or error out here (or earlier) instead of assertion. I'm leaning towards limiting ALL bitmaps to the same length (as we've already debated the idea of being able to convert an existing bitmap from transient to persistent). > > We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1) Except that BME_MAX_NAME_SIZE is not (currently) in a public .h file. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] nbd: Don't send oversize strings 2019-10-15 15:07 ` Eric Blake @ 2019-10-15 16:16 ` Vladimir Sementsov-Ogievskiy 2019-11-04 17:41 ` Maxim Levitsky 2019-11-13 15:47 ` Eric Blake 0 siblings, 2 replies; 10+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-15 16:16 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: kwolf, Max Reitz, qemu-block, mlevitsk 15.10.2019 18:07, Eric Blake wrote: > On 10/11/19 2:32 AM, Vladimir Sementsov-Ogievskiy wrote: >> 11.10.2019 0:00, Eric Blake wrote: >>> Qemu as server currently won't accept export names larger than 256 >>> bytes, nor create dirty bitmap names longer than 1023 bytes, so most >>> uses of qemu as client or server have no reason to get anywhere near >>> the NBD spec maximum of a 4k limit per string. >>> >>> However, we weren't actually enforcing things, ignoring when the >>> remote side violates the protocol on input, and also having several >>> code paths where we send oversize strings on output (for example, >>> qemu-nbd --description could easily send more than 4k). Tighten >>> things up as follows: >>> >>> client: >>> - Perform bounds check on export name and dirty bitmap request prior >>> to handing it to server >>> - Validate that copied server replies are not too long (ignoring >>> NBD_INFO_* replies that are not copied is not too bad) >>> server: >>> - Perform bounds check on export name and description prior to >>> advertising it to client >>> - Reject client name or metadata query that is too long >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- > >>> +++ b/include/block/nbd.h >>> @@ -232,6 +232,7 @@ enum { >>> * going larger would require an audit of more code to make sure we >>> * aren't overflowing some other buffer. */ >> >> This comment says, that we restrict export name to 256... > > Yes, because we still stack-allocate the name in places, but 4k is too large for stack allocation. But we're inconsistent on where we use the smaller 256-limit; the server won't serve an image that large, but doesn't prevent a client from requesting a 4k name export (even though that export will not be present). > > >>> +++ b/blockdev-nbd.c >>> @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, >>> name = device; >>> } >>> >>> + if (strlen(name) > NBD_MAX_STRING_SIZE) { >>> + error_setg(errp, "export name '%s' too long", name); >>> + return; >>> + } >> >> Hmmm, no, so here we restrict to 4096, but, we will not allow client to request more than >> 256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE and do the >> audit mentioned in the comment above its definition. > > Yeah, I guess it's time to just get rid of NBD_MAX_NAME_SIZE, and move away from stack allocations. Should I do that as a followup to this patch, or spin a v3? Hmm. It's OK too. With - fixed mem-leak in nbd_process_options - s/x_dirty_bitmap/x-dirty-bitmap in nbd_process_options in error message - following yours new wordings Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> However, this patch introduces possible crash point, asserting on bitmap name below, so it would better be fixed before this patch or immediately after it.. Still, it's unlikely to have a bitmap with name longer than 4k.. > >>> +++ b/nbd/client.c >>> @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, >>> return -1; >>> } >>> len -= sizeof(namelen); >>> - if (len < namelen) { >>> - error_setg(errp, "incorrect option name length"); >>> + if (len < namelen || namelen > NBD_MAX_STRING_SIZE) { >>> + error_setg(errp, "incorrect list name length"); >> >> New wording made me go above and read the comment, what functions does. Comment is good, but without >> it, it sounds like name of the list for me... > > Maybe: > > incorrect name length in server's list response Yes, this is better, thanks > >> >>> nbd_send_opt_abort(ioc); >>> return -1; >>> } >>> @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, >>> local_name[namelen] = '\0'; >>> len -= namelen; >>> if (len) { >>> + if (len > NBD_MAX_STRING_SIZE) { >>> + error_setg(errp, "incorrect list description length"); > > and > > incorrect description length in server's list response > > >>> @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt, >>> if (query) { >>> query_len = strlen(query); >>> data_len += sizeof(query_len) + query_len; >>> + assert(query_len <= NBD_MAX_STRING_SIZE); >>> } else { >>> assert(opt == NBD_OPT_LIST_META_CONTEXT); >>> } >> >> you may assert export_len as well.. > > It was asserted earlier, but doing it again might not hurt, especially if I do the followup patch getting rid of NBD_MAX_NAME_SIZE > > >>> @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, >>> exp->export_bitmap = bm; >>> exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", >>> bitmap); >>> + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */ >> >> Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But for non-persistent >> name length is actually unlimited. So, we should either limit all bitmap names to 1023 (hope, >> this will not break existing scenarios) or error out here (or earlier) instead of assertion. > > I'm leaning towards limiting ALL bitmaps to the same length (as we've already debated the idea of being able to convert an existing bitmap from transient to persistent). Agreed, but .. > >> >> We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1) > > Except that BME_MAX_NAME_SIZE is not (currently) in a public .h file. > .. I think, than it should be new BLOCK_DIRTY_BITMAP_MAX_NAME_SIZE.. And we'll have to note it in qapi doc.. Should this change go through deprecation? Or we consider non-persistent bitmaps as something not really useful? -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] nbd: Don't send oversize strings 2019-10-15 16:16 ` Vladimir Sementsov-Ogievskiy @ 2019-11-04 17:41 ` Maxim Levitsky 2019-11-13 15:47 ` Eric Blake 1 sibling, 0 replies; 10+ messages in thread From: Maxim Levitsky @ 2019-11-04 17:41 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, Eric Blake, qemu-devel Cc: kwolf, qemu-block, Max Reitz On Tue, 2019-10-15 at 16:16 +0000, Vladimir Sementsov-Ogievskiy wrote: > 15.10.2019 18:07, Eric Blake wrote: > > On 10/11/19 2:32 AM, Vladimir Sementsov-Ogievskiy wrote: > > > 11.10.2019 0:00, Eric Blake wrote: > > > > Qemu as server currently won't accept export names larger than 256 > > > > bytes, nor create dirty bitmap names longer than 1023 bytes, so most > > > > uses of qemu as client or server have no reason to get anywhere near > > > > the NBD spec maximum of a 4k limit per string. > > > > > > > > However, we weren't actually enforcing things, ignoring when the > > > > remote side violates the protocol on input, and also having several > > > > code paths where we send oversize strings on output (for example, > > > > qemu-nbd --description could easily send more than 4k). Tighten > > > > things up as follows: > > > > > > > > client: > > > > - Perform bounds check on export name and dirty bitmap request prior > > > > to handing it to server > > > > - Validate that copied server replies are not too long (ignoring > > > > NBD_INFO_* replies that are not copied is not too bad) > > > > server: > > > > - Perform bounds check on export name and description prior to > > > > advertising it to client > > > > - Reject client name or metadata query that is too long > > > > > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > > --- > > > > +++ b/include/block/nbd.h > > > > @@ -232,6 +232,7 @@ enum { > > > > * going larger would require an audit of more code to make sure we > > > > * aren't overflowing some other buffer. */ > > > > > > This comment says, that we restrict export name to 256... > > > > Yes, because we still stack-allocate the name in places, but 4k is too large for stack allocation. But we're inconsistent on where we use the smaller 256-limit; the server won't serve an image > > that large, but doesn't prevent a client from requesting a 4k name export (even though that export will not be present). > > > > > > > > +++ b/blockdev-nbd.c > > > > @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > > > > name = device; > > > > } > > > > > > > > + if (strlen(name) > NBD_MAX_STRING_SIZE) { > > > > + error_setg(errp, "export name '%s' too long", name); > > > > + return; > > > > + } > > > > > > Hmmm, no, so here we restrict to 4096, but, we will not allow client to request more than > > > 256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE and do the > > > audit mentioned in the comment above its definition. > > > > Yeah, I guess it's time to just get rid of NBD_MAX_NAME_SIZE, and move away from stack allocations. Should I do that as a followup to this patch, or spin a v3? > > Hmm. It's OK too. > > With > - fixed mem-leak in nbd_process_options > - s/x_dirty_bitmap/x-dirty-bitmap in nbd_process_options in error message > - following yours new wordings > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > However, this patch introduces possible crash point, asserting on bitmap name below, so it would better > be fixed before this patch or immediately after it.. Still, it's unlikely to have a bitmap with name > longer than 4k.. > > > > > > > +++ b/nbd/client.c > > > > @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, > > > > return -1; > > > > } > > > > len -= sizeof(namelen); > > > > - if (len < namelen) { > > > > - error_setg(errp, "incorrect option name length"); > > > > + if (len < namelen || namelen > NBD_MAX_STRING_SIZE) { > > > > + error_setg(errp, "incorrect list name length"); > > > > > > New wording made me go above and read the comment, what functions does. Comment is good, but without > > > it, it sounds like name of the list for me... > > > > Maybe: > > > > incorrect name length in server's list response > > Yes, this is better, thanks > > > > > > > > > > nbd_send_opt_abort(ioc); > > > > return -1; > > > > } > > > > @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, > > > > local_name[namelen] = '\0'; > > > > len -= namelen; > > > > if (len) { > > > > + if (len > NBD_MAX_STRING_SIZE) { > > > > + error_setg(errp, "incorrect list description length"); > > > > and > > > > incorrect description length in server's list response > > > > > > > > @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt, > > > > if (query) { > > > > query_len = strlen(query); > > > > data_len += sizeof(query_len) + query_len; > > > > + assert(query_len <= NBD_MAX_STRING_SIZE); > > > > } else { > > > > assert(opt == NBD_OPT_LIST_META_CONTEXT); > > > > } > > > > > > you may assert export_len as well.. > > > > It was asserted earlier, but doing it again might not hurt, especially if I do the followup patch getting rid of NBD_MAX_NAME_SIZE > > > > > > > > @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, > > > > exp->export_bitmap = bm; > > > > exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", > > > > bitmap); > > > > + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */ > > > > > > Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But for non-persistent > > > name length is actually unlimited. So, we should either limit all bitmap names to 1023 (hope, > > > this will not break existing scenarios) or error out here (or earlier) instead of assertion. > > > > I'm leaning towards limiting ALL bitmaps to the same length (as we've already debated the idea of being able to convert an existing bitmap from transient to persistent). > > Agreed, but .. > > > > > > > > > We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1) > > > > Except that BME_MAX_NAME_SIZE is not (currently) in a public .h file. > > > > .. I think, than it should be new BLOCK_DIRTY_BITMAP_MAX_NAME_SIZE.. And we'll have to note it in qapi doc.. > Should this change go through deprecation? Or we consider non-persistent bitmaps as something not really useful? > > -- > Best regards, > Vladimir I followed upon the new patch and the comments, and it seems ok now to me, (including the comments that were already made) but I haven't checked if there are more cases of missing length checks. Best regards, Maxim Levitsky ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] nbd: Don't send oversize strings 2019-10-15 16:16 ` Vladimir Sementsov-Ogievskiy 2019-11-04 17:41 ` Maxim Levitsky @ 2019-11-13 15:47 ` Eric Blake 1 sibling, 0 replies; 10+ messages in thread From: Eric Blake @ 2019-11-13 15:47 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel Cc: kwolf, Max Reitz, qemu-block, mlevitsk On 10/15/19 11:16 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, >>>> exp->export_bitmap = bm; >>>> exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", >>>> bitmap); >>>> + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */ >>> >>> Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But for non-persistent >>> name length is actually unlimited. So, we should either limit all bitmap names to 1023 (hope, >>> this will not break existing scenarios) or error out here (or earlier) instead of assertion. I'm seriously doubting that any existing scenarios try to use a name that long. If no one was relying on a long name (especially since it was inconsistent between persistent being limited to qcow2 constraints and non-persistent having no limit), we can consider it as a bug-fix rather than something needing a deprecation period. >> >> I'm leaning towards limiting ALL bitmaps to the same length (as we've already debated the idea of being able to convert an existing bitmap from transient to persistent). > > Agreed, but .. > >> >>> >>> We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1) >> >> Except that BME_MAX_NAME_SIZE is not (currently) in a public .h file. >> > > .. I think, than it should be new BLOCK_DIRTY_BITMAP_MAX_NAME_SIZE.. And we'll have to note it in qapi doc.. > Should this change go through deprecation? Or we consider non-persistent bitmaps as something not really useful? I'm preparing a v3 patch that just goes ahead and adds the limit on bitmap names everywhere, as a separate patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev 2019-10-10 21:00 [PATCH v2 0/2] Better NBD string length handling Eric Blake 2019-10-10 21:00 ` [PATCH v2 1/2] nbd: Don't send oversize strings Eric Blake @ 2019-10-10 21:00 ` Eric Blake 2019-10-11 7:41 ` Vladimir Sementsov-Ogievskiy 2019-11-04 17:53 ` Maxim Levitsky 1 sibling, 2 replies; 10+ messages in thread From: Eric Blake @ 2019-10-10 21:00 UTC (permalink / raw) To: qemu-devel Cc: kwolf, vsementsov, qemu-block, Markus Armbruster, Dr. David Alan Gilbert, Max Reitz, mlevitsk Allow blockdevs to match the feature already present in qemu-nbd -D. Enhance iotest 223 to cover it. Signed-off-by: Eric Blake <eblake@redhat.com> --- qapi/block.json | 8 +++++--- blockdev-nbd.c | 9 ++++++++- monitor/hmp-cmds.c | 4 ++-- tests/qemu-iotests/223 | 2 +- tests/qemu-iotests/223.out | 1 + 5 files changed, 17 insertions(+), 7 deletions(-) diff --git a/qapi/block.json b/qapi/block.json index 145c268bb646..a6617b5bd03a 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -250,9 +250,11 @@ # @name: Export name. If unspecified, the @device parameter is used as the # export name. (Since 2.12) # +# @description: Free-form description of the export. (Since 4.2) +# # @writable: Whether clients should be able to write to the device via the # NBD connection (default false). - +# # @bitmap: Also export the dirty bitmap reachable from @device, so the # NBD client can use NBD_OPT_SET_META_CONTEXT with # "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) @@ -263,8 +265,8 @@ # Since: 1.3.0 ## { 'command': 'nbd-server-add', - 'data': {'device': 'str', '*name': 'str', '*writable': 'bool', - '*bitmap': 'str' } } + 'data': {'device': 'str', '*name': 'str', '*description': 'str', + '*writable': 'bool', '*bitmap': 'str' } } ## # @NbdServerRemoveMode: diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 8c20baa4a4b9..de2f2ff71320 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -144,6 +144,7 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr, } void qmp_nbd_server_add(const char *device, bool has_name, const char *name, + bool has_description, const char *description, bool has_writable, bool writable, bool has_bitmap, const char *bitmap, Error **errp) { @@ -167,6 +168,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, return; } + if (has_description && strlen(description) > NBD_MAX_STRING_SIZE) { + error_setg(errp, "description '%s' too long", description); + return; + } + if (nbd_export_find(name)) { error_setg(errp, "NBD server already has export named '%s'", name); return; @@ -195,7 +201,8 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, writable = false; } - exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable, + exp = nbd_export_new(bs, 0, len, name, description, bitmap, + !writable, !writable, NULL, false, on_eject_blk, errp); if (!exp) { goto out; diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index b2551c16d129..574c6321c9d0 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -2352,7 +2352,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) continue; } - qmp_nbd_server_add(info->value->device, false, NULL, + qmp_nbd_server_add(info->value->device, false, NULL, false, NULL, true, writable, false, NULL, &local_err); if (local_err != NULL) { @@ -2374,7 +2374,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) bool writable = qdict_get_try_bool(qdict, "writable", false); Error *local_err = NULL; - qmp_nbd_server_add(device, !!name, name, true, writable, + qmp_nbd_server_add(device, !!name, name, false, NULL, true, writable, false, NULL, &local_err); hmp_handle_error(mon, &local_err); } diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index 2ba3d8124b4f..06bdc96be48f 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -144,7 +144,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", "bitmap":"b3"}}' "error" # Missing bitmap _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "writable":true, - "bitmap":"b2"}}' "return" + "description":"some text", "bitmap":"b2"}}' "return" $QEMU_NBD_PROG -L -k "$TEST_DIR/nbd" echo diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 23b34fcd202e..16d597585b4f 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -49,6 +49,7 @@ exports available: 2 base:allocation qemu:dirty-bitmap:b export: 'n2' + description: some text size: 4194304 flags: 0xced ( flush fua trim zeroes df cache fast-zero ) min block: 1 -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev 2019-10-10 21:00 ` [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev Eric Blake @ 2019-10-11 7:41 ` Vladimir Sementsov-Ogievskiy 2019-11-04 17:53 ` Maxim Levitsky 1 sibling, 0 replies; 10+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-10-11 7:41 UTC (permalink / raw) To: Eric Blake, qemu-devel Cc: kwolf, qemu-block, Markus Armbruster, Max Reitz, mlevitsk, Dr. David Alan Gilbert 11.10.2019 0:00, Eric Blake wrote: > Allow blockdevs to match the feature already present in qemu-nbd -D. > Enhance iotest 223 to cover it. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block.json | 8 +++++--- > blockdev-nbd.c | 9 ++++++++- > monitor/hmp-cmds.c | 4 ++-- > tests/qemu-iotests/223 | 2 +- > tests/qemu-iotests/223.out | 1 + > 5 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/qapi/block.json b/qapi/block.json > index 145c268bb646..a6617b5bd03a 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -250,9 +250,11 @@ > # @name: Export name. If unspecified, the @device parameter is used as the > # export name. (Since 2.12) > # > +# @description: Free-form description of the export. (Since 4.2) Worth mention maximum of 4096? > +# > # @writable: Whether clients should be able to write to the device via the > # NBD connection (default false). > - > +# > # @bitmap: Also export the dirty bitmap reachable from @device, so the > # NBD client can use NBD_OPT_SET_META_CONTEXT with > # "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) > @@ -263,8 +265,8 @@ > # Since: 1.3.0 > ## [..] -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev 2019-10-10 21:00 ` [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev Eric Blake 2019-10-11 7:41 ` Vladimir Sementsov-Ogievskiy @ 2019-11-04 17:53 ` Maxim Levitsky 1 sibling, 0 replies; 10+ messages in thread From: Maxim Levitsky @ 2019-11-04 17:53 UTC (permalink / raw) To: Eric Blake, qemu-devel Cc: kwolf, vsementsov, qemu-block, Markus Armbruster, Dr. David Alan Gilbert, Max Reitz On Thu, 2019-10-10 at 16:00 -0500, Eric Blake wrote: > Allow blockdevs to match the feature already present in qemu-nbd -D. > Enhance iotest 223 to cover it. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qapi/block.json | 8 +++++--- > blockdev-nbd.c | 9 ++++++++- > monitor/hmp-cmds.c | 4 ++-- > tests/qemu-iotests/223 | 2 +- > tests/qemu-iotests/223.out | 1 + > 5 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/qapi/block.json b/qapi/block.json > index 145c268bb646..a6617b5bd03a 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -250,9 +250,11 @@ > # @name: Export name. If unspecified, the @device parameter is used as the > # export name. (Since 2.12) > # > +# @description: Free-form description of the export. (Since 4.2) > +# > # @writable: Whether clients should be able to write to the device via the > # NBD connection (default false). > - > +# > # @bitmap: Also export the dirty bitmap reachable from @device, so the > # NBD client can use NBD_OPT_SET_META_CONTEXT with > # "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) > @@ -263,8 +265,8 @@ > # Since: 1.3.0 > ## > { 'command': 'nbd-server-add', > - 'data': {'device': 'str', '*name': 'str', '*writable': 'bool', > - '*bitmap': 'str' } } > + 'data': {'device': 'str', '*name': 'str', '*description': 'str', > + '*writable': 'bool', '*bitmap': 'str' } } > > ## > # @NbdServerRemoveMode: > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 8c20baa4a4b9..de2f2ff71320 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -144,6 +144,7 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr, > } > > void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > + bool has_description, const char *description, > bool has_writable, bool writable, > bool has_bitmap, const char *bitmap, Error **errp) > { > @@ -167,6 +168,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > return; > } > > + if (has_description && strlen(description) > NBD_MAX_STRING_SIZE) { > + error_setg(errp, "description '%s' too long", description); > + return; > + } > + > if (nbd_export_find(name)) { > error_setg(errp, "NBD server already has export named '%s'", name); > return; > @@ -195,7 +201,8 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > writable = false; > } > > - exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable, > + exp = nbd_export_new(bs, 0, len, name, description, bitmap, > + !writable, !writable, > NULL, false, on_eject_blk, errp); > if (!exp) { > goto out; > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index b2551c16d129..574c6321c9d0 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -2352,7 +2352,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) > continue; > } > > - qmp_nbd_server_add(info->value->device, false, NULL, > + qmp_nbd_server_add(info->value->device, false, NULL, false, NULL, > true, writable, false, NULL, &local_err); > > if (local_err != NULL) { > @@ -2374,7 +2374,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) > bool writable = qdict_get_try_bool(qdict, "writable", false); > Error *local_err = NULL; > > - qmp_nbd_server_add(device, !!name, name, true, writable, > + qmp_nbd_server_add(device, !!name, name, false, NULL, true, writable, > false, NULL, &local_err); > hmp_handle_error(mon, &local_err); > } > diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 > index 2ba3d8124b4f..06bdc96be48f 100755 > --- a/tests/qemu-iotests/223 > +++ b/tests/qemu-iotests/223 > @@ -144,7 +144,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", > "bitmap":"b3"}}' "error" # Missing bitmap > _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", > "arguments":{"device":"n", "name":"n2", "writable":true, > - "bitmap":"b2"}}' "return" > + "description":"some text", "bitmap":"b2"}}' "return" > $QEMU_NBD_PROG -L -k "$TEST_DIR/nbd" > > echo > diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out > index 23b34fcd202e..16d597585b4f 100644 > --- a/tests/qemu-iotests/223.out > +++ b/tests/qemu-iotests/223.out > @@ -49,6 +49,7 @@ exports available: 2 > base:allocation > qemu:dirty-bitmap:b > export: 'n2' > + description: some text > size: 4194304 > flags: 0xced ( flush fua trim zeroes df cache fast-zero ) > min block: 1 Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-13 16:26 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-10 21:00 [PATCH v2 0/2] Better NBD string length handling Eric Blake 2019-10-10 21:00 ` [PATCH v2 1/2] nbd: Don't send oversize strings Eric Blake 2019-10-11 7:32 ` Vladimir Sementsov-Ogievskiy 2019-10-15 15:07 ` Eric Blake 2019-10-15 16:16 ` Vladimir Sementsov-Ogievskiy 2019-11-04 17:41 ` Maxim Levitsky 2019-11-13 15:47 ` Eric Blake 2019-10-10 21:00 ` [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev Eric Blake 2019-10-11 7:41 ` Vladimir Sementsov-Ogievskiy 2019-11-04 17:53 ` Maxim Levitsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).