24.03.2019 0:26, Eric Blake wrote: > The NBD spec states that NBD_CMD_FLAG_REQ_ONE (which we currently > always use) should not reply with an extent larger than our request, > and that the server's response should be exactly one extent. Right > now, that means that if a server sends more than one extent, we treat > the server as broken, fail the block status request, and disconnect, > which prevents all further use of the block device. But while good > software should be strict in what it sends, it should be tolerant in > what it receives. > > While trying to implement NBD_CMD_BLOCK_STATUS in nbdkit, we > temporarily had a non-compliant server sending too many extents in > spite of REQ_ONE. Oddly enough, 'qemu-img convert' with qemu 3.1 > failed with a somewhat useful message: > qemu-img: Protocol error: invalid payload for NBD_REPLY_TYPE_BLOCK_STATUS > > which then disappeared with commit d8b4bad8, on the grounds that an > error message flagged only at the time of coroutine teardown is > pointless, and instead we should rely on the actual failed API to > report an error - in other words, the 3.1 behavior was masking the > fact that qemu-img was not reporting an error. That has since been > fixed in the previous patch, where qemu-img convert now fails with: > qemu-img: Could not read sector 0 metadata: Invalid argument > > But even that is harsh. Since we already partially relaxed things in > commit acfd8f7a to tolerate a server that exceeds the cap (although > that change was made prior to the NBD spec actually putting a cap on > the extent length during REQ_ONE - in fact, the NBD spec change was > BECAUSE of the qemu behavior prior to that commit), it's not that much > harder to argue that we should also tolerate a server that sends too > many extents. But at the same time, it's nice to trace when we are > being tolerant of server non-compliance, in order to help server > writers fix their implementations to be more portable (if they refer > to our traces, rather than just stderr).
if they.. which is unlikely. So don't it worth a warn_report instead? anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]> > > Reported-by: Richard W.M. Jones <[email protected]> > Signed-off-by: Eric Blake <[email protected]> > --- > block/nbd-client.c | 21 ++++++++++++++++----- > block/trace-events | 1 + > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/block/nbd-client.c b/block/nbd-client.c > index 5fc9ea40383..2b388b1e422 100644 > --- a/block/nbd-client.c > +++ b/block/nbd-client.c > @@ -240,8 +240,8 @@ static int > nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk, > } > > /* nbd_parse_blockstatus_payload > - * support only one extent in reply and only for > - * base:allocation context > + * Based on our request, we expect only one extent in reply, for the > + * base:allocation context. > */ > static int nbd_parse_blockstatus_payload(NBDClientSession *client, > NBDStructuredReplyChunk *chunk, > @@ -250,7 +250,8 @@ static int nbd_parse_blockstatus_payload(NBDClientSession > *client, > { > uint32_t context_id; > > - if (chunk->length != sizeof(context_id) + sizeof(*extent)) { > + /* The server succeeded, so it must have sent [at least] one extent */ > + if (chunk->length < sizeof(context_id) + sizeof(*extent)) { > error_setg(errp, "Protocol error: invalid payload for " > "NBD_REPLY_TYPE_BLOCK_STATUS"); > return -EINVAL; > @@ -276,10 +277,20 @@ static int > nbd_parse_blockstatus_payload(NBDClientSession *client, > return -EINVAL; > } > > - /* The server is allowed to send us extra information on the final > - * extent; just clamp it to the length we requested. */ > + /* > + * We used NBD_CMD_FLAG_REQ_ONE, so the server should not have > + * sent us any more than one extent, nor should it have included > + * status beyond our request in that extent. However, it's easy > + * enough to ignore the server's noncompliance without killing the > + * connection; just ignore trailing extents, and clamp things to > + * the length of our request. > + */ > + if (chunk->length > sizeof(context_id) + sizeof(*extent)) { > + trace_nbd_parse_blockstatus_compliance("more than one extent"); > + } > if (extent->length > orig_length) { > extent->length = orig_length; > + trace_nbd_parse_blockstatus_compliance("extent length too large"); > } > > return 0; > diff --git a/block/trace-events b/block/trace-events > index 70056eafd20..01ae6475707 100644 > --- a/block/trace-events > +++ b/block/trace-events > @@ -158,6 +158,7 @@ nvme_cmd_map_qiov_iov(void *s, int i, void *page, int > pages) "s %p iov[%d] %p pa > iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t > dst_off, uint64_t bytes, int ret) "src_lun %p offset %"PRIu64" dst_lun %p > offset %"PRIu64" bytes %"PRIu64" ret %d" > > # block/nbd-client.c > +nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from > non-compliant server: %s" > nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s" > nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t > flags, uint16_t type, const char *name, int ret, const char *err) "Request > failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", > .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s" > -- Best regards, Vladimir
