[Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload
Vladimir Sementsov-Ogievskiy
vsementsov at yandex-team.ru
Wed Sep 27 08:55:41 UTC 2023
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, actually we handle a syntactic request with len=0 and return success... I'm afraid, that in the most scenarios that would not be what client want, but client will be confused by success return.
So, for example, if client pass READ with positive length and accidentlly set NBD_CMD_FLAG_PAYLOAD_LEN bit, it will get successful result with wrong length=0.
Or, for WRITE_ZEROES (with accidental NBD_CMD_FLAG_PAYLOAD_LEN) it will just get successful result, when actually nothing is done.
> + }
> 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
More information about the Libguestfs
mailing list