Skip to content

Commit

Permalink
nbd/client: Accept 64-bit block status chunks
Browse files Browse the repository at this point in the history 
Once extended mode is enabled, we need to accept 64-bit status replies
(even for replies that don't exceed a 32-bit length).  It is easier to
normalize narrow replies into wide format so that the rest of our code
only has to handle one width.  Although a server is non-compliant if
it sends a 64-bit reply in compact mode, or a 32-bit reply in extended
mode, it is still easy enough to tolerate these mismatches.

In normal execution, we are only requesting "base:allocation" which
never exceeds 32 bits for flag values. But during testing with
x-dirty-bitmap, we can force qemu to connect to some other context
that might have 64-bit status bit; however, we ignore those upper bits
(other than mapping qemu:allocation-depth into something that
'qemu-img map --output=json' can expose), and since that only affects
testing, we really don't bother with checking whether more than the
two least-significant bits are set.

Signed-off-by: Eric Blake <[email protected]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Message-ID: <[email protected]>
  • Loading branch information
@ebblake
ebblake committed Oct 5, 2023
1 parent 4fc55bf commit a7c1867
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 15 deletions.
49 changes: 34 additions & 15 deletions block/nbd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -615,13 +615,17 @@ static int nbd_parse_offset_hole_payload(BDRVNBDState *s,
*/
static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
NBDStructuredReplyChunk *chunk,
uint8_t *payload, uint64_t orig_length,
NBDExtent32 *extent, Error **errp)
uint8_t *payload, bool wide,
uint64_t orig_length,
NBDExtent64 *extent, Error **errp)
{
uint32_t context_id;
uint32_t count;
size_t ext_len = wide ? sizeof(*extent) : sizeof(NBDExtent32);
size_t pay_len = sizeof(context_id) + wide * sizeof(count) + ext_len;

/* The server succeeded, so it must have sent [at least] one extent */
if (chunk->length < sizeof(context_id) + sizeof(*extent)) {
if (chunk->length < pay_len) {
error_setg(errp, "Protocol error: invalid payload for "
"NBD_REPLY_TYPE_BLOCK_STATUS");
return -EINVAL;
Expand All @@ -636,8 +640,15 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
return -EINVAL;
}

extent->length = payload_advance32(&payload);
extent->flags = payload_advance32(&payload);
if (wide) {
count = payload_advance32(&payload);
extent->length = payload_advance64(&payload);
extent->flags = payload_advance64(&payload);
} else {
count = 0;
extent->length = payload_advance32(&payload);
extent->flags = payload_advance32(&payload);
}

if (extent->length == 0) {
error_setg(errp, "Protocol error: server sent status chunk with "
Expand All @@ -658,7 +669,7 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
* (always a safe status, even if it loses information).
*/
if (s->info.min_block && !QEMU_IS_ALIGNED(extent->length,
s->info.min_block)) {
s->info.min_block)) {
trace_nbd_parse_blockstatus_compliance("extent length is unaligned");
if (extent->length > s->info.min_block) {
extent->length = QEMU_ALIGN_DOWN(extent->length,
Expand All @@ -672,13 +683,15 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
/*
* 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
* status beyond our request in that extent. Furthermore, a wide
* server should have replied with an accurate count (we left
* count at 0 for a narrow server). 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 (count != wide || chunk->length > pay_len) {
trace_nbd_parse_blockstatus_compliance("unexpected extent count");
}
if (extent->length > orig_length) {
extent->length = orig_length;
Expand Down Expand Up @@ -1124,7 +1137,7 @@ nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t cookie,

static int coroutine_fn
nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t cookie,
uint64_t length, NBDExtent32 *extent,
uint64_t length, NBDExtent64 *extent,
int *request_ret, Error **errp)
{
NBDReplyChunkIter iter;
Expand All @@ -1137,21 +1150,27 @@ nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t cookie,
NBD_FOREACH_REPLY_CHUNK(s, iter, cookie, false, NULL, &reply, &payload) {
int ret;
NBDStructuredReplyChunk *chunk = &reply.structured;
bool wide;

assert(nbd_reply_is_structured(&reply));

switch (chunk->type) {
case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
case NBD_REPLY_TYPE_BLOCK_STATUS:
wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT;
if ((s->info.mode >= NBD_MODE_EXTENDED) != wide) {
trace_nbd_extended_headers_compliance("block_status");
}
if (received) {
nbd_channel_error(s, -EINVAL);
error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
nbd_iter_channel_error(&iter, -EINVAL, &local_err);
}
received = true;

ret = nbd_parse_blockstatus_payload(s, &reply.structured,
payload, length, extent,
&local_err);
ret = nbd_parse_blockstatus_payload(
s, &reply.structured, payload, wide,
length, extent, &local_err);
if (ret < 0) {
nbd_channel_error(s, ret);
nbd_iter_channel_error(&iter, ret, &local_err);
Expand Down Expand Up @@ -1381,7 +1400,7 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status(
int64_t *pnum, int64_t *map, BlockDriverState **file)
{
int ret, request_ret;
NBDExtent32 extent = { 0 };
NBDExtent64 extent = { 0 };
BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
Error *local_err = NULL;

Expand Down
1 change: 1 addition & 0 deletions block/trace-events
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, ui
# nbd.c
nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from non-compliant server: %s"
nbd_structured_read_compliance(const char *type) "server sent non-compliant unaligned read %s chunk"
nbd_extended_headers_compliance(const char *type) "server sent non-compliant %s chunk not matching choice of extended headers"
nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
nbd_co_request_fail(uint64_t from, uint64_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) "Request failed { .from = %" PRIu64", .len = %" PRIu64 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"
nbd_client_handshake(const char *export_name) "export '%s'"
Expand Down

0 comments on commit a7c1867

Please sign in to comment.