| Message ID | 20230515195343.1915857-12-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu patches for 64-bit NBD extensions | expand |
On 15.05.23 22:53, Eric Blake wrote: > Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a > client in narrow mode should not be able to provoke a server into > sending a block status result larger than the client's 32-bit request. > But in extended mode, a 64-bit status request must be able to handle a > 64-bit status result, once a future patch enables the client > requesting extended mode. We can also tolerate a non-compliant server > sending the new chunk even when it should not. > > In normal execution, we are only requesting "base:allocation" which > never exceeds 32 bits. 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 it is only testing, we really don't bother with > checking whether more than the two least-significant bits are set. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/nbd.c | 39 ++++++++++++++++++++++++++++----------- > block/trace-events | 1 + > 2 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/block/nbd.c b/block/nbd.c > index d6caea44928..150dfe7170c 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -610,13 +610,16 @@ 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, > - NBDExtent *extent, Error **errp) > + uint8_t *payload, bool wide, > + uint64_t orig_length, > + NBDExtentExt *extent, Error **errp) > { > uint32_t context_id; > + uint32_t count = 0; > + size_t len = wide ? sizeof(*extent) : sizeof(NBDExtent); > > /* The server succeeded, so it must have sent [at least] one extent */ > - if (chunk->length < sizeof(context_id) + sizeof(*extent)) { > + if (chunk->length < sizeof(context_id) + wide * sizeof(count) + len) { > error_setg(errp, "Protocol error: invalid payload for " > "NBD_REPLY_TYPE_BLOCK_STATUS"); > return -EINVAL; > @@ -631,8 +634,14 @@ 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 { > + extent->length = payload_advance32(&payload); > + extent->flags = payload_advance32(&payload); > + } > > if (extent->length == 0) { > error_setg(errp, "Protocol error: server sent status chunk with " > @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s, > * connection; just ignore trailing extents, and clamp things to > * the length of our request. > */ > - if (chunk->length > sizeof(context_id) + sizeof(*extent)) { > + if (count != wide || hard to read for me. Could it be simply "count > 1 ||" ? > + chunk->length > sizeof(context_id) + wide * sizeof(count) + len) { > trace_nbd_parse_blockstatus_compliance("more than one extent"); > } > if (extent->length > orig_length) { > @@ -1117,7 +1127,7 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h > > static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > uint64_t handle, uint64_t length, > - NBDExtent *extent, > + NBDExtentExt *extent, > int *request_ret, Error **errp) > { > NBDReplyChunkIter iter; > @@ -1125,6 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > void *payload = NULL; > Error *local_err = NULL; > bool received = false; > + bool wide = false; > > assert(!extent->length); > NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { > @@ -1134,7 +1145,13 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > assert(nbd_reply_is_structured(&reply)); > > switch (chunk->type) { > + case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: > + wide = true; > + /* fallthrough */ > case NBD_REPLY_TYPE_BLOCK_STATUS: > + if (s->info.extended_headers != wide) { > + trace_nbd_extended_headers_compliance("block_status"); You set wide to true once, on first "NBD_REPLY_TYPE_BLOCK_STATUS_EXT", and then parse following "NBD_REPLY_TYPE_BLOCK_STATUS" if the come with wide=true. Should it be: --- a/block/nbd.c +++ b/block/nbd.c @@ -1135,7 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, void *payload = NULL; Error *local_err = NULL; bool received = false; - bool wide = false; + bool wide; assert(!extent->length); NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { @@ -1146,9 +1146,8 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, switch (chunk->type) { case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: - wide = true; - /* fallthrough */ case NBD_REPLY_TYPE_BLOCK_STATUS: + wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT; if (s->info.extended_headers != 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"); > @@ -1142,9 +1159,9 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > } > 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); > @@ -1374,7 +1391,7 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status( > int64_t *pnum, int64_t *map, BlockDriverState **file) > { > int ret, request_ret; > - NBDExtent extent = { 0 }; > + NBDExtentExt extent = { 0 }; > BDRVNBDState *s = (BDRVNBDState *)bs->opaque; > Error *local_err = NULL; > > diff --git a/block/trace-events b/block/trace-events > index 48dbf10c66f..afb32fcce5b 100644 > --- a/block/trace-events > +++ b/block/trace-events > @@ -168,6 +168,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, 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" > nbd_client_handshake(const char *export_name) "export '%s'"
On Wed, May 31, 2023 at 08:00:43PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 15.05.23 22:53, Eric Blake wrote: > > Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a > > client in narrow mode should not be able to provoke a server into > > sending a block status result larger than the client's 32-bit request. > > But in extended mode, a 64-bit status request must be able to handle a > > 64-bit status result, once a future patch enables the client > > requesting extended mode. We can also tolerate a non-compliant server > > sending the new chunk even when it should not. > > > > @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s, > > * connection; just ignore trailing extents, and clamp things to > > * the length of our request. > > */ > > - if (chunk->length > sizeof(context_id) + sizeof(*extent)) { > > + if (count != wide || > > hard to read for me. Could it be simply "count > 1 ||" ? For existing commands (compact), count is initialized to 0 as it is not transferred over the wire. For extended commands, count is transferred over the wire, but we expect it to be 1 (and not 0). Comparing count != wide is more precise than checking count > 0 (which should never happen for compact, but would be a bug for extended). > > > + chunk->length > sizeof(context_id) + wide * sizeof(count) + len) { > > trace_nbd_parse_blockstatus_compliance("more than one extent"); > > } > > if (extent->length > orig_length) { > > @@ -1117,7 +1127,7 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h > > > > static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > > uint64_t handle, uint64_t length, > > - NBDExtent *extent, > > + NBDExtentExt *extent, > > int *request_ret, Error **errp) > > { > > NBDReplyChunkIter iter; > > @@ -1125,6 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > > void *payload = NULL; > > Error *local_err = NULL; > > bool received = false; > > + bool wide = false; > > > > assert(!extent->length); > > NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { > > @@ -1134,7 +1145,13 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > > assert(nbd_reply_is_structured(&reply)); > > > > switch (chunk->type) { > > + case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: > > + wide = true; > > + /* fallthrough */ > > case NBD_REPLY_TYPE_BLOCK_STATUS: > > + if (s->info.extended_headers != wide) { > > + trace_nbd_extended_headers_compliance("block_status"); > > You set wide to true once, on first "NBD_REPLY_TYPE_BLOCK_STATUS_EXT", and then parse following "NBD_REPLY_TYPE_BLOCK_STATUS" if the come with wide=true. > > Should it be: > > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -1135,7 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > void *payload = NULL; > Error *local_err = NULL; > bool received = false; > - bool wide = false; > + bool wide; > assert(!extent->length); > NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { > @@ -1146,9 +1146,8 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, > switch (chunk->type) { > case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: > - wide = true; > - /* fallthrough */ > case NBD_REPLY_TYPE_BLOCK_STATUS: > + wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT; > if (s->info.extended_headers != wide) { Good observation, since we reach this multiple times in a loop. I'm squashing that in.
On 31.05.23 20:40, Eric Blake wrote: > On Wed, May 31, 2023 at 08:00:43PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 15.05.23 22:53, Eric Blake wrote: >>> Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a >>> client in narrow mode should not be able to provoke a server into >>> sending a block status result larger than the client's 32-bit request. >>> But in extended mode, a 64-bit status request must be able to handle a >>> 64-bit status result, once a future patch enables the client >>> requesting extended mode. We can also tolerate a non-compliant server >>> sending the new chunk even when it should not. >>> >>> @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s, >>> * connection; just ignore trailing extents, and clamp things to >>> * the length of our request. >>> */ >>> - if (chunk->length > sizeof(context_id) + sizeof(*extent)) { >>> + if (count != wide || >> >> hard to read for me. Could it be simply "count > 1 ||" ? > > For existing commands (compact), count is initialized to 0 as it is > not transferred over the wire. For extended commands, count is > transferred over the wire, but we expect it to be 1 (and not 0). > Comparing count != wide is more precise than checking count > 0 (which > should never happen for compact, but would be a bug for extended). The only case you add to the check is when count = 0 for extended. But in this case "more than one extent" message is counterintuitive. > >> >>> + chunk->length > sizeof(context_id) + wide * sizeof(count) + len) { >>> trace_nbd_parse_blockstatus_compliance("more than one extent"); >>> } >>> if (extent->length > orig_length) { >>> @@ -1117,7 +1127,7 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h >>> >>> static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, >>> uint64_t handle, uint64_t length, >>> - NBDExtent *extent, >>> + NBDExtentExt *extent, >>> int *request_ret, Error **errp) >>> { >>> NBDReplyChunkIter iter; >>> @@ -1125,6 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, >>> void *payload = NULL; >>> Error *local_err = NULL; >>> bool received = false; >>> + bool wide = false; >>> >>> assert(!extent->length); >>> NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { >>> @@ -1134,7 +1145,13 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, >>> assert(nbd_reply_is_structured(&reply)); >>> >>> switch (chunk->type) { >>> + case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: >>> + wide = true; >>> + /* fallthrough */ >>> case NBD_REPLY_TYPE_BLOCK_STATUS: >>> + if (s->info.extended_headers != wide) { >>> + trace_nbd_extended_headers_compliance("block_status"); >> >> You set wide to true once, on first "NBD_REPLY_TYPE_BLOCK_STATUS_EXT", and then parse following "NBD_REPLY_TYPE_BLOCK_STATUS" if the come with wide=true. >> >> Should it be: >> >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -1135,7 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, >> void *payload = NULL; >> Error *local_err = NULL; >> bool received = false; >> - bool wide = false; >> + bool wide; >> assert(!extent->length); >> NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { >> @@ -1146,9 +1146,8 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, >> switch (chunk->type) { >> case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: >> - wide = true; >> - /* fallthrough */ >> case NBD_REPLY_TYPE_BLOCK_STATUS: >> + wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT; >> if (s->info.extended_headers != wide) { > > Good observation, since we reach this multiple times in a loop. I'm > squashing that in. >
diff --git a/block/nbd.c b/block/nbd.c index d6caea44928..150dfe7170c 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -610,13 +610,16 @@ 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, - NBDExtent *extent, Error **errp) + uint8_t *payload, bool wide, + uint64_t orig_length, + NBDExtentExt *extent, Error **errp) { uint32_t context_id; + uint32_t count = 0; + size_t len = wide ? sizeof(*extent) : sizeof(NBDExtent); /* The server succeeded, so it must have sent [at least] one extent */ - if (chunk->length < sizeof(context_id) + sizeof(*extent)) { + if (chunk->length < sizeof(context_id) + wide * sizeof(count) + len) { error_setg(errp, "Protocol error: invalid payload for " "NBD_REPLY_TYPE_BLOCK_STATUS"); return -EINVAL; @@ -631,8 +634,14 @@ 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 { + extent->length = payload_advance32(&payload); + extent->flags = payload_advance32(&payload); + } if (extent->length == 0) { error_setg(errp, "Protocol error: server sent status chunk with " @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s, * connection; just ignore trailing extents, and clamp things to * the length of our request. */ - if (chunk->length > sizeof(context_id) + sizeof(*extent)) { + if (count != wide || + chunk->length > sizeof(context_id) + wide * sizeof(count) + len) { trace_nbd_parse_blockstatus_compliance("more than one extent"); } if (extent->length > orig_length) { @@ -1117,7 +1127,7 @@ static int coroutine_fn nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t handle, uint64_t length, - NBDExtent *extent, + NBDExtentExt *extent, int *request_ret, Error **errp) { NBDReplyChunkIter iter; @@ -1125,6 +1135,7 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, void *payload = NULL; Error *local_err = NULL; bool received = false; + bool wide = false; assert(!extent->length); NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) { @@ -1134,7 +1145,13 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, assert(nbd_reply_is_structured(&reply)); switch (chunk->type) { + case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: + wide = true; + /* fallthrough */ case NBD_REPLY_TYPE_BLOCK_STATUS: + if (s->info.extended_headers != 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"); @@ -1142,9 +1159,9 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s, } 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); @@ -1374,7 +1391,7 @@ static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status( int64_t *pnum, int64_t *map, BlockDriverState **file) { int ret, request_ret; - NBDExtent extent = { 0 }; + NBDExtentExt extent = { 0 }; BDRVNBDState *s = (BDRVNBDState *)bs->opaque; Error *local_err = NULL; diff --git a/block/trace-events b/block/trace-events index 48dbf10c66f..afb32fcce5b 100644 --- a/block/trace-events +++ b/block/trace-events @@ -168,6 +168,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, 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" nbd_client_handshake(const char *export_name) "export '%s'"
Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a client in narrow mode should not be able to provoke a server into sending a block status result larger than the client's 32-bit request. But in extended mode, a 64-bit status request must be able to handle a 64-bit status result, once a future patch enables the client requesting extended mode. We can also tolerate a non-compliant server sending the new chunk even when it should not. In normal execution, we are only requesting "base:allocation" which never exceeds 32 bits. 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 it is only testing, we really don't bother with checking whether more than the two least-significant bits are set. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/nbd.c | 39 ++++++++++++++++++++++++++++----------- block/trace-events | 1 + 2 files changed, 29 insertions(+), 11 deletions(-)