Re: [Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload

[Eric Blake]

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