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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
