On Mon, Sep 25, 2023 at 02:22:31PM -0500, 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). [...]
+++ b/nbd/server.c @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, Error **errp) { NBDClient *client = req->client; + bool extended_with_payload; bool check_length = false; bool check_rofs = false; bool allocate_buffer = false; + bool payload_okay = false; unsigned payload_len = 0;
Pre-existing type mismatch caught as a result of Vladimir's review of 12/12, but:
int valid_flags = NBD_CMD_FLAG_FUA; int ret; @@ -2338,6 +2340,13 @@ 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)); + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; + if (extended_with_payload) { + payload_len = request->len;
this can assign a 64-bit number into a 32-bit variable, which can truncate to 0,...
+ check_length = true; + } + switch (request->type) { case NBD_CMD_DISC: /* Special case: we're going to disconnect without a reply, @@ -2354,6 +2363,15 @@ 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_okay = true; payload_len = request->len;
...the pre-existing code is safe only as long as request->len cannot exceed 32 bytes (which it can't do until later in this series actually enables extended requests). Switching the type now is prudent...
check_length = true; allocate_buffer = true; @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, request->len, NBD_MAX_BUFFER_SIZE); return -EINVAL; } + 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); + request->len = 0;
...otherwise, this check is bypassed for a request size of exactly 4G if check_length is false and thus the previous conditional for request->len vs. NBD_MAX_BUFFER_SIZE didn't trigger (prior to this patch, payload_len was only set for CND_WRITE which also set check_length). Thus, I'm squashing in: diff --git i/nbd/server.c w/nbd/server.c index 5258064e5ac..1cb66e86a89 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -2327,7 +2327,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, bool check_rofs = false; bool allocate_buffer = false; bool payload_okay = false; - unsigned payload_len = 0; + uint64_t payload_len = 0; int valid_flags = NBD_CMD_FLAG_FUA; int ret; -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org