On 11/10/22 00:26, Eric Blake wrote:

Modern qemu tends to advertise a strict 32M payload limit. Yet we default to allowing the user to attempt up to 64M in pwrite. For pread, qemu will reply with EINVAL but keep the connection up; but for pwrite, an overlarge buffer is fatal. It's time to teach libnbd to honor qemu's max buffer by default, while still allowing the user to disable the strict mode setting if they want to try 64M anyways.

This also involves advertising a new LIBNBD_SIZE_PAYLOAD via nbd_get_block_size, which is more reliable than asking the user to manually obey LIBNBD_SIZE_MAXIMUM which may not be set or may be larger than 64M. --- lib/internal.h | 4 +++ generator/API.ml | 39 ++++++++++++++++----- generator/states-newstyle-opt-export-name.c | 1 + generator/states-newstyle-opt-go.c | 1 + generator/states-oldstyle.c | 1 + lib/flags.c | 25 +++++++++++++ lib/rw.c | 8 ++++- 7 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h index 22f500b4..bbbd2639 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -146,6 +146,8 @@ struct nbd_handle { uint32_t block_minimum; uint32_t block_preferred; uint32_t block_maximum; + /* Payload cap, always non-zero, based on block_maximum when feasible */ + uint32_t payload_maximum;

/* Server canonical name and description, or NULL if not advertised. */ char *canonical_name; @@ -448,6 +450,8 @@ extern int nbd_internal_set_size_and_flags (struct nbd_handle *h, extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min, uint32_t pref, uint32_t max) LIBNBD_ATTRIBUTE_NONNULL(1); +extern void nbd_internal_set_payload (struct nbd_handle *h) + LIBNBD_ATTRIBUTE_NONNULL(1);

/* is-state.c */ extern bool nbd_internal_is_state_created (enum state state); diff --git a/generator/API.ml b/generator/API.ml index 8d0d9d15..0ebc9298 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -184,6 +184,7 @@ let block_size_enum = "MINIMUM", 0; "PREFERRED", 1; "MAXIMUM", 2; + "PAYLOAD", 3; ] } let all_enums = [ tls_enum; block_size_enum ] @@ -221,6 +222,7 @@ let strict_flags = "BOUNDS", 1 lsl 2; "ZERO_SIZE", 1 lsl 3; "ALIGN", 1 lsl 4; + "PAYLOAD", 1 lsl 5; ] } let allow_transport_flags = { @@ -1060,10 +1062,21 @@ "set_strict_mode", { =item C<LIBNBD_STRICT_ALIGN> = 5

If set, and the server provided minimum block sizes (see -L<nbd_get_block_size(3)>, this flag rejects client requests that -do not have length and offset aligned to the server's minimum -requirements. If clear, unaligned requests are sent to the server, -where it is up to the server whether to honor or reject the request. +C<LIBNBD_SIZE_MINIMUM> for L<nbd_get_block_size(3)>), this +flag rejects client requests that do not have length and offset +aligned to the server's minimum requirements. If clear, +unaligned requests are sent to the server, where it is up to +the server whether to honor or reject the request. + +=item C<LIBNBD_STRICT_PAYLOAD> = 6 + +If set, the client refuses to send a command to the server +with more than libnbd's outgoing payload maximum (see +C<LIBNBD_SIZE_PAYLOAD> for L<nbd_get_block_size(3)>), whether +or not the server advertised a block size maximum. If clear, +oversize requests up to 64MiB may be attempted, although +requests larger than 32MiB are liable to cause some servers to +disconnect.

=back

@@ -2286,6 +2299,15 @@ "get_block_size", { itself imposes a maximum of 64M. If zero, some NBD servers will abruptly disconnect if a transaction involves more than 32M.

+=item C<LIBNBD_SIZE_PAYLOAD> = 3 + +This value is not advertised by the server, but rather represents +the maximum outgoing payload size for a given connection that +libnbd will enforce unless C<LIBNBD_STRICT_PAYLOAD> is cleared +in C<nbd_set_strict_mode(3)>. It is always non-zero: never +smaller than 1M, never larger than 64M, and matches +C<LIBNBD_SIZE_MAXIMUM> when possible. + =back

Future NBD extensions may result in additional C<size_type> values. @@ -2449,10 +2471,11 @@ "pwrite", { acknowledged by the server, or there is an error. Note this will generally return an error if L<nbd_is_read_only(3)> is true.

-Note that libnbd currently enforces a maximum write buffer of 64MiB, -even if the server would permit a larger buffer in a single transaction; -attempts to exceed this will result in an C<ERANGE> error. The server -may enforce a smaller limit, which can be learned with +Note that libnbd defaults to enforcing a maximum write buffer +of the lesser of 64MiB or any maximum payload size advertised +by the server; attempts to exceed this will generally result in +a client-side C<ERANGE> error, rather than a server-side +disconnection. The actual limit can be learned with L<nbd_get_block_size(3)>.

The C<flags> parameter may be C<0> for no flags, or may contain diff --git a/generator/states-newstyle-opt-export-name.c b/generator/states-newstyle-opt-export-name.c index 398aa680..88296297 100644 --- a/generator/states-newstyle-opt-export-name.c +++ b/generator/states-newstyle-opt-export-name.c @@ -70,6 +70,7 @@ NEWSTYLE.OPT_EXPORT_NAME.CHECK_REPLY: SET_NEXT_STATE (%.DEAD); return 0; } + nbd_internal_set_payload (h); SET_NEXT_STATE (%^FINISHED); CALL_CALLBACK (h->opt_cb.completion, &err); nbd_internal_free_option (h); diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c index f2d9f846..ac9613d4 100644 --- a/generator/states-newstyle-opt-go.c +++ b/generator/states-newstyle-opt-go.c @@ -276,6 +276,7 @@ NEWSTYLE.OPT_GO.CHECK_REPLY: err = nbd_get_errno () ? : ENOTSUP; break; case NBD_REP_ACK: + nbd_internal_set_payload (h); err = 0; break; } diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c index 7f668936..1eb5e940 100644 --- a/generator/states-oldstyle.c +++ b/generator/states-oldstyle.c @@ -68,6 +68,7 @@ OLDSTYLE.CHECK: SET_NEXT_STATE (%.DEAD); return 0; } + nbd_internal_set_payload (h);

h->protocol = "oldstyle";

diff --git a/lib/flags.c b/lib/flags.c index 5def16e8..eaf4ff85 100644 --- a/lib/flags.c +++ b/lib/flags.c @@ -26,6 +26,7 @@ #include <errno.h>

#include "internal.h" +#include "minmax.h"

/* Reset connection data. Called after swapping export name, after * failed OPT_GO/OPT_INFO, or after successful OPT_STARTTLS. @@ -38,6 +39,7 @@ nbd_internal_reset_size_and_flags (struct nbd_handle *h) h->block_minimum = 0; h->block_preferred = 0; h->block_maximum = 0; + h->payload_maximum = 0; free (h->canonical_name); h->canonical_name = NULL; free (h->description); @@ -118,6 +120,27 @@ nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min, return 0; /* Use return -1 if we want to reject such servers */ }

+/* Set the payload size constraint on the handle. + * This is called from the state machine after all other information + * about an export is available, regardless of oldstyle or newstyle. + */ +void +nbd_internal_set_payload (struct nbd_handle *h) +{ + /* The NBD protocol says we should not assume the server will allow + * more than 32M payload if it did not advertise anything. If it + * advertised more than 64M, we still cap at the maximum buffer size + * we are willing to allocate as a client; if it advertised smaller + * than 1M (presumably because the export itself was small), the NBD + * protocol says we can still send payloads that large. + */ + if (h->block_maximum) + h->payload_maximum = MIN (MAX_REQUEST_SIZE, + MAX (1024 * 1024, h->block_maximum)); + else + h->payload_maximum = 32 * 1024 * 1024; +} + static int get_flag (struct nbd_handle *h, uint16_t flag) { @@ -237,6 +260,8 @@ nbd_unlocked_get_block_size (struct nbd_handle *h, int type) return h->block_preferred; case LIBNBD_SIZE_MAXIMUM: return h->block_maximum; + case LIBNBD_SIZE_PAYLOAD: + return h->payload_maximum; } return 0; } diff --git a/lib/rw.c b/lib/rw.c index 2ab85f3d..6120edd0 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -207,8 +207,14 @@ nbd_internal_command_common (struct nbd_handle *h,

switch (type) { /* Commands which send or receive data are limited to MAX_REQUEST_SIZE. */ - case NBD_CMD_READ: case NBD_CMD_WRITE: + if (h->strict & LIBNBD_STRICT_PAYLOAD && count > h->payload_maximum) { + set_error (ERANGE, "request too large: maximum payload size is %d",

For pedantry's sake, I think we shouldn't print a uint32_t value with a %d format specifier (I understand the value is limited to MAX_REQUEST_SIZE, which would be fine to print with %d *if* passed in as an "unsigned int"; but for pedantry we may not want to assume that uint32_t is actually "unsigned int".) PRIu32 would fit better.

+ h->payload_maximum); + goto err; + } + /* fallthrough */ + case NBD_CMD_READ: if (count > MAX_REQUEST_SIZE) { set_error (ERANGE, "request too large: maximum request size is %d", MAX_REQUEST_SIZE);

Otherwise the patch looks OK to me, but I don't understand where it really matters. * If a client program using libnbd connects to an NBD server that advertises LIBNBD_SIZE_MAXIMUM, then it is up to the client application (not libnbd) to adhere to that maximum, or face the consequences. Is that right? * If a client program using libnbd connects to an NBD server that does *not* advertise LIBNBD_SIZE_MAXIMUM, then the client can (per libnbd's own limit) attempt writes up to 64MB. If the NBD server then abruptly disconnects, *without having advertised* a lower limit (and IIUC this applies to *old* qemu-nbd), then -- per NBD spec / protocol -- the fault is with either the client program or libnbd, because the request size should not have exceeded 32MB. The question is who is responsible for enforcing this protocol-default-limit: the client application, or libnbd? Before the patch, the server might abruptly disconnect, after the patch, the client app will get a local error -- the request will not succeed either way, and the client application will have to be modified (= reprogrammed by a human) anyways, to consider such a 32MB default limit explicitly. So what use case / failure scenario is improved by this patch? Is it that the client application's programmer gets a friendlier / more understandable error message, rather than just the client app being silently kicked out by (old) qemu-nbd? Anyway, the patch looks OK; feel free to extend (or not) the commit message and/or to change (or not) the %d format specifier, from my end: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo

On Wed, Nov 09, 2022 at 05:26:06PM -0600, Eric Blake wrote:

Modern qemu tends to advertise a strict 32M payload limit. Yet we default to allowing the user to attempt up to 64M in pwrite. For pread, qemu will reply with EINVAL but keep the connection up; but for pwrite, an overlarge buffer is fatal. It's time to teach libnbd to honor qemu's max buffer by default, while still allowing the user to disable the strict mode setting if they want to try 64M anyways.

This also involves advertising a new LIBNBD_SIZE_PAYLOAD via nbd_get_block_size, which is more reliable than asking the user to manually obey LIBNBD_SIZE_MAXIMUM which may not be set or may be larger than 64M.

I think you pushed this, but a couple of comments: (1) New APIs are cheap! Did we really want to overload nbd_get_block_size? (2) There are quite a lot of places elsewhere in libnbd which hard-code the maximum block size. Grepping for "REQUEST_SIZE" is instructive. I think we should fix those as well. Rich.

lib/internal.h | 4 +++ generator/API.ml | 39 ++++++++++++++++----- generator/states-newstyle-opt-export-name.c | 1 + generator/states-newstyle-opt-go.c | 1 + generator/states-oldstyle.c | 1 + lib/flags.c | 25 +++++++++++++ lib/rw.c | 8 ++++- 7 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h index 22f500b4..bbbd2639 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -146,6 +146,8 @@ struct nbd_handle { uint32_t block_minimum; uint32_t block_preferred; uint32_t block_maximum; + /* Payload cap, always non-zero, based on block_maximum when feasible */ + uint32_t payload_maximum;

/* Server canonical name and description, or NULL if not advertised. */ char *canonical_name; @@ -448,6 +450,8 @@ extern int nbd_internal_set_size_and_flags (struct nbd_handle *h, extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min, uint32_t pref, uint32_t max) LIBNBD_ATTRIBUTE_NONNULL(1); +extern void nbd_internal_set_payload (struct nbd_handle *h) + LIBNBD_ATTRIBUTE_NONNULL(1);

/* is-state.c */ extern bool nbd_internal_is_state_created (enum state state); diff --git a/generator/API.ml b/generator/API.ml index 8d0d9d15..0ebc9298 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -184,6 +184,7 @@ let block_size_enum = "MINIMUM", 0; "PREFERRED", 1; "MAXIMUM", 2; + "PAYLOAD", 3; ] } let all_enums = [ tls_enum; block_size_enum ] @@ -221,6 +222,7 @@ let strict_flags = "BOUNDS", 1 lsl 2; "ZERO_SIZE", 1 lsl 3; "ALIGN", 1 lsl 4; + "PAYLOAD", 1 lsl 5; ] } let allow_transport_flags = { @@ -1060,10 +1062,21 @@ "set_strict_mode", { =item C<LIBNBD_STRICT_ALIGN> = 5

If set, and the server provided minimum block sizes (see -L<nbd_get_block_size(3)>, this flag rejects client requests that -do not have length and offset aligned to the server's minimum -requirements. If clear, unaligned requests are sent to the server, -where it is up to the server whether to honor or reject the request. +C<LIBNBD_SIZE_MINIMUM> for L<nbd_get_block_size(3)>), this +flag rejects client requests that do not have length and offset +aligned to the server's minimum requirements. If clear, +unaligned requests are sent to the server, where it is up to +the server whether to honor or reject the request. + +=item C<LIBNBD_STRICT_PAYLOAD> = 6 + +If set, the client refuses to send a command to the server +with more than libnbd's outgoing payload maximum (see +C<LIBNBD_SIZE_PAYLOAD> for L<nbd_get_block_size(3)>), whether +or not the server advertised a block size maximum. If clear, +oversize requests up to 64MiB may be attempted, although +requests larger than 32MiB are liable to cause some servers to +disconnect.

=back

@@ -2286,6 +2299,15 @@ "get_block_size", { itself imposes a maximum of 64M. If zero, some NBD servers will abruptly disconnect if a transaction involves more than 32M.

+=item C<LIBNBD_SIZE_PAYLOAD> = 3 + +This value is not advertised by the server, but rather represents +the maximum outgoing payload size for a given connection that +libnbd will enforce unless C<LIBNBD_STRICT_PAYLOAD> is cleared +in C<nbd_set_strict_mode(3)>. It is always non-zero: never +smaller than 1M, never larger than 64M, and matches +C<LIBNBD_SIZE_MAXIMUM> when possible. + =back

Future NBD extensions may result in additional C<size_type> values. @@ -2449,10 +2471,11 @@ "pwrite", { acknowledged by the server, or there is an error. Note this will generally return an error if L<nbd_is_read_only(3)> is true.

-Note that libnbd currently enforces a maximum write buffer of 64MiB, -even if the server would permit a larger buffer in a single transaction; -attempts to exceed this will result in an C<ERANGE> error. The server -may enforce a smaller limit, which can be learned with +Note that libnbd defaults to enforcing a maximum write buffer +of the lesser of 64MiB or any maximum payload size advertised +by the server; attempts to exceed this will generally result in +a client-side C<ERANGE> error, rather than a server-side +disconnection. The actual limit can be learned with L<nbd_get_block_size(3)>.

The C<flags> parameter may be C<0> for no flags, or may contain diff --git a/generator/states-newstyle-opt-export-name.c b/generator/states-newstyle-opt-export-name.c index 398aa680..88296297 100644 --- a/generator/states-newstyle-opt-export-name.c +++ b/generator/states-newstyle-opt-export-name.c @@ -70,6 +70,7 @@ NEWSTYLE.OPT_EXPORT_NAME.CHECK_REPLY: SET_NEXT_STATE (%.DEAD); return 0; } + nbd_internal_set_payload (h); SET_NEXT_STATE (%^FINISHED); CALL_CALLBACK (h->opt_cb.completion, &err); nbd_internal_free_option (h); diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c index f2d9f846..ac9613d4 100644 --- a/generator/states-newstyle-opt-go.c +++ b/generator/states-newstyle-opt-go.c @@ -276,6 +276,7 @@ NEWSTYLE.OPT_GO.CHECK_REPLY: err = nbd_get_errno () ? : ENOTSUP; break; case NBD_REP_ACK: + nbd_internal_set_payload (h); err = 0; break; } diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c index 7f668936..1eb5e940 100644 --- a/generator/states-oldstyle.c +++ b/generator/states-oldstyle.c @@ -68,6 +68,7 @@ OLDSTYLE.CHECK: SET_NEXT_STATE (%.DEAD); return 0; } + nbd_internal_set_payload (h);

h->protocol = "oldstyle";

diff --git a/lib/flags.c b/lib/flags.c index 5def16e8..eaf4ff85 100644 --- a/lib/flags.c +++ b/lib/flags.c @@ -26,6 +26,7 @@ #include <errno.h>

#include "internal.h" +#include "minmax.h"

/* Reset connection data. Called after swapping export name, after * failed OPT_GO/OPT_INFO, or after successful OPT_STARTTLS. @@ -38,6 +39,7 @@ nbd_internal_reset_size_and_flags (struct nbd_handle *h) h->block_minimum = 0; h->block_preferred = 0; h->block_maximum = 0; + h->payload_maximum = 0; free (h->canonical_name); h->canonical_name = NULL; free (h->description); @@ -118,6 +120,27 @@ nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min, return 0; /* Use return -1 if we want to reject such servers */ }

+/* Set the payload size constraint on the handle. + * This is called from the state machine after all other information + * about an export is available, regardless of oldstyle or newstyle. + */ +void +nbd_internal_set_payload (struct nbd_handle *h) +{ + /* The NBD protocol says we should not assume the server will allow + * more than 32M payload if it did not advertise anything. If it + * advertised more than 64M, we still cap at the maximum buffer size + * we are willing to allocate as a client; if it advertised smaller + * than 1M (presumably because the export itself was small), the NBD + * protocol says we can still send payloads that large. + */ + if (h->block_maximum) + h->payload_maximum = MIN (MAX_REQUEST_SIZE, + MAX (1024 * 1024, h->block_maximum)); + else + h->payload_maximum = 32 * 1024 * 1024; +} + static int get_flag (struct nbd_handle *h, uint16_t flag) { @@ -237,6 +260,8 @@ nbd_unlocked_get_block_size (struct nbd_handle *h, int type) return h->block_preferred; case LIBNBD_SIZE_MAXIMUM: return h->block_maximum; + case LIBNBD_SIZE_PAYLOAD: + return h->payload_maximum; } return 0; } diff --git a/lib/rw.c b/lib/rw.c index 2ab85f3d..6120edd0 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -207,8 +207,14 @@ nbd_internal_command_common (struct nbd_handle *h,

switch (type) { /* Commands which send or receive data are limited to MAX_REQUEST_SIZE. */ - case NBD_CMD_READ: case NBD_CMD_WRITE: + if (h->strict & LIBNBD_STRICT_PAYLOAD && count > h->payload_maximum) { + set_error (ERANGE, "request too large: maximum payload size is %d", + h->payload_maximum); + goto err; + } + /* fallthrough */ + case NBD_CMD_READ: if (count > MAX_REQUEST_SIZE) { set_error (ERANGE, "request too large: maximum request size is %d", MAX_REQUEST_SIZE); -- 2.38.1

_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

On 11/10/22 00:26, Eric Blake wrote:

Improve some existing tests to cover client safety valves for oversize requests, and add a new test that tests libnbd behavior on server errors to the same sort of requests. The new test requires a parallel patch posted to nbdkit to teach 'nbdkit eval' how to forcefully disconnect the server on particarly oversize write requests, emulating

s/particarly/partially/ (I believe)

qemu-nbd behavior. --- tests/Makefile.am | 15 +++- tests/errors-client-oversize.c | 18 +++- tests/errors-server-oversize.c | 151 ++++++++++++++++++++++++++++++++ tests/errors-server-unaligned.c | 2 + .gitignore | 1 + 5 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 tests/errors-server-oversize.c

diff --git a/tests/Makefile.am b/tests/Makefile.am index 70083e5c..0b9c454e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -177,6 +177,7 @@ check_PROGRAMS += \ errors-multiple-disconnects \ errors-server-invalid-offset \ errors-client-oversize \ + errors-server-oversize \ errors-client-unadvertised-cmd \ errors-server-unadvertised-cmd \ errors-client-unaligned \ @@ -247,6 +248,7 @@ TESTS += \ errors-multiple-disconnects \ errors-server-invalid-offset \ errors-client-oversize \ + errors-server-oversize \ errors-client-unadvertised-cmd \ errors-server-unadvertised-cmd \ errors-client-unaligned \ @@ -343,9 +345,20 @@ errors_multiple_disconnects_LDADD = $(top_builddir)/lib/libnbd.la errors_server_invalid_offset_SOURCES = errors-server-invalid-offset.c errors_server_invalid_offset_LDADD = $(top_builddir)/lib/libnbd.la

-errors_client_oversize_SOURCES = errors-client-oversize.c +errors_client_oversize_SOURCES = \ + errors-client-oversize.c \ + requires.c \ + requires.h \ + $(NULL) errors_client_oversize_LDADD = $(top_builddir)/lib/libnbd.la

+errors_server_oversize_SOURCES = \ + errors-server-oversize.c \ + requires.c \ + requires.h \ + $(NULL) +errors_server_oversize_LDADD = $(top_builddir)/lib/libnbd.la + errors_client_unadvertised_cmd_SOURCES = errors-client-unadvertised-cmd.c \ requires.c requires.h errors_client_unadvertised_cmd_LDADD = $(top_builddir)/lib/libnbd.la diff --git a/tests/errors-client-oversize.c b/tests/errors-client-oversize.c index 7e5b5421..739b41e9 100644 --- a/tests/errors-client-oversize.c +++ b/tests/errors-client-oversize.c @@ -30,6 +30,7 @@ #include <sys/stat.h>

#include <libnbd.h> +#include "requires.h"

#define MAXSIZE 68157440 /* 65M, oversize on purpose */

@@ -62,10 +63,15 @@ main (int argc, char *argv[]) { struct nbd_handle *nbd; const char *cmd[] = { - "nbdkit", "-s", "-v", "--exit-with-parent", "memory", "68157440", NULL, + "nbdkit", "-s", "-v", "--exit-with-parent", + "memory", "68157440", + "--filter=blocksize-policy", "blocksize-maximum=32M", + "blocksize-error-policy=error", + NULL };

progname = argv[0]; + requires ("nbdkit --version --filter=blocksize-policy null");

nbd = nbd_create (); if (nbd == NULL) { @@ -87,6 +93,7 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } check (ERANGE, "nbd_pread: "); + if (nbd_aio_pwrite (nbd, buf, MAXSIZE, 0, NBD_NULL_COMPLETION, 0) != -1) { fprintf (stderr, "%s: test failed: " @@ -96,6 +103,15 @@ main (int argc, char *argv[]) } check (ERANGE, "nbd_aio_pwrite: ");

+ if (nbd_aio_pwrite (nbd, buf, 33*1024*1024, 0, + NBD_NULL_COMPLETION, 0) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_aio_pwrite did not fail with oversize request\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (ERANGE, "nbd_aio_pwrite: "); + nbd_close (nbd); exit (EXIT_SUCCESS); } diff --git a/tests/errors-server-oversize.c b/tests/errors-server-oversize.c new file mode 100644 index 00000000..e55aafc3 --- /dev/null +++ b/tests/errors-server-oversize.c @@ -0,0 +1,151 @@ +/* NBD client library in userspace + * Copyright (C) 2013-2022 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Deliberately provoke some errors and check the error messages from + * nbd_get_error etc look reasonable. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <unistd.h> +#include <inttypes.h> +#include <sys/stat.h> + +#include <libnbd.h> +#include "requires.h" + +#define MAXSIZE 68157440 /* 65M, oversize on purpose */ + +static char *progname; +static char buf[MAXSIZE]; + +static void +check (int experr, const char *prefix) +{ + const char *msg = nbd_get_error (); + int errnum = nbd_get_errno (); + + fprintf (stderr, "error: \"%s\"\n", msg); + fprintf (stderr, "errno: %d (%s)\n", errnum, strerror (errnum)); + if (strncmp (msg, prefix, strlen (prefix)) != 0) { + fprintf (stderr, "%s: test failed: missing context prefix: %s\n", + progname, msg); + exit (EXIT_FAILURE); + } + if (errnum != experr) { + fprintf (stderr, "%s: test failed: " + "expected errno = %d (%s), but got %d\n", + progname, experr, strerror (experr), errnum); + exit (EXIT_FAILURE); + } +} + +static void +check_server_fail (struct nbd_handle *h, int64_t cookie, + const char *cmd, int experr) +{ + int r; + + if (cookie == -1) { + fprintf (stderr, "%s: test failed: %s not sent to server\n", + progname, cmd); + exit (EXIT_FAILURE); + } + + while ((r = nbd_aio_command_completed (h, cookie)) == 0) { + if (nbd_poll (h, -1) == -1) { + fprintf (stderr, "%s: test failed: poll failed while awaiting %s: %s\n", + progname, cmd, nbd_get_error ()); + exit (EXIT_FAILURE); + } + } + + if (r != -1) { + fprintf (stderr, "%s: test failed: %s did not fail at server\n", + progname, cmd); + exit (EXIT_FAILURE); + } + check (experr, "nbd_aio_command_completed: "); +} + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + const char *cmd[] = { + "nbdkit", "-s", "-v", "--exit-with-parent", "eval", + "get_size= echo 68157440", + "block_size= echo 1 512 16M", + "pread= echo EIO >&2; exit 1", + "pwrite= if test $3 -gt $((32*1024*1024)); then\n" + " exit 6\n" /* Hard disconnect */ + " elif test $3 -gt $((16*1024*1024)); then\n" + " echo EOVERFLOW >&2; exit 1\n" + " fi\n" + " cat >/dev/null", + NULL, + }; + uint32_t strict; + + progname = argv[0]; + requires ("nbdkit --dump-plugin eval | grep ^max_known_status="); + + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Connect to the server. */ + if (nbd_connect_command (nbd, (char **) cmd) == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Check the advertised max sizes. */ + printf ("server block size maximum: %" PRIi64 "\n", + nbd_get_block_size (nbd, LIBNBD_SIZE_MAXIMUM)); + printf ("libnbd payload size maximum: %" PRIi64 "\n", + nbd_get_block_size (nbd, LIBNBD_SIZE_PAYLOAD)); + + /* Disable client-side safety check */ + strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_PAYLOAD;

The current macros are (without the first patch applied): #define LIBNBD_STRICT_COMMANDS 0x01 #define LIBNBD_STRICT_FLAGS 0x02 #define LIBNBD_STRICT_BOUNDS 0x04 #define LIBNBD_STRICT_ZERO_SIZE 0x08 #define LIBNBD_STRICT_ALIGN 0x10 #define LIBNBD_STRICT_MASK 0x1f The 0x prefix *permits* the compiler to consider unsigned integer types when picking the size (type) for the integer constant, but the prefix does not *force* an unsigned type. And, because these values are really small, they all have type "int". In turn, when we apply the bitwise negation operator, we flip the sign bit. Not necessarily a problem on the platforms we care about, but not nice. There are two ways to remove this concern. One is the following: strict = nbd_get_strict_mode (nbd) & ~(sometype)LIBNBD_STRICT_PAYLOAD; where "sometype" is an unsigned integer type having at least the conversion rank of "int" (so that it would not be promoted to "int"), *and* large enough to accommodate all uint32_t values. C99 permits "unsigned int" to be just 16 bits, so using "unsigned int" for "sometype" is only safe if we are sure that on our platforms, "unsigned int" will always be able to represent "uint32_t". Otherwise we need a larger type, such as "unsigned long" (which is guaranteed by C99 even to hold a uint32_t). The other way is more generic: change the generator to output all these constants with a "u" suffix, such as: #define LIBNBD_STRICT_COMMANDS 0x01u #define LIBNBD_STRICT_FLAGS 0x02u #define LIBNBD_STRICT_BOUNDS 0x04u #define LIBNBD_STRICT_ZERO_SIZE 0x08u #define LIBNBD_STRICT_ALIGN 0x10u #define LIBNBD_STRICT_MASK 0x1fu This is better because the compiler will pick "unsigned int" for them (once we reach huge values, the compiler will pick "unsigned long" then "unsigned long long"), the bitwise negation will be applied to unsigned integers, and then the bitwise "and" will be between a uint32_t and some unsigned integer type (and at that one that's not going to be promoted to "int", having a rank already at least as large). Then in case "uint32_t" fits in an "int", it's going to be promoted to "int", and then the bit-and is between int and unsigned int --> the int is converted to unsigned int, done. Otherwise, we have two unsigned integers, the one with the smaller rank is converted to the type of the other. Long story short: probably totally harmless in practice on our platforms (the code happens to do what's expected due to two's complement representation of signed integers), but it's general hygiene to avoid bit manipulations on signed integers (some of those happen to be undefined behavior even). It's good to be aware IMO that the generator currently produces LIBNBD_STRICT_* values that have type "int", and not signed int. Feel free to ignore this comment if you think it's needlessly cautious. (I think it'd be best to keep the patch as-is, and then modify the generator in a separate patch.) ...We could also consider the C99 standard macro UINT32_C(...): #define LIBNBD_STRICT_COMMANDS UINT32_C(0x01) #define LIBNBD_STRICT_FLAGS UINT32_C(0x02) #define LIBNBD_STRICT_BOUNDS UINT32_C(0x04) #define LIBNBD_STRICT_ZERO_SIZE UINT32_C(0x08) #define LIBNBD_STRICT_ALIGN UINT32_C(0x10) #define LIBNBD_STRICT_MASK UINT32_C(0x1f) which creates values of uint_least32_t -- but that type can (in theory) still be promoted to "int", if "int" is large enough. So I think the "u" suffix is best.

+ if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Handle graceful server rejection of oversize request */ + check_server_fail (nbd, + nbd_aio_pwrite (nbd, buf, 17*1024*1024, 0, + NBD_NULL_COMPLETION, 0), + "17M nbd_aio_pwrite", EINVAL); + + /* Handle abrupt server rejection of oversize request */ + check_server_fail (nbd, + nbd_aio_pwrite (nbd, buf, 33*1024*1024, 0, + NBD_NULL_COMPLETION, 0), + "33M nbd_aio_pwrite", ENOTCONN); + + nbd_close (nbd); + exit (EXIT_SUCCESS); +} diff --git a/tests/errors-server-unaligned.c b/tests/errors-server-unaligned.c index ac374976..6dbd6e29 100644 --- a/tests/errors-server-unaligned.c +++ b/tests/errors-server-unaligned.c @@ -123,6 +123,8 @@ main (int argc, char *argv[]) nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED)); printf ("server block size maximum: %" PRIi64 "\n", nbd_get_block_size (nbd, LIBNBD_SIZE_MAXIMUM)); + printf ("libnbd payload size maximum: %" PRIi64 "\n", + nbd_get_block_size (nbd, LIBNBD_SIZE_PAYLOAD)); fflush (stdout);

/* Send an unaligned read, server-side */ diff --git a/.gitignore b/.gitignore index fe929d6d..f4273713 100644 --- a/.gitignore +++ b/.gitignore @@ -215,6 +215,7 @@ Makefile.in /tests/errors-poll-no-fd /tests/errors-pread-structured /tests/errors-server-invalid-offset +/tests/errors-server-oversize /tests/errors-server-unadvertised-cmd /tests/errors-server-unaligned /tests/errors-server-unknown-flags

Looks good to me otherwise: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo