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;