search for: nbd_opt_set_meta_context

On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote: > Fixes the fact that clients could not request the maximum string > length except with NBD_OPT_EXPORT_LEN. Updates the testsuite to > match. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > server/protocol-handshake-newstyle.c | 12 +++++++----- > tests/test-long-name.sh | 10

...if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) { + assert (h->opt_mode); + assert (h->structured_replies); + assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context)); + opt = h->opt_current; + } + else { + assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); + opt = NBD_OPT_SET_META_CONTEXT; + if (!h->structured_replies || + nbd_internal_string_list_length (h->request_meta_contexts) == 0) { + SET_NEXT_STATE (%^OPT_GO.START); + return 0; + } } assert (h->meta_contexts == NULL); @@ -44,7 +54,7 @@ STATE_MACHINE { len += 4 /* length of query */...

I'm posting this now, as I'm at the end of a workday and I got things working for manual experimentation. Still to do: - write interop tests for qemu-nbd and nbdkit (including my proposed patch addition of qemu-nbd -A to show qemu:allocation-depth) - figure out if we can make 'nbdinfo --map' use the new API to automatically select all contexts advertised by the server Eric Blake

...xportname) parameter if they need the export name. Filter .reopen also takes the exportname. The only filter that uses it (retry) must save the exportname from .open in order to pass it to .reopen. This filter already does the same for the readonly flag so this seems reasonable. The handling of NBD_OPT_SET_META_CONTEXT interacted with the export name (see commit 20b8509a9ccdab118ce7b7043be63bbad74f1e79). I have attempted to keep previous behaviour in this change, but note that there is no regression test for this. I added a debug message so we can tell when this unusual case actually happens which should help w...

...t.sh preconnect: status 2 nbdkit: sh[1]: debug: newstyle negotiation: flags: global 0x3 nbdkit: sh[1]: debug: newstyle negotiation: client flags: 0x3 nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_STRUCTURED_REPLY: client requested structured replies nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: client requested export '' nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: set count: 1 nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: set base:allocation nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: replying with base:a...

I'm still playing with ideas on how to split rstate from wstate (so that we can send a request without waiting for POLLIN to complete a pending reply), but this is some preliminary refactoring I found useful. I also fixed a couple of bugs while in the area (already pushed). There's a question of whether we want nbd_handle to be nearly 5k, or if we should instead keep it small and add one

...tus 2 > nbdkit: sh[1]: debug: newstyle negotiation: flags: global 0x3 > nbdkit: sh[1]: debug: newstyle negotiation: client flags: 0x3 > nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_STRUCTURED_REPLY: client requested structured replies > nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: client requested export '' > nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: set count: 1 > nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: set base:allocation > nbdkit: sh[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT: reply...

...); + return -1; + } + + return 0; +} + /* Sub-function during negotiate_handshake_newstyle, to uniformly handle * a client hanging up on a message boundary. */ @@ -452,6 +480,8 @@ negotiate_handshake_newstyle_options (struct connection *conn) case NBD_OPT_LIST_META_CONTEXT: case NBD_OPT_SET_META_CONTEXT: { + int r; + bool can_extents; uint32_t opt_index; uint32_t exportnamelen; uint32_t nr_queries; @@ -469,6 +499,16 @@ negotiate_handshake_newstyle_options (struct connection *conn) continue; } + /* Work out if the server s...

...ke-newstyle.c >> @@ -48,7 +48,7 @@ >> #define MAX_NR_OPTIONS 32 >> >> /* Maximum length of any option data (bytes). */ >> -#define MAX_OPTION_LENGTH 4096 >> +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4) > > I may have missed it - why was * 4 chosen? NBD_OPT_SET_META_CONTEXT allows two strings plus a few glue bytes, so more than 8k of data from a compliant client. 16k is the next power of 2. We can bump it larger if we want, especially since 16k pales in comparison to our 32M limit on NBD_CMD_WRITE, but for now, there is nothing in the NBD protocol larger than NB...

...ndex 0aadd46..b03555e 100644 --- a/server/protocol.h +++ b/server/protocol.h @@ -105,6 +105,8 @@ extern const char *name_of_nbd_opt (int); #define NBD_OPT_INFO 6 #define NBD_OPT_GO 7 #define NBD_OPT_STRUCTURED_REPLY 8 +#define NBD_OPT_LIST_META_CONTEXT 9 +#define NBD_OPT_SET_META_CONTEXT 10 extern const char *name_of_nbd_rep (int); #define NBD_REP_ACK 1 diff --git a/server/connections.c b/server/connections.c index aeb27f8..7e32f00 100644 --- a/server/connections.c +++ b/server/connections.c @@ -926,6 +926,90 @@ _negotiate_handshake_newstyle_options (struct connectio...

...n), (int) exportnamelen, buf); @@ -451,7 +455,9 @@ negotiate_handshake_newstyle_options (struct connection *conn) } /* As with NBD_OPT_EXPORT_NAME we print the export name and - * save it in the connection. + * save it in the connection. If an earlier + * NBD_OPT_SET_META_CONTEXT used an export name, it must match + * or else we drop the support for that context. */ if (check_export_name (conn, option, &data[4], exportnamelen, optlen - 6, true) == -1) @@ -574,12 +580,16 @@ negotiate_handshake_newstyle_options (s...

...return -1; @@ -420,14 +421,21 @@ negotiate_handshake_newstyle_options (struct connection *conn) } /* As with NBD_OPT_EXPORT_NAME we print the export name and - * save it in the connection. + * save it in the connection. For NBD_OPT_GO, if an earlier + * NBD_OPT_SET_META_CONTEXT used an export name, it must match + * or else we drop the support for that context. */ + if (option == NBD_OPT_GO && conn->meta_context_base_allocation && + (exportnamelen != conn->exportnamelen || + memcmp (conn->exportname,...

...dvertise this bit unless the client successfully negotiates > > + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT > > + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or > > + `NBD_OPT_GO` if the client does not negotiate metacontexts with > > + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the > > + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless > > + this transmission flag is set. > > Given that we are introducing this at the same time as the extended > headers (and given that we're running out of available flag...

...ONTEXT", newstyle_opt_meta_context_state_machine); Group ("OPT_GO", newstyle_opt_go_state_machine); Group ("OPT_EXPORT_NAME", newstyle_opt_export_name_state_machine); @@ -453,8 +453,8 @@ and newstyle_opt_structured_reply_state_machine = [ }; ] -(* Fixed newstyle NBD_OPT_SET_META_CONTEXT option. *) -and newstyle_opt_set_meta_context_state_machine = [ +(* Fixed newstyle NBD_OPT_SET/LIST_META_CONTEXT option. *) +and newstyle_opt_meta_context_state_machine = [ State { default_state with name = "START"; diff --git a/generator/states-newstyle-opt-set-meta-context....

The first one is the nastiest - it is an assertion failure caused by a spec-compliant client and introduced by our security fix that was released in 1.14.1. Eric Blake (4): server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO server: Fix back-to-back SET_META_CONTEXT server: Forbid NUL in export and context names server: Fix OPT_GO on different export than SET_META_CONTEXT

...around in nbdkit. > > > > And what workaround would that be? Looking at the qemu patch, the > problem is that qemu asked for "base:allocation" and nbdkit replied with > 0 contexts. The workaround would either be to reply with > NBD_REP_ERR_UNSUP (the behavior before NBD_OPT_SET_META_CONTEXT was > added), or to implement "base:allocation" (even if we implement the > poor-man's version that always reports that the entire image is data, > for all requests, without any feedback from the plugins). The former > feels odd, but the latter seems oddly appealing as pr...

...t warrant a CVE. However, since it is a case where a server's reply can prompt a libnbd denial of service crash, I will be sending a security announcement and upcoming patch be to the libnbd-security man page once backports are in place. Fixes: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT fails", v1.15.3) Signed-off-by: Eric Blake <eblake at redhat.com> --- generator/states-reply-chunk.c | 1 + 1 file changed, 1 insertion(+) diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 02c65414..26b8a6b0 100644 --- a/generator/states-reply-chunk.c +...

...l connections to the given device. In particular, if this flag is @@ -1212,10 +1309,10 @@ of the newstyle negotiation. When this command succeeds, the server MUST NOT preserve any negotiation state (such as a request for - `NBD_OPT_STRUCTURED_REPLY`, or metadata contexts from - `NBD_OPT_SET_META_CONTEXT`) issued before this command. A client - SHOULD defer all stateful option requests until after it - determines whether encryption is available. + `NBD_OPT_STRUCTURED_REPLY` or `NBD_OPT_EXTENDED_HEADERS`, or + metadata contexts from `NBD_OPT_SET_META_CONTEXT`) issued before + this co...

Updated based on other changes that have happened in the meantime: - rely more on cleanup.h (throughout) - split structured read for easier review (patch 2 and 3 were combined in v1) - rely on nbdkit not leaking a server's partial answer (patch 3) - add tests (patch 5) - other bug fixes I found while testing it - drop EOVERFLOW patch for now; it will be separate once upstream NBD protocol

In v2: ack'ed preliminary patches have been pushed, and I've added a lot of testsuite coverage as well as putting the new API to use in nbdinfo. Eric Blake (2): api: Add nbd_opt_list_meta_context info: List available meta-contexts lib/internal.h | 1 + generator/API.ml | 84 ++++++++-