Re: [PATCH v6 06/17] nbd/server: Support a request payload

[Vladimir Sementsov-Ogievskiy]

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"

--
Best regards,
Vladimir