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", + 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

On Fri, Nov 11, 2022 at 08:52:31AM +0100, Laszlo Ersek wrote: Indeed (would matter more for a platform with 16-bit int, but those are long-relegated to museum pieces now). Mostly true. However, we can argue that libnbd should default to obeying the spec unless the user opts in to intentionally pushing the boundaries, and the spec says a client should obey an advertised maximum. So enforcing the maximum at client side is a way of avoiding undefined behavior for an application using libnbd that is not paying attention to advertised maximums. Indeed, and we've already had patches in the past to things like nbdcopy to explicitly implement a default 32M buffer size to avoid tripping up qemu-nbd. Yes. Thanks for the close review; will fix and push shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

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 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; + 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 -- 2.38.1