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

Vladimir Sementsov-Ogievskiy
2023-09-30T13:34:47Z

On 25.09.23 22:22, 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.
>
> According to the spec, a client should never send a command with a
> payload without the negotiation phase proving such extension is
> available.  So in the unlikely event the bit is set or cleared
> incorrectly, the client is already at fault; if the client then
> provides the payload, we can gracefully consume it off the wire and
> fail the command with NBD_EINVAL (subsequent checks for magic numbers
> ensure we are still in sync), while if the client fails to send
> payload we block waiting for it (basically deadlocking our connection
> to the bad client, but not negatively impacting our ability to service
> other clients, so not a security risk).  Note that we do not support
> the payload version of BLOCK_STATUS yet.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>
> v7: another try at better logic [Vladimir]
>
> 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     | 37 +++++++++++++++++++++++++++++++++----
>   nbd/trace-events |  1 +
>   2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 7a6f95071f8..1eabcfc908d 100644
> --- a/nbd/server.c
> +++ 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;
>       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;
> +        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;
>           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;

So, for sure, after this we go to

if (requests->flags & ~valid_flags)... and return -EINVAL.

Why we need to set request->len to 0? Just to not return "operation past
EOF" instead of "unsupported flags", if len is too big? Maybe,
that worth a comment.

Anyway, I now see nothing wrong in it, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov at yandex-team.ru>

> +    }
>       if (allocate_buffer) {
>           /* READ, WRITE */
>           req->data = blk_try_blockalign(client->exp->common.blk,
> @@ -2405,10 +2431,13 @@ 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 (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;
>           }
> 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