Re: [Qemu-devel] [PATCH v3 7/6] nbd/client: Ignore inaccessible tail of inconsistent server
On 3/29/19 11:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2019 19:34, Eric Blake wrote:
>> The NBD spec suggests that a server should never advertise a size
>> inconsistent with its minimum block alignment, as that tail is
>> effectively inaccessible to a compliant client obeying those block
>> constraints. Although the block layer likes to round up, here, we'd
>> prefer to truncate down to obey the spec, and note that it is the
>> server's fault for advertising bogus size.
>>
>> Does not impact either qemu (which always sends properly aligned
>> sizes) or nbdkit (which does not send minimum block requirements yet);
>> so this is mostly theoretical, to avoid potential asserts elsewhere in
>> the code that assume the size is aligned.
>>
>> }
>> +if (info->min_block &&
>> +!QEMU_IS_ALIGNED(info->size, info->min_block)) {
>> +trace_nbd_opt_info_go_unaligned_size(info->size,
>> + info->min_block);
>> +info->size = QEMU_ALIGN_DOWN(info->size, info->min_block);
>> +}
>> trace_nbd_receive_negotiate_size_flags(info->size,
>> info->flags);
>
> And this again leads to silently skip file tail on qemu-img convert from such
> nbd export.
> I don't really care, but if not Qemu neither nbdkit are affected, isn't it
> better just
> nbd_send_opt_abort and return -1 in this case? So we'll never have hidden
> troubles with
> third-party bad servers, and their developers will have to fix their code
> instead (even
> not written yet, I suppose).
The code below is an example of killing connection to an impossible server:
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);
nbd_send_opt_abort(ioc);
return -1;
I'll copy that instead of silently truncating and proceeding on.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3 7/6] nbd/client: Ignore inaccessible tail of inconsistent server
The NBD spec suggests that a server should never advertise a size
inconsistent with its minimum block alignment, as that tail is
effectively inaccessible to a compliant client obeying those block
constraints. Although the block layer likes to round up, here, we'd
prefer to truncate down to obey the spec, and note that it is the
server's fault for advertising bogus size.
Does not impact either qemu (which always sends properly aligned
sizes) or nbdkit (which does not send minimum block requirements yet);
so this is mostly theoretical, to avoid potential asserts elsewhere in
the code that assume the size is aligned.
Signed-off-by: Eric Blake
---
nbd/client.c | 8 +++-
nbd/trace-events | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/nbd/client.c b/nbd/client.c
index de7da48246b..be437051429 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016-2018 Red Hat, Inc.
+ * Copyright (C) 2016-2019 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori
*
* Network Block Device Client Side
@@ -426,6 +426,12 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t
opt,
nbd_send_opt_abort(ioc);
return -1;
}
+if (info->min_block &&
+!QEMU_IS_ALIGNED(info->size, info->min_block)) {
+trace_nbd_opt_info_go_unaligned_size(info->size,
+ info->min_block);
+info->size = QEMU_ALIGN_DOWN(info->size, info->min_block);
+}
trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
break;
diff --git a/nbd/trace-events b/nbd/trace-events
index a6cca8fdf83..49dd48c9620 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -8,6 +8,7 @@ nbd_reply_err_unsup(uint32_t option, const char *name) "server
doesn't understan
nbd_receive_list(const char *name, const char *desc) "export list includes
'%s', description '%s'"
nbd_opt_info_go_start(const char *opt, const char *name) "Attempting %s for
export '%s'"
nbd_opt_info_go_success(const char *opt) "Export is ready after %s request"
+nbd_opt_info_go_unaligned_size(uint64_t size, uint32_t align) "Server reported
unaligned size 0x%" PRIx64 " for alignment 0x%x; truncating"
nbd_opt_info_unknown(int info, const char *name) "Ignoring unknown info %d
(%s)"
nbd_opt_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t
maximum) "Block sizes are 0x%" PRIx32 ", 0x%" PRIx32 ", 0x%" PRIx32
nbd_receive_query_exports_start(const char *wantname) "Querying export list
for '%s'"
--
2.20.1
Re: [Qemu-devel] [PATCH v3 7/6] nbd/client: Ignore inaccessible tail of inconsistent server
29.03.2019 19:34, Eric Blake wrote:
> The NBD spec suggests that a server should never advertise a size
> inconsistent with its minimum block alignment, as that tail is
> effectively inaccessible to a compliant client obeying those block
> constraints. Although the block layer likes to round up, here, we'd
> prefer to truncate down to obey the spec, and note that it is the
> server's fault for advertising bogus size.
>
> Does not impact either qemu (which always sends properly aligned
> sizes) or nbdkit (which does not send minimum block requirements yet);
> so this is mostly theoretical, to avoid potential asserts elsewhere in
> the code that assume the size is aligned.
>
> Signed-off-by: Eric Blake
> ---
> nbd/client.c | 8 +++-
> nbd/trace-events | 1 +
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index de7da48246b..be437051429 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2016-2018 Red Hat, Inc.
> + * Copyright (C) 2016-2019 Red Hat, Inc.
>* Copyright (C) 2005 Anthony Liguori
>*
>* Network Block Device Client Side
> @@ -426,6 +426,12 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t
> opt,
> nbd_send_opt_abort(ioc);
> return -1;
> }
> +if (info->min_block &&
> +!QEMU_IS_ALIGNED(info->size, info->min_block)) {
> +trace_nbd_opt_info_go_unaligned_size(info->size,
> + info->min_block);
> +info->size = QEMU_ALIGN_DOWN(info->size, info->min_block);
> +}
> trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
> break;
>
> diff --git a/nbd/trace-events b/nbd/trace-events
> index a6cca8fdf83..49dd48c9620 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -8,6 +8,7 @@ nbd_reply_err_unsup(uint32_t option, const char *name)
> "server doesn't understan
> nbd_receive_list(const char *name, const char *desc) "export list includes
> '%s', description '%s'"
> nbd_opt_info_go_start(const char *opt, const char *name) "Attempting %s for
> export '%s'"
> nbd_opt_info_go_success(const char *opt) "Export is ready after %s request"
> +nbd_opt_info_go_unaligned_size(uint64_t size, uint32_t align) "Server
> reported unaligned size 0x%" PRIx64 " for alignment 0x%x; truncating"
> nbd_opt_info_unknown(int info, const char *name) "Ignoring unknown info %d
> (%s)"
> nbd_opt_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t
> maximum) "Block sizes are 0x%" PRIx32 ", 0x%" PRIx32 ", 0x%" PRIx32
> nbd_receive_query_exports_start(const char *wantname) "Querying export list
> for '%s'"
>
And this again leads to silently skip file tail on qemu-img convert from such
nbd export.
I don't really care, but if not Qemu neither nbdkit are affected, isn't it
better just
nbd_send_opt_abort and return -1 in this case? So we'll never have hidden
troubles with
third-party bad servers, and their developers will have to fix their code
instead (even
not written yet, I suppose).
--
Best regards,
Vladimir
