While working on later patches, I noticed that I was frequently referring to the wire contents of h->sbuf.reply.hdr.structured.length, byte-swapping it, then repeating open-coded math for how many bytes were consumed in earlier states. It's easier to normalize the remaining bytes to be parsed into a variable that persists across the life of the reply, and also makes it possible to track that by the end of any structured reply, we have dealt with all of the bytes that the header advertised. Some of the worst implicit assumptions in block status code will be fixed in the next patch with the help of this new variable. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: new patch, replacing part of v3's 5/22 --- lib/internal.h | 3 ++ generator/states-reply-chunk.c | 59 ++++++++++++++++------------------ 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 4b0043b3..b38ae524 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -304,6 +304,9 @@ struct nbd_handle { string_vector querylist; size_t querynum; + /* Chunk payload length remaining to be parsed */ + size_t payload_left; + /* When receiving block status, this is used. */ uint32_t *bs_entries; diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 26b8a6b0..da5fc426 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -69,6 +69,7 @@ REPLY.CHUNK_REPLY.START: /* Skip an unexpected structured reply, including to an unknown cookie. */ if (cmd == NULL || !h->structured_replies) goto resync; + h->payload_left = length; switch (type) { case NBD_REPLY_TYPE_NONE: @@ -87,6 +88,7 @@ REPLY.CHUNK_REPLY.START: goto resync; h->rbuf = &h->sbuf.reply.payload.offset_data; h->rlen = sizeof h->sbuf.reply.payload.offset_data; + h->payload_left -= h->rlen; SET_NEXT_STATE (%RECV_OFFSET_DATA); break; @@ -96,6 +98,7 @@ REPLY.CHUNK_REPLY.START: goto resync; h->rbuf = &h->sbuf.reply.payload.offset_hole; h->rlen = sizeof h->sbuf.reply.payload.offset_hole; + h->payload_left -= h->rlen; SET_NEXT_STATE (%RECV_OFFSET_HOLE); break; @@ -141,7 +144,8 @@ REPLY.CHUNK_REPLY.START: resync: h->rbuf = NULL; - h->rlen = length; + h->rlen = h->payload_left; + h->payload_left = 0; SET_NEXT_STATE (%RESYNC); return 0; @@ -156,7 +160,8 @@ REPLY.CHUNK_REPLY.RECV_ERROR: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->payload_left; + h->payload_left -= MIN (length, sizeof h->sbuf.reply.payload.error.error); assert (length >= sizeof h->sbuf.reply.payload.error.error.error); assert (cmd); @@ -164,12 +169,13 @@ REPLY.CHUNK_REPLY.RECV_ERROR: goto resync; msglen = be16toh (h->sbuf.reply.payload.error.error.len); - if (msglen > length - sizeof h->sbuf.reply.payload.error.error || + if (msglen > h->payload_left || msglen > sizeof h->sbuf.reply.payload.error.msg) goto resync; h->rbuf = h->sbuf.reply.payload.error.msg; h->rlen = msglen; + h->payload_left -= h->rlen; SET_NEXT_STATE (%RECV_ERROR_MESSAGE); } return 0; @@ -180,12 +186,13 @@ REPLY.CHUNK_REPLY.RECV_ERROR: if (cmd->error == 0) cmd->error = nbd_internal_errno_of_nbd_error (error); h->rbuf = NULL; - h->rlen = length - MIN (length, sizeof h->sbuf.reply.payload.error.error); + h->rlen = h->payload_left; + h->payload_left = 0; SET_NEXT_STATE (%RESYNC); return 0; REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE: - uint32_t length, msglen; + uint32_t msglen; uint16_t type; switch (recv_into_rbuf (h)) { @@ -195,33 +202,31 @@ REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); msglen = be16toh (h->sbuf.reply.payload.error.error.len); type = be16toh (h->sbuf.reply.hdr.structured.type); - length -= sizeof h->sbuf.reply.payload.error.error + msglen; - if (msglen) debug (h, "structured error server message: %.*s", (int)msglen, h->sbuf.reply.payload.error.msg); /* Special case two specific errors; silently ignore tail for all others */ h->rbuf = NULL; - h->rlen = length; + h->rlen = h->payload_left; switch (type) { case NBD_REPLY_TYPE_ERROR: - if (length != 0) + if (h->payload_left != 0) debug (h, "ignoring unexpected slop after error message, " "the server may have a bug"); break; case NBD_REPLY_TYPE_ERROR_OFFSET: - if (length != sizeof h->sbuf.reply.payload.error.offset) + if (h->payload_left != sizeof h->sbuf.reply.payload.error.offset) debug (h, "unable to safely extract error offset, " "the server may have a bug"); else h->rbuf = &h->sbuf.reply.payload.error.offset; break; } + h->payload_left = 0; SET_NEXT_STATE (%RECV_ERROR_TAIL); } return 0; @@ -286,7 +291,6 @@ REPLY.CHUNK_REPLY.RECV_ERROR_TAIL: REPLY.CHUNK_REPLY.RECV_OFFSET_DATA: struct command *cmd = h->reply_cmd; uint64_t offset; - uint32_t length; switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; @@ -295,29 +299,24 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); offset = be64toh (h->sbuf.reply.payload.offset_data.offset); assert (cmd); /* guaranteed by CHECK */ - assert (cmd->data && cmd->type == NBD_CMD_READ); - /* Length of the data following. */ - length -= 8; - /* Is the data within bounds? */ - if (! structured_reply_in_bounds (offset, length, cmd)) { + if (! structured_reply_in_bounds (offset, h->payload_left, cmd)) { SET_NEXT_STATE (%.DEAD); return 0; } if (cmd->data_seen <= cmd->count) - cmd->data_seen += length; + cmd->data_seen += h->payload_left; /* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; /* Set up to receive the data directly to the user buffer. */ h->rbuf = (char *)cmd->data + offset; - h->rlen = length; + h->rlen = h->payload_left; SET_NEXT_STATE (%RECV_OFFSET_DATA_DATA); } return 0; @@ -325,7 +324,6 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA: REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA: struct command *cmd = h->reply_cmd; uint64_t offset; - uint32_t length; switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; @@ -334,7 +332,6 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); offset = be64toh (h->sbuf.reply.payload.offset_data.offset); assert (cmd); /* guaranteed by CHECK */ @@ -343,11 +340,12 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA: if (CALL_CALLBACK (cmd->cb.fn.chunk, (char *)cmd->data + (offset - cmd->offset), - length - sizeof offset, offset, + h->payload_left, offset, LIBNBD_READ_DATA, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; } + h->payload_left = 0; SET_NEXT_STATE (%FINISH); } @@ -369,7 +367,6 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE: length = be32toh (h->sbuf.reply.payload.offset_hole.length); assert (cmd); /* guaranteed by CHECK */ - assert (cmd->data && cmd->type == NBD_CMD_READ); /* Is the data within bounds? */ @@ -405,8 +402,8 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE: REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: struct command *cmd = h->reply_cmd; - uint32_t length; size_t i; + size_t count; uint32_t context_id; switch (recv_into_rbuf (h)) { @@ -416,20 +413,20 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); - assert (cmd); /* guaranteed by CHECK */ assert (cmd->type == NBD_CMD_BLOCK_STATUS); assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); assert (h->bs_entries); - assert (length >= 12); + assert (h->payload_left >= 12); assert (h->meta_valid); /* Need to byte-swap the entries returned, but apart from that we * don't validate them. */ - for (i = 0; i < length/4; ++i) + for (i = 0; i < h->payload_left / sizeof *h->bs_entries; ++i) h->bs_entries[i] = be32toh (h->bs_entries[i]); + count = (h->payload_left / sizeof *h->bs_entries) - 1; + h->payload_left = 0; /* Look up the context ID. */ context_id = h->bs_entries[0]; @@ -443,8 +440,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: if (CALL_CALLBACK (cmd->cb.fn.extent, h->meta_contexts.ptr[i].name, cmd->offset, - &h->bs_entries[1], (length-4) / 4, - &error) == -1) + &h->bs_entries[1], count, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; } @@ -494,6 +490,7 @@ REPLY.CHUNK_REPLY.RESYNC: REPLY.CHUNK_REPLY.FINISH: uint16_t flags; + assert (h->payload_left == 0); flags = be16toh (h->sbuf.reply.hdr.structured.flags); if (flags & NBD_REPLY_FLAG_DONE) { SET_NEXT_STATE (%^FINISH_COMMAND);

For 32-bit block status, we were able to cheat and use an array with an odd number of elements, with array[0] holding the context id, and passing &array[1] to the user's callback. But once we have 64-bit extents, we can no longer abuse array element 0 like that, for two reasons: 64-bit extents contain uint64_t which might not be alignment-compatible with an array of uint32_t on all architectures, and the new NBD_REPLY_TYPE_BLOCK_STATUS_EXT adds an additional count field before the array. What's more, there's no need to byte-swap the entries if we won't be calling the user's callback, when the server sent us an unknown context id. Split out a new state CHUNK_REPLY.BS_HEADER to receive the context id (and eventually the new count field for 64-bit replies) separately from the extents array. Add another type nbd_chunk_block_status_32 in the payload section for tracking it (with a name anticipating the 64-bit counterpart type under extended headers). Add a helper function bs_reply_length_ok() to enable more assertions that our memory management is sane without quite so much open-coding of magic values. With these changes, the byteswap of the entries can be delayed until we know we need to call the user's callback. No behavioral change, other than the rare possibility of landing in the new state. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: add more assertions, add bs_reply_length_ok() to factor out computation of length checks [Laszlo], rebase to state machine rename, do byte-swap later, add bs->count for tracking number of descriptors --- lib/internal.h | 2 + lib/nbd-protocol.h | 16 +++-- generator/state_machine.ml | 9 ++- generator/states-reply-chunk.c | 108 ++++++++++++++++++++++++--------- 4 files changed, 98 insertions(+), 37 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index b38ae524..c7793f1f 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -254,6 +254,7 @@ struct nbd_handle { uint64_t align_; /* Start reply.payload on an 8-byte alignment */ struct nbd_chunk_offset_data offset_data; struct nbd_chunk_offset_hole offset_hole; + struct nbd_chunk_block_status_32 bs_hdr_32; struct { struct nbd_chunk_error error; char msg[NBD_MAX_STRING]; /* Common to all error types */ @@ -308,6 +309,7 @@ struct nbd_handle { size_t payload_left; /* When receiving block status, this is used. */ + size_t bs_count; /* count of block descriptors (not array entries!) */ uint32_t *bs_entries; /* Commands which are waiting to be issued [meaning the request diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index c967c840..58583d1d 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -182,12 +182,6 @@ struct nbd_fixed_new_option_reply_meta_context { /* followed by a string */ } NBD_ATTRIBUTE_PACKED; -/* NBD_REPLY_TYPE_BLOCK_STATUS block descriptor. */ -struct nbd_block_descriptor { - uint32_t length; /* length of block */ - uint32_t status_flags; /* block type (hole etc) */ -} NBD_ATTRIBUTE_PACKED; - /* Request (client -> server). */ struct nbd_request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ @@ -224,6 +218,16 @@ struct nbd_chunk_offset_hole { uint32_t length; /* Length of hole. */ } NBD_ATTRIBUTE_PACKED; +struct nbd_chunk_block_status_32 { + uint32_t context_id; /* metadata context ID */ + /* followed by array of nbd_block_descriptor_32 extents */ +} NBD_ATTRIBUTE_PACKED; + +struct nbd_block_descriptor_32 { + uint32_t length; /* length of block */ + uint32_t status_flags; /* block type (hole etc) */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_chunk_error { uint32_t error; /* NBD_E* error number */ uint16_t len; /* Length of human readable error. */ diff --git a/generator/state_machine.ml b/generator/state_machine.ml index 3a912508..7a5bc59b 100644 --- a/generator/state_machine.ml +++ b/generator/state_machine.ml @@ -864,10 +864,17 @@ and external_events = []; }; + State { + default_state with + name = "RECV_BS_HEADER"; + comment = "Receive header of a chunk reply block-status payload"; + external_events = []; + }; + State { default_state with name = "RECV_BS_ENTRIES"; - comment = "Receive a chunk reply block-status payload"; + comment = "Receive entries array of chunk reply block-status payload"; external_events = []; }; diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index da5fc426..0d76c159 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -43,6 +43,28 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length, return true; } +/* Return true if payload length of block status reply is valid. + */ +static bool +bs_reply_length_ok (uint16_t type, uint32_t length) +{ + /* TODO support 64-bit replies */ + size_t prefix_len = sizeof (struct nbd_chunk_block_status_32); + size_t extent_len = sizeof (struct nbd_block_descriptor_32); + assert (type == NBD_REPLY_TYPE_BLOCK_STATUS); + + /* At least one descriptor is needed after id prefix */ + if (length < prefix_len + extent_len) + return false; + + /* There must be an integral number of extents */ + length -= prefix_len; + if (length % extent_len != 0) + return false; + + return true; +} + STATE_MACHINE { REPLY.CHUNK_REPLY.START: struct command *cmd = h->reply_cmd; @@ -105,22 +127,13 @@ REPLY.CHUNK_REPLY.START: case NBD_REPLY_TYPE_BLOCK_STATUS: if (cmd->type != NBD_CMD_BLOCK_STATUS || !h->meta_valid || h->meta_contexts.len == 0 || - length < 12 || ((length-4) & 7) != 0) + !bs_reply_length_ok (type, length)) goto resync; assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); - /* We read the context ID followed by all the entries into a - * single array and deal with it at the end. - */ - free (h->bs_entries); - h->bs_entries = malloc (length); - if (h->bs_entries == NULL) { - SET_NEXT_STATE (%.DEAD); - set_error (errno, "malloc"); - break; - } - h->rbuf = h->bs_entries; - h->rlen = length; - SET_NEXT_STATE (%RECV_BS_ENTRIES); + /* Start by reading the context ID. */ + h->rbuf = &h->sbuf.reply.payload.bs_hdr_32; + h->rlen = sizeof h->sbuf.reply.payload.bs_hdr_32; + SET_NEXT_STATE (%RECV_BS_HEADER); break; default: @@ -400,10 +413,46 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE: } return 0; + REPLY.CHUNK_REPLY.RECV_BS_HEADER: + struct command *cmd = h->reply_cmd; + uint16_t type; + + switch (recv_into_rbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 1: + save_reply_state (h); + SET_NEXT_STATE (%.READY); + return 0; + case 0: + type = be16toh (h->sbuf.reply.hdr.structured.type); + + assert (cmd); /* guaranteed by CHECK */ + assert (cmd->type == NBD_CMD_BLOCK_STATUS); + assert (bs_reply_length_ok (type, h->payload_left)); + STATIC_ASSERT (sizeof (struct nbd_block_descriptor_32) == + 2 * sizeof *h->bs_entries, + _block_desc_is_multiple_of_bs_entry); + h->payload_left -= sizeof h->sbuf.reply.payload.bs_hdr_32; + assert (h->payload_left % sizeof (struct nbd_block_descriptor_32) == 0); + h->bs_count = h->payload_left / sizeof (struct nbd_block_descriptor_32); + + free (h->bs_entries); + h->bs_entries = malloc (h->payload_left); + if (h->bs_entries == NULL) { + SET_NEXT_STATE (%.DEAD); + set_error (errno, "malloc"); + return 0; + } + h->rbuf = h->bs_entries; + h->rlen = h->payload_left; + h->payload_left = 0; + SET_NEXT_STATE (%RECV_BS_ENTRIES); + } + return 0; + REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: struct command *cmd = h->reply_cmd; size_t i; - size_t count; uint32_t context_id; switch (recv_into_rbuf (h)) { @@ -416,31 +465,30 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: assert (cmd); /* guaranteed by CHECK */ assert (cmd->type == NBD_CMD_BLOCK_STATUS); assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); - assert (h->bs_entries); - assert (h->payload_left >= 12); + assert (h->bs_count && h->bs_entries); assert (h->meta_valid); - /* Need to byte-swap the entries returned, but apart from that we - * don't validate them. - */ - for (i = 0; i < h->payload_left / sizeof *h->bs_entries; ++i) - h->bs_entries[i] = be32toh (h->bs_entries[i]); - count = (h->payload_left / sizeof *h->bs_entries) - 1; - h->payload_left = 0; - /* Look up the context ID. */ - context_id = h->bs_entries[0]; + context_id = be32toh (h->sbuf.reply.payload.bs_hdr_32.context_id); for (i = 0; i < h->meta_contexts.len; ++i) if (context_id == h->meta_contexts.ptr[i].context_id) break; if (i < h->meta_contexts.len) { - /* Call the caller's extent function. */ int error = cmd->error; + const char *name = h->meta_contexts.ptr[i].name; - if (CALL_CALLBACK (cmd->cb.fn.extent, - h->meta_contexts.ptr[i].name, cmd->offset, - &h->bs_entries[1], count, &error) == -1) + /* Need to byte-swap the entries returned, but apart from that + * we don't validate them. Yes, our 32-bit public API foolishly + * tracks the number of uint32_t instead of block descriptors; + * see _block_desc_is_multiple_of_bs_entry above. + */ + for (i = 0; i < h->bs_count * 2; ++i) + h->bs_entries[i] = be32toh (h->bs_entries[i]); + + /* Call the caller's extent function. */ + if (CALL_CALLBACK (cmd->cb.fn.extent, name, cmd->offset, + h->bs_entries, h->bs_count * 2, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; }

Add the magic numbers and new structs necessary to implement the NBD protocol extension of extended headers providing 64-bit lengths. This corresponds to upstream nbd commits 36abf47d and a9384e2f on the extension-ext-header branch[1] (commit e6f3b94a for NBD_FLAG_BLOCK_STATUS_PAYLOAD is saved for a later patch). [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/p... Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- v4: tweak comment on block_status_64.count [Laszlo], rebase to earlier struct renames (for example, nbd_block_descriptor_64 instead of nbd_structured_reply_block_status_hdr). Seemed obvious enough that I felt okay keeping Laszlo's R-b. --- lib/nbd-protocol.h | 55 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index 58583d1d..b5a28ae4 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -124,6 +124,7 @@ struct nbd_fixed_new_option_reply { #define NBD_OPT_STRUCTURED_REPLY 8 #define NBD_OPT_LIST_META_CONTEXT 9 #define NBD_OPT_SET_META_CONTEXT 10 +#define NBD_OPT_EXTENDED_HEADERS 11 #define NBD_REP_ERR(val) (0x80000000 | (val)) #define NBD_REP_IS_ERR(val) (!!((val) & 0x80000000)) @@ -141,6 +142,7 @@ struct nbd_fixed_new_option_reply { #define NBD_REP_ERR_SHUTDOWN NBD_REP_ERR (7) #define NBD_REP_ERR_BLOCK_SIZE_REQD NBD_REP_ERR (8) #define NBD_REP_ERR_TOO_BIG NBD_REP_ERR (9) +#define NBD_REP_ERR_EXT_HEADER_REQD NBD_REP_ERR (10) #define NBD_INFO_EXPORT 0 #define NBD_INFO_NAME 1 @@ -182,16 +184,26 @@ struct nbd_fixed_new_option_reply_meta_context { /* followed by a string */ } NBD_ATTRIBUTE_PACKED; -/* Request (client -> server). */ +/* Compact request (client -> server). */ struct nbd_request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ - uint16_t flags; /* Request flags. */ - uint16_t type; /* Request type. */ + uint16_t flags; /* Request flags: NBD_CMD_FLAG_*. */ + uint16_t type; /* Request type: NBD_CMD_*. */ uint64_t cookie; /* Opaque handle. */ uint64_t offset; /* Request offset. */ uint32_t count; /* Request length. */ } NBD_ATTRIBUTE_PACKED; +/* Extended request (client -> server). */ +struct nbd_request_ext { + uint32_t magic; /* NBD_EXTENDED_REQUEST_MAGIC. */ + uint16_t flags; /* Request flags: NBD_CMD_FLAG_*. */ + uint16_t type; /* Request type: NBD_CMD_*. */ + uint64_t cookie; /* Opaque handle. */ + uint64_t offset; /* Request offset. */ + uint64_t count; /* Request effect or payload length. */ +} NBD_ATTRIBUTE_PACKED; + /* Simple reply (server -> client). */ struct nbd_simple_reply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC. */ @@ -208,6 +220,16 @@ struct nbd_structured_reply { uint32_t length; /* Length of following nbd_chunk_* payload. */ } NBD_ATTRIBUTE_PACKED; +/* Extended reply (server -> client). */ +struct nbd_extended_reply { + uint32_t magic; /* NBD_EXTENDED_REPLY_MAGIC. */ + uint16_t flags; /* NBD_REPLY_FLAG_* */ + uint16_t type; /* NBD_REPLY_TYPE_* */ + uint64_t cookie; /* Opaque handle. */ + uint64_t offset; /* Client's offset. */ + uint64_t length; /* Length of following nbd_chunk_* payload. */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_chunk_offset_data { uint64_t offset; /* offset */ /* Followed by data. */ @@ -228,6 +250,17 @@ struct nbd_block_descriptor_32 { uint32_t status_flags; /* block type (hole etc) */ } NBD_ATTRIBUTE_PACKED; +struct nbd_chunk_block_status_64 { + uint32_t context_id; /* metadata context ID */ + uint32_t count; /* non-zero descriptor count */ + /* followed by nbd_block_descriptor_64[count] extents */ +} NBD_ATTRIBUTE_PACKED; + +struct nbd_block_descriptor_64 { + uint64_t length; /* length of block */ + uint64_t status_flags; /* block type (hole etc) */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_chunk_error { uint32_t error; /* NBD_E* error number */ uint16_t len; /* Length of human readable error. */ @@ -235,8 +268,10 @@ struct nbd_chunk_error { } NBD_ATTRIBUTE_PACKED; #define NBD_REQUEST_MAGIC 0x25609513 +#define NBD_EXTENDED_REQUEST_MAGIC 0x21e41c71 #define NBD_SIMPLE_REPLY_MAGIC 0x67446698 #define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef +#define NBD_EXTENDED_REPLY_MAGIC 0x6e8a278c /* Structured reply flags. */ #define NBD_REPLY_FLAG_DONE (1U << 0) @@ -245,12 +280,13 @@ struct nbd_chunk_error { #define NBD_REPLY_TYPE_IS_ERR(val) (!!((val) & (1U<<15))) /* Structured reply types. */ -#define NBD_REPLY_TYPE_NONE 0 -#define NBD_REPLY_TYPE_OFFSET_DATA 1 -#define NBD_REPLY_TYPE_OFFSET_HOLE 2 -#define NBD_REPLY_TYPE_BLOCK_STATUS 5 -#define NBD_REPLY_TYPE_ERROR NBD_REPLY_TYPE_ERR (1) -#define NBD_REPLY_TYPE_ERROR_OFFSET NBD_REPLY_TYPE_ERR (2) +#define NBD_REPLY_TYPE_NONE 0 +#define NBD_REPLY_TYPE_OFFSET_DATA 1 +#define NBD_REPLY_TYPE_OFFSET_HOLE 2 +#define NBD_REPLY_TYPE_BLOCK_STATUS 5 +#define NBD_REPLY_TYPE_BLOCK_STATUS_EXT 6 +#define NBD_REPLY_TYPE_ERROR NBD_REPLY_TYPE_ERR (1) +#define NBD_REPLY_TYPE_ERROR_OFFSET NBD_REPLY_TYPE_ERR (2) /* NBD commands. */ #define NBD_CMD_READ 0 @@ -267,6 +303,7 @@ struct nbd_chunk_error { #define NBD_CMD_FLAG_DF (1U << 2) #define NBD_CMD_FLAG_REQ_ONE (1U << 3) #define NBD_CMD_FLAG_FAST_ZERO (1U << 4) +#define NBD_CMD_FLAG_PAYLOAD_LEN (1U << 5) /* NBD error codes. */ #define NBD_SUCCESS 0

Support sending 64-bit requests if extended headers were negotiated. This includes setting NBD_CMD_FLAG_PAYLOAD_LEN any time we send an extended NBD_CMD_WRITE; this is such a fundamental part of the protocol that for now it is easier to silently ignore whatever value the user passes in for that bit in the flags parameter of nbd_pwrite regardless of the current settings in set_strict_mode, rather than trying to force the user to pass in the correct value to match whether extended mode is negotiated. However, when we later add APIs to give the user more control for interoperability testing, it may be worth adding a new set_strict_mode control knob to explicitly enable the client to intentionally violate the protocol (the testsuite added in this patch would then be updated to match). At this point, h->extended_headers is permanently false (we can't enable it until all other aspects of the protocol have likewise been converted). Note that struct command.data_seen remains typed as uint32_t despite being an accumulator for count values that are now stored in 64 bits; this is okay, because we drop the connection if count exceeds MAX_REQUEST_SIZE on any one transaction, such that data_seen's use as a bounded accumulator can never exceed 2*MAX_REQUEST_SIZE. Support for using FLAG_PAYLOAD_LEN with NBD_CMD_BLOCK_STATUS is less fundamental, and deserves to be in its own patch. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- v4: fix Makefile indentation [Laszlo], prefer progname over argv[0] in test [Laszlo], enhance commit message about command.data_seen --- lib/internal.h | 10 ++- generator/API.ml | 20 +++--- generator/states-issue-command.c | 29 ++++++--- generator/states-reply-chunk.c | 2 +- lib/rw.c | 17 +++-- tests/Makefile.am | 4 ++ tests/pwrite-extended.c | 108 +++++++++++++++++++++++++++++++ .gitignore | 1 + 8 files changed, 165 insertions(+), 26 deletions(-) create mode 100644 tests/pwrite-extended.c diff --git a/lib/internal.h b/lib/internal.h index c7793f1f..2e4f987c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -110,6 +110,9 @@ struct nbd_handle { char *tls_username; /* Username, NULL = use current username */ char *tls_psk_file; /* PSK filename, NULL = no PSK */ + /* Extended headers. */ + bool extended_headers; /* If we negotiated NBD_OPT_EXTENDED_HEADERS */ + /* Desired metadata contexts. */ bool request_sr; bool request_meta; @@ -273,7 +276,10 @@ struct nbd_handle { /* Issuing a command must use a buffer separate from sbuf, for the * case when we interrupt a request to service a reply. */ - struct nbd_request request; + union { + struct nbd_request compact; + struct nbd_request_ext extended; + } req; bool in_write_payload; bool in_write_shutdown; @@ -378,7 +384,7 @@ struct command { uint16_t type; uint64_t cookie; uint64_t offset; - uint32_t count; + uint64_t count; void *data; /* Buffer for read/write */ struct command_cb cb; bool initialized; /* For read, true if getting a hole may skip memset */ diff --git a/generator/API.ml b/generator/API.ml index 5fcb0e1f..02c1260d 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -198,11 +198,12 @@ let cmd_flags = flag_prefix = "CMD_FLAG"; guard = Some "((h->strict & LIBNBD_STRICT_FLAGS) || flags > UINT16_MAX)"; flags = [ - "FUA", 1 lsl 0; - "NO_HOLE", 1 lsl 1; - "DF", 1 lsl 2; - "REQ_ONE", 1 lsl 3; - "FAST_ZERO", 1 lsl 4; + "FUA", 1 lsl 0; + "NO_HOLE", 1 lsl 1; + "DF", 1 lsl 2; + "REQ_ONE", 1 lsl 3; + "FAST_ZERO", 1 lsl 4; + "PAYLOAD_LEN", 1 lsl 5; ] } let handshake_flags = { @@ -2507,7 +2508,7 @@ "pread_structured", { "pwrite", { default_call with args = [ BytesIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"]) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"; "PAYLOAD_LEN"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -2530,7 +2531,10 @@ "pwrite", { C<LIBNBD_CMD_FLAG_FUA> meaning that the server should not return until the data has been committed to permanent storage (if that is supported - some servers cannot do this, see -L<nbd_can_fua(3)>)." +L<nbd_can_fua(3)>). For convenience, libnbd ignores the presence +or absence of the flag C<LIBNBD_CMD_FLAG_PAYLOAD_LEN> in C<flags>, +while correctly using the flag over the wire according to whether +extended headers were negotiated." ^ strict_call_description; see_also = [Link "can_fua"; Link "is_read_only"; Link "aio_pwrite"; Link "get_block_size"; @@ -3220,7 +3224,7 @@ "aio_pwrite", { default_call with args = [ BytesPersistIn ("buf", "count"); UInt64 "offset" ]; optargs = [ OClosure completion_closure; - OFlags ("flags", cmd_flags, Some ["FUA"]) ]; + OFlags ("flags", cmd_flags, Some ["FUA"; "PAYLOAD_LEN"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c index 30721946..59b05835 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -41,15 +41,24 @@ ISSUE_COMMAND.START: return 0; } - h->request.magic = htobe32 (NBD_REQUEST_MAGIC); - h->request.flags = htobe16 (cmd->flags); - h->request.type = htobe16 (cmd->type); - h->request.cookie = htobe64 (cmd->cookie); - h->request.offset = htobe64 (cmd->offset); - h->request.count = htobe32 (cmd->count); + /* These fields are coincident between req.compact and req.extended */ + h->req.compact.flags = htobe16 (cmd->flags); + h->req.compact.type = htobe16 (cmd->type); + h->req.compact.cookie = htobe64 (cmd->cookie); + h->req.compact.offset = htobe64 (cmd->offset); + if (h->extended_headers) { + h->req.extended.magic = htobe32 (NBD_EXTENDED_REQUEST_MAGIC); + h->req.extended.count = htobe64 (cmd->count); + h->wlen = sizeof (h->req.extended); + } + else { + assert (cmd->count <= UINT32_MAX); + h->req.compact.magic = htobe32 (NBD_REQUEST_MAGIC); + h->req.compact.count = htobe32 (cmd->count); + h->wlen = sizeof (h->req.compact); + } h->chunks_sent++; - h->wbuf = &h->request; - h->wlen = sizeof (h->request); + h->wbuf = &h->req; if (cmd->type == NBD_CMD_WRITE || cmd->next) h->wflags = MSG_MORE; SET_NEXT_STATE (%SEND_REQUEST); @@ -74,7 +83,7 @@ ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD: assert (h->cmds_to_issue != NULL); cmd = h->cmds_to_issue; - assert (cmd->cookie == be64toh (h->request.cookie)); + assert (cmd->cookie == be64toh (h->req.compact.cookie)); if (cmd->type == NBD_CMD_WRITE) { h->wbuf = cmd->data; h->wlen = cmd->count; @@ -120,7 +129,7 @@ ISSUE_COMMAND.FINISH: assert (!h->wlen); assert (h->cmds_to_issue != NULL); cmd = h->cmds_to_issue; - assert (cmd->cookie == be64toh (h->request.cookie)); + assert (cmd->cookie == be64toh (h->req.compact.cookie)); h->cmds_to_issue = cmd->next; if (h->cmds_to_issue_tail == cmd) h->cmds_to_issue_tail = NULL; diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 0d76c159..1758ac34 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -34,7 +34,7 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length, offset + length > cmd->offset + cmd->count) { set_error (0, "range of structured reply is out of bounds, " "offset=%" PRIu64 ", cmd->offset=%" PRIu64 ", " - "length=%" PRIu32 ", cmd->count=%" PRIu32 ": " + "length=%" PRIu32 ", cmd->count=%" PRIu64 ": " "this is likely to be a bug in the NBD server", offset, cmd->offset, length, cmd->count); return false; diff --git a/lib/rw.c b/lib/rw.c index 3dc3499e..8b2bd4cc 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -223,13 +223,11 @@ nbd_internal_command_common (struct nbd_handle *h, } break; - /* Other commands are currently limited by the 32 bit field in the - * command structure on the wire, but in future we hope to support - * 64 bit values here with a change to the NBD protocol which is - * being discussed upstream. + /* Other commands are limited by the 32 bit field in the command + * structure on the wire, unless extended headers were negotiated. */ default: - if (count > UINT32_MAX) { + if (!h->extended_headers && count > UINT32_MAX) { set_error (ERANGE, "request too large: maximum request size is %" PRIu32, UINT32_MAX); goto err; @@ -358,6 +356,15 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, return -1; } } + /* It is more convenient to manage PAYLOAD_LEN by what was negotiated + * than to require the user to have to set it correctly. + * TODO: Add new h->strict bit to allow intentional protocol violation + * for interoperability testing. + */ + if (h->extended_headers) + flags |= LIBNBD_CMD_FLAG_PAYLOAD_LEN; + else + flags &= ~LIBNBD_CMD_FLAG_PAYLOAD_LEN; SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count, diff --git a/tests/Makefile.am b/tests/Makefile.am index 6eddcb7a..52fadd9c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -233,6 +233,7 @@ check_PROGRAMS += \ closure-lifetimes \ pread-initialize \ socket-activation-name \ + pwrite-extended \ $(NULL) TESTS += \ @@ -655,6 +656,9 @@ socket_activation_name_SOURCES = \ requires.h socket_activation_name_LDADD = $(top_builddir)/lib/libnbd.la +pwrite_extended_SOURCES = pwrite-extended.c +pwrite_extended_LDADD = $(top_builddir)/lib/libnbd.la + #---------------------------------------------------------------------- # Testing TLS support. diff --git a/tests/pwrite-extended.c b/tests/pwrite-extended.c new file mode 100644 index 00000000..c6f4dd29 --- /dev/null +++ b/tests/pwrite-extended.c @@ -0,0 +1,108 @@ +/* NBD client library in userspace + * Copyright Red Hat + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Check behavior of pwrite with PAYLOAD_LEN flag for extended headers. */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <unistd.h> +#include <sys/stat.h> + +#include <libnbd.h> + +static char *progname; +static char buf[512]; + +static void +check (int experr, const char *prefix) +{ + const char *msg = nbd_get_error (); + int errnum = nbd_get_errno (); + + fprintf (stderr, "error: \"%s\"\n", msg); + fprintf (stderr, "errno: %d (%s)\n", errnum, strerror (errnum)); + if (strncmp (msg, prefix, strlen (prefix)) != 0) { + fprintf (stderr, "%s: test failed: missing context prefix: %s\n", + progname, msg); + exit (EXIT_FAILURE); + } + if (errnum != experr) { + fprintf (stderr, "%s: test failed: " + "expected errno = %d (%s), but got %d\n", + progname, experr, strerror (experr), errnum); + exit (EXIT_FAILURE); + } +} + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + const char *cmd[] = { + "nbdkit", "-s", "-v", "--exit-with-parent", "memory", "1048576", NULL + }; + uint32_t strict; + + progname = argv[0]; + + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Connect to the server. */ + if (nbd_connect_command (nbd, (char **)cmd) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* FIXME: future API addition to test if server negotiated extended mode. + * Until then, strict flags must ignore the PAYLOAD_LEN flag for pwrite, + * even though it is rejected for other commands. + */ + strict = nbd_get_strict_mode (nbd); + if (!(strict & LIBNBD_STRICT_FLAGS)) { + fprintf (stderr, "%s: test failed: " + "nbd_get_strict_mode did not have expected flag set\n", + progname); + exit (EXIT_FAILURE); + } + if (nbd_aio_pread (nbd, buf, 512, 0, NBD_NULL_COMPLETION, + LIBNBD_CMD_FLAG_PAYLOAD_LEN) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_aio_pread did not fail with unexpected flag\n", + progname); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_aio_pread: "); + + if (nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION, + LIBNBD_CMD_FLAG_PAYLOAD_LEN) == -1 || + nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION, 0) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + + nbd_close (nbd); + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index b43e83c0..e5304f16 100644 --- a/.gitignore +++ b/.gitignore @@ -252,6 +252,7 @@ Makefile.in /tests/pki/ /tests/pread-initialize /tests/private-data +/tests/pwrite-extended /tests/read-only-flag /tests/read-write-flag /tests/server-death

Support receiving headers for 64-bit replies if extended headers were negotiated. We already insist that the server not send us too much payload in one reply, so we can exploit that and continue to use h->payload_left as a 32-bit length for the rest of the payload parsing. The NBD protocol specifically documents that extended mode takes precedence over structured replies, and that there are no simple replies in extended mode. We can also take advantage that the cookie field is in the same offset in all the various reply types. Checking that the server sent back the correct offset adds a bit more verbosity for the sake of nicer debug messages. Note that if we negotiate extended headers, but a non-compliant server replies with a non-extended header, this patch will stall waiting for the server to send more bytes rather than noticing that the magic number is wrong (for aio operations, you'll get a magic number mismatch once you send a second command that elicits a reply; but for blocking operations, we basically deadlock). The easy alternative would be to read just the first 4 bytes of magic, then determine how many more bytes to expect; but that would require more states and syscalls, and not worth it since the typical server will be compliant. The other alternative is what the next patch implements: teaching REPLY.RECV_REPLY to handle short reads that were at least long enough to transmit magic to specifically look for magic number mismatch. At this point, h->extended_headers is permanently false (we can't enable it until all other aspects of the protocol have likewise been converted). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: don't normalize length in-place [Laszlo], rebase on earlier changes, better debug reporting if offsets mismatch --- lib/internal.h | 1 + generator/states-reply.c | 89 ++++++++++++++++++++++++---------- generator/states-reply-chunk.c | 40 +++++++++++---- 3 files changed, 95 insertions(+), 35 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 2e4f987c..9df6a685 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -244,6 +244,7 @@ struct nbd_handle { union reply_header { struct nbd_simple_reply simple; struct nbd_structured_reply structured; + struct nbd_extended_reply extended; /* The wire formats share magic and cookie at the same offsets; * provide aliases for one less level of typing. */ diff --git a/generator/states-reply.c b/generator/states-reply.c index 0d200513..05301ae8 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -68,22 +68,30 @@ REPLY.START: */ ssize_t r; - /* We read all replies initially as if they are simple replies, but - * check the magic in CHECK_REPLY_MAGIC below. This works because - * the structured_reply header is larger, and because the last - * member of a simple reply, cookie, is coincident between the two - * structs (an intentional design decision in the NBD spec when - * structured replies were added). + /* With extended headers, there is only one size to read, so we can + * do it all in one syscall. But for older structured replies, we + * don't know if we have a simple or structured reply until we read + * the magic number, requiring a two-part read with + * CHECK_REPLY_MAGIC below. This works because the structured_reply + * header is larger, and because the last member of a simple reply, + * cookie, is coincident between all three structs (intentional + * design decisions in the NBD spec when structured and extended + * replies were added). */ ASSERT_MEMBER_ALIAS (union reply_header, simple.magic, magic); ASSERT_MEMBER_ALIAS (union reply_header, simple.cookie, cookie); ASSERT_MEMBER_ALIAS (union reply_header, structured.magic, magic); ASSERT_MEMBER_ALIAS (union reply_header, structured.cookie, cookie); + ASSERT_MEMBER_ALIAS (union reply_header, extended.magic, magic); + ASSERT_MEMBER_ALIAS (union reply_header, extended.cookie, cookie); assert (h->reply_cmd == NULL); assert (h->rlen == 0); h->rbuf = &h->sbuf.reply.hdr; - h->rlen = sizeof h->sbuf.reply.hdr.simple; + if (h->extended_headers) + h->rlen = sizeof h->sbuf.reply.hdr.extended; + else + h->rlen = sizeof h->sbuf.reply.hdr.simple; r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen); if (r == -1) { @@ -129,10 +137,25 @@ REPLY.CHECK_REPLY_MAGIC: uint64_t cookie; magic = be32toh (h->sbuf.reply.hdr.magic); - if (magic == NBD_SIMPLE_REPLY_MAGIC) { + switch (magic) { + case NBD_SIMPLE_REPLY_MAGIC: + if (h->extended_headers) + /* Server is non-compliant, and we've already read more bytes + * than a simple header contains; no recovery possible + */ + goto invalid; + + /* All other payload checks handled in the simple payload engine */ SET_NEXT_STATE (%SIMPLE_REPLY.START); - } - else if (magic == NBD_STRUCTURED_REPLY_MAGIC) { + break; + + case NBD_STRUCTURED_REPLY_MAGIC: + if (h->extended_headers) + /* Server is non-compliant, and we've already read more bytes + * than a structured header contains; no recovery possible + */ + goto invalid; + /* We've only read the bytes that fill hdr.simple. But * hdr.structured is longer, so prepare to read the remaining * bytes. We depend on the memory aliasing in union sbuf to @@ -145,23 +168,29 @@ REPLY.CHECK_REPLY_MAGIC: h->rlen = sizeof h->sbuf.reply.hdr.structured; h->rlen -= sizeof h->sbuf.reply.hdr.simple; SET_NEXT_STATE (%RECV_STRUCTURED_REMAINING); - } - else { - /* We've probably lost synchronization. */ - SET_NEXT_STATE (%.DEAD); - set_error (0, "invalid reply magic 0x%" PRIx32, magic); -#if 0 /* uncomment to see desynchronized data */ - nbd_internal_hexdump (&h->sbuf.reply.hdr.simple, - sizeof (h->sbuf.reply.hdr.simple), - stderr); -#endif - return 0; + break; + + case NBD_EXTENDED_REPLY_MAGIC: + if (!h->extended_headers) + /* Server is non-compliant. We could continue reading bytes up + * to the length of an extended reply to regain sync, but old + * servers are unlikely to send this magic, so it's just as easy + * to punt. + */ + goto invalid; + + /* All other payload checks handled in the chunk payload engine */ + SET_NEXT_STATE (%CHUNK_REPLY.START); + break; + + default: + goto invalid; } - /* NB: This works for both simple and structured replies, even - * though we haven't finished reading the structured header yet, - * because the cookie is stored at the same offset. See the - * STATIC_ASSERT above in state REPLY.START that confirmed this. + /* NB: This works for all three reply types, even though we haven't + * finished reading a structured header yet, because the cookie is + * stored at the same offset. See the ASSERT_MEMBER_ALIAS above in + * state REPLY.START that confirmed this. */ h->chunks_received++; cookie = be64toh (h->sbuf.reply.hdr.cookie); @@ -175,6 +204,16 @@ REPLY.CHECK_REPLY_MAGIC: h->reply_cmd = cmd; return 0; + invalid: + SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */ + set_error (0, "invalid or unexpected reply magic 0x%" PRIx32, magic); +#if 0 /* uncomment to see desynchronized data */ + nbd_internal_hexdump (&h->sbuf.reply.hdr.simple, + sizeof (h->sbuf.reply.hdr.simple), + stderr); +#endif + return 0; + REPLY.RECV_STRUCTURED_REMAINING: switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 1758ac34..17bb5149 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -69,11 +69,17 @@ STATE_MACHINE { REPLY.CHUNK_REPLY.START: struct command *cmd = h->reply_cmd; uint16_t flags, type; - uint32_t length; + uint64_t length; + uint64_t offset = -1; flags = be16toh (h->sbuf.reply.hdr.structured.flags); type = be16toh (h->sbuf.reply.hdr.structured.type); - length = be32toh (h->sbuf.reply.hdr.structured.length); + if (h->extended_headers) { + length = be64toh (h->sbuf.reply.hdr.extended.length); + offset = be64toh (h->sbuf.reply.hdr.extended.offset); + } + else + length = be32toh (h->sbuf.reply.hdr.structured.length); /* Reject a server that replies with too much information, but don't * reject a single structured reply to NBD_CMD_READ on the largest @@ -83,13 +89,14 @@ REPLY.CHUNK_REPLY.START: * not worth keeping the connection alive. */ if (length > MAX_REQUEST_SIZE + sizeof h->sbuf.reply.payload.offset_data) { - set_error (0, "invalid server reply length %" PRIu32, length); + set_error (0, "invalid server reply length %" PRIu64, length); SET_NEXT_STATE (%.DEAD); return 0; } /* Skip an unexpected structured reply, including to an unknown cookie. */ - if (cmd == NULL || !h->structured_replies) + if (cmd == NULL || !h->structured_replies || + (h->extended_headers && offset != cmd->offset)) goto resync; h->payload_left = length; @@ -504,7 +511,8 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: REPLY.CHUNK_REPLY.RESYNC: struct command *cmd = h->reply_cmd; uint16_t type; - uint32_t length; + uint64_t length; + uint64_t offset = -1; assert (h->rbuf == NULL); switch (recv_into_rbuf (h)) { @@ -524,11 +532,23 @@ REPLY.CHUNK_REPLY.RESYNC: return 0; } type = be16toh (h->sbuf.reply.hdr.structured.type); - length = be32toh (h->sbuf.reply.hdr.structured.length); - debug (h, "unexpected reply type %u or payload length %" PRIu32 - " for cookie %" PRIu64 " and command %" PRIu32 - ", this is probably a server bug", - type, length, cmd->cookie, cmd->type); + if (h->extended_headers) { + length = be64toh (h->sbuf.reply.hdr.extended.length); + offset = be64toh (h->sbuf.reply.hdr.extended.offset); + if (offset != cmd->offset) + debug (h, "unexpected reply offset %" PRIu64 " for cookie %" PRIu64 + " and command %" PRIu32 ", this is probably a server bug", + length, cmd->cookie, cmd->type); + else + offset = -1; + } + else + length = be32toh (h->sbuf.reply.hdr.structured.length); + if (offset == -1) + debug (h, "unexpected reply type %u or payload length %" PRIu64 + " for cookie %" PRIu64 " and command %" PRIu32 + ", this is probably a server bug", + type, length, cmd->cookie, cmd->type); if (cmd->error == 0) cmd->error = EPROTO; SET_NEXT_STATE (%FINISH);

One of the benefits of extended replies is that we can do a fixed-length read for the entire header of every server reply, which is fewer syscalls than the split-read approach required by structured replies. But one of the drawbacks of doing a large read is that if the server is non-compliant (not a problem for normal servers, but something I hit rather more than I'd like to admit while developing extended header support in servers), nbd_pwrite() and friends will deadlock if the server replies with the wrong header. Add in some code to catch that failure mode and move the state machine to DEAD sooner, to make it easier to diagnose the fault in the server. Unlike in the case of an unexpected simple reply from a structured server (where, since b31e7bac, we can merely fail the command with EPROTO and successfully move on to the next server reply, because we didn't read too many bytes yet), in this case we really do have to move to DEAD: we cannot assume our short read was just the 16 (simple) or 20 (structured) bytes that the server sent for this command, because it is also possible that we can see a short read even while reading two back-to-back replies; if we have already read the initial bytes of the server's next reply, we have no way to push those extra bytes back onto our read stream for parsing on our next pass through the state machine. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- v4: rebase to master, improve comment accuracy [Laszlo], drop bogus #if 0 --- generator/states-reply.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 05301ae8..9f0918e2 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -126,7 +126,25 @@ REPLY.START: REPLY.RECV_REPLY: switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; - case 1: SET_NEXT_STATE (%.READY); return 0; + case 1: SET_NEXT_STATE (%.READY); + /* Special case: if we have a short read, but got at least far + * enough to decode the magic number, we can check if the server + * is matching our expectations. This lets us avoid deadlocking if + * we are blocked waiting for a 32-byte extended reply, while a + * buggy server only sent a shorter simple or structured reply. + * Magic number checks here must be repeated in CHECK_REPLY_MAGIC, + * since we do not always encounter a short read. + */ + if (h->extended_headers && + (char *)h->rbuf >= + (char *)&h->sbuf.reply.hdr + sizeof h->sbuf.reply.hdr.magic) { + uint32_t magic = be32toh (h->sbuf.reply.hdr.magic); + if (magic != NBD_EXTENDED_REPLY_MAGIC) { + SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */ + set_error (0, "invalid or unexpected reply magic 0x%" PRIx32, magic); + } + } + return 0; case 0: SET_NEXT_STATE (%CHECK_REPLY_MAGIC); } return 0;