various NBD fixes for 2.11
cover various NBD fixes for 2.11
Sparked by my question on whether we were handling zero-length reads correctly according to the NBD spec, but grew to fix other things as well such as some typos, tracing weaknesses, and better handling of read-only exports.
The order these patches are listed here is bisection-friendly, but applying the patches out of order (for example, applying 7/8 before any short-circuiting patches stop the client from sending 0-length requests, and before the server is fixed to stop botching replies to 0-length requests) proved important to my ability to test that the patches are needed.
One of the patches is not strictly NBD, and begs the question of whether other filter drivers (mirror, throttle, etc) should also be reflecting the read-only status of their underlying BDS. Some of my testing included setting up a read-only NBD server and a read-write qemu-io session, then attempting 'w 0 0', 'w -z 0 0', and 'd 0 0'; these should not succeed if the corresponding 'w 0 1' fails, but the end goal is that the server shouldn't even need to be involved in these corner cases.
Eric Blake (8):
nbd-client: Fix error message typos
nbd/client: Nicer trace of structured reply
raw: Reflect read-only protocol layer nbd-client: Honor server read-only advertisement nbd: Fix struct name for structured reads nbd-client: Short-circuit 0-length operations nbd-client: Stricter enforcing of structured reply spec nbd/server: Fix structured read of length 0
include/block/nbd.h | 18 +++++++++++++----- block/nbd-client.c | 38 +++++++++++++++++++++++++++++++------- block/raw-format.c | 6 ++++++ nbd/client.c | 4 +++- nbd/server.c | 23 +++++++++++++++++++++-- nbd/trace-events | 3 ++- 6 files changed, 76 insertions(+), 16 deletions(-)
Signature · 1 line
2.13.6
PATCH 1/8 nbd-client: Fix error message typos
Provide missing spaces that are required when using string concatenation to break error messages across source lines.
--- block/nbd-client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
block/nbd-client.c +3 -3
@@ -248,7 +248,7 @@ static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk, error =nbd_errno_to_system_errno (payload_advance32 (&payload));if (error ==0 ) { -error_setg (errp,"Protocol error: server sent structured error chunk" +error_setg (errp,"Protocol error: server sent structured error chunk " "with error = 0" );return -EINVAL ; }
@@ -257,7 +257,7 @@ static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk, message_size =payload_advance16 (&payload);if (message_size > chunk->length -sizeof (error) -sizeof (message_size)) { -error_setg (errp,"Protocol error: server sent structured error chunk" +error_setg (errp,"Protocol error: server sent structured error chunk " "with incorrect message size" );return -EINVAL ; }
@@ -408,7 +408,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(if (chunk->type ==NBD_REPLY_TYPE_NONE ) {if (!(chunk->flags &NBD_REPLY_FLAG_DONE )) {error_setg (errp,"Protocol error: NBD_REPLY_TYPE_NONE chunk without" -"NBD_REPLY_FLAG_DONE flag set" ); +" NBD_REPLY_FLAG_DONE flag set" );return -EINVAL ; }return 0 ;
Signature · 1 line
2.13.6
PATCH 2/8 nbd/client: Nicer trace of structured reply
In reply to Eric BlakeIt's useful to know which structured reply chunk is being processed.
--- nbd/client.c | 4 +++- nbd/trace-events | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-)
nbd/client.c +3 -1
@@ -979,6 +979,7 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,int nbd_receive_reply (QIOChannel *ioc,NBDReply *reply,Error **errp) {int ret; +const char *type; ret =nbd_read_eof (ioc, &reply->magic,sizeof (reply->magic), errp);if (ret <=0 ) {
@@ -1008,8 +1009,9 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)if (ret <0 ) {break ; } + type =nbd_reply_type_lookup (reply->structured.type);trace_nbd_receive_structured_reply_chunk (reply->structured.flags, - reply->structured.type, + reply->structured.type, type, reply->structured.handle, reply->structured.length);break ;
nbd/trace-events +1 -1
@@ -27,7 +27,7 @@ nbd_client_clear_queue(void) "Clearing NBD queue" nbd_client_clear_socket(void) "Clearing NBD socket" nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }" nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t handle) "Got simple reply: { .error = %" PRId32 " (%s), handle = %" PRIu64" }" -nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d, handle = %" PRIu64 ", length = %" PRIu32 " }" +nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char *name, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d (%s), handle = %" PRIu64 ", length = %" PRIu32 " }" # nbd/common.c nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
Signature · 1 line
2.13.6
PATCH 3/8 raw: Reflect read-only protocol layer
In reply to Eric BlakeWe forbid operations like a zero-length write zero or a discard at the protocol layer when it is marked read-only, but those same operations were succeeding at the format layer because the raw format was not reflecting the underlying read-only status to the block layer, which then took short circuit paths on zero-length operations.
--- block/raw-format.c | 6 ++++++ 1 file changed, 6 insertions(+)
block/raw-format.c +6 -0
@@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, bs->file->bs->supported_write_flags; bs->supported_zero_flags = (BDRV_REQ_FUA |BDRV_REQ_MAY_UNMAP ) & bs->file->bs->supported_zero_flags; +if (bdrv_is_read_only (bs->file->bs)) { + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } + }if (bs->probed && !bdrv_is_read_only (bs)) { fprintf(stderr,
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 3/8] raw: Reflect read-only protocol layer
Quoted 20 lines · Eric Blake
We forbid operations like a zero-length write zero or a discard at the protocol layer when it is marked read-only, but those same operations were succeeding at the format layer because the raw format was not reflecting the underlying read-only status to the block layer, which then took short circuit paths on zero-length operations.
Signed-off-by Eric Blake <[email protected]>--- block/raw-format.c | 6 ++++++ 1 file changed, 6 insertions(+)block/raw-format.c +0
-0@@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, bs->file->bs->supported_write_flags; bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
block/raw-format.c static int raw_open(BlockDriverState *bs, QDict *options, int flags, · line 429
Quoted 10 lines
block/raw-format.c +6
-0bs->file->bs->supported_zero_flags; +if (bdrv_is_read_only (bs->file->bs)) { + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } + }if (bs->probed && !bdrv_is_read_only (bs)) { fprintf(stderr,
Kevin, perhaps this should be done straight in block.c?
Thanks,
Paolo
Re: [Qemu-devel] [PATCH 3/8] raw: Reflect read-only protocol layer
Am 07.11.2017 um 12:00 hat Paolo Bonzini geschrieben:
Quoted 28 lines · Eric Blake
Quoted 27 lines
We forbid operations like a zero-length write zero or a discard at the protocol layer when it is marked read-only, but those same operations were succeeding at the format layer because the raw format was not reflecting the underlying read-only status to the block layer, which then took short circuit paths on zero-length operations.
Signed-off-by Eric Blake <[email protected]>--- block/raw-format.c | 6 ++++++ 1 file changed, 6 insertions(+)block/raw-format.c +6
-0@@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, bs->file->bs->supported_write_flags; bs->supported_zero_flags = (BDRV_REQ_FUA |BDRV_REQ_MAY_UNMAP ) & bs->file->bs->supported_zero_flags; +if (bdrv_is_read_only (bs->file->bs)) { + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } + }
Quoted 5 lines
block/raw-format.c static int raw_open(BlockDriverState *bs, QDict *options, int flags, · line 429
Quoted 3 lines
block/raw-format.c +0
-0if (bs->probed && !bdrv_is_read_only (bs)) { fprintf(stderr,Kevin, perhaps this should be done straight in
block.c?
No, I just discussed this with Eric on IRC, and it shouldn't be done anywhere.
Basically, the way qemu works with read-only images is that you need to be explicit about it and specify read-only=on. Drivers are not supposed to magically set the read-only flag if the user didn't request it.
So what NBD needs to do is to error out if you try to open a read-write connection to a read-only NBD server.
There's a second part, related specifically to this patch, that was a bit surprising to me at first, but it actually makes sense. I can successfully create a read-write raw node on top of a read-only file-posix node:
-blockdev driver=file,filename=/tmp/test.qcow2,node-name=proto,read-only=on -blockdev driver=raw,file=proto,node-name=format
This is because in this state without a user, the raw node doesn't actually want to write to the file-posix node. However, if I add a disk:
-device ide-hd,drive=format
That will actually request write permissions throughout the whole chain and cause an error:
qemu-system-x86_64: -device ide-hd,drive=format: Block node is read-only
Somewhat surprising at first, but it does make sense.
Kevin
PATCH 4/8 nbd-client: Honor server read-only advertisement
In reply to Eric BlakeThe NBD spec says that clients should not try to write/trim to an export advertised as read-only by the server. But we failed to inform the block layer of what we learned from the server, meaning that we were depending on the server sending a proper EPERM failure for various commands. In fact, this was noticeable in the case of a 0-length write: we had a difference in behavior depending on whether we were writing zero (silent success locally, because the driver is not invoked from the block layer) or normal writes (noisy failure, because we sent a 0-length request across the wire and the server flagged it as invalid); now we always fail locally without sending anything over the wire.
--- block/nbd-client.c | 9 +++++++++ 1 file changed, 9 insertions(+)
block/nbd-client.c +9 -0
@@ -697,6 +697,7 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset, .len = bytes, }; +assert (!(client->info.flags &NBD_FLAG_READ_ONLY ));if (flags &BDRV_REQ_FUA ) {assert (client->info.flags &NBD_FLAG_SEND_FUA ); request.flags |=NBD_CMD_FLAG_FUA ;
@@ -717,6 +718,7 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, .len = bytes, }; +assert (!(client->info.flags &NBD_FLAG_READ_ONLY ));if (!(client->info.flags &NBD_FLAG_SEND_WRITE_ZEROES )) {return -ENOTSUP ; }
@@ -756,6 +758,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes) .len = bytes, }; +assert (!(client->info.flags &NBD_FLAG_READ_ONLY ));if (!(client->info.flags &NBD_FLAG_SEND_TRIM )) {return 0 ; }
@@ -814,6 +817,12 @@ int nbd_client_init(BlockDriverState *bs,logout ("Failed to negotiate with the NBD server\n" );return ret; } +if (client->info.flags &NBD_FLAG_READ_ONLY ) { + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } + }if (client->info.flags &NBD_FLAG_SEND_FUA ) { bs->supported_write_flags =BDRV_REQ_FUA ; bs->supported_zero_flags |=BDRV_REQ_FUA ;
Signature · 1 line
2.13.6
PATCH 5/8 nbd: Fix struct name for structured reads
In reply to Eric BlakeA closer read of the NBD spec shows that a structured reply chunk for a hole is not quite identical to the prefix of a data chunk, because the hole has to also send a 32-bit size field. Although we do not yet send holes, we should fix the misleading information in our header and make it easier for a future patch to support sparse reads.
--- include/block/nbd.h | 18 +++++++++++++----- nbd/server.c | 2 +- 2 files changed, 14 insertions(+), 6 deletions(-)
include/block/nbd.h +13 -5
@@ -86,15 +86,23 @@ typedef union NBDReply { }QEMU_PACKED ; } NBDReply; -/* Header of NBD_REPLY_TYPE_OFFSET_DATA, complete NBD_REPLY_TYPE_OFFSET_HOLE */ -typedef struct NBDStructuredRead { -NBDStructuredReplyChunk h; +/* Header of chunk for NBD_REPLY_TYPE_OFFSET_DATA */ +typedef struct NBDStructuredReadData { +NBDStructuredReplyChunk h;/* h.length >= 9 */ uint64_t offset; -} QEMU_PACKED NBDStructuredRead; +/* At least one byte of data payload follows, calculated from h.length */ +} QEMU_PACKED NBDStructuredReadData; + +/* Complete chunk for NBD_REPLY_TYPE_OFFSET_HOLE */ +typedef struct NBDStructuredReadHole { +NBDStructuredReplyChunk h;/* h.length == 12 */ +uint64_t offset; +uint32_t length; +} QEMU_PACKED NBDStructuredReadHole;/* Header of all NBD_REPLY_TYPE_ERROR* errors */ typedef struct NBDStructuredError { -NBDStructuredReplyChunk h; +NBDStructuredReplyChunk h;/* h.length >= 6 */ uint32_t error;uint16_t message_length; } QEMU_PACKED NBDStructuredError;
nbd/server.c +1 -1
@@ -1280,7 +1280,7 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,size_t size, Error **errp) { - NBDStructuredRead chunk; + NBDStructuredReadData chunk; struct iovec iov[] = { {.iov_base = &chunk, .iov_len = sizeof(chunk)}, {.iov_base = data, .iov_len = size}
Signature · 1 line
2.13.6
PATCH 6/8 nbd-client: Short-circuit 0-length operations
In reply to Eric BlakeThe NBD spec is being clarified to state that clients should not send 0-length requests to the server, as the server behavior is undefined. While such requests are unlikely to occur from real guest behavior, qemu-io can force the driver to perform them; avoid any questionable server implementations by short-circuiting such operations as a no-op success (we are relying on the block layer for already filtering out things like invalid offset, write to a read-only volume, and so forth).
--- block/nbd-client.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
block/nbd-client.c +10 -1
@@ -673,6 +673,9 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,assert (bytes <=NBD_MAX_BUFFER_SIZE );assert (!flags); +if (!bytes) { +return 0 ; + } ret =nbd_co_send_request (bs, &request,NULL );if (ret <0 ) {
@@ -705,6 +708,9 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,assert (bytes <=NBD_MAX_BUFFER_SIZE ); +if (!bytes) { +return 0 ; + }return nbd_co_request (bs, &request, qiov); }
@@ -731,6 +737,9 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, request.flags |=NBD_CMD_FLAG_NO_HOLE ; } +if (!bytes) { +return 0 ; + }return nbd_co_request (bs, &request,NULL ); }
@@ -759,7 +768,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes) };assert (!(client->info.flags &NBD_FLAG_READ_ONLY )); -if (!(client->info.flags &NBD_FLAG_SEND_TRIM )) { +if (!(client->info.flags &NBD_FLAG_SEND_TRIM ) || !bytes) {return 0 ; }
Signature · 1 line
2.13.6
PATCH 7/8 nbd-client: Stricter enforcing of structured reply spec
In reply to Eric BlakeEnsure that the server is not sending unexpected chunk lengths for either the NONE or the OFFSET_DATA chunk, nor unexpected hole length for OFFSET_HOLE. This will flag any server that responds to a zero-length read with an OFFSET_DATA as broken, but we just fixed things to never send that request.
--- block/nbd-client.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
block/nbd-client.c +9 -3
@@ -216,7 +216,7 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk, offset =payload_advance64 (&payload); hole_size =payload_advance32 (&payload); -if (offset < orig_offset || hole_size > qiov->size || +if (!hole_size || offset < orig_offset || hole_size > qiov->size || offset > orig_offset + qiov->size - hole_size) {error_setg (errp,"Protocol error: server sent chunk exceeding requested" " region" );
@@ -281,7 +281,8 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,assert (nbd_reply_is_structured (&s->reply)); -if (chunk->length <sizeof (offset)) { +/* The NBD spec requires at least one byte of payload */ +if (chunk->length <=sizeof (offset)) {error_setg (errp,"Protocol error: invalid payload for " "NBD_REPLY_TYPE_OFFSET_DATA" );return -EINVAL ;
@@ -293,7 +294,7 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,be64_to_cpus (&offset); data_size = chunk->length -sizeof (offset); -if (offset < orig_offset || data_size > qiov->size || +if (!data_size || offset < orig_offset || data_size > qiov->size || offset > orig_offset + qiov->size - data_size) {error_setg (errp,"Protocol error: server sent chunk exceeding requested" " region" );
@@ -411,6 +412,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( " NBD_REPLY_FLAG_DONE flag set");return -EINVAL ; } +if (chunk->length) { +error_setg (errp,"Protocol error: NBD_REPLY_TYPE_NONE chunk with" +" nonzero length" ); +return -EINVAL ; + }return 0 ; }
Signature · 1 line
2.13.6
PATCH 8/8 nbd/server: Fix structured read of length 0
In reply to Eric BlakeThe NBD spec is being clarified that a read of length 0 should not be attempted by a compliant client; but that a server must handle it correctly. However, it also implies that NBD_REPLY_TYPE_OFFSET_DATA must have a non-zero payload length, which means that we must either give an error or use a no-op REPLY_TYPE_NONE in response to a zero-length read, so that we don't trip up any clients that might kill the connection if we send a data chunk without data.
--- nbd/server.c | 21 ++++++++++++++++++++- nbd/trace-events | 1 + 2 files changed, 21 insertions(+), 1 deletion(-)
nbd/server.c +20 -1
@@ -1273,6 +1273,21 @@ static inline void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags,stl_be_p (&chunk->length, length); } +static int coroutine_fn nbd_co_send_structured_done(NBDClient *client, +uint64_t handle, +Error **errp) +{ +NBDStructuredReplyChunk chunk; +struct iovec iov[] = { + {.iov_base = &chunk, .iov_len =sizeof (chunk)}, + }; + +trace_nbd_co_send_structured_done (handle); +set_be_chunk (&chunk,NBD_REPLY_FLAG_DONE ,NBD_REPLY_TYPE_NONE , handle,0 ); + +return nbd_co_send_iov (client, iov,1 , errp); +} + static int coroutine_fn nbd_co_send_structured_read(NBDClient *client, uint64_t handle, uint64_t offset,
@@ -1286,6 +1301,7 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client, {.iov_base = data, .iov_len = size} }; +assert (size);trace_nbd_co_send_structured_read (handle, offset, data, size);set_be_chunk (&chunk.h,NBD_REPLY_FLAG_DONE ,NBD_REPLY_TYPE_OFFSET_DATA , handle,sizeof (chunk) -sizeof (chunk.h) + size);
@@ -1544,10 +1560,13 @@ reply:if (ret <0 ) { ret =nbd_co_send_structured_error (req->client, request.handle, -ret, msg, &local_err); - }else { + }else if (reply_data_len) { ret =nbd_co_send_structured_read (req->client, request.handle, request.from, req->data, reply_data_len, &local_err); + }else { + ret =nbd_co_send_structured_done (req->client, request.handle, + &local_err); } }else { ret = nbd_co_send_simple_reply(req->client, request.handle,
nbd/trace-events +1 -0
@@ -55,6 +55,7 @@ nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n" nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n" nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d" +nbd_co_send_structured_done(uint64_t handle) "Send structured reply done: handle = %" PRIu64 nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu" nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
Signature · 1 line
2.13.6