[PATCH nbdkit v2 0/7] server: Add new plugin/filter .block_size
v1 was posted here: https://listman.redhat.com/archives/libguestfs/2022-February/thread.html#002... The changes since v1: Extra checks for plugin: * minimum between 1 and 64K * preferred must be a power of 2 * prefered must be 512 .. 32M * maximum must be 0xffff_ffff or a multiple of minimum Same extra checks added to the filter Add the tls-fallback filter patch. This has to be added early on to avoid a bisection problem with the tests. Documentation * fixed minor typos * document what "preferred" means wrt read-modify-write * document returning all zeroes to mean no information Rename new filter to "nbdkit-blocksize-policy-filter" Document how to use nbdkit-blocksize-filter + blocksize-policy together Implement error policy * add new test to cover it However I chose _not_ to implement other error policies. We can extend them in future if required. Rich.
This commit implements backend_block_size() and wires it up to a new .block_size callback from the plugin through any filters on top. This callback will allow plugins to pass their minimum, preferred and maximum block size through the NBD protocol NBD_INFO_BLOCK_SIZE info option. The new function is not called from anywhere yet, so for the moment this commit does nothing. --- include/nbdkit-filter.h | 4 +++ include/nbdkit-plugin.h | 3 ++ server/internal.h | 9 ++++++ server/backend.c | 29 +++++++++++++++++++ server/filters.c | 17 +++++++++++ server/plugins.c | 62 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+) diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 8b587d70..4c620b4f 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -84,6 +84,8 @@ struct nbdkit_next_ops { /* These callbacks are the same as normal plugin operations. */ int64_t (*get_size) (nbdkit_next *nxdata); const char * (*export_description) (nbdkit_next *nxdata); + int (*block_size) (nbdkit_next *nxdata, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum); int (*can_write) (nbdkit_next *nxdata); int (*can_flush) (nbdkit_next *nxdata); @@ -219,6 +221,8 @@ struct nbdkit_filter { int64_t (*get_size) (nbdkit_next *next, void *handle); const char * (*export_description) (nbdkit_next *next, void *handle); + int (*block_size) (nbdkit_next *next, void *handle, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum); int (*can_write) (nbdkit_next *next, void *handle); diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index 59ec11b6..50ff93df 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -146,6 +146,9 @@ struct nbdkit_plugin { const char * (*export_description) (void *handle); void (*cleanup) (void); + + int (*block_size) (void *handle, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum); }; NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err)); diff --git a/server/internal.h b/server/internal.h index f4843164..c91d196e 100644 --- a/server/internal.h +++ b/server/internal.h @@ -216,6 +216,9 @@ struct context { unsigned char state; /* Bitmask of HANDLE_* values */ uint64_t exportsize; + uint32_t minimum_block_size; + uint32_t preferred_block_size; + uint32_t maximum_block_size; int can_write; int can_flush; int is_rotational; @@ -368,6 +371,8 @@ struct backend { const char *(*export_description) (struct context *); int64_t (*get_size) (struct context *); + int (*block_size) (struct context *, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum); int (*can_write) (struct context *); int (*can_flush) (struct context *); int (*is_rotational) (struct context *); @@ -433,6 +438,10 @@ extern const char *backend_export_description (struct context *c) __attribute__((__nonnull__ (1))); extern int64_t backend_get_size (struct context *c) __attribute__((__nonnull__ (1))); +extern int backend_block_size (struct context *c, + uint32_t *minimum, uint32_t *preferred, + uint32_t *maximum) + __attribute__((__nonnull__ (1, 2, 3, 4))); extern int backend_can_write (struct context *c) __attribute__((__nonnull__ (1))); extern int backend_can_flush (struct context *c) diff --git a/server/backend.c b/server/backend.c index 1bb3dac9..7d46b99d 100644 --- a/server/backend.c +++ b/server/backend.c @@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = { .finalize = backend_finalize, .export_description = backend_export_description, .get_size = backend_get_size, + .block_size = backend_block_size, .can_write = backend_can_write, .can_flush = backend_can_flush, .is_rotational = backend_is_rotational, @@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char *exportname, c->conn = shared ? NULL : conn; c->state = 0; c->exportsize = -1; + c->minimum_block_size = c->preferred_block_size = c->maximum_block_size = -1; c->can_write = readonly ? 0 : -1; c->can_flush = -1; c->is_rotational = -1; @@ -418,6 +420,33 @@ backend_get_size (struct context *c) return c->exportsize; } +int +backend_block_size (struct context *c, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + PUSH_CONTEXT_FOR_SCOPE (c); + struct backend *b = c->b; + int r; + + assert (c->handle && (c->state & HANDLE_CONNECTED)); + if (c->minimum_block_size != -1) { + *minimum = c->minimum_block_size; + *preferred = c->preferred_block_size; + *maximum = c->maximum_block_size; + return 0; + } + else { + controlpath_debug ("%s: block_size", b->name); + r = b->block_size (c, minimum, preferred, maximum); + if (r == 0) { + c->minimum_block_size = *minimum; + c->preferred_block_size = *preferred; + c->maximum_block_size = *maximum; + } + return r; + } +} + int backend_can_write (struct context *c) { diff --git a/server/filters.c b/server/filters.c index 93ee55f0..ae7e1a56 100644 --- a/server/filters.c +++ b/server/filters.c @@ -358,6 +358,22 @@ filter_get_size (struct context *c) return backend_get_size (c_next); } +static int +filter_block_size (struct context *c, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + struct backend *b = c->b; + struct backend_filter *f = container_of (b, struct backend_filter, backend); + struct context *c_next = c->c_next; + + if (f->filter.block_size) + return f->filter.block_size (c_next, c->handle, + minimum, preferred, maximum); + else + return backend_block_size (c_next, + minimum, preferred, maximum); +} + static int filter_can_write (struct context *c) { @@ -621,6 +637,7 @@ static struct backend filter_functions = { .close = filter_close, .export_description = filter_export_description, .get_size = filter_get_size, + .block_size = filter_block_size, .can_write = filter_can_write, .can_flush = filter_can_flush, .is_rotational = filter_is_rotational, diff --git a/server/plugins.c b/server/plugins.c index b633c107..346c6d52 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -46,6 +46,7 @@ #include "internal.h" #include "minmax.h" +#include "ispowerof2.h" /* We extend the generic backend struct with extra fields relating * to this plugin. @@ -173,6 +174,7 @@ plugin_dump_fields (struct backend *b) HAS (close); HAS (export_description); HAS (get_size); + HAS (block_size); HAS (can_write); HAS (can_flush); HAS (is_rotational); @@ -408,6 +410,65 @@ plugin_get_size (struct context *c) return p->plugin.get_size (c->handle); } +static int +plugin_block_size (struct context *c, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + struct backend *b = c->b; + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + int r; + + if (p->plugin.block_size) { + r = p->plugin.block_size (c->handle, minimum, preferred, maximum); + if (r == 0) { + /* To make scripting easier, it's permitted to set + * minimum = preferred = maximum = 0 and return 0. + * That means "no information", and works the same + * way as the else clause below. + */ + if (*minimum == 0 && *preferred == 0 && *maximum == 0) + return 0; + + if (*minimum < 1 || *minimum > 65536) { + nbdkit_error ("plugin must set minimum block size between 1 and 64K"); + r = -1; + } + if (! is_power_of_2 (*minimum)) { + nbdkit_error ("plugin must set minimum block size to a power of 2"); + r = -1; + } + if (! is_power_of_2 (*preferred)) { + nbdkit_error ("plugin must set preferred block size to a power of 2"); + r = -1; + } + if (*preferred < 512 || *preferred > 32 * 1024 * 1024) { + nbdkit_error ("plugin must set preferred block size " + "between 512 and 32M"); + r = -1; + } + if (*maximum != (uint32_t)-1 && (*maximum % *minimum) != 0) { + nbdkit_error ("plugin must set maximum block size " + "to -1 or a multiple of minimum block size"); + r = -1; + } + if (*minimum > *preferred || *preferred > *maximum) { + nbdkit_error ("plugin must set minimum block size " + "<= preferred <= maximum"); + r = -1; + } + } + return r; + } + else { + /* If there is no .block_size call then return minimum = preferred + * = maximum = 0, which is a sentinel meaning don't send the + * NBD_INFO_BLOCK_SIZE message. + */ + *minimum = *preferred = *maximum = 0; + return 0; + } +} + static int normalize_bool (int value) { @@ -824,6 +885,7 @@ static struct backend plugin_functions = { .close = plugin_close, .export_description = plugin_export_description, .get_size = plugin_get_size, + .block_size = plugin_block_size, .can_write = plugin_can_write, .can_flush = plugin_can_flush, .is_rotational = plugin_is_rotational, -- 2.35.1
On Thu, Feb 17, 2022 at 02:36:42PM +0000, Richard W.M. Jones wrote:
This commit implements backend_block_size() and wires it up to a new .block_size callback from the plugin through any filters on top.
This callback will allow plugins to pass their minimum, preferred and maximum block size through the NBD protocol NBD_INFO_BLOCK_SIZE info option.
The new function is not called from anywhere yet, so for the moment this commit does nothing. --- include/nbdkit-filter.h | 4 +++ include/nbdkit-plugin.h | 3 ++ server/internal.h | 9 ++++++ server/backend.c | 29 +++++++++++++++++++ server/filters.c | 17 +++++++++++ server/plugins.c | 62 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+)
I may still play with adding a 'bool strict' parameter as a followup series on top of yours in the next couple of days; as long as I post it prior to this going in a stable release, we will be able to see what the difference would look like. Or after playing with it, I may also decide that the work to add in the parameter just for the sake of the nbd plugin turns out too complex, at which point living with your signature would be the solution anyways. So I don't see a reason to hold up your series just waiting for me to do a proposed followup on top.
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 8b587d70..4c620b4f 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -84,6 +84,8 @@ struct nbdkit_next_ops { /* These callbacks are the same as normal plugin operations. */ int64_t (*get_size) (nbdkit_next *nxdata); const char * (*export_description) (nbdkit_next *nxdata); + int (*block_size) (nbdkit_next *nxdata, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum);
I think even when we add 64-bit extensions that we will still want to keep maximum as a 32-bit size, because it controls the buffer size we pass to read and write. Part of the 64-bit extension work is figuring out how a server can advertise that it supports 64-bit zero/trim/cache even when it is limited to 32-bit (more specifically, 32M or 64M) read/write; so I will probably be proposing an additional NBD_INFO_* tag for advertising a 64-bit non-data limit down the road. In fact, maybe we should plan for that right away, by having four parameters, ending with 'uint32_t *maxdata, uint64_t *maxlen'. This would match the fact that right now, the blocksize filter allows maxdata=32M maxlen=3G to break up large pwrite requests differently than large zero requests. For the existing NBD protocol, only the first three will be advertised to the client (and we ignore the fourth for now), but down the road with 64-bit extensions, it will make it easier to advertise a 64-bit non-data max length alongside the existing 3 32-bit constraints, as the NBD protocol gains another extension. The alternative is that if NBD gains a new NBD_INFO_* tag, we would have to add a new plugin callback to supply the value for that tag. I still think that it is nicer to have one parameter for tag we know about (whether or not we can send the tag to the guest). I can think of a more extensible interface, shown below, but think it is harder for plugins fto use correctly, and therefore do not recommend it: enum nbdkit_size_tag { NBDKIT_SIZE_TAG_MINIMUM = 0, NBDKIT_SIZE_TAG_PREFERRED = 1, NBDKIT_SIZE_TAG_MAXDATA = 2, NBDKIT_SIZE_TAG_MAXLEN = 3, }; struct nbdkit_size_constraint_set; /* opaque */ int (*block_size) (nbdkit_next *next, void *handle, nbdkit_size_constraint_set *set); int nbdkit_size_constraint_add (nbdkit_size_constraint_set *set, enum nbdkit_size_tag tag, uint64_t value); where the plugin would then call something like: plugin_block_size (...) { nbdkit_size_constraint_add (set, NBDKIT_SIZE_TAG_MINIMUM, 1); nbdkit_size_constraint_add (set, NBDKIT_SIZE_TAG_PREFERRED, 64 * 1024); nbdkit_size_constraint_add (set, NBDKIT_SIZE_TAG_MAXLEN, 1ULL<<40); }
+++ b/server/backend.c @@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = { .finalize = backend_finalize, .export_description = backend_export_description, .get_size = backend_get_size, + .block_size = backend_block_size, .can_write = backend_can_write, .can_flush = backend_can_flush, .is_rotational = backend_is_rotational, @@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char *exportname, c->conn = shared ? NULL : conn; c->state = 0; c->exportsize = -1; + c->minimum_block_size = c->preferred_block_size = c->maximum_block_size = -1;
So we actually implement two sentinels - all -1 (we haven't initialized yet, and a plugin cannot explicitly set things to this value, because it violates minimum <= 64k), and all 0 (initialized but nothing to advertise), in addition to all other values (initialized, and passed our constraint validator so worth advertising).
+static int +plugin_block_size (struct context *c, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + struct backend *b = c->b; + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + int r; + + if (p->plugin.block_size) { + r = p->plugin.block_size (c->handle, minimum, preferred, maximum); + if (r == 0) { + /* To make scripting easier, it's permitted to set + * minimum = preferred = maximum = 0 and return 0. + * That means "no information", and works the same + * way as the else clause below. + */ + if (*minimum == 0 && *preferred == 0 && *maximum == 0) + return 0; + + if (*minimum < 1 || *minimum > 65536) { + nbdkit_error ("plugin must set minimum block size between 1 and 64K"); + r = -1; + } + if (! is_power_of_2 (*minimum)) { + nbdkit_error ("plugin must set minimum block size to a power of 2"); + r = -1; + }
Did we want to stick all of the constraint validation code in a common helper function, rather than open-coding it at each place that wants to perform validation? Otherwise, this is looking good. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Feb 17, 2022 at 10:19:54AM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 02:36:42PM +0000, Richard W.M. Jones wrote:
This commit implements backend_block_size() and wires it up to a new .block_size callback from the plugin through any filters on top.
This callback will allow plugins to pass their minimum, preferred and maximum block size through the NBD protocol NBD_INFO_BLOCK_SIZE info option.
The new function is not called from anywhere yet, so for the moment this commit does nothing. --- include/nbdkit-filter.h | 4 +++ include/nbdkit-plugin.h | 3 ++ server/internal.h | 9 ++++++ server/backend.c | 29 +++++++++++++++++++ server/filters.c | 17 +++++++++++ server/plugins.c | 62 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+)
I may still play with adding a 'bool strict' parameter as a followup series on top of yours in the next couple of days; as long as I post it prior to this going in a stable release, we will be able to see what the difference would look like. Or after playing with it, I may also decide that the work to add in the parameter just for the sake of the nbd plugin turns out too complex, at which point living with your signature would be the solution anyways. So I don't see a reason to hold up your series just waiting for me to do a proposed followup on top.
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 8b587d70..4c620b4f 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -84,6 +84,8 @@ struct nbdkit_next_ops { /* These callbacks are the same as normal plugin operations. */ int64_t (*get_size) (nbdkit_next *nxdata); const char * (*export_description) (nbdkit_next *nxdata); + int (*block_size) (nbdkit_next *nxdata, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum);
I think even when we add 64-bit extensions that we will still want to keep maximum as a 32-bit size, because it controls the buffer size we pass to read and write. Part of the 64-bit extension work is figuring out how a server can advertise that it supports 64-bit zero/trim/cache even when it is limited to 32-bit (more specifically, 32M or 64M) read/write; so I will probably be proposing an additional NBD_INFO_* tag for advertising a 64-bit non-data limit down the road.
In fact, maybe we should plan for that right away, by having four parameters, ending with 'uint32_t *maxdata, uint64_t *maxlen'. This would match the fact that right now, the blocksize filter allows maxdata=32M maxlen=3G to break up large pwrite requests differently than large zero requests. For the existing NBD protocol, only the first three will be advertised to the client (and we ignore the fourth for now), but down the road with 64-bit extensions, it will make it easier to advertise a 64-bit non-data max length alongside the existing 3 32-bit constraints, as the NBD protocol gains another extension.
Presuming that we're going to need a new NBDKIT_API_VERSION for this anyway, we can in fact change the signature of .block_size by adding a new parameter. This doesn't break backwards compatiblity. [...]
+++ b/server/backend.c @@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = { .finalize = backend_finalize, .export_description = backend_export_description, .get_size = backend_get_size, + .block_size = backend_block_size, .can_write = backend_can_write, .can_flush = backend_can_flush, .is_rotational = backend_is_rotational, @@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char *exportname, c->conn = shared ? NULL : conn; c->state = 0; c->exportsize = -1; + c->minimum_block_size = c->preferred_block_size = c->maximum_block_size = -1;
So we actually implement two sentinels - all -1 (we haven't initialized yet, and a plugin cannot explicitly set things to this value, because it violates minimum <= 64k), and all 0 (initialized but nothing to advertise), in addition to all other values (initialized, and passed our constraint validator so worth advertising).
Hopefully the -1 sentinel is never visible outside the server, and could in fact be changed (eg. adding an extra "initialized" boolean). If I've got it right, it should neither be seen by plugins, filters or the client(!), nor should the plugin or filter be allowed to set any of these to -1, except maximum.
+static int +plugin_block_size (struct context *c, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + struct backend *b = c->b; + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + int r; + + if (p->plugin.block_size) { + r = p->plugin.block_size (c->handle, minimum, preferred, maximum); + if (r == 0) { + /* To make scripting easier, it's permitted to set + * minimum = preferred = maximum = 0 and return 0. + * That means "no information", and works the same + * way as the else clause below. + */ + if (*minimum == 0 && *preferred == 0 && *maximum == 0) + return 0; + + if (*minimum < 1 || *minimum > 65536) { + nbdkit_error ("plugin must set minimum block size between 1 and 64K"); + r = -1; + } + if (! is_power_of_2 (*minimum)) { + nbdkit_error ("plugin must set minimum block size to a power of 2"); + r = -1; + }
Did we want to stick all of the constraint validation code in a common helper function, rather than open-coding it at each place that wants to perform validation?
The code is a bit different unfortunately .. In the filter code we have to consider the case where some of the config_* variables are unset. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Thu, Feb 17, 2022 at 05:51:59PM +0000, Richard W.M. Jones wrote:
In fact, maybe we should plan for that right away, by having four parameters, ending with 'uint32_t *maxdata, uint64_t *maxlen'. This would match the fact that right now, the blocksize filter allows maxdata=32M maxlen=3G to break up large pwrite requests differently than large zero requests. For the existing NBD protocol, only the first three will be advertised to the client (and we ignore the fourth for now), but down the road with 64-bit extensions, it will make it easier to advertise a 64-bit non-data max length alongside the existing 3 32-bit constraints, as the NBD protocol gains another extension.
Presuming that we're going to need a new NBDKIT_API_VERSION for this anyway, we can in fact change the signature of .block_size by adding a new parameter. This doesn't break backwards compatiblity.
Indeed, and part of my work on 64-bit extensions is figuring out how to implement NBDKIT_API_VERSION 3 for plugins. Maybe it's worth an addition in TODO to remind us of yet another thing worth visiting as part of v3 api.
[...]
+++ b/server/backend.c @@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = { .finalize = backend_finalize, .export_description = backend_export_description, .get_size = backend_get_size, + .block_size = backend_block_size, .can_write = backend_can_write, .can_flush = backend_can_flush, .is_rotational = backend_is_rotational, @@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char *exportname, c->conn = shared ? NULL : conn; c->state = 0; c->exportsize = -1; + c->minimum_block_size = c->preferred_block_size = c->maximum_block_size = -1;
So we actually implement two sentinels - all -1 (we haven't initialized yet, and a plugin cannot explicitly set things to this value, because it violates minimum <= 64k), and all 0 (initialized but nothing to advertise), in addition to all other values (initialized, and passed our constraint validator so worth advertising).
Hopefully the -1 sentinel is never visible outside the server, and could in fact be changed (eg. adding an extra "initialized" boolean).
If I've got it right, it should neither be seen by plugins, filters or the client(!), nor should the plugin or filter be allowed to set any of these to -1, except maximum.
It also looked that way to me - other than maximum of -1 (which the NBD protocol permits), I could not spot anywhere that you would leak -1 unintended.
Did we want to stick all of the constraint validation code in a common helper function, rather than open-coding it at each place that wants to perform validation?
The code is a bit different unfortunately .. In the filter code we have to consider the case where some of the config_* variables are unset.
Yeah, having re-read blocksize-policy code, that is indeed the case (we don't know the full picture at .config_complete). So living with the near-duplication is probably the best we can do for now. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL. This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next). To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index fab9e58b..b34e0431 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -138,6 +138,20 @@ tls_fallback_get_size (nbdkit_next *next, return next->get_size (next); } +static int +tls_fallback_block_size (nbdkit_next *next, + void *handle, + uint32_t *minimum, + uint32_t *preferred, + uint32_t *maximum) +{ + if (NOT_TLS) { + *minimum = *preferred = *maximum = 0; + return 0; + } + return next->block_size (next, minimum, preferred, maximum); +} + static int tls_fallback_can_write (nbdkit_next *next, void *handle) @@ -215,6 +229,7 @@ static struct nbdkit_filter filter = { .open = tls_fallback_open, .export_description = tls_fallback_export_description, .get_size = tls_fallback_get_size, + .block_size = tls_fallback_block_size, .can_write = tls_fallback_can_write, .can_flush = tls_fallback_can_flush, .is_rotational = tls_fallback_is_rotational, -- 2.35.1
On Thu, Feb 17, 2022 at 02:36:43PM +0000, Richard W.M. Jones wrote:
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL.
This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next).
To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
ACK. Would you like to squash this in, and/or have me commit this separately? commit 8c00ca2fe418aeecf0818feed227a72e76d87f18 Author: Eric Blake <eblake@redhat.com> Date: Thu Feb 17 10:24:50 2022 -0600 tls-fallback: Enhance comments about required callbacks Rich ran into a crash while trying to add a new .block_size callback reachable during the client connection handshake (via .prepare), and nearly worked around the crash by reintroducing CVE-2019-14850 instead of the proper fix of implementing the new callback. Add some comments to help us avoid a similar regression the next time we add a callback. diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index fab9e58b..64e84926 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -107,7 +107,11 @@ tls_fallback_open (nbdkit_next_open *next, nbdkit_context *nxdata, const char *exportname, int is_tls) { /* We do NOT want to call next() when insecure, because we don't - * know how long it will take. + * know how long it will take. See also CVE-2019-14850 in + * nbdkit-security.pod. But that means that this filter must + * override every possible callback that can be reached during + * handshake, to avoid passing through a non-TLS call to a missing + * backend. */ if (!is_tls) return &message; /* See NOT_TLS for this choice of handle */ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Feb 17, 2022 at 10:30:13AM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 02:36:43PM +0000, Richard W.M. Jones wrote:
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL.
This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next).
To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
ACK.
Would you like to squash this in, and/or have me commit this separately?
I was actually thinking about squashing my patches 1-4 together. They're all really the same change, but I kept them separate for ease of review. What do you think? But I think this patch:
commit 8c00ca2fe418aeecf0818feed227a72e76d87f18 Author: Eric Blake <eblake@redhat.com> Date: Thu Feb 17 10:24:50 2022 -0600
tls-fallback: Enhance comments about required callbacks
Rich ran into a crash while trying to add a new .block_size callback reachable during the client connection handshake (via .prepare), and nearly worked around the crash by reintroducing CVE-2019-14850 instead of the proper fix of implementing the new callback. Add some comments to help us avoid a similar regression the next time we add a callback.
diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index fab9e58b..64e84926 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -107,7 +107,11 @@ tls_fallback_open (nbdkit_next_open *next, nbdkit_context *nxdata, const char *exportname, int is_tls) { /* We do NOT want to call next() when insecure, because we don't - * know how long it will take. + * know how long it will take. See also CVE-2019-14850 in + * nbdkit-security.pod. But that means that this filter must + * override every possible callback that can be reached during + * handshake, to avoid passing through a non-TLS call to a missing + * backend. */ if (!is_tls) return &message; /* See NOT_TLS for this choice of handle */
... would stay separate, and you can push it before or after. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Thu, Feb 17, 2022 at 04:41:31PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 17, 2022 at 10:30:13AM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 02:36:43PM +0000, Richard W.M. Jones wrote:
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL.
This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next).
To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
ACK.
Would you like to squash this in, and/or have me commit this separately?
I was actually thinking about squashing my patches 1-4 together. They're all really the same change, but I kept them separate for ease of review. What do you think?
Seems reasonable (I'll confirm it again when I get through reviewing 4).
But I think this patch:
commit 8c00ca2fe418aeecf0818feed227a72e76d87f18 Author: Eric Blake <eblake@redhat.com> Date: Thu Feb 17 10:24:50 2022 -0600
tls-fallback: Enhance comments about required callbacks
... would stay separate, and you can push it before or after.
Before - it is now commit 8c00ca2f ;) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 02/17/22 17:56, Eric Blake wrote:
On Thu, Feb 17, 2022 at 04:41:31PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 17, 2022 at 10:30:13AM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 02:36:43PM +0000, Richard W.M. Jones wrote:
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL.
This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next).
To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
ACK.
Would you like to squash this in, and/or have me commit this separately?
I was actually thinking about squashing my patches 1-4 together. They're all really the same change, but I kept them separate for ease of review. What do you think?
I think patch#2 should be squashed into patch#1 (and the commit messages should be merged). However, I'd suggest keeping each of patches #3 and #4 separate. Patch#2 fixes a bug exposed by patch#1, so patch#1 in itself doesn't just start introducing a feature, but also introduces (effectively) a bug (a crash), temporarily. That should be avoided. However, erecting a feature in multiple stages (patches #3 and #4) is what all feature series should do. It's natural that a feature doesn't (fully) work until the end of the series. Also, "ease of review" is not necessarily a one-time benefit; if we need to look at these patches in some years, I think we'll appreciate a fine granularity in the git history. Thanks, Laszlo
Seems reasonable (I'll confirm it again when I get through reviewing 4).
But I think this patch:
commit 8c00ca2fe418aeecf0818feed227a72e76d87f18 Author: Eric Blake <eblake@redhat.com> Date: Thu Feb 17 10:24:50 2022 -0600
tls-fallback: Enhance comments about required callbacks
... would stay separate, and you can push it before or after.
Before - it is now commit 8c00ca2f ;)
If the client requests NBD_INFO_BLOCK_SIZE, _and_ either the plugin or a filter provides this information, then send a reply. Otherwise ignore the client request. --- server/protocol-handshake-newstyle.c | 69 ++++++++++++++++++++++++++-- TODO | 3 +- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 5c073af6..f3cba48f 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -195,6 +195,38 @@ send_newstyle_option_reply_info_str (uint32_t option, uint32_t reply, return 0; } +/* Send reply containing NBD_INFO_BLOCK_SIZE. */ +static int +send_newstyle_option_reply_info_block_size (uint32_t option, uint32_t reply, + uint16_t info, + uint32_t minimum, + uint32_t preferred, + uint32_t maximum) +{ + GET_CONN; + struct nbd_fixed_new_option_reply fixed_new_option_reply; + struct nbd_fixed_new_option_reply_info_block_size block_size; + + fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); + fixed_new_option_reply.option = htobe32 (option); + fixed_new_option_reply.reply = htobe32 (reply); + fixed_new_option_reply.replylen = htobe32 (14); + block_size.info = htobe16 (info); + block_size.minimum = htobe32 (minimum); + block_size.preferred = htobe32 (preferred); + block_size.maximum = htobe32 (maximum); + + if (conn->send (&fixed_new_option_reply, + sizeof fixed_new_option_reply, SEND_MORE) == -1 || + conn->send (&block_size, + sizeof block_size, 0) == -1) { + nbdkit_error ("write: %s: %m", name_of_nbd_opt (option)); + return -1; + } + + return 0; +} + static int send_newstyle_option_reply_meta_context (uint32_t option, uint32_t reply, uint32_t context_id, @@ -589,10 +621,10 @@ negotiate_handshake_newstyle_options (void) exportsize) == -1) return -1; - /* For now we send NBD_INFO_NAME and NBD_INFO_DESCRIPTION if - * requested, and ignore all other info requests (including - * NBD_INFO_EXPORT if it was requested, because we replied - * already above). + /* For now we send NBD_INFO_NAME, NBD_INFO_DESCRIPTION and + * NBD_INFO_BLOCK_SIZE if requested, and ignore all other info + * requests (including NBD_INFO_EXPORT if it was requested, + * because we replied already above). */ for (i = 0; i < nrinfos; ++i) { memcpy (&info, &data[4 + exportnamelen + 2 + i*2], 2); @@ -637,6 +669,35 @@ negotiate_handshake_newstyle_options (void) return -1; } break; + case NBD_INFO_BLOCK_SIZE: + { + uint32_t minimum, preferred, maximum; + int r = backend_block_size (conn->top_context, + &minimum, &preferred, &maximum); + + if (r == -1) { + debug ("newstyle negotiation: %s: " + "NBD_INFO_BLOCK_SIZE: error from plugin, " + "ignoring client request", + optname); + break; + } + if (minimum == 0) { + debug ("newstyle negotiation: %s: " + "NBD_INFO_BLOCK_SIZE: client requested but " + "no plugin or filter provided block size information, " + "ignoring client request", + optname); + break; + } + if (send_newstyle_option_reply_info_block_size + (option, + NBD_REP_INFO, + NBD_INFO_BLOCK_SIZE, + minimum, preferred, maximum) == -1) + return -1; + } + break; default: debug ("newstyle negotiation: %s: " "ignoring NBD_INFO_* request %u (%s)", diff --git a/TODO b/TODO index a4a0e0e7..35e68d15 100644 --- a/TODO +++ b/TODO @@ -31,8 +31,7 @@ General ideas for improvements https://www.redhat.com/archives/libguestfs/2018-January/msg00149.html * More NBD protocol features. The currently missing features are - structured replies for sparse reads, block size constraints, and - online resize. + structured replies for sparse reads, and online resize. * Add a callback to let plugins request minimum alignment for the buffer to pread/pwrite; useful for a plugin utilizing O_DIRECT or -- 2.35.1
On Thu, Feb 17, 2022 at 02:36:44PM +0000, Richard W.M. Jones wrote:
If the client requests NBD_INFO_BLOCK_SIZE, _and_ either the plugin or a filter provides this information, then send a reply. Otherwise ignore the client request. --- server/protocol-handshake-newstyle.c | 69 ++++++++++++++++++++++++++-- TODO | 3 +- 2 files changed, 66 insertions(+), 6 deletions(-)
It may be worth advertising the size solely based on whether the plugin provided .block_size with non-zero values, even if the client didn't request it, to more fully mirror what qemu-nbd does (but that's where the 'bool strict' parameter might come in handy, as qemu-nbd advertises different numbers based on whether the client requested or not). But as I mentioned in 1/7, that's something I can play with in my followup series, so we can see whether we like it.
+++ b/server/protocol-handshake-newstyle.c @@ -195,6 +195,38 @@ send_newstyle_option_reply_info_str (uint32_t option, uint32_t reply, return 0; }
+/* Send reply containing NBD_INFO_BLOCK_SIZE. */ +static int +send_newstyle_option_reply_info_block_size (uint32_t option, uint32_t reply, + uint16_t info, + uint32_t minimum, + uint32_t preferred, + uint32_t maximum)
At any rate, this helper function is good, regardless of whether we only call it on client request, or whether we call it as often as the plugin gives us values to advertise.
+{ + GET_CONN; + struct nbd_fixed_new_option_reply fixed_new_option_reply; + struct nbd_fixed_new_option_reply_info_block_size block_size; + + fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); + fixed_new_option_reply.option = htobe32 (option); + fixed_new_option_reply.reply = htobe32 (reply); + fixed_new_option_reply.replylen = htobe32 (14);
Worth a sizeof computation instead of a magic number here, to explain why we got at 14?
@@ -637,6 +669,35 @@ negotiate_handshake_newstyle_options (void) return -1; } break; + case NBD_INFO_BLOCK_SIZE: + { + uint32_t minimum, preferred, maximum; + int r = backend_block_size (conn->top_context, + &minimum, &preferred, &maximum); + + if (r == -1) { + debug ("newstyle negotiation: %s: " + "NBD_INFO_BLOCK_SIZE: error from plugin, " + "ignoring client request", + optname); + break;
Should this be 'return -1', so that the plugin isn't forced to continue serving later requests after reporting a failure at determining block sizes it wants to use? But I can also see your argument that if we trust the plugin to be robust, our communication with the client is still in sync so it isn't a fatal error...
+ } + if (minimum == 0) { + debug ("newstyle negotiation: %s: " + "NBD_INFO_BLOCK_SIZE: client requested but " + "no plugin or filter provided block size information, " + "ignoring client request", + optname); + break;
This break makes sense.
+ } + if (send_newstyle_option_reply_info_block_size + (option, + NBD_REP_INFO, + NBD_INFO_BLOCK_SIZE, + minimum, preferred, maximum) == -1) + return -1; + } + break;
...and this return -1 is absolutely right as it indicates we are out of sync communicating with the client. So despite my ramblings, I think I ended up with: ACK whether or not you s/14/sizeof(...)/ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Feb 17, 2022 at 10:39:04AM -0600, Eric Blake wrote:
+{ + GET_CONN; + struct nbd_fixed_new_option_reply fixed_new_option_reply; + struct nbd_fixed_new_option_reply_info_block_size block_size; + + fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); + fixed_new_option_reply.option = htobe32 (option); + fixed_new_option_reply.reply = htobe32 (reply); + fixed_new_option_reply.replylen = htobe32 (14);
Worth a sizeof computation instead of a magic number here, to explain why we got at 14?
Actually I copied the number straight out of the NBD spec! NBD_INFO_BLOCK_SIZE (3) ... The length MUST be 14,...
@@ -637,6 +669,35 @@ negotiate_handshake_newstyle_options (void) return -1; } break; + case NBD_INFO_BLOCK_SIZE: + { + uint32_t minimum, preferred, maximum; + int r = backend_block_size (conn->top_context, + &minimum, &preferred, &maximum); + + if (r == -1) { + debug ("newstyle negotiation: %s: " + "NBD_INFO_BLOCK_SIZE: error from plugin, " + "ignoring client request", + optname); + break;
Should this be 'return -1', so that the plugin isn't forced to continue serving later requests after reporting a failure at determining block sizes it wants to use? But I can also see your argument that if we trust the plugin to be robust, our communication with the client is still in sync so it isn't a fatal error...
Unclear. There's an argument that if the plugin returns an error then we should stop the connection, but also an argument that this is just an optional info message. Maybe return -1 makes more sense? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
--- docs/nbdkit-filter.pod | 9 +++++++++ docs/nbdkit-plugin.pod | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 36151e69..d63ee559 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -711,6 +711,15 @@ This intercepts the plugin C<.export_description> method and can be used to read or modify the export description that the NBD client will see. +=head2 C<.block_size> + + int block_size (nbdkit_next *next, void *handle, uint32_t *minimum, + uint32_t *preferred, uint32_t *maximum); + +This intercepts the plugin C<.block_size> method and can be used to +read or modify the block size constraints that the NBD client will +see. + =head2 C<.can_write> =head2 C<.can_flush> diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 329bcd31..7f1da781 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -872,6 +872,41 @@ returning a compile-time constant string, you may find C<nbdkit_strdup_intern> helpful for returning a value that avoids a memory leak. +=head2 C<.block_size> + + int block_size (void *handle, uint32_t *minimum, + uint32_t *preferred, uint32_t *maximum); + +This is called during the option negotiation phase of the protocol to +get the minimum, preferred and maximum block size (all in bytes) of +the block device. The client should obey these constraints by not +making requests which are smaller than the minimum size or larger than +the maximum size, and usually making requests of a multiple of the +preferred size. Furthermore requests should be aligned to at least +the minimum block size, and usually the preferred block size. + +"Preferred" block size in the NBD specification can be misinterpreted. +It means the I/O size which does not have a penalty for +read-modify-write. + +Even if the plugin implements this callback, this does B<not> mean +that all client requests will obey the constraints. A client could +still ignore the constraints. nbdkit passes all requests through to +the plugin, because what the plugin does depends on the plugin's +policy. It might decide to serve the requests correctly anyway, or +reject them with an error. + +The minimum block size must be E<ge> 1. The maximum block size must +be E<le> 0xffff_ffff. minimum E<le> preferred E<le> maximum. + +As a special case, the plugin may return minimum == preferred == +maximum == 0, meaning no information. + +If this callback is not used, then the NBD protocol assumes by default +minimum = 1, preferred = 4096. (Maximum block size depends on various +factors, see the NBD protocol specification, section "Block size +constraints"). + =head2 C<.can_write> int can_write (void *handle); -- 2.35.1
On Thu, Feb 17, 2022 at 02:36:45PM +0000, Richard W.M. Jones wrote:
--- docs/nbdkit-filter.pod | 9 +++++++++ docs/nbdkit-plugin.pod | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)
+++ b/docs/nbdkit-plugin.pod @@ -872,6 +872,41 @@ returning a compile-time constant string, you may find C<nbdkit_strdup_intern> helpful for returning a value that avoids a memory leak.
+=head2 C<.block_size> + + int block_size (void *handle, uint32_t *minimum, + uint32_t *preferred, uint32_t *maximum); + +This is called during the option negotiation phase of the protocol to +get the minimum, preferred and maximum block size (all in bytes) of +the block device. The client should obey these constraints by not +making requests which are smaller than the minimum size or larger than +the maximum size, and usually making requests of a multiple of the +preferred size. Furthermore requests should be aligned to at least +the minimum block size, and usually the preferred block size. + +"Preferred" block size in the NBD specification can be misinterpreted. +It means the I/O size which does not have a penalty for +read-modify-write. + +Even if the plugin implements this callback, this does B<not> mean +that all client requests will obey the constraints. A client could +still ignore the constraints. nbdkit passes all requests through to +the plugin, because what the plugin does depends on the plugin's +policy. It might decide to serve the requests correctly anyway, or +reject them with an error. + +The minimum block size must be E<ge> 1. The maximum block size must +be E<le> 0xffff_ffff. minimum E<le> preferred E<le> maximum.
Given that maximum is uint32_t, 'must be <= 0xffff_ffff' is a tautology.
+ +As a special case, the plugin may return minimum == preferred == +maximum == 0, meaning no information. + +If this callback is not used, then the NBD protocol assumes by default +minimum = 1, preferred = 4096. (Maximum block size depends on various +factors, see the NBD protocol specification, section "Block size +constraints").
ACK. I'm okay with you squashing 1-4. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
The main reason to implement block_size in these plugins first is so we then have an easy way to implement tests of the feature. --- plugins/eval/nbdkit-eval-plugin.pod | 2 + plugins/sh/nbdkit-sh-plugin.pod | 10 +++++ plugins/sh/methods.h | 3 ++ plugins/eval/eval.c | 2 + plugins/sh/methods.c | 66 +++++++++++++++++++++++++++++ plugins/sh/sh.c | 1 + 6 files changed, 84 insertions(+) diff --git a/plugins/eval/nbdkit-eval-plugin.pod b/plugins/eval/nbdkit-eval-plugin.pod index 22167ec5..fa9784ae 100644 --- a/plugins/eval/nbdkit-eval-plugin.pod +++ b/plugins/eval/nbdkit-eval-plugin.pod @@ -70,6 +70,8 @@ features): =item B<after_fork=>SCRIPT +=item B<block_size=>SCRIPT + =item B<cache=>SCRIPT =item B<can_cache=>SCRIPT diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod index c4cb9fac..aede1c65 100644 --- a/plugins/sh/nbdkit-sh-plugin.pod +++ b/plugins/sh/nbdkit-sh-plugin.pod @@ -372,6 +372,16 @@ L<nbdkit-plugin(3)/PARSING SIZE PARAMETERS>). This method is required. +=item C<block_size> + + /path/to/script block_size <handle> + +This script should print three numbers on stdout, separated by +whitespace. These are (in order) the minimum block size, the +preferred block size, and the maximum block size. You can print the +sizes in bytes or use any format understood by C<nbdkit_parse_size> +such as C<1M> (see L<nbdkit-plugin(3)/PARSING SIZE PARAMETERS>). + =item C<can_write> =item C<can_flush> diff --git a/plugins/sh/methods.h b/plugins/sh/methods.h index 42eb560c..d405ef00 100644 --- a/plugins/sh/methods.h +++ b/plugins/sh/methods.h @@ -51,6 +51,9 @@ extern void *sh_open (int readonly); extern void sh_close (void *handle); extern const char *sh_export_description (void *handle); extern int64_t sh_get_size (void *handle); +extern int sh_block_size (void *handle, + uint32_t *minimum, uint32_t *preferred, + uint32_t *maximum); extern int sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset, uint32_t flags); extern int sh_pwrite (void *handle, const void *buf, uint32_t count, diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c index b312a59c..8ce7a650 100644 --- a/plugins/eval/eval.c +++ b/plugins/eval/eval.c @@ -55,6 +55,7 @@ static char *missing; static const char *known_methods[] = { "after_fork", + "block_size", "cache", "can_cache", "can_extents", @@ -402,6 +403,7 @@ static struct nbdkit_plugin plugin = { .export_description = sh_export_description, .get_size = sh_get_size, + .block_size = sh_block_size, .can_write = sh_can_write, .can_flush = sh_can_flush, .is_rotational = sh_is_rotational, diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c index 51bc89e5..1216c212 100644 --- a/plugins/sh/methods.c +++ b/plugins/sh/methods.c @@ -526,6 +526,72 @@ sh_get_size (void *handle) } } +int +sh_block_size (void *handle, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + const char *method = "block_size"; + const char *script = get_script (method); + struct sh_handle *h = handle; + const char *args[] = { script, method, h->h, NULL }; + CLEANUP_FREE char *s = NULL; + size_t slen; + const char *delim = " \t\n"; + char *sp, *p; + int64_t r; + + switch (call_read (&s, &slen, args)) { + case OK: + if ((p = strtok_r (s, delim, &sp)) == NULL) { + parse_error: + nbdkit_error ("%s: %s method cannot be parsed", script, method); + return -1; + } + r = nbdkit_parse_size (p); + if (r == -1 || r > UINT32_MAX) + goto parse_error; + *minimum = r; + + if ((p = strtok_r (NULL, delim, &sp)) == NULL) + goto parse_error; + r = nbdkit_parse_size (p); + if (r == -1 || r > UINT32_MAX) + goto parse_error; + *preferred = r; + + if ((p = strtok_r (NULL, delim, &sp)) == NULL) + goto parse_error; + r = nbdkit_parse_size (p); + if (r == -1 || r > UINT32_MAX) + goto parse_error; + *maximum = r; + +#if 0 + nbdkit_debug ("setting block_size: " + "minimum=%" PRIu32 " " + "preferred=%" PRIu32 " " + "maximum=%" PRIu32, + *minimum, *preferred, *maximum); +#endif + return 0; + + case MISSING: + *minimum = *preferred = *maximum = 0; + return 0; + + case ERROR: + return -1; + + case RET_FALSE: + nbdkit_error ("%s: %s method returned unexpected code (3/false)", + script, method); + errno = EIO; + return -1; + + default: abort (); + } +} + int sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset, uint32_t flags) diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c index db75d386..35972104 100644 --- a/plugins/sh/sh.c +++ b/plugins/sh/sh.c @@ -307,6 +307,7 @@ static struct nbdkit_plugin plugin = { .export_description = sh_export_description, .get_size = sh_get_size, + .block_size = sh_block_size, .can_write = sh_can_write, .can_flush = sh_can_flush, .is_rotational = sh_is_rotational, -- 2.35.1
--- tests/Makefile.am | 4 +++ tests/test-block-size-constraints.sh | 50 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 16f76eb8..e888b1a0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -562,6 +562,10 @@ EXTRA_DIST += test-eflags.sh TESTS += test-export-name.sh test-export-info.sh EXTRA_DIST += test-export-name.sh test-export-info.sh +# Test block size constraints. +TESTS += test-block-size-constraints.sh +EXTRA_DIST += test-block-size-constraints.sh + # cdi plugin test. TESTS += test-cdi.sh EXTRA_DIST += test-cdi.sh diff --git a/tests/test-block-size-constraints.sh b/tests/test-block-size-constraints.sh new file mode 100755 index 00000000..658445cd --- /dev/null +++ b/tests/test-block-size-constraints.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2022 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_plugin eval +requires nbdsh -c 'print(h.get_block_size)' + +# Create an nbdkit eval plugin which presents block size constraints. +# Check the advertised block size constraints can be read. +nbdkit -U - eval \ + block_size="echo 64K 128K 32M" \ + get_size="echo 0" \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 * 1024" \ + ' -- 2.35.1
This filter can add block size constraints to plugins which don't already support them. It can also enforce an error policy for badly behaved clients which do not obey the block size constraints. --- docs/nbdkit-plugin.pod | 4 +- .../nbdkit-blocksize-policy-filter.pod | 136 +++++++ configure.ac | 2 + filters/blocksize-policy/Makefile.am | 70 ++++ tests/Makefile.am | 4 + filters/blocksize-policy/policy.c | 361 ++++++++++++++++++ tests/test-blocksize-error-policy.sh | 93 +++++ tests/test-blocksize-policy.sh | 117 ++++++ 8 files changed, 786 insertions(+), 1 deletion(-) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 7f1da781..6c9b450f 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -894,7 +894,9 @@ that all client requests will obey the constraints. A client could still ignore the constraints. nbdkit passes all requests through to the plugin, because what the plugin does depends on the plugin's policy. It might decide to serve the requests correctly anyway, or -reject them with an error. +reject them with an error. Plugins can avoid this complexity by using +L<nbdkit-blocksize-policy-filter(1)> which allows both +setting/adjusting the constraints, and selecting an error policy. The minimum block size must be E<ge> 1. The maximum block size must be E<le> 0xffff_ffff. minimum E<le> preferred E<le> maximum. diff --git a/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod b/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod new file mode 100644 index 00000000..d02ecfe2 --- /dev/null +++ b/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod @@ -0,0 +1,136 @@ +=head1 NAME + +nbdkit-blocksize-policy-filter - set minimum, preferred and +maximum block size, and apply error policy + +=head1 SYNOPSIS + + nbdkit --filter=blocksize-policy PLUGIN + [blocksize-error-policy=allow|error] + [blocksize-minimum=N] + [blocksize-preferred=N] + [blocksize-maximum=N] + +=head1 DESCRIPTION + +C<nbdkit-blocksize-policy-filter> is an L<nbdkit(1)> filter that +can add block size constraints to plugins which don't already support +then. It can also enforce an error policy for badly behaved clients +which do not obey the block size constraints. + +For more information about block size constraints, see section +"Block size constraints" in +L<https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>. + +The simplest usage is to place this filter on top of any plugin which +does not advertise block size constraints, and set the +C<blocksize-minimum>, C<blocksize-preferred> and C<blocksize-maximum> +parameters with the desired constraints. For example: + + nbdkit --filter=blocksize-policy memory 1G \ + blocksize-preferred=32K + +would adjust L<nbdkit-memory-plugin(1)> so that clients should +prefer 32K requests. You can query the NBD server advertised constraints +using L<nbdinfo(1)>: + + $ nbdinfo nbd://localhost + [...] + block_size_minimum: 1 + block_size_preferred: 32768 + block_size_maximum: 4294967295 + +The second part of this filter is adjusting the error policy when +badly behaved clients do not obey the minimum or maximum request size. +Normally nbdkit permits these requests, leaving it up to the plugin +whether it rejects the request with an error or tries to process the +request (eg. trying to split an over-large request). With this filter +you can use C<blocksize-error-policy=error> to reject these requests +in the filter with EIO error. The plugin will not see them. + +=head2 Combining with L<nbdkit-blocksize-filter(1)> + +A related filter is L<nbdkit-blocksize-filter(1)>. That filter can +split and combine requests for plugins that cannot handle requests +under or over a particular size. + +Both filters may be used together like this (note that the order of +the filters is important): + + nbdkit --filter=blocksize-policy \ + --filter=blocksize \ + PLUGIN ... \ + blocksize-error-policy=allow \ + blocksize-minimum=64K minblock=64K + +This says to advertise a minimum block size of 64K. Well-behaved +clients will obey this. Badly behaved clients will send requests +S<E<lt> 64K> which will be converted to slow 64K read-modify-write +cycles to the underlying plugin. In either case the plugin will only +see requests on 64K (or multiples of 64K) boundaries. + +=head1 PARAMETERS + +=over 4 + +=item B<blocksize-error-policy=allow> + +=item B<blocksize-error-policy=error> + +If a client sends a request which is smaller than the permitted +minimum size or larger than the permitted maximum size, or not aligned +to the minimum size, C<blocksize-error-policy> chooses what the filter +will do. The default (and also nbdkit's default) is C<allow> which +means pass the request through to the plugin. + +Use C<error> to return an EIO error back to the client. The plugin +will not see the badly formed request in this case. + +=item B<blocksize-minimum=>N + +=item B<blocksize-preferred=>N + +=item B<blocksize-maximum=>N + +Advertise minimum, preferred and/or maximum block size to the client. +Well-behaved clients should obey these constraints. + +For each parameter, you can specify it as a size (using the usual +modifiers like C<4K>). + +If the parameter is omitted then either the constraint advertised by +the plugin itself is used, or a sensible default for plugins which do +not advertise block size constraints. + +=back + +=head1 FILES + +=over 4 + +=item F<$filterdir/nbdkit-blocksize-policy-filter.so> + +The filter. + +Use C<nbdkit --dump-config> to find the location of C<$filterdir>. + +=back + +=head1 VERSION + +C<nbdkit-limit-filter> first appeared in nbdkit 1.30. + +=head1 SEE ALSO + +L<nbdkit(1)>, +L<nbdkit-blocksize-filter(1)>, +L<nbdkit-filter(3)>, +L<nbdkit-plugin(3)>. + +=head1 AUTHORS + +Richard W.M. Jones + +=head1 COPYRIGHT + +Copyright (C) 2022 Red Hat Inc. diff --git a/configure.ac b/configure.ac index 1c7200c9..f453664f 100644 --- a/configure.ac +++ b/configure.ac @@ -109,6 +109,7 @@ non_lang_plugins="\ plugins="$(echo $(printf %s\\n $lang_plugins $non_lang_plugins | sort -u))" filters="\ blocksize \ + blocksize-policy \ cache \ cacheextents \ checkwrite \ @@ -1335,6 +1336,7 @@ AC_CONFIG_FILES([Makefile plugins/zero/Makefile filters/Makefile filters/blocksize/Makefile + filters/blocksize-policy/Makefile filters/cache/Makefile filters/cacheextents/Makefile filters/checkwrite/Makefile diff --git a/filters/blocksize-policy/Makefile.am b/filters/blocksize-policy/Makefile.am new file mode 100644 index 00000000..004c8474 --- /dev/null +++ b/filters/blocksize-policy/Makefile.am @@ -0,0 +1,70 @@ +# nbdkit +# Copyright (C) 2019-2021 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +include $(top_srcdir)/common-rules.mk + +EXTRA_DIST = nbdkit-blocksize-policy-filter.pod + +filter_LTLIBRARIES = nbdkit-blocksize-policy-filter.la + +nbdkit_blocksize_policy_filter_la_SOURCES = \ + policy.c \ + $(top_srcdir)/include/nbdkit-filter.h \ + $(NULL) + +nbdkit_blocksize_policy_filter_la_CPPFLAGS = \ + -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils \ + -I$(top_srcdir)/include \ + $(NULL) +nbdkit_blocksize_policy_filter_la_CFLAGS = $(WARNINGS_CFLAGS) +nbdkit_blocksize_policy_filter_la_LDFLAGS = \ + -module -avoid-version -shared $(NO_UNDEFINED_ON_WINDOWS) \ + -Wl,--version-script=$(top_srcdir)/filters/filters.syms \ + $(NULL) +nbdkit_blocksize_policy_filter_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la \ + $(top_builddir)/common/replacements/libcompat.la \ + $(IMPORT_LIBRARY_ON_WINDOWS) \ + $(NULL) + +if HAVE_POD + +man_MANS = nbdkit-blocksize-policy-filter.1 +CLEANFILES += $(man_MANS) + +nbdkit-blocksize-policy-filter.1: nbdkit-blocksize-policy-filter.pod \ + $(top_builddir)/podwrapper.pl + $(PODWRAPPER) --section=1 --man $@ \ + --html $(top_builddir)/html/$@.html \ + $< + +endif HAVE_POD diff --git a/tests/Makefile.am b/tests/Makefile.am index e888b1a0..66156a3b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1409,6 +1409,10 @@ test_layers_filter3_la_LIBADD = $(IMPORT_LIBRARY_ON_WINDOWS) TESTS += test-blocksize.sh test-blocksize-extents.sh EXTRA_DIST += test-blocksize.sh test-blocksize-extents.sh +# blocksize-policy filter test. +TESTS += test-blocksize-policy.sh test-blocksize-error-policy.sh +EXTRA_DIST += test-blocksize-policy.sh test-blocksize-error-policy.sh + # cache filter test. TESTS += \ test-cache.sh \ diff --git a/filters/blocksize-policy/policy.c b/filters/blocksize-policy/policy.c new file mode 100644 index 00000000..2cc13ee2 --- /dev/null +++ b/filters/blocksize-policy/policy.c @@ -0,0 +1,361 @@ +/* nbdkit + * Copyright (C) 2019-2022 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdint.h> +#include <inttypes.h> +#include <string.h> +#include <errno.h> + +#include <nbdkit-filter.h> + +#include "ispowerof2.h" + +/* Block size constraints configured on the command line (0 = unset). */ +static uint32_t config_minimum; +static uint32_t config_preferred; +static uint32_t config_maximum; + +/* Error policy. */ +static enum { ALLOW, ERROR } error_policy = ALLOW; + +static int +policy_config (nbdkit_next_config *next, nbdkit_backend *nxdata, + const char *key, const char *value) +{ + int64_t r; + + if (strcmp (key, "blocksize-error-policy") == 0) { + if (strcmp (value, "allow") == 0) + error_policy = ALLOW; + else if (strcmp (value, "error") == 0) + error_policy = ERROR; + else { + nbdkit_error ("unknown %s: %s", key, value); + return -1; + } + return 0; + } + else if (strcmp (key, "blocksize-minimum") == 0) { + r = nbdkit_parse_size (value); + if (r == -1 || r > UINT32_MAX) { + parse_error: + nbdkit_error ("%s: could not parse %s", key, value); + return -1; + } + config_minimum = r; + return 0; + } + else if (strcmp (key, "blocksize-preferred") == 0) { + r = nbdkit_parse_size (value); + if (r == -1 || r > UINT32_MAX) goto parse_error; + config_preferred = r; + return 0; + } + else if (strcmp (key, "blocksize-maximum") == 0) { + r = nbdkit_parse_size (value); + if (r == -1 || r > UINT32_MAX) goto parse_error; + config_maximum = r; + return 0; + } + + return next (nxdata, key, value); +} + +static int +policy_config_complete (nbdkit_next_config_complete *next, + nbdkit_backend *nxdata) +{ + /* These checks roughly reflect the same checks made in + * server/plugins.c: plugin_block_size + */ + + if (config_minimum) { + if (! is_power_of_2 (config_minimum)) { + nbdkit_error ("blocksize-minimum must be a power of 2"); + return -1; + } + if (config_minimum > 65536) { + nbdkit_error ("blocksize-minimum must be <= 64K"); + return -1; + } + } + + if (config_preferred) { + if (! is_power_of_2 (config_preferred)) { + nbdkit_error ("blocksize-preferred must be a power of 2"); + return -1; + } + if (config_preferred < 512 || config_preferred > 32 * 1024 * 1024) { + nbdkit_error ("blocksize-preferred must be between 512 and 32M"); + return -1; + } + } + + if (config_minimum && config_maximum) { + if (config_maximum != (uint32_t)-1 && + (config_maximum % config_maximum) != 0) { + nbdkit_error ("blocksize-maximum must be -1 " + "or a multiple of blocksize-minimum"); + return -1; + } + } + + if (config_minimum && config_preferred) { + if (config_minimum > config_preferred) { + nbdkit_error ("blocksize-minimum must be <= blocksize-preferred"); + return -1; + } + } + + if (config_preferred && config_maximum) { + if (config_preferred > config_maximum) { + nbdkit_error ("blocksize-preferred must be <= blocksize-maximum"); + return -1; + } + } + + return next (nxdata); +} + +static int +policy_block_size (nbdkit_next *next, void *handle, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + /* If the user has set all of the block size parameters then we + * don't need to ask the plugin, we can go ahead and advertise them. + */ + if (config_minimum && config_preferred && config_maximum) { + *minimum = config_minimum; + *preferred = config_preferred; + *maximum = config_maximum; + return 0; + } + + /* Otherwise, ask the plugin. */ + if (next->block_size (next, minimum, preferred, maximum) == -1) + return -1; + + /* If the user of this filter didn't configure anything, then return + * the plugin values (even if unset). + */ + if (!config_minimum && !config_preferred && !config_maximum) + return 0; + + /* Now we get to the awkward case where the user configured some + * values but not others. There's all kinds of room for things to + * go wrong here, so try to check for obvious user errors as best we + * can. + */ + if (*minimum == 0) { /* Plugin didn't set anything. */ + if (config_minimum) + *minimum = config_minimum; + else + *minimum = 1; + + if (config_preferred) + *preferred = config_preferred; + else + *preferred = 4096; + + if (config_maximum) + *maximum = config_maximum; + else + *maximum = 0xffffffff; + } + else { /* Plugin set some values. */ + if (config_minimum) + *minimum = config_minimum; + + if (config_preferred) + *preferred = config_preferred; + + if (config_maximum) + *maximum = config_maximum; + } + + if (*minimum > *preferred || *preferred > *maximum) { + nbdkit_error ("computed block size values are invalid, minimum %" PRIu32 + " > preferred %" PRIu32 + " or preferred > maximum %" PRIu32, + *minimum, *preferred, *maximum); + return -1; + } + return 0; +} + +/* This function checks the error policy for all request functions below. */ +static int +check_policy (nbdkit_next *next, const char *type, + uint32_t count, uint64_t offset, int *err) +{ + uint32_t minimum, preferred, maximum; + + if (error_policy == ALLOW) + return 0; + + /* Get the current block size constraints. Note these are cached in + * the backend so if they've already been computed then this simply + * returns the cached values. The plugin is only asked once per + * connection. + */ + errno = 0; + if (next->block_size (next, &minimum, &preferred, &maximum) == -1) { + *err = errno ? : EIO; + return -1; + } + + /* If there are no constraints, allow. */ + if (minimum == 0) + return 0; + + /* Check constraints. */ + if (count < minimum) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is smaller than minimum size %" PRIu32, + type, count, minimum); + return -1; + } + if (count > maximum) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is larger than maximum size %" PRIu32, + type, count, maximum); + return -1; + } + if ((count % minimum) != 0) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is not a multiple " + "of minimum size %" PRIu32, + type, count, minimum); + return -1; + } + if ((offset % minimum) != 0) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "offset %" PRIu64 " is not aligned to a multiple " + "of minimum size %" PRIu32, + type, offset, minimum); + return -1; + } + + return 0; +} + +static int +policy_pread (nbdkit_next *next, + void *handle, void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + if (check_policy (next, "pread", count, offset, err) == -1) + return -1; + + return next->pread (next, buf, count, offset, flags, err); +} + +static int +policy_pwrite (nbdkit_next *next, + void *handle, const void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + if (check_policy (next, "write", count, offset, err) == -1) + return -1; + + return next->pwrite (next, buf, count, offset, flags, err); +} + +static int +policy_zero (nbdkit_next *next, + void *handle, uint32_t count, uint64_t offset, uint32_t flags, + int *err) +{ + if (check_policy (next, "zero", count, offset, err) == -1) + return -1; + + return next->zero (next, count, offset, flags, err); +} + +static int +policy_trim (nbdkit_next *next, + void *handle, uint32_t count, uint64_t offset, uint32_t flags, + int *err) +{ + if (check_policy (next, "trim", count, offset, err) == -1) + return -1; + + return next->trim (next, count, offset, flags, err); +} + +static int +policy_cache (nbdkit_next *next, + void *handle, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + if (check_policy (next, "cache", count, offset, err) == -1) + return -1; + + return next->cache (next, count, offset, flags, err); +} + +static int +policy_extents (nbdkit_next *next, + void *handle, uint32_t count, uint64_t offset, uint32_t flags, + struct nbdkit_extents *extents, int *err) +{ + if (check_policy (next, "extents", count, offset, err) == -1) + return -1; + + return next->extents (next, count, offset, flags, extents, err); +} + +static struct nbdkit_filter filter = { + .name = "blocksize-policy", + .longname = "nbdkit blocksize policy filter", + .config = policy_config, + .config_complete = policy_config_complete, + + .block_size = policy_block_size, + + .pread = policy_pread, + .pwrite = policy_pwrite, + .zero = policy_zero, + .trim = policy_trim, + .cache = policy_cache, + .extents = policy_extents, +}; + +NBDKIT_REGISTER_FILTER(filter) diff --git a/tests/test-blocksize-error-policy.sh b/tests/test-blocksize-error-policy.sh new file mode 100755 index 00000000..dc7d274c --- /dev/null +++ b/tests/test-blocksize-error-policy.sh @@ -0,0 +1,93 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2022 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_plugin eval +requires nbdsh -c 'print(h.get_block_size)' +requires nbdsh -c 'print(h.get_strict_mode)' +requires dd iflag=count_bytes </dev/null + +nbdkit -v -U - eval \ + block_size="echo 512 4096 1M" \ + get_size="echo 64M" \ + pread=" dd if=/dev/zero count=\$3 iflag=count_bytes " \ + --filter=blocksize-policy \ + blocksize-error-policy=error \ + --run ' +nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 512" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 4096" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1024 * 1024" \ + -c "h.set_strict_mode(h.get_strict_mode() & ~nbd.STRICT_ALIGN)" \ + -c " +# These requests should work +b = h.pread(512, 0) +b = h.pread(512, 4096) +b = h.pread(1024*1024, 0) +" \ + -c " +# Count not a multiple of minimum size +try: + h.pread(768, 0) + assert False +except nbd.Error as ex: + assert ex.errno == \"EIO\" +" \ + -c " +# Offset not a multiple of minimum size +try: + h.pread(512, 768) + assert False +except nbd.Error as ex: + assert ex.errno == \"EIO\" +" \ + -c " +# Count smaller than minimum size +try: + h.pread(256, 0) + assert False +except nbd.Error as ex: + assert ex.errno == \"EIO\" +" \ + -c " +# Count larger than maximum size +try: + h.pread(2*1024*1024, 0) + assert False +except nbd.Error as ex: + assert ex.errno == \"EIO\" +" +' diff --git a/tests/test-blocksize-policy.sh b/tests/test-blocksize-policy.sh new file mode 100755 index 00000000..4550c71f --- /dev/null +++ b/tests/test-blocksize-policy.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2022 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_plugin eval +requires nbdsh -c 'print(h.get_block_size)' + +# Run nbdkit eval + filter and check resulting block size constraints +# using some nbdsh. + +# No parameters. +nbdkit -U - eval \ + block_size="echo 64K 128K 32M" \ + get_size="echo 0" \ + --filter=blocksize-policy \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 * 1024" \ + ' + +# Adjust single values. +nbdkit -U - eval \ + block_size="echo 64K 128K 32M" \ + get_size="echo 0" \ + --filter=blocksize-policy \ + blocksize-minimum=1 \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 1" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 * 1024" \ + ' +nbdkit -U - eval \ + block_size="echo 64K 128K 32M" \ + get_size="echo 0" \ + --filter=blocksize-policy \ + blocksize-preferred=64K \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 64 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 * 1024" \ + ' +nbdkit -U - eval \ + block_size="echo 64K 128K 32M" \ + get_size="echo 0" \ + --filter=blocksize-policy \ + blocksize-maximum=1M \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1 * 1024 * 1024" \ + ' + +# Adjust all values for a plugin which is advertising. +nbdkit -U - eval \ + block_size="echo 64K 128K 32M" \ + get_size="echo 0" \ + --filter=blocksize-policy \ + blocksize-minimum=1 \ + blocksize-preferred=4K \ + blocksize-maximum=1M \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 1" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 4 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1 * 1024 * 1024" \ + ' + +# Set all values for a plugin which is not advertising. +nbdkit -U - eval \ + get_size="echo 0" \ + --filter=blocksize-policy \ + blocksize-minimum=1 \ + blocksize-preferred=4K \ + blocksize-maximum=1M \ + --run 'nbdsh \ + -u "$uri" \ + -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 1" \ + -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 4 * 1024" \ + -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1 * 1024 * 1024" \ + ' -- 2.35.1
On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones wrote:
+static int +policy_pread (nbdkit_next *next, + void *handle, void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + if (check_policy (next, "pread", count, offset, err) == -1) + return -1; + + return next->pread (next, buf, count, offset, flags, err); +} + +static int +policy_pwrite (nbdkit_next *next, + void *handle, const void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + if (check_policy (next, "write", count, offset, err) == -1)
Typo, should be "pwrite" here. Fixed in my copy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones wrote:
This filter can add block size constraints to plugins which don't already support them. It can also enforce an error policy for badly behaved clients which do not obey the block size constraints. --- + nbdkit --filter=blocksize-policy PLUGIN + [blocksize-error-policy=allow|error] + [blocksize-minimum=N] + [blocksize-preferred=N] + [blocksize-maximum=N] + +=head1 DESCRIPTION + +C<nbdkit-blocksize-policy-filter> is an L<nbdkit(1)> filter that +can add block size constraints to plugins which don't already support +then. It can also enforce an error policy for badly behaved clients
s/then/them/
+which do not obey the block size constraints. + +For more information about block size constraints, see section +"Block size constraints" in +L<https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>. + +The simplest usage is to place this filter on top of any plugin which +does not advertise block size constraints, and set the +C<blocksize-minimum>, C<blocksize-preferred> and C<blocksize-maximum> +parameters with the desired constraints. For example: + + nbdkit --filter=blocksize-policy memory 1G \ + blocksize-preferred=32K + +would adjust L<nbdkit-memory-plugin(1)> so that clients should +prefer 32K requests. You can query the NBD server advertised constraints +using L<nbdinfo(1)>: + + $ nbdinfo nbd://localhost + [...] + block_size_minimum: 1 + block_size_preferred: 32768 + block_size_maximum: 4294967295 + +The second part of this filter is adjusting the error policy when +badly behaved clients do not obey the minimum or maximum request size. +Normally nbdkit permits these requests, leaving it up to the plugin +whether it rejects the request with an error or tries to process the +request (eg. trying to split an over-large request). With this filter
Maybe also mention 'or doing a read-modify-write for an unaligned write'?
+you can use C<blocksize-error-policy=error> to reject these requests +in the filter with EIO error. The plugin will not see them. + +=head2 Combining with L<nbdkit-blocksize-filter(1)> + +A related filter is L<nbdkit-blocksize-filter(1)>. That filter can +split and combine requests for plugins that cannot handle requests +under or over a particular size. + +Both filters may be used together like this (note that the order of +the filters is important): + + nbdkit --filter=blocksize-policy \ + --filter=blocksize \ + PLUGIN ... \ + blocksize-error-policy=allow \ + blocksize-minimum=64K minblock=64K + +This says to advertise a minimum block size of 64K. Well-behaved +clients will obey this. Badly behaved clients will send requests +S<E<lt> 64K> which will be converted to slow 64K read-modify-write +cycles to the underlying plugin. In either case the plugin will only +see requests on 64K (or multiples of 64K) boundaries.
Should the blocksize filter implement .block_size? But that's a separate patch compared to this one adding a new filter. The example looks good.
+++ b/filters/blocksize-policy/policy.c
+ +static int +policy_config (nbdkit_next_config *next, nbdkit_backend *nxdata, + const char *key, const char *value) +{ + int64_t r; + + if (strcmp (key, "blocksize-error-policy") == 0) { + if (strcmp (value, "allow") == 0) + error_policy = ALLOW; + else if (strcmp (value, "error") == 0) + error_policy = ERROR; + else { + nbdkit_error ("unknown %s: %s", key, value); + return -1; + } + return 0; + } + else if (strcmp (key, "blocksize-minimum") == 0) { + r = nbdkit_parse_size (value); + if (r == -1 || r > UINT32_MAX) {
Should this be 'r <= -1' (or 'r < 0') to flag all negative values as invalid? /me goes back and re-reads the docs for nbdkit_parse_size Nope, correct as is, because nbdkit_parse_size already rejects negatives, and reserves just -1 to indicate any parse error.
+ +static int +policy_block_size (nbdkit_next *next, void *handle, + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) +{ + /* If the user has set all of the block size parameters then we + * don't need to ask the plugin, we can go ahead and advertise them. + */ + if (config_minimum && config_preferred && config_maximum) { + *minimum = config_minimum; + *preferred = config_preferred; + *maximum = config_maximum; + return 0; + } + + /* Otherwise, ask the plugin. */ + if (next->block_size (next, minimum, preferred, maximum) == -1) + return -1; + + /* If the user of this filter didn't configure anything, then return + * the plugin values (even if unset). + */ + if (!config_minimum && !config_preferred && !config_maximum) + return 0; + + /* Now we get to the awkward case where the user configured some + * values but not others. There's all kinds of room for things to + * go wrong here, so try to check for obvious user errors as best we + * can. + */ + if (*minimum == 0) { /* Plugin didn't set anything. */ + if (config_minimum) + *minimum = config_minimum; + else + *minimum = 1; + + if (config_preferred) + *preferred = config_preferred; + else + *preferred = 4096; + + if (config_maximum) + *maximum = config_maximum; + else + *maximum = 0xffffffff;
I'd be inclined to advertise 64*1024*1024 here instead of 0xffffffff, to match the actual buffer size that we know nbdkit is constrained by. Then again, doing that may cause clients to self-limit calls to .zero with at most 64M in a chunk, instead of calling it with 4G in a chunk. It's a toss-up. (The NBD protocol really does need a fourth value, to distinguish between maximum buffer and maximum length)
+ } + else { /* Plugin set some values. */ + if (config_minimum) + *minimum = config_minimum; + + if (config_preferred) + *preferred = config_preferred; + + if (config_maximum) + *maximum = config_maximum; + } + + if (*minimum > *preferred || *preferred > *maximum) { + nbdkit_error ("computed block size values are invalid, minimum %" PRIu32 + " > preferred %" PRIu32 + " or preferred > maximum %" PRIu32, + *minimum, *preferred, *maximum);
Good enough for a first cut. We may find ourselves having to refine it over time (my biggest worry is a plugin that clamps maximum to the advertised .get_length size, even if that is smaller than a hardcoded preferred size - even though right now we are rejecting that in server/plugins.c), but that is doable.
+/* This function checks the error policy for all request functions below. */ +static int +check_policy (nbdkit_next *next, const char *type, + uint32_t count, uint64_t offset, int *err)
Ah, new code compared to the last draft ;)
+{ + uint32_t minimum, preferred, maximum; + + if (error_policy == ALLOW) + return 0; + + /* Get the current block size constraints. Note these are cached in + * the backend so if they've already been computed then this simply + * returns the cached values. The plugin is only asked once per + * connection. + */ + errno = 0; + if (next->block_size (next, &minimum, &preferred, &maximum) == -1) { + *err = errno ? : EIO; + return -1; + }
Correct.
+ + /* If there are no constraints, allow. */ + if (minimum == 0) + return 0; + + /* Check constraints. */ + if (count < minimum) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is smaller than minimum size %" PRIu32, + type, count, minimum); + return -1; + } + if (count > maximum) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is larger than maximum size %" PRIu32, + type, count, maximum); + return -1; + }
This is where I worry about size constraints on data (pread/pwrite) vs. length (zero/trim/cache). Again, I'm happy to propose a patch on top of this, so we can use this as our starting point.
+ if ((count % minimum) != 0) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is not a multiple " + "of minimum size %" PRIu32, + type, count, minimum); + return -1; + } + if ((offset % minimum) != 0) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "offset %" PRIu64 " is not aligned to a multiple " + "of minimum size %" PRIu32, + type, offset, minimum); + return -1; + }
Do we also want to override .get_size, to flag (or round down) the case where the size reported by the plugin is not a multiple of the plugin's minsize, at which point those trailing bytes are inaccessbile to the client? Could be done on top. Overall the series looks good, I'll leave it up to you if you think it is ready to push now or if you want to adjust for one more round of reviews, and meanwhile I can start working on my proposed tweaks to compare on top to see how they look. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Subject: Re: [PATCH nbdkit v2 7/7] New filter: nbdkit-block-size-constraint-filter
Now also notice that I didn't change the commit message! Fixed in my local copy. On Thu, Feb 17, 2022 at 12:40:23PM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones wrote: ...
+ nbdkit --filter=blocksize-policy \ + --filter=blocksize \ + PLUGIN ... \ + blocksize-error-policy=allow \ + blocksize-minimum=64K minblock=64K + +This says to advertise a minimum block size of 64K. Well-behaved +clients will obey this. Badly behaved clients will send requests +S<E<lt> 64K> which will be converted to slow 64K read-modify-write +cycles to the underlying plugin. In either case the plugin will only +see requests on 64K (or multiples of 64K) boundaries.
Should the blocksize filter implement .block_size? But that's a separate patch compared to this one adding a new filter. The example looks good.
I think there's an argument for the blocksize filter returning minimum the same as minblock (if set) and maximum the same as MAX (maxlen, maxdata), but I think it is a separate patch. ...
+ if (config_maximum) + *maximum = config_maximum; + else + *maximum = 0xffffffff;
I'd be inclined to advertise 64*1024*1024 here instead of 0xffffffff, to match the actual buffer size that we know nbdkit is constrained by.
Then again, doing that may cause clients to self-limit calls to .zero with at most 64M in a chunk, instead of calling it with 4G in a chunk. It's a toss-up. (The NBD protocol really does need a fourth value, to distinguish between maximum buffer and maximum length)
Agreed. Clients shouldn't set > 32M pread/pwrite requests regardless of the limit, if I'm reading the spec correctly. ...
+/* This function checks the error policy for all request functions below. */ +static int +check_policy (nbdkit_next *next, const char *type, + uint32_t count, uint64_t offset, int *err)
Ah, new code compared to the last draft ;)
+{ + uint32_t minimum, preferred, maximum; + + if (error_policy == ALLOW) + return 0; + + /* Get the current block size constraints. Note these are cached in + * the backend so if they've already been computed then this simply + * returns the cached values. The plugin is only asked once per + * connection. + */ + errno = 0; + if (next->block_size (next, &minimum, &preferred, &maximum) == -1) { + *err = errno ? : EIO; + return -1; + }
Correct.
+ + /* If there are no constraints, allow. */ + if (minimum == 0) + return 0; + + /* Check constraints. */ + if (count < minimum) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is smaller than minimum size %" PRIu32, + type, count, minimum); + return -1; + } + if (count > maximum) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is larger than maximum size %" PRIu32, + type, count, maximum); + return -1; + }
This is where I worry about size constraints on data (pread/pwrite) vs. length (zero/trim/cache). Again, I'm happy to propose a patch on top of this, so we can use this as our starting point.
Hmm, this is a good point that I didn't think about. Maybe make the maximum check here conditional on it not being a non-data request?
+ if ((count % minimum) != 0) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "count %" PRIu32 " is not a multiple " + "of minimum size %" PRIu32, + type, count, minimum); + return -1; + } + if ((offset % minimum) != 0) { + *err = EIO; + nbdkit_error ("client %s request rejected: " + "offset %" PRIu64 " is not aligned to a multiple " + "of minimum size %" PRIu32, + type, offset, minimum); + return -1; + }
Do we also want to override .get_size, to flag (or round down) the case where the size reported by the plugin is not a multiple of the plugin's minsize, at which point those trailing bytes are inaccessbile to the client? Could be done on top.
Overall the series looks good, I'll leave it up to you if you think it is ready to push now or if you want to adjust for one more round of reviews, and meanwhile I can start working on my proposed tweaks to compare on top to see how they look.
I've got a bit of thinking to do on it. BTW I extended nbdkit-nbd-plugin proxy, OCaml and Python already, will send those patches separately. Rich. -- 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 Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones wrote:
This filter can add block size constraints to plugins which don't already support them. It can also enforce an error policy for badly behaved clients which do not obey the block size constraints. --- +=item B<blocksize-error-policy=allow> + +=item B<blocksize-error-policy=error> + +If a client sends a request which is smaller than the permitted +minimum size or larger than the permitted maximum size, or not aligned +to the minimum size, C<blocksize-error-policy> chooses what the filter +will do. The default (and also nbdkit's default) is C<allow> which +means pass the request through to the plugin. + +Use C<error> to return an EIO error back to the client. The plugin +will not see the badly formed request in this case. +
+ +/* This function checks the error policy for all request functions below. */ +static int +check_policy (nbdkit_next *next, const char *type, + uint32_t count, uint64_t offset, int *err) +{ + uint32_t minimum, preferred, maximum; + + if (error_policy == ALLOW) + return 0; + + /* Get the current block size constraints. Note these are cached in + * the backend so if they've already been computed then this simply + * returns the cached values. The plugin is only asked once per + * connection. + */ + errno = 0; + if (next->block_size (next, &minimum, &preferred, &maximum) == -1) { + *err = errno ? : EIO; + return -1; + }
After re-reading libnbd's nbd_get_block_size(), I noticed something: The NBD protocol spec recommends EINVAL (rather than EIO) for requests that violate block size constraints. Do we want to hardcode EINVAL, or else tweak the error policy to allow the user to specify which errno to provide? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Feb 17, 2022 at 03:58:07PM -0600, Eric Blake wrote:
The NBD protocol spec recommends EINVAL (rather than EIO) for requests that violate block size constraints. Do we want to hardcode EINVAL, or else tweak the error policy to allow the user to specify which errno to provide?
I err on not overthinking this. I'll change it to EINVAL, and if ever we need to choose another errno then we can add a knob for it. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Thu, Feb 17, 2022 at 10:00:52PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 17, 2022 at 03:58:07PM -0600, Eric Blake wrote:
The NBD protocol spec recommends EINVAL (rather than EIO) for requests that violate block size constraints. Do we want to hardcode EINVAL, or else tweak the error policy to allow the user to specify which errno to provide?
I err on not overthinking this. I'll change it to EINVAL, and if ever we need to choose another errno then we can add a knob for it.
Just noticed this XXX comment that we should now be able to test with blocksize-policy of error: https://gitlab.com/nbdkit/libnbd/-/blob/master/tests/errors.c#L464 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Feb 17, 2022 at 04:44:56PM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 10:00:52PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 17, 2022 at 03:58:07PM -0600, Eric Blake wrote:
The NBD protocol spec recommends EINVAL (rather than EIO) for requests that violate block size constraints. Do we want to hardcode EINVAL, or else tweak the error policy to allow the user to specify which errno to provide?
I err on not overthinking this. I'll change it to EINVAL, and if ever we need to choose another errno then we can add a knob for it.
Just noticed this XXX comment that we should now be able to test with blocksize-policy of error: https://gitlab.com/nbdkit/libnbd/-/blob/master/tests/errors.c#L464
Wow, that test is huge! Looking at the history it seems to have accreted new tests over several years. I've partially split it into smaller, more mangeable tests. There's more to be done before I can add that check. I may have another go at this later. https://gitlab.com/nbdkit/libnbd/-/commit/3f57d7a37a7c5a1d8495f3bc1880e22da3... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Fri, Feb 18, 2022 at 12:25:56PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 17, 2022 at 04:44:56PM -0600, Eric Blake wrote:
On Thu, Feb 17, 2022 at 10:00:52PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 17, 2022 at 03:58:07PM -0600, Eric Blake wrote:
The NBD protocol spec recommends EINVAL (rather than EIO) for requests that violate block size constraints. Do we want to hardcode EINVAL, or else tweak the error policy to allow the user to specify which errno to provide?
I err on not overthinking this. I'll change it to EINVAL, and if ever we need to choose another errno then we can add a knob for it.
Just noticed this XXX comment that we should now be able to test with blocksize-policy of error: https://gitlab.com/nbdkit/libnbd/-/blob/master/tests/errors.c#L464
Wow, that test is huge! Looking at the history it seems to have accreted new tests over several years.
I've partially split it into smaller, more mangeable tests. There's more to be done before I can add that check. I may have another go at this later.
https://gitlab.com/nbdkit/libnbd/-/commit/3f57d7a37a7c5a1d8495f3bc1880e22da3...
I fixed this and added the test: https://gitlab.com/nbdkit/libnbd/-/commit/6f5fec2ec3728e67475087fa055c8bd05b... and that test revealed a bug in the new filter (in nbdkit): https://gitlab.com/nbdkit/nbdkit/-/commit/f5dfd30f380ef1185f6fcca4e6345e0434... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones