[nbdkit PATCH 0/4] RFC: Better handling of extents alignment
This is in response to https://gitlab.com/nbdkit/libnbd/-/issues/12 where nbdinfo is having a hard time because nbdkit is outputting data that is not in compliance with the current NBD spec (which states that if client and server agreed on a minimum blocksize, then extents must be aligned to that). I still have more to write: - add a unit test for the server change at 4G (in addition to the unit test I got working for blocksize-policy tripping on the same problem with a much smaller unaligned-size data image) - libnbd: patch nbdinfo to tolerate unaligned extents from the server. Even if the spec says its wrong, existing servers in the wild do it, so we should be tolerant. - upstream NBD spec: propose weakening the standard from SHALL to SHOULD provide aligned extents - blocksize-policy: Consider patches to allow aligned offset but unaligned length IF the length extends from offset to the end of the unaligned-size disk. Document that an unaligned tail is hard to read, and suggest use of the truncate filter to deal with it. But as this has taken me the better part of today, I wanted to put out what I have so far. Eric Blake (4): human-size: Add human_size_parse_substr data: Allow scaling suffixes on @decimal blocksize-policy: Round extents to match minimum alignment server: Align the truncation of large block_status results plugins/data/nbdkit-data-plugin.pod | 3 +- tests/Makefile.am | 2 + common/include/human-size-test-cases.h | 83 ++++++++++-------- common/include/human-size.h | 57 ++++++++---- common/include/test-human-size.c | 52 +++++++++-- server/protocol.c | 12 ++- server/test-public.c | 13 +-- plugins/data/format.c | 115 ++++++++++++------------- filters/blocksize-policy/policy.c | 15 +++- tests/test-data-format.sh | 8 +- tests/test-eval-extents.sh | 6 +- 11 files changed, 230 insertions(+), 136 deletions(-) -- 2.49.0
I want to teach the data plugin how to parse @4k out of a larger input string, but that entails the need for a way to opt in to not erroring out when an unrecognized suffix is present. Some of the existing unit tests expose cases where parsing a value substring out of a larger string can have interesting results; but in the case of the data plugin, the caller will validate that any remaining suffix starts with whitespace (and not '.' or a letter), which should avoid the oddest cases due to the additional context that the unit test lacks. Signed-off-by: Eric Blake <eblake@redhat.com> --- common/include/human-size-test-cases.h | 83 ++++++++++++++------------ common/include/human-size.h | 57 ++++++++++++------ common/include/test-human-size.c | 52 ++++++++++++++-- server/test-public.c | 13 ++-- 4 files changed, 137 insertions(+), 68 deletions(-) diff --git a/common/include/human-size-test-cases.h b/common/include/human-size-test-cases.h index f351f7c5..ef845f4c 100644 --- a/common/include/human-size-test-cases.h +++ b/common/include/human-size-test-cases.h @@ -38,17 +38,22 @@ /* Just some common test cases shared (in nbdkit) between * common/include/test-human-size.c and server/test-public.c */ -static struct pair { +static struct tuple { const char *str; int64_t res; -} pairs[] = { + const char *tail; +} tuples[] = { /* Bogus strings */ { "", -1 }, - { "0x0", -1 }, + { " ", -1 }, + { "+ 1", -1 }, { "garbage", -1 }, - { "0garbage", -1 }, - { "8E", -1 }, - { "8192P", -1 }, + + /* Extracting substring value from larger string */ + { "0junk", 0, "junk" }, /* no suffix j */ + { "0garbage", 0, "arbage" }, /* 0g recognized */ + { "1 1", 1, " 1" }, + { "1M 1", 1024 * 1024, " 1" }, /* Strings leading to overflow */ { "9223372036854775808", -1 }, /* INT_MAX + 1 */ @@ -56,6 +61,8 @@ static struct pair { { "18446744073709551615", -1 }, /* UINT64_MAX */ { "18446744073709551616", -1 }, /* UINT64_MAX + 1 */ { "999999999999999999999999", -1 }, + { "8E", -1 }, + { "8192P", -1 }, /* Strings representing negative values */ { "-1", -1 }, @@ -68,39 +75,41 @@ static struct pair { { "-18446744073709551614", -1 }, /* -UINT64_MAX + 1 */ /* Strings we may want to support in the future */ - { "M", -1 }, - { "1MB", -1 }, - { "1MiB", -1 }, - { "1.5M", -1 }, + { "0x0", 0, "x0" }, /* Nicer would be 0 with tail "" */ + { "M", -1 }, /* Nicer would be 1024 * 1024 with tail "" */ + { "1MB", 1024 * 1024, "B" }, /* Nicer would be 1000 * 1000 with tail "" */ + { "1MiB", 1024 * 1024, "iB" }, /* Nicer would be 1024 * 1024 with tail "" */ + { "1.5M", 1, ".5M" }, /* Nicer would be 512 * 3 * 1024 with tail "" */ + { "1.5MB", 1, ".5MB" }, /* Nicer would be 1500 * 1000 with tail "" */ /* Valid strings */ - { "-0", 0 }, - { "0", 0 }, - { "+0", 0 }, - { " 08", 8 }, - { "1", 1 }, - { "+1", 1 }, - { "1234567890", 1234567890 }, - { "+1234567890", 1234567890 }, - { "9223372036854775807", INT64_MAX }, - { "1s", 512 }, - { "2S", 1024 }, - { "1b", 1 }, - { "1B", 1 }, - { "1k", 1024 }, - { "1K", 1024 }, - { "1m", 1024 * 1024 }, - { "1M", 1024 * 1024 }, - { "+1M", 1024 * 1024 }, - { "1g", 1024 * 1024 * 1024 }, - { "1G", 1024 * 1024 * 1024 }, - { "1t", 1024LL * 1024 * 1024 * 1024 }, - { "1T", 1024LL * 1024 * 1024 * 1024 }, - { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 }, - { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 }, - { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191 }, - { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 }, - { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 }, + { " -0", 0, "" }, + { "0", 0, "" }, + { "+0", 0, "" }, + { " 08", 8, "" }, + { "1", 1, "" }, + { "+1", 1, "" }, + { "1234567890", 1234567890, "" }, + { "+1234567890", 1234567890, "" }, + { "9223372036854775807", INT64_MAX, "" }, + { "1s", 512, "" }, + { "2S", 1024, "" }, + { "1b", 1, "" }, + { "1B", 1, "" }, + { "1k", 1024, "" }, + { "1K", 1024, "" }, + { "1m", 1024 * 1024, "" }, + { "1M", 1024 * 1024, "" }, + { "+1M", 1024 * 1024, "" }, + { "1g", 1024 * 1024 * 1024, "" }, + { "1G", 1024 * 1024 * 1024, "" }, + { "1t", 1024LL * 1024 * 1024 * 1024, "" }, + { "1T", 1024LL * 1024 * 1024 * 1024, "" }, + { "1p", 1024LL * 1024 * 1024 * 1024 * 1024, "" }, + { "1P", 1024LL * 1024 * 1024 * 1024 * 1024, "" }, + { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191, "" }, + { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024, "" }, + { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024, "" }, }; #endif /* NBDKIT_HUMAN_SIZE_TEST_CASES_H */ diff --git a/common/include/human-size.h b/common/include/human-size.h index 788dbd7b..aa551888 100644 --- a/common/include/human-size.h +++ b/common/include/human-size.h @@ -39,14 +39,17 @@ /* Attempt to parse a string with a possible scaling suffix, such as * "2M". Disk sizes cannot usefully exceed off_t (which is signed) - * and cannot be negative. + * and cannot be negative. If rest is not NULL, the number being + * parsed is treated as a substring within a larger input; if a value + * was parsed, *rest is set to the first unparsed byte of str. If + * rest is NULL, any trailing garbage is treated as an error. * * On error, returns -1 and sets *error and *pstr. You can form a * final error message by appending "<error>: <pstr>". */ static inline int64_t -human_size_parse (const char *str, - const char **error, const char **pstr) +human_size_parse_substr (const char *str, char **rest, + const char **error, const char **pstr) { int64_t size; char *end; @@ -56,6 +59,8 @@ human_size_parse (const char *str, /* XXX Should we allow hex? If so, hex cannot use scaling suffixes, * because some of them are valid hex digits. */ + if (rest) + *rest = NULL; errno = 0; size = strtoimax (str, &end, 10); if (str == end) { @@ -77,7 +82,7 @@ human_size_parse (const char *str, switch (*end) { /* No suffix */ case '\0': - end--; /* Safe, since we already filtered out empty string */ + /* Safe, since we already filtered out empty string */ break; /* Powers of 1024 */ @@ -100,6 +105,7 @@ human_size_parse (const char *str, scale *= 1024; /* fallthru */ case 'b': case 'B': + end++; break; /* "sectors", ie. units of 512 bytes, even if that's not the real @@ -107,22 +113,8 @@ human_size_parse (const char *str, */ case 's': case 'S': scale = 512; + end++; break; - - default: - *error = "could not parse size: unknown suffix"; - *pstr = end; - return -1; - } - - /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and 'MB' - * for powers of 1000, for similarity to GNU tools. But for now, - * anything beyond 'M' is dropped. - */ - if (end[1]) { - *error = "could not parse size: unknown suffix"; - *pstr = end; - return -1; } if (INT64_MAX / scale < size) { @@ -131,7 +123,34 @@ human_size_parse (const char *str, return -1; } + /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and 'MB' + * for powers of 1000, for similarity to GNU tools. But for now, + * anything beyond 'M' is dropped. + */ + if (rest) + *rest = end; + else if (*end) { + *error = "could not parse size: unknown suffix"; + *pstr = end; + return -1; + } + return size * scale; } +/* Attempt to parse a string with a possible scaling suffix, such as + * "2M". Disk sizes cannot usefully exceed off_t (which is signed) + * and cannot be negative. str must not have any trailing garbage. + * + * On error, returns -1 and sets *error and *pstr. You can form a + * final error message by appending "<error>: <pstr>". + */ +static inline int64_t +human_size_parse (const char *str, + const char **error, const char **pstr) +{ + return human_size_parse_substr (str, NULL, error, pstr); +} + + #endif /* NBDKIT_HUMAN_SIZE_H */ diff --git a/common/include/test-human-size.c b/common/include/test-human-size.c index 7f75bb4b..3e388e39 100644 --- a/common/include/test-human-size.c +++ b/common/include/test-human-size.c @@ -36,10 +36,11 @@ #include <stdlib.h> #include <stdbool.h> #include <stdint.h> +#include <string.h> #include "array-size.h" #include "human-size.h" -#include "human-size-test-cases.h" /* defines 'pairs' below */ +#include "human-size-test-cases.h" /* defines 'tuples' below */ int main (void) @@ -47,20 +48,59 @@ main (void) size_t i; bool pass = true; - for (i = 0; i < ARRAY_SIZE (pairs); i++) { + for (i = 0; i < ARRAY_SIZE (tuples); i++) { const char *error = NULL, *pstr = NULL; + char *rest = NULL; int64_t r; + int64_t expect; - r = human_size_parse (pairs[i].str, &error, &pstr); - if (r != pairs[i].res) { + r = human_size_parse (tuples[i].str, &error, &pstr); + expect = tuples[i].tail && *tuples[i].tail ? -1 : tuples[i].res; + if (r != expect) { fprintf (stderr, "Wrong parse for %s, got %" PRId64 ", expected %" PRId64 "\n", - pairs[i].str, r, pairs[i].res); + tuples[i].str, r, expect); pass = false; } if (r == -1) { if (error == NULL || pstr == NULL) { - fprintf (stderr, "Wrong error message handling for %s\n", pairs[i].str); + fprintf (stderr, "Wrong error message handling for %s\n", + tuples[i].str); + pass = false; + } + } + + r = human_size_parse_substr (tuples[i].str, &rest, &error, &pstr); + if (r != tuples[i].res) { + fprintf (stderr, + "Wrong parse for %s, got %" PRId64 ", expected %" PRId64 "\n", + tuples[i].str, r, tuples[i].res); + pass = false; + } + if (r == -1) { + if (error == NULL || pstr == NULL) { + fprintf (stderr, "Wrong error message handling for %s\n", + tuples[i].str); + pass = false; + } + if (rest != NULL) { + fprintf (stderr, + "Wrong suffix handling for %s, expected NULL, got '%s'\n", + tuples[i].str, rest); + pass = false; + } + } + else { + if (rest == NULL) { + fprintf (stderr, + "Wrong suffix handling for %s, expected '%s', got NULL\n", + tuples[i].str, tuples[i].tail); + pass = false; + } + else if (strcmp (rest, tuples[i].tail) != 0) { + fprintf (stderr, + "Wrong suffix handling for %s, expected '%s', got '%s'\n", + tuples[i].str, tuples[i].tail, rest); pass = false; } } diff --git a/server/test-public.c b/server/test-public.c index 0eddd6ca..bb30134e 100644 --- a/server/test-public.c +++ b/server/test-public.c @@ -110,7 +110,7 @@ backend_default_export (struct backend *b, int readonly) /* Unit tests. */ -#include "human-size-test-cases.h" /* defines 'pairs' below */ +#include "human-size-test-cases.h" /* defines 'tuples' below */ static bool test_nbdkit_parse_size (void) @@ -118,19 +118,20 @@ test_nbdkit_parse_size (void) size_t i; bool pass = true; - for (i = 0; i < ARRAY_SIZE (pairs); i++) { + for (i = 0; i < ARRAY_SIZE (tuples); i++) { int64_t r; + int64_t expect = tuples[i].tail && *tuples[i].tail ? -1 : tuples[i].res; error_flagged = false; - r = nbdkit_parse_size (pairs[i].str); - if (r != pairs[i].res) { + r = nbdkit_parse_size (tuples[i].str); + if (r != expect) { fprintf (stderr, "Wrong parse for %s, got %" PRId64 ", expected %" PRId64 "\n", - pairs[i].str, r, pairs[i].res); + tuples[i].str, r, expect); pass = false; } if ((r == -1) != error_flagged) { - fprintf (stderr, "Wrong error message handling for %s\n", pairs[i].str); + fprintf (stderr, "Wrong error message handling for %s\n", tuples[i].str); pass = false; } } -- 2.49.0
Although you can write 'nbdkit data "@$((4*1024)) 1"', it is more convenient to be able to write 'nbdkit data "@4k 1"'. To do that, make use of our existing code for suffix scaling when we know we have a decimal value, while still accepting non-decimal values without scaling for backwards compatability. Since we must do substring parsing (there is likely more data after the offset directive), we insist that after the value is parsed, the string must be on whitespace before the next directive. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/data/nbdkit-data-plugin.pod | 3 +- plugins/data/format.c | 115 ++++++++++++++-------------- tests/test-data-format.sh | 8 +- 3 files changed, 65 insertions(+), 61 deletions(-) diff --git a/plugins/data/nbdkit-data-plugin.pod b/plugins/data/nbdkit-data-plugin.pod index e94102ba..bb57e912 100644 --- a/plugins/data/nbdkit-data-plugin.pod +++ b/plugins/data/nbdkit-data-plugin.pod @@ -230,7 +230,8 @@ generates the 4 byte sequence C<0 0 0 1>. Moves the current offset to C<OFFSET>. The offset may be specified as either decimal, octal (prefixed by C<0>) or hexadecimal (prefixed by -C<0x>). Offset C<@0> is the first byte of the disk. +C<0x>); when using decimal, it is also possible to supply a scaling +suffix such as C<k> or C<M>. Offset C<@0> is the first byte of the disk. =item B<@+>N diff --git a/plugins/data/format.c b/plugins/data/format.c index 81255b07..796a493c 100644 --- a/plugins/data/format.c +++ b/plugins/data/format.c @@ -41,6 +41,7 @@ #include <string.h> #include <assert.h> #include <signal.h> +#include <ctype.h> #define NBDKIT_API_VERSION 2 #include <nbdkit-plugin.h> @@ -51,6 +52,7 @@ #include "checked-overflow.h" #include "cleanup.h" #include "hexdigit.h" +#include "human-size.h" #include "ispowerof2.h" #include "minmax.h" #include "nbdkit-string.h" @@ -494,6 +496,9 @@ parser (int level, const char *value, size_t *start, size_t len, int j, n; int64_t i64, m; size_t flen; + char sign; + char *end; + const char *err, *pstr; switch (value[i]) { case '#': /* # comment */ @@ -505,68 +510,60 @@ parser (int level, const char *value, size_t *start, size_t len, case '@': /* @OFFSET */ if (++i == len) goto parse_error; switch (value[i]) { - case '+': /* @+N */ - if (++i == len) goto parse_error; - if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n) == 1) { - if (i64 < 0) { - nbdkit_error ("data parameter after @+ must not be negative"); - return -1; - } - i += n; - APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, i64))); - } - else - goto parse_error; - break; - case '-': /* @-N */ - if (++i == len) goto parse_error; - if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n) == 1) { - if (i64 < 0) { - nbdkit_error ("data parameter after @- must not be negative"); - return -1; - } - i += n; - APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, -i64))); - } - else - goto parse_error; - break; - case '^': /* @^ALIGNMENT */ - if (++i == len) goto parse_error; - /* We must use %i into i64 in order to parse 0x etc. */ - if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n) == 1) { - if (i64 < 0) { - nbdkit_error ("data parameter after @^ must not be negative"); - return -1; - } - /* XXX fix this arbitrary restriction */ - if (!is_power_of_2 (i64)) { - nbdkit_error ("data parameter @^%" PRIi64 " must be a power of 2", - i64); - return -1; - } - i += n; - APPEND_EXPR (new_node (expr (EXPR_ALIGN_OFFSET, (uint64_t) i64))); - } - else - goto parse_error; - break; - case '0': case '1': case '2': case '3': case '4': - case '5': case '6': case '7': case '8': case '9': - /* We must use %i into i64 in order to parse 0x etc. */ - if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n) == 1) { - if (i64 < 0) { - nbdkit_error ("data parameter @OFFSET must not be negative"); - return -1; - } - i += n; - APPEND_EXPR (new_node (expr (EXPR_ABS_OFFSET, (uint64_t) i64))); - } - else - goto parse_error; + case '+': + case '-': + case '^': + sign = value[i++]; break; default: + sign = '\0'; + } + if (value[i] == '0') { + /* hex or octal, no scaling suffix possible */ + if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n) == 1) { + if (i64 < 0) { + nbdkit_error ("data parameter after @ must not be negative"); + return -1; + } + i += n; + } + else + goto parse_error; + } + else if (isdigit(value[i])) { + /* decimal, scaling suffix allowed */ + i64 = human_size_parse_substr (&value[i], &end, &err, &pstr); + if (i64 == -1) { + nbdkit_error ("%s: %s", err, pstr); + return -1; + } + if (*end && !isspace (*end)) + goto parse_error; + i = end - value; + } + else + /* empty string or unknown byte */ goto parse_error; + + switch (sign) { + case '+': /* @+N */ + APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, i64))); + break; + case '-': /* @-N */ + APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, -i64))); + break; + case '^': /* @^ALIGNMENT */ + /* XXX fix this arbitrary restriction */ + if (!is_power_of_2 (i64)) { + nbdkit_error ("data parameter @^%" PRIi64 " must be a power of 2", + i64); + return -1; + } + APPEND_EXPR (new_node (expr (EXPR_ALIGN_OFFSET, (uint64_t) i64))); + break; + default: + APPEND_EXPR (new_node (expr (EXPR_ABS_OFFSET, (uint64_t) i64))); + break; } break; diff --git a/tests/test-data-format.sh b/tests/test-data-format.sh index 59f2b102..43929136 100755 --- a/tests/test-data-format.sh +++ b/tests/test-data-format.sh @@ -66,7 +66,7 @@ if size > 0: actual = h.pread(size, 0) else: actual = b\"\" -def trunc(s): return s[:128] + (s[128:] and b\"...\") +def trunc(s): return s[:128] + (s[128:] and b\"...\" + s[-4:]) print(\"actual: %r\" % trunc(actual)) print(\"expected: %r\" % trunc(expected)) assert actual == expected @@ -117,6 +117,12 @@ do_test '@+4 "\x00"' 'b"\x00\x00\x00\x00\x00"' do_test '@+4 @-1 "\x00"' 'b"\x00\x00\x00\x00"' do_test '1 @^4 "\x00"' 'b"\x01\x00\x00\x00\x00"' +#---------------------------------------------------------------------- +# Larger offsets. +do_test '@1k "\x01"' 'bytearray(1024) + b"\x01"' +do_test '@0x400 "\x01"' 'bytearray(1024) + b"\x01"' +do_test '@02000 "\x01"' 'bytearray(1024) + b"\x01"' + #---------------------------------------------------------------------- # Comments. do_test '#' 'b""' -- 2.49.0
Since a client is very likely to make further I/O calls at offsets returned in a block status call (for example, 'nbdinfo --map --totals'), and the blocksize-policy filter can set a minimum alignment larger than what the plugin does by default, it makes sense to force extents to that alignment. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/Makefile.am | 2 ++ filters/blocksize-policy/policy.c | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 63730821..242a03e4 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1656,11 +1656,13 @@ TESTS += \ test-blocksize-policy.sh \ test-blocksize-error-policy.sh \ test-blocksize-write-disconnect.sh \ + test-blocksize-policy-extents.sh \ $(NULL) EXTRA_DIST += \ test-blocksize-policy.sh \ test-blocksize-error-policy.sh \ test-blocksize-write-disconnect.sh \ + test-blocksize-policy-extents.sh \ $(NULL) if HAVE_BZLIB diff --git a/filters/blocksize-policy/policy.c b/filters/blocksize-policy/policy.c index 51bea91e..d656c956 100644 --- a/filters/blocksize-policy/policy.c +++ b/filters/blocksize-policy/policy.c @@ -38,6 +38,7 @@ #include <inttypes.h> #include <string.h> #include <errno.h> +#include <assert.h> #include <nbdkit-filter.h> @@ -373,10 +374,22 @@ policy_extents (nbdkit_next *next, void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents, int *err) { + uint32_t minimum, preferred, maximum; + int res; + if (check_policy (next, handle, "extents", false, count, offset, err) == -1) return -1; - return next->extents (next, count, offset, flags, extents, err); + /* Munge the plugin's response to values that match the alignment we + * are enforcing; that way, a client that performs later operations + * at boundaries determined by the block status result will still be + * aligned. Since check_policy succeeded, policy_block_size will too. + */ + res = policy_block_size (next, handle, + &minimum, &preferred, &maximum); + assert (res == 0); + return nbdkit_extents_aligned (next, count, offset, flags, + minimum ?: 1, extents, err); } static struct nbdkit_filter filter = { -- 2.49.0
The NBD spec currently requires that block status results be aligned to any advertised minimum block size (although I'm submitting a patch in parallel to relax that slightly, to cater to the fact that not all existing servers comply). And while most plugins and filters _do_ happen to provide aligned results when given aligned inputs, we have the corner case where an aligned request that results in a 4G+ aligned response from the plugin is truncated in the server code to an unaligned answer, which can confuse clients such as 'nbdinfo --map'. Prior to advertising block sizes, the server didn't have to care about alignments: the spec only requires aligned results when the client asks and the server replies with a minimum block size. And even with the introduction of the .block_size callback, the server still doesn't have to care for reads or writes: if the client is compliant (or you use a filter like blocksize or blocksize-policy), the plugin never sees an unaligned request, and the response will have the same alignment as the request (because we don't yet implement the ability to report holes via NBD_CMD_READ - that would require the v3 plugin API). It is _only_ for extents where we are at the mercy of the plugin not splitting extents differently than what it advertised as its minimum block size, which is generally the case, with one obvious exception: any time the plugin widens an extent and reports 4G or larger, it is the server (not the plugin) that truncates the request down to the 32-bit protocol limits. But since we know that .block_size cannot set a minimum block size larger than 64k, it is far easier to just truncate the server's response at a point that is guaranteed to be aligned for all possible plugins, than to have the server try and munge alignment itself. The change here is observable in test-eval-extents.sh which was added as part of the fix for CVE-2025-47711; but fortunately, spec non-compliance is different than denial of service, so versions of nbdkit with this bug do NOT represent yet another security lapse, and the changed test is not any weaker. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/protocol.c | 12 +++++++++--- tests/test-eval-extents.sh | 6 +++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/server/protocol.c b/server/protocol.c index b4b1c162..28b1a554 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -493,13 +493,19 @@ extents_to_block_descriptors (struct nbdkit_extents *extents, if (i == 0) assert (e.offset == offset); - /* Must not exceed UINT32_MAX. */ - blocks[i].length = length = MIN (e.length, UINT32_MAX); + /* Must not exceed UINT32_MAX. However, if the plugin was + * supplying aligned answers, we don't want our truncation to + * result in an unaligned point. Rather than tracking whether a + * minimumg block size was advertised, it is simpler to just + * truncate at 64k shy of 4G, since minimum block size cannot + * exceed 64k. + */ + blocks[i].length = length = MIN (e.length, UINT32_MAX & ~0xffff); blocks[i].status_flags = e.type & 3; (*nr_blocks)++; pos += length; - if (pos >= offset + count) /* this must be the last block */ + if (pos >= offset + count || length < e.length) break; /* If we reach here then we must have consumed this whole diff --git a/tests/test-eval-extents.sh b/tests/test-eval-extents.sh index 92b503e6..af0f2885 100755 --- a/tests/test-eval-extents.sh +++ b/tests/test-eval-extents.sh @@ -65,7 +65,7 @@ nbdkit eval \ > eval-extents.out cat eval-extents.out diff -u - eval-extents.out <<EOF -[4294967294, 1, 1073741824, 2] -[4294967295, 1] -[4294967295, 1] +[4294901760, 1] +[4294901760, 1] +[4294901760, 1] EOF -- 2.49.0
On Fri, May 30, 2025 at 05:12:35PM -0500, Eric Blake via Libguestfs wrote:
This is in response to https://gitlab.com/nbdkit/libnbd/-/issues/12 where nbdinfo is having a hard time because nbdkit is outputting data that is not in compliance with the current NBD spec (which states that if client and server agreed on a minimum blocksize, then extents must be aligned to that).
I still have more to write: - add a unit test for the server change at 4G (in addition to the unit test I got working for blocksize-policy tripping on the same problem with a much smaller unaligned-size data image) - libnbd: patch nbdinfo to tolerate unaligned extents from the server. Even if the spec says its wrong, existing servers in the wild do it, so we should be tolerant. - upstream NBD spec: propose weakening the standard from SHALL to SHOULD provide aligned extents - blocksize-policy: Consider patches to allow aligned offset but unaligned length IF the length extends from offset to the end of the unaligned-size disk. Document that an unaligned tail is hard to read, and suggest use of the truncate filter to deal with it.
I've moved this to a gitlab merge request while I continue to refine the series: https://gitlab.com/nbdkit/nbdkit/-/merge_requests/97
But as this has taken me the better part of today, I wanted to put out what I have so far.
Eric Blake (4): human-size: Add human_size_parse_substr data: Allow scaling suffixes on @decimal blocksize-policy: Round extents to match minimum alignment server: Align the truncation of large block_status results
plugins/data/nbdkit-data-plugin.pod | 3 +- tests/Makefile.am | 2 + common/include/human-size-test-cases.h | 83 ++++++++++-------- common/include/human-size.h | 57 ++++++++---- common/include/test-human-size.c | 52 +++++++++-- server/protocol.c | 12 ++- server/test-public.c | 13 +-- plugins/data/format.c | 115 ++++++++++++------------- filters/blocksize-policy/policy.c | 15 +++- tests/test-data-format.sh | 8 +- tests/test-eval-extents.sh | 6 +- 11 files changed, 230 insertions(+), 136 deletions(-)
In particular, my emails forgot to add the new tests/test-blocksize-policy-extents.sh file before committing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
participants (1)
-
Eric Blake