Re: [PATCH v5 04/17] nbd: Prepare for 64-bit request effect lengths

[Eric Blake]

On Thu, Aug 10, 2023 at 12:36:51PM -0500, Eric Blake wrote:
> Widen the length field of NBDRequest to 64-bits, although we can
> assert that all current uses are still under 32 bits: either because
> of NBD_MAX_BUFFER_SIZE which is even smaller (and where size_t can
> still be appropriate, even on 32-bit platforms), or because nothing
> ever puts us into NBD_MODE_EXTENDED yet (and while future patches will
> allow larger transactions, the lengths in play here are still capped
> at 32-bit).  Thus no semantic change.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> v5: tweak commit message, adjust a few more spots [Vladimir].
> 
> v4: split off enum changes to earlier patches [Vladimir]
> ---

> +++ b/include/block/nbd.h
> @@ -71,8 +71,8 @@ typedef enum NBDMode {
>   */
>  typedef struct NBDRequest {
>      uint64_t cookie;
> -    uint64_t from;
> -    uint32_t len;
> +    uint64_t from;  /* Offset touched by the command */
> +    uint64_t len;   /* Effect length; 32 bit limit without extended headers 
> */

Despite using unsigned types here...

> +++ b/nbd/server.c
> @@ -1441,7 +1441,7 @@ static int coroutine_fn nbd_receive_request(NBDClient 
> *client, NBDRequest *reque
>      request->type   = lduw_be_p(buf + 6);
>      request->cookie = ldq_be_p(buf + 8);
>      request->from   = ldq_be_p(buf + 16);
> -    request->len    = ldl_be_p(buf + 24);
> +    request->len    = ldl_be_p(buf + 24); /* widen 32 to 64 bits */

...this code has a nasty bug in that ldl_be_p() returns int instead of
an unsigned type, so it sign extends, breaking any client that
requests a value larger than 2G but still less than 4G.  As there are
still a few patches needing review, I'll go ahead and post a v6 with
the obvious fix folded in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org