[Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload
Vladimir Sementsov-Ogievskiy
vsementsov at yandex-team.ru
Sat Sep 30 13:34:47 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, 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
More information about the Libguestfs
mailing list