[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 16/19] nbd/client: Add meta contexts to nbd_r
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v3 16/19] nbd/client: Add meta contexts to nbd_receive_export_list() |
Date: |
Wed, 16 Jan 2019 10:54:07 +0000 |
12.01.2019 20:58, Eric Blake wrote:
> We want to be able to detect whether a given qemu NBD server is
> exposing the right export(s) and dirty bitmaps, at least for
> regression testing. We could use 'nbd-client -l' from the upstream
> NBD project to list exports, but it's annoying to rely on
> out-of-tree binaries; furthermore, nbd-client doesn't necessarily
> know about all of the qemu NBD extensions. Thus, we plan on adding
> a new mode to qemu-nbd that merely sniffs all possible information
> from the server during handshake phase, then disconnects and dumps
> the information.
>
> This patch continues the work of the previous patch, by adding the
> ability to track the list of available meta contexts into
> NBDExportInfo. It benefits from the recent refactoring patches
> with a new nbd_list_meta_contexts() that reuses much of the same
> framework as setting a meta context.
>
> Note: a malicious server could exhaust memory of a client by feeding
> an unending loop of contexts; perhaps we could place a limit on how
> many we are willing to receive. But this is no different from our
> earlier analysis on a server sending an unending list of exports,
> and the death of a client due to memory exhaustion when the client
> was going to exit soon anyways is not really a denial of service
> attack.
>
> Signed-off-by: Eric Blake <address@hidden>
> Message-Id: <address@hidden>
> Reviewed-by: Richard W.M. Jones <address@hidden>
>
> ---
> v3: mention security (non-)issue in commit message [Rich]
> ---
> include/block/nbd.h | 2 ++
> nbd/client.c | 41 +++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index e9a442ce7e9..80ee3cc997e 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -284,6 +284,8 @@ struct NBDExportInfo {
>
> /* Set by server results during nbd_receive_export_list() */
> char *description;
> + int n_contexts;
> + char **contexts;
> };
> typedef struct NBDExportInfo NBDExportInfo;
>
> diff --git a/nbd/client.c b/nbd/client.c
> index a5a705a67f7..2001e6e8160 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -817,6 +817,36 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return received;
> }
>
> +/*
> + * nbd_list_meta_contexts:
> + * Request the server to list all meta contexts for export @info->name.
> + * return 0 if list is complete (even if empty),
> + * -1 with errp set for any other error
Not sure about need of "other" word in this context.
> + */
> +static int nbd_list_meta_contexts(QIOChannel *ioc,
> + NBDExportInfo *info,
> + Error **errp)
> +{
> + int ret;
> +
> + if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
> + info->name, NULL, errp) < 0) {
Hmm, this made me to add one more comment to previous patch about
nbd_send_one_meta_context, the it don't send context but query.
> + return -1;
> + }
> +
> + while (1) {
> + char *context;
> +
> + ret = nbd_receive_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
> + &context, NULL, errp);
> + if (ret <= 0) {
> + return ret;
> + }
> + info->contexts = g_renew(char *, info->contexts, ++info->n_contexts);
> + info->contexts[info->n_contexts - 1] = context;
> + }
A bit odd for me, that function leaves allocated memory on error path, but it
will
be freed anyway in nbd_free_export_list, so, don't mind.
> +}
> +
> /*
> * nbd_start_negotiate:
> * Start the handshake to the server. After a positive return, the server
> @@ -1063,7 +1093,7 @@ int nbd_receive_negotiate(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> /* Clean up result of nbd_receive_export_list */
> void nbd_free_export_list(NBDExportInfo *info, int count)
> {
> - int i;
> + int i, j;
>
> if (!info) {
> return;
> @@ -1072,6 +1102,10 @@ void nbd_free_export_list(NBDExportInfo *info, int
> count)
> for (i = 0; i < count; i++) {
> g_free(info[i].name);
> g_free(info[i].description);
> + for (j = 0; j < info[i].n_contexts; j++) {
> + g_free(info[i].contexts[j]);
> + }
> + g_free(info[i].contexts);
> }
> g_free(info);
> }
> @@ -1139,7 +1173,10 @@ int nbd_receive_export_list(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> break;
> }
>
> - /* TODO: Grab meta contexts */
> + if (result == 3 &&
> + nbd_list_meta_contexts(ioc, &array[i], errp) < 0) {
> + goto out;
> + }
> }
>
> /* Send NBD_OPT_ABORT as a courtesy before hanging up */
>
anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
--
Best regards,
Vladimir
[Qemu-block] [PATCH v3 17/19] qemu-nbd: Add --list option, Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 16/19] nbd/client: Add meta contexts to nbd_receive_export_list(), Eric Blake, 2019/01/12
- Re: [Qemu-block] [PATCH v3 16/19] nbd/client: Add meta contexts to nbd_receive_export_list(),
Vladimir Sementsov-Ogievskiy <=
[Qemu-block] [PATCH v3 13/19] nbd/client: Split handshake into two functions, Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 09/19] nbd/client: Change signature of nbd_negotiate_simple_meta_context(), Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 15/19] nbd/client: Add nbd_receive_export_list(), Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 12/19] nbd/client: Refactor return of nbd_receive_negotiate(), Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 08/19] nbd/client: Move export name into NBDExportInfo, Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t, Eric Blake, 2019/01/12