[libnbd PATCH 0/3] Another round of server bug toleration patches
I found some more 1-line hacks to nbdkit that can produce a subtly buggy server that is still easy enough to work around in libnbd. Eric Blake (3): states: Refactor where interrupted reply state is stored states: Skip server replies to unknown command cookies states: Tolerate simple reply to structured read request lib/internal.h | 4 +-- generator/states-reply-simple.c | 46 +++++++++++++++++++++------- generator/states-reply-structured.c | 24 ++++++++------- generator/states-reply.c | 47 ++++++++++++++--------------- 4 files changed, 73 insertions(+), 48 deletions(-) -- 2.37.1
There can be at most one active reply at a time. Storing a resume state per command is wasteful compared to storing a single resume state in the overall handle. Furthermore, if we want to be able to skip bogus server replies to an unexpected cookie, we need a way to resume reading bytes off the wire even when there is no matching h->reply_cmd for the current cookie. --- lib/internal.h | 4 ++-- generator/states-reply.c | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 8dbe2e4..0a61b15 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -303,8 +303,9 @@ struct nbd_handle { /* length (cmds_to_issue) + length (cmds_in_flight). */ int in_flight; - /* Current command during a REPLY cycle */ + /* Current command and POLLIN resumption point during a REPLY cycle */ struct command *reply_cmd; + enum state reply_state; bool disconnect_request; /* True if we've queued NBD_CMD_DISC */ bool tls_shut_writes; /* Used by lib/crypto.c to track disconnect. */ @@ -352,7 +353,6 @@ struct command { uint32_t count; void *data; /* Buffer for read/write */ struct command_cb cb; - enum state state; /* State to resume with on next POLLIN */ bool initialized; /* For read, true if getting a hole may skip memset */ uint32_t data_seen; /* For read, cumulative size of data chunks seen */ uint32_t error; /* Local errno value */ diff --git a/generator/states-reply.c b/generator/states-reply.c index bff6f4b..1a3a20a 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -25,26 +25,28 @@ * until POLLIN, we instead track where in the state machine we left * off, then return to READY to actually block. Then, on entry to * REPLY.START, we can tell if this is the start of a new reply (rlen - * is 0, stay put), a continuation of the preamble (reply_cmd is NULL, - * resume with RECV_REPLY), or a continuation from any other location - * (reply_cmd contains the state to jump to). + * is 0, stay put), a continuation of the preamble (reply_state is + * STATE_START, resume with RECV_REPLY), or a continuation from any + * other location (reply_state contains the state to jump to). */ static void save_reply_state (struct nbd_handle *h) { - assert (h->reply_cmd); assert (h->rlen); - h->reply_cmd->state = get_next_state (h); + assert (h->reply_state == STATE_START); + h->reply_state = get_next_state (h); + assert (h->reply_state != STATE_START); } STATE_MACHINE { REPLY.START: /* If rlen is non-zero, we are resuming an earlier reply cycle. */ if (h->rlen > 0) { - if (h->reply_cmd) { - assert (nbd_internal_is_state_processing (h->reply_cmd->state)); - SET_NEXT_STATE (h->reply_cmd->state); + if (h->reply_state != STATE_START) { + assert (nbd_internal_is_state_processing (h->reply_state)); + SET_NEXT_STATE (h->reply_state); + h->reply_state = STATE_START; } else SET_NEXT_STATE (%RECV_REPLY); -- 2.37.1
Instead of killing the connection when the server gives us an unexpected structured or error reply for a cookie we don't recognize, we can log the issue and otherwise ignore that packet (an unexpected successful simple reply is harder if we did not negotiate structured replies; see the lengthy comment in states-reply-simple.c). A compliant server will never do this to us, but a transmission error might. Here's a quick patch to nbdkit to demonstrate such a scenario: | diff --git i/server/protocol.c w/server/protocol.c | index 2ac77055..55b27e2e 100644 | --- i/server/protocol.c | +++ w/server/protocol.c | @@ -742,7 +742,7 @@ protocol_recv_request_send_reply (void) | (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) { | if (!error) { | if (cmd == NBD_CMD_READ) | - return send_structured_reply_read (request.handle, cmd, | + return send_structured_reply_read (0x100^request.handle, cmd, | buf, count, offset); | else /* NBD_CMD_BLOCK_STATUS */ | return send_structured_reply_block_status (request.handle, Note that if a server bit-flips a cookie, our original command waiting for the right cookie may never get a completion packet from the server; thus, testing this requires aio commands (unless we someday add APIs to make it easier for blocking commands that give up when they hit a timeout). With the following command line and the hack to nbdkit above: $ ./run /path/to/nbdkit -U - memory 1M --run 'nbdsh -u "$uri" -c "\ buf=nbd.Buffer(1) cookie=h.aio_pread(buf, 0) try: print(h.poll(1)) except nbd.Error as ex: print(ex.string) h.flush() print(h.get_size()) "' pre-patch we see that the connection is killed when h.poll() encounters the garbage reply, before we get to the second command: nbd_poll: no matching cookie found for server reply, this is probably a bug in the server nbdkit: memory.1: error: read request: Connection reset by peer nbdsh: command line script failed: nbd_flush: invalid state: DEAD: the handle must be connected with the server: Invalid argument while post-patch the read never retires, but the unexpected cookie is skipped and we get to the flush: 1 1048576 Watching the LIBNBD_DEBUG=1 stream, we also see: libnbd: debug: nbdsh: nbd_poll: skipped reply for unexpected cookie 281474976710657, this is probably a bug in the server --- generator/states-reply-simple.c | 25 ++++++++++++++++++++++--- generator/states-reply-structured.c | 24 +++++++++++++----------- generator/states-reply.c | 27 ++++++++++++--------------- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 2a7b9a9..24f7efa 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -25,10 +25,29 @@ STATE_MACHINE { uint64_t cookie; error = be32toh (h->sbuf.simple_reply.error); - cookie = be64toh (h->sbuf.simple_reply.handle); - assert (cmd); - assert (cmd->cookie == cookie); + if (cmd == NULL) { + /* Unexpected reply. If error was set or we have structured + * replies, we know there should be no payload, so the next byte + * on the wire (if any) will be another reply, and we can let + * FINISH_COMMAND diagnose/ignore the server bug. If not, we lack + * context to know whether the server thinks it was responding to + * NBD_CMD_READ, so it is safer to move to DEAD now than to risk + * consuming a server's potential data payload as a reply stream + * (even though we would be likely to produce a magic number + * mismatch on the next pass that would also move us to DEAD). + */ + if (error || h->structured_replies) + SET_NEXT_STATE (%^FINISH_COMMAND); + else { + cookie = be64toh (h->sbuf.simple_reply.handle); + SET_NEXT_STATE (%.DEAD); + set_error (EPROTO, + "no matching cookie %" PRIu64 " found for server reply, " + "this is probably a server bug", cookie); + } + return 0; + } if (cmd->type == NBD_CMD_READ && h->structured_replies) { set_error (0, "server sent unexpected simple reply for read"); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 2db2cf6..4fbd62f 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -69,17 +69,12 @@ STATE_MACHINE { REPLY.STRUCTURED_REPLY.CHECK: struct command *cmd = h->reply_cmd; uint16_t flags, type; - uint64_t cookie; uint32_t length; flags = be16toh (h->sbuf.sr.structured_reply.flags); type = be16toh (h->sbuf.sr.structured_reply.type); - cookie = be64toh (h->sbuf.sr.structured_reply.handle); length = be32toh (h->sbuf.sr.structured_reply.length); - assert (cmd); - assert (cmd->cookie == cookie); - /* Reject a server that replies with too much information, but don't * reject a single structured reply to NBD_CMD_READ on the largest * size we were willing to send. The most likely culprit is a server @@ -88,12 +83,13 @@ STATE_MACHINE { * not worth keeping the connection alive. */ if (length > MAX_REQUEST_SIZE + sizeof h->sbuf.sr.payload.offset_data) { - set_error (0, "invalid server reply length"); + set_error (0, "invalid server reply length %" PRIu32, length); SET_NEXT_STATE (%.DEAD); return 0; } - if (!h->structured_replies) { + /* Skip an unexpected structured reply, including to an unknown cookie. */ + if (cmd == NULL || !h->structured_replies) { resync: h->rbuf = NULL; h->rlen = length; @@ -487,7 +483,6 @@ STATE_MACHINE { uint16_t type; uint32_t length; - assert (cmd); assert (h->rbuf == NULL); switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; @@ -496,6 +491,15 @@ STATE_MACHINE { SET_NEXT_STATE (%.READY); return 0; case 0: + /* If this reply is to an unknown command, FINISH_COMMAND will + * diagnose and ignore the server bug. Otherwise, ensure the + * pending command sees a failure of EPROTO if it does not already + * have an error. + */ + if (cmd == NULL) { + SET_NEXT_STATE (%^FINISH_COMMAND); + return 0; + } type = be16toh (h->sbuf.sr.structured_reply.type); length = be32toh (h->sbuf.sr.structured_reply.length); debug (h, "unexpected reply type %u or payload length %" PRIu32 @@ -505,10 +509,8 @@ STATE_MACHINE { if (cmd->error == 0) cmd->error = EPROTO; SET_NEXT_STATE (%FINISH); - return 0; - default: - abort (); } + return 0; REPLY.STRUCTURED_REPLY.FINISH: uint16_t flags; diff --git a/generator/states-reply.c b/generator/states-reply.c index 1a3a20a..0736342 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -124,7 +124,7 @@ STATE_MACHINE { } else { SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */ - set_error (0, "invalid reply magic"); + set_error (0, "invalid reply magic 0x%" PRIx32, magic); return 0; } @@ -132,22 +132,13 @@ STATE_MACHINE { * handle (our cookie) is stored at the same offset. */ cookie = be64toh (h->sbuf.simple_reply.handle); - /* Find the command amongst the commands in flight. */ + /* Find the command amongst the commands in flight. If the server sends + * a reply for an unknown cookie, FINISH will diagnose that later. + */ for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) { if (cmd->cookie == cookie) break; } - if (cmd == NULL) { - /* An unexpected structured reply could be skipped, since it - * includes a length; similarly an unexpected simple reply can be - * skipped if we assume it was not a read. However, it's more - * likely we've lost synchronization with the server. - */ - SET_NEXT_STATE (%.DEAD); - set_error (0, "no matching cookie found for server reply, " - "this is probably a bug in the server"); - return 0; - } h->reply_cmd = cmd; return 0; @@ -167,10 +158,16 @@ STATE_MACHINE { if (cmd->cookie == cookie) break; } - assert (cmd != NULL); assert (h->reply_cmd == cmd); - h->reply_cmd = NULL; + if (cmd == NULL) { + debug (h, "skipped reply for unexpected cookie %" PRIu64 + ", this is probably a bug in the server", cookie); + SET_NEXT_STATE (%.READY); + return 0; + } + retire = cmd->type == NBD_CMD_DISC; + h->reply_cmd = NULL; /* Notify the user */ if (CALLBACK_IS_NOT_NULL (cmd->cb.completion)) { -- 2.37.1
Another patch in the series of being more tolerant to various server bugs. Although a server is non-compliant for using a simple reply to NBD_CMD_READ once structured commands are negotiated, it is just as easy to read the packet and continue on after guaranteeing we report an error. In the process, I noticed that in the corner case of a server that starts with a structured packet and follows with a simple reply, we can no longer assume that the simple reply is the only receipt of data, so we have to be more careful with assertions and increments to data_seen. As in earlier patches, the following temporary one-line tweak to nbdkit will produce a server that demonstrates the scenario: | diff --git i/server/protocol.c w/server/protocol.c | index 2ac77055..ac359ca1 100644 | --- i/server/protocol.c | +++ w/server/protocol.c | @@ -738,7 +738,7 @@ protocol_recv_request_send_reply (void) | * us from sending human-readable error messages to the client, so | * we should reconsider this in future. | */ | - if (conn->structured_replies && | + if (conn->structured_replies && !error && | (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) { | if (!error) { | if (cmd == NBD_CMD_READ) The following command line: $ ./run /path/to/nbdkit -U - memory 1M --filter=error error-pread-rate=100% \ --run 'nbdsh -u "$uri" -c "\ try: h.pread(1,0) except nbd.Error as ex: print(ex.string) h.flush() print(h.get_size()) "' demonstrates the following issue pre-patch: nbdkit: memory.0: error: injecting EIO error into pread nbd_pread: server sent unexpected simple reply for read nbdsh: command line script failed: nbd_flush: invalid state: DEAD: the handle must be connected with the server: Invalid argument and the nicer flow post-patch: nbdkit: memory.0: error: injecting EIO error into pread nbd_pread: read: command failed: Protocol error 1048576 where we report EPROTO rather than the server's EIO. --- generator/states-reply-simple.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 24f7efa..ac58823 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -49,17 +49,23 @@ STATE_MACHINE { return 0; } + /* Although a server with structured replies negotiated is in error + * for using a simple reply to NBD_CMD_READ, we can cope with the + * packet, but diagnose it by failing the read with EPROTO. + */ if (cmd->type == NBD_CMD_READ && h->structured_replies) { - set_error (0, "server sent unexpected simple reply for read"); - SET_NEXT_STATE(%.DEAD); - return 0; + debug (h, "server sent unexpected simple reply for read"); + if (cmd->error == 0) + cmd->error = EPROTO; } - cmd->error = nbd_internal_errno_of_nbd_error (error); - if (cmd->error == 0 && cmd->type == NBD_CMD_READ) { + error = nbd_internal_errno_of_nbd_error (error); + if (cmd->error == 0) + cmd->error = error; + if (error == 0 && cmd->type == NBD_CMD_READ) { h->rbuf = cmd->data; h->rlen = cmd->count; - cmd->data_seen = cmd->count; + cmd->data_seen += cmd->count; SET_NEXT_STATE (%RECV_READ_PAYLOAD); } else { @@ -80,9 +86,8 @@ STATE_MACHINE { /* guaranteed by START */ assert (cmd); if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) { - int error = 0; + int error = cmd->error; - assert (cmd->error == 0); if (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data, cmd->count, cmd->offset, LIBNBD_READ_DATA, -- 2.37.1
On Fri, Aug 12, 2022 at 04:03:58PM -0500, Eric Blake wrote:
I found some more 1-line hacks to nbdkit that can produce a subtly buggy server that is still easy enough to work around in libnbd.
Eric Blake (3): states: Refactor where interrupted reply state is stored states: Skip server replies to unknown command cookies states: Tolerate simple reply to structured read request
I've now pushed this series as 32af175..b31e7ba I'm still thinking about adding a strictness knob to let us decide whether to try and cope with servers or fail as fast as possible on the first non-compliance; but whether that belongs as a new bit to the existing nbd_set_strict_mode (which to date has controlled client strictness, not our response to the server) or a new API, is still worth thinking about. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (1)
-
Eric Blake