| Message ID | 20230829175826.377251-25-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu patches for 64-bit NBD extensions | expand |
On 29.08.23 20:58, Eric Blake wrote: > Upcoming additions to support NBD 64-bit effect lengths allow for the > possibility to distinguish between payload length (capped at 32M) and > effect length (64 bits, although we generally assume 63 bits because > of off_t limitations). Without that extension, only the NBD_CMD_WRITE > request has a payload; but with the extension, it makes sense to allow > at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length > in a future patch (where the payload is a limited-size struct that in > turn gives the real effect length as well as a subset of known ids for > which status is requested). Other future NBD commands may also have a > request payload, so the 64-bit extension introduces a new > NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header > length is a payload length or an effect length, rather than > hard-coding the decision based on the command; although a client > should never send a command with a payload without the negotiation > phase proving such extension is available, we are now able to > gracefully fail unexpected client payloads while keeping the > connection alive. Note that we do not support the payload version of > BLOCK_STATUS yet. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > v5: retitled from v4 13/24, rewrite on top of previous patch's switch > statement [Vladimir] > > v4: less indentation on several 'if's [Vladimir] > --- > nbd/server.c | 33 ++++++++++++++++++++++++++++----- > nbd/trace-events | 1 + > 2 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index dd3ab59224c..adcfcdeacb7 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -2334,7 +2334,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > Error **errp) > { > NBDClient *client = req->client; > - bool check_length = false; > + bool extended_with_payload; > + bool check_length; > bool check_rofs = false; > bool allocate_buffer = false; > unsigned payload_len = 0; > @@ -2350,6 +2351,9 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > trace_nbd_co_receive_request_decode_type(request->cookie, request->type, > nbd_cmd_lookup(request->type)); > + check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED && > + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; > + > switch (request->type) { > case NBD_CMD_DISC: > /* Special case: we're going to disconnect without a reply, > @@ -2366,6 +2370,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > break; > > case NBD_CMD_WRITE: > + if (client->mode >= NBD_MODE_EXTENDED) { > + if (!extended_with_payload) { > + /* The client is noncompliant. Trace it, but proceed. */ > + trace_nbd_co_receive_ext_payload_compliance(request->from, > + request->len); > + } > + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; > + } > payload_len = request->len; > check_length = true; > allocate_buffer = true; > @@ -2407,6 +2419,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, more context: /* Payload and buffer handling. */ if (!payload_len) { req->complete = true; } if (check_length && request->len > NBD_MAX_BUFFER_SIZE) { /* READ, WRITE, CACHE */ error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)", request->len, NBD_MAX_BUFFER_SIZE); return -EINVAL; } > + if (extended_with_payload && !allocate_buffer) { it's correct but strange, as allocate_buffer is (READ or WRITE), and READ is totally unrelated here. > + /* > + * For now, we don't support payloads on other commands; but > + * we can keep the connection alive by ignoring the payload. Was it in specification, that we can safely ignore unknown payload for known commands? > + */ > + assert(request->type != NBD_CMD_WRITE); > + payload_len = request->len; what I don't like here, is that we already set req->complete to true for this request, when payload_len was zero. Probably, for extended_with_payload requests we should initialize payload_len at top of the function? > + request->len = 0; > + } > if (allocate_buffer) {> /* READ, WRITE */ > req->data = blk_try_blockalign(client->exp->common.blk, > @@ -2417,10 +2438,12 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > } > } > if (payload_len) { > - /* WRITE */ > - assert(req->data); > - ret = nbd_read(client->ioc, req->data, payload_len, > - "CMD_WRITE data", errp); > + if (req->data) { and req->data is actually (READ or WRITE) ( == allocated_buffer) as well. I'd prefer here "if (request->type == NBD_CMD_WRITE)" here > + ret = nbd_read(client->ioc, req->data, payload_len, > + "CMD_WRITE data", errp); > + } else { > + ret = nbd_drop(client->ioc, payload_len, errp); > + } > if (ret < 0) { > return -EIO; > } > diff --git a/nbd/trace-events b/nbd/trace-events > index f9dccfcfb44..c1a3227613f 100644 > --- a/nbd/trace-events > +++ b/nbd/trace-events > @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t > nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'" > nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)" > nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64 > +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64 > nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32 > nbd_trip(void) "Reading request" >
On Tue, Sep 05, 2023 at 05:36:15PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 29.08.23 20:58, Eric Blake wrote: > > Upcoming additions to support NBD 64-bit effect lengths allow for the > > possibility to distinguish between payload length (capped at 32M) and > > effect length (64 bits, although we generally assume 63 bits because > > of off_t limitations). Without that extension, only the NBD_CMD_WRITE > > request has a payload; but with the extension, it makes sense to allow > > at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length > > in a future patch (where the payload is a limited-size struct that in > > turn gives the real effect length as well as a subset of known ids for > > which status is requested). Other future NBD commands may also have a > > request payload, so the 64-bit extension introduces a new > > NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header > > length is a payload length or an effect length, rather than > > hard-coding the decision based on the command; although a client > > should never send a command with a payload without the negotiation > > phase proving such extension is available, we are now able to > > gracefully fail unexpected client payloads while keeping the > > connection alive. Note that we do not support the payload version of > > BLOCK_STATUS yet. > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > > > v5: retitled from v4 13/24, rewrite on top of previous patch's switch > > statement [Vladimir] > > > > v4: less indentation on several 'if's [Vladimir] > > --- > > nbd/server.c | 33 ++++++++++++++++++++++++++++----- > > nbd/trace-events | 1 + > > 2 files changed, 29 insertions(+), 5 deletions(-) > > > > diff --git a/nbd/server.c b/nbd/server.c > > index dd3ab59224c..adcfcdeacb7 100644 > > --- a/nbd/server.c > > +++ b/nbd/server.c > > @@ -2334,7 +2334,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > Error **errp) > > { > > NBDClient *client = req->client; > > - bool check_length = false; > > + bool extended_with_payload; > > + bool check_length; > > bool check_rofs = false; > > bool allocate_buffer = false; > > unsigned payload_len = 0; > > @@ -2350,6 +2351,9 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > > > trace_nbd_co_receive_request_decode_type(request->cookie, request->type, > > nbd_cmd_lookup(request->type)); > > + check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED && > > + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; > > + > > switch (request->type) { > > case NBD_CMD_DISC: > > /* Special case: we're going to disconnect without a reply, > > @@ -2366,6 +2370,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > break; > > > > case NBD_CMD_WRITE: > > + if (client->mode >= NBD_MODE_EXTENDED) { > > + if (!extended_with_payload) { > > + /* The client is noncompliant. Trace it, but proceed. */ > > + trace_nbd_co_receive_ext_payload_compliance(request->from, > > + request->len); > > + } > > + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; > > + } > > payload_len = request->len; > > check_length = true; > > allocate_buffer = true; > > @@ -2407,6 +2419,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > more context: > > /* Payload and buffer handling. */ > if (!payload_len) { > req->complete = true; At this point, payload_len is equal to 0 for all but NBD_CMD_WRITE. [1] > } > if (check_length && request->len > NBD_MAX_BUFFER_SIZE) { > /* READ, WRITE, CACHE */ > error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)", > request->len, NBD_MAX_BUFFER_SIZE); > return -EINVAL; > } > > > > + if (extended_with_payload && !allocate_buffer) { > > it's correct but strange, as allocate_buffer is (READ or WRITE), and READ is totally unrelated here. Oh, you do have a point. If a client mistakenly passes the extended_with_payload flag on NBD_CMD_READ, we end up skipping this code which tries to parse off that payload, meaning we could be out of sync for reacting to the next command; if the client is particularly malicious, they could send payload that resembles another valid command. Checking specifically for !WRITE rather than for !allocate_buffer is more accurate, although I was trying to avoid duplicating checks that were covered in the switch statement above. I'll have to think about this a bit. > > > + /* > > + * For now, we don't support payloads on other commands; but > > + * we can keep the connection alive by ignoring the payload. > > Was it in specification, that we can safely ignore unknown payload for known commands? Well, at this point, the client is already non-compliant, so anything we do is best effort only (the client may have set the bit but not sent any payload, at which case we block until the client does send another command but we consume that command as payload; or it sent more payload than the size it advertises and so we see a magic number mismatch when trying to interpret those extra bytes as the next command). But yes, the current spec wording mentions consuming payload bytes and then failing with NBD_EINVAL. Is there a potential that we need to change the spec wording, if we think a malicious client can abuse the flag to force the server into a deadlock scenario which can be abused as a denial-of-service attack against other clients, compared to the more conservative approach of just disconnecting because the client sent a bad flag? Generally, I look at disconnecting in response to client behavior as the least favorable response to take; attempting to return an error message is nicer. And being blocked on I/O is not necessarily a denial of service (it isn't actively consuming CPU cycles, and we don't stop servicing parallel clients). > > > + */ > > + assert(request->type != NBD_CMD_WRITE); > > + payload_len = request->len; > > what I don't like here, is that we already set req->complete to true for this request, when payload_len was zero. > > Probably, for extended_with_payload requests we should initialize payload_len at top of the function? Well, we DO initialize it to 0, and then assign it under NBD_CMD_READ. But the code that sets req->complete should probably NOT be setting it if we are about to attempt reading payload bytes; that is, back at [1], the code there probably needs to be conditional on extended_with_payload. > > > + request->len = 0; > > + } > > if (allocate_buffer) {> /* READ, WRITE */ > > req->data = blk_try_blockalign(client->exp->common.blk, > > @@ -2417,10 +2438,12 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > } > > } > > if (payload_len) { > > - /* WRITE */ > > - assert(req->data); > > - ret = nbd_read(client->ioc, req->data, payload_len, > > - "CMD_WRITE data", errp); > > + if (req->data) { > > and req->data is actually (READ or WRITE) ( == allocated_buffer) as well. > > I'd prefer here "if (request->type == NBD_CMD_WRITE)" here Looks like I'll be respinning this patch slightly. > > > + ret = nbd_read(client->ioc, req->data, payload_len, > > + "CMD_WRITE data", errp); > > + } else { > > + ret = nbd_drop(client->ioc, payload_len, errp); > > + } > > if (ret < 0) { > > return -EIO; > > } > > diff --git a/nbd/trace-events b/nbd/trace-events > > index f9dccfcfb44..c1a3227613f 100644 > > --- a/nbd/trace-events > > +++ b/nbd/trace-events > > @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t > > nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'" > > nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)" > > nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64 > > +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64 > > nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32 > > nbd_trip(void) "Reading request" > > > > -- > Best regards, > Vladimir >
On Wed, Sep 06, 2023 at 12:52:22PM -0500, Eric Blake wrote: > On Tue, Sep 05, 2023 at 05:36:15PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > On 29.08.23 20:58, Eric Blake wrote: > > > Upcoming additions to support NBD 64-bit effect lengths allow for the > > > possibility to distinguish between payload length (capped at 32M) and > > > effect length (64 bits, although we generally assume 63 bits because > > > of off_t limitations). Without that extension, only the NBD_CMD_WRITE > > > request has a payload; but with the extension, it makes sense to allow > > > at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length > > > in a future patch (where the payload is a limited-size struct that in > > > turn gives the real effect length as well as a subset of known ids for > > > which status is requested). Other future NBD commands may also have a > > > request payload, so the 64-bit extension introduces a new > > > NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header > > > length is a payload length or an effect length, rather than > > > hard-coding the decision based on the command; although a client > > > should never send a command with a payload without the negotiation > > > phase proving such extension is available, we are now able to > > > gracefully fail unexpected client payloads while keeping the > > > connection alive. Note that we do not support the payload version of > > > BLOCK_STATUS yet. > > > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > --- > > > > > > v5: retitled from v4 13/24, rewrite on top of previous patch's switch > > > statement [Vladimir] > > > > > > v4: less indentation on several 'if's [Vladimir] > > > --- > > > nbd/server.c | 33 ++++++++++++++++++++++++++++----- > > > nbd/trace-events | 1 + > > > 2 files changed, 29 insertions(+), 5 deletions(-) > > > > > > diff --git a/nbd/server.c b/nbd/server.c > > > index dd3ab59224c..adcfcdeacb7 100644 > > > --- a/nbd/server.c > > > +++ b/nbd/server.c > > > @@ -2334,7 +2334,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > > Error **errp) > > > { > > > NBDClient *client = req->client; > > > - bool check_length = false; > > > + bool extended_with_payload; > > > + bool check_length; > > > bool check_rofs = false; > > > bool allocate_buffer = false; > > > unsigned payload_len = 0; > > > @@ -2350,6 +2351,9 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > > > > > trace_nbd_co_receive_request_decode_type(request->cookie, request->type, > > > nbd_cmd_lookup(request->type)); > > > + check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED && > > > + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; > > > + > > > switch (request->type) { > > > case NBD_CMD_DISC: > > > /* Special case: we're going to disconnect without a reply, > > > @@ -2366,6 +2370,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > > break; > > > > > > case NBD_CMD_WRITE: > > > + if (client->mode >= NBD_MODE_EXTENDED) { > > > + if (!extended_with_payload) { > > > + /* The client is noncompliant. Trace it, but proceed. */ > > > + trace_nbd_co_receive_ext_payload_compliance(request->from, > > > + request->len); > > > + } > > > + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; > > > + } > > > payload_len = request->len; > > > check_length = true; > > > allocate_buffer = true; > > > @@ -2407,6 +2419,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > > > more context: > > > > /* Payload and buffer handling. */ > > if (!payload_len) { > > req->complete = true; > > At this point, payload_len is equal to 0 for all but NBD_CMD_WRITE. [1] > > > } > > if (check_length && request->len > NBD_MAX_BUFFER_SIZE) { > > /* READ, WRITE, CACHE */ > > error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)", > > request->len, NBD_MAX_BUFFER_SIZE); > > return -EINVAL; > > } > > > > > > > + if (extended_with_payload && !allocate_buffer) { > > > > it's correct but strange, as allocate_buffer is (READ or WRITE), and READ is totally unrelated here. > > Oh, you do have a point. If a client mistakenly passes the > extended_with_payload flag on NBD_CMD_READ, we end up skipping this > code which tries to parse off that payload, meaning we could be out of > sync for reacting to the next command; if the client is particularly > malicious, they could send payload that resembles another valid > command. Checking specifically for !WRITE rather than for > !allocate_buffer is more accurate, although I was trying to avoid > duplicating checks that were covered in the switch statement above. > > I'll have to think about this a bit. > > > > > > + /* > > > + * For now, we don't support payloads on other commands; but > > > + * we can keep the connection alive by ignoring the payload. > > > > Was it in specification, that we can safely ignore unknown payload for known commands? > > Well, at this point, the client is already non-compliant, so anything > we do is best effort only (the client may have set the bit but not > sent any payload, at which case we block until the client does send > another command but we consume that command as payload; or it sent > more payload than the size it advertises and so we see a magic number > mismatch when trying to interpret those extra bytes as the next > command). > > But yes, the current spec wording mentions consuming payload bytes and > then failing with NBD_EINVAL. Is there a potential that we need to > change the spec wording, if we think a malicious client can abuse the > flag to force the server into a deadlock scenario which can be abused > as a denial-of-service attack against other clients, compared to the > more conservative approach of just disconnecting because the client > sent a bad flag? Generally, I look at disconnecting in response to > client behavior as the least favorable response to take; attempting to > return an error message is nicer. And being blocked on I/O is not > necessarily a denial of service (it isn't actively consuming CPU > cycles, and we don't stop servicing parallel clients). > > > > > > + */ > > > + assert(request->type != NBD_CMD_WRITE); > > > + payload_len = request->len; > > > > what I don't like here, is that we already set req->complete to true for this request, when payload_len was zero. > > > > Probably, for extended_with_payload requests we should initialize payload_len at top of the function? > > Well, we DO initialize it to 0, and then assign it under NBD_CMD_READ. > But the code that sets req->complete should probably NOT be setting it > if we are about to attempt reading payload bytes; that is, back at > [1], the code there probably needs to be conditional on > extended_with_payload. > > > > > > + request->len = 0; > > > + } > > > if (allocate_buffer) {> /* READ, WRITE */ > > > req->data = blk_try_blockalign(client->exp->common.blk, > > > @@ -2417,10 +2438,12 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, > > > } > > > } > > > if (payload_len) { > > > - /* WRITE */ > > > - assert(req->data); > > > - ret = nbd_read(client->ioc, req->data, payload_len, > > > - "CMD_WRITE data", errp); > > > + if (req->data) { > > > > and req->data is actually (READ or WRITE) ( == allocated_buffer) as well. > > > > I'd prefer here "if (request->type == NBD_CMD_WRITE)" here > > Looks like I'll be respinning this patch slightly. Here's what I'm planning to squash in. Your idea of setting payload_len up front makes sense. I will also have to squash in some followups to 17/17, when I start accepting payload for NBD_CMD_BLOCK_STATUS. diff --git i/nbd/server.c w/nbd/server.c index d54dd0604bf..35d3f8989f2 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -2317,36 +2317,41 @@ static int coroutine_fn nbd_co_send_bitmap(NBDClient *client, * to the client (although the caller may still need to disconnect after * reporting the error). */ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, Error **errp) { NBDClient *client = req->client; bool extended_with_payload; - bool check_length; + bool check_length = false; bool check_rofs = false; bool allocate_buffer = false; + bool payload_okay = false; unsigned payload_len = 0; int valid_flags = NBD_CMD_FLAG_FUA; int ret; g_assert(qemu_in_coroutine()); assert(client->recv_coroutine == qemu_coroutine_self()); ret = nbd_receive_request(client, request, errp); if (ret < 0) { return ret; } trace_nbd_co_receive_request_decode_type(request->cookie, request->type, nbd_cmd_lookup(request->type)); - check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED && + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; + if (extended_with_payload) { + payload_len = request->len; + check_length = true; + } switch (request->type) { case NBD_CMD_DISC: /* Special case: we're going to disconnect without a reply, * whether or not flags, from, or len are bogus */ req->complete = true; return -EIO; case NBD_CMD_READ: @@ -2360,18 +2365,19 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, case NBD_CMD_WRITE: if (client->mode >= NBD_MODE_EXTENDED) { if (!extended_with_payload) { /* The client is noncompliant. Trace it, but proceed. */ trace_nbd_co_receive_ext_payload_compliance(request->from, request->len); } valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; } + payload_okay = true; payload_len = request->len; check_length = true; allocate_buffer = true; check_rofs = true; break; case NBD_CMD_FLUSH: break; @@ -2401,38 +2407,38 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, if (!payload_len) { req->complete = true; } if (check_length && request->len > NBD_MAX_BUFFER_SIZE) { /* READ, WRITE, CACHE */ error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)", request->len, NBD_MAX_BUFFER_SIZE); return -EINVAL; } - if (extended_with_payload && !allocate_buffer) { + if (payload_len && !payload_okay) { /* * For now, we don't support payloads on other commands; but * we can keep the connection alive by ignoring the payload. */ assert(request->type != NBD_CMD_WRITE); - payload_len = request->len; request->len = 0; } if (allocate_buffer) { /* READ, WRITE */ req->data = blk_try_blockalign(client->exp->common.blk, request->len); if (req->data == NULL) { error_setg(errp, "No memory"); return -ENOMEM; } } if (payload_len) { - if (req->data) { + if (payload_okay) { + assert(req->data); ret = nbd_read(client->ioc, req->data, payload_len, "CMD_WRITE data", errp); } else { ret = nbd_drop(client->ioc, payload_len, errp); } if (ret < 0) { return -EIO; } req->complete = true;
diff --git a/nbd/server.c b/nbd/server.c index dd3ab59224c..adcfcdeacb7 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2334,7 +2334,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, Error **errp) { NBDClient *client = req->client; - bool check_length = false; + bool extended_with_payload; + bool check_length; bool check_rofs = false; bool allocate_buffer = false; unsigned payload_len = 0; @@ -2350,6 +2351,9 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, trace_nbd_co_receive_request_decode_type(request->cookie, request->type, nbd_cmd_lookup(request->type)); + check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED && + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; + switch (request->type) { case NBD_CMD_DISC: /* Special case: we're going to disconnect without a reply, @@ -2366,6 +2370,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, break; case NBD_CMD_WRITE: + if (client->mode >= NBD_MODE_EXTENDED) { + if (!extended_with_payload) { + /* The client is noncompliant. Trace it, but proceed. */ + trace_nbd_co_receive_ext_payload_compliance(request->from, + request->len); + } + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } payload_len = request->len; check_length = true; allocate_buffer = true; @@ -2407,6 +2419,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, request->len, NBD_MAX_BUFFER_SIZE); return -EINVAL; } + if (extended_with_payload && !allocate_buffer) { + /* + * For now, we don't support payloads on other commands; but + * we can keep the connection alive by ignoring the payload. + */ + assert(request->type != NBD_CMD_WRITE); + payload_len = request->len; + request->len = 0; + } if (allocate_buffer) { /* READ, WRITE */ req->data = blk_try_blockalign(client->exp->common.blk, @@ -2417,10 +2438,12 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, } } if (payload_len) { - /* WRITE */ - assert(req->data); - ret = nbd_read(client->ioc, req->data, payload_len, - "CMD_WRITE data", errp); + if (req->data) { + ret = nbd_read(client->ioc, req->data, payload_len, + "CMD_WRITE data", errp); + } else { + ret = nbd_drop(client->ioc, payload_len, errp); + } if (ret < 0) { return -EIO; } diff --git a/nbd/trace-events b/nbd/trace-events index f9dccfcfb44..c1a3227613f 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'" nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)" nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64 +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64 nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32 nbd_trip(void) "Reading request"
Upcoming additions to support NBD 64-bit effect lengths allow for the possibility to distinguish between payload length (capped at 32M) and effect length (64 bits, although we generally assume 63 bits because of off_t limitations). Without that extension, only the NBD_CMD_WRITE request has a payload; but with the extension, it makes sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length in a future patch (where the payload is a limited-size struct that in turn gives the real effect length as well as a subset of known ids for which status is requested). Other future NBD commands may also have a request payload, so the 64-bit extension introduces a new NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header length is a payload length or an effect length, rather than hard-coding the decision based on the command; although a client should never send a command with a payload without the negotiation phase proving such extension is available, we are now able to gracefully fail unexpected client payloads while keeping the connection alive. Note that we do not support the payload version of BLOCK_STATUS yet. Signed-off-by: Eric Blake <eblake@redhat.com> --- v5: retitled from v4 13/24, rewrite on top of previous patch's switch statement [Vladimir] v4: less indentation on several 'if's [Vladimir] --- nbd/server.c | 33 ++++++++++++++++++++++++++++----- nbd/trace-events | 1 + 2 files changed, 29 insertions(+), 5 deletions(-)