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

[Eric Blake]

On Mon, Sep 04, 2023 at 07:15:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 29.08.23 20:58, 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).  There are no semantic changes, other than a typo fix in a
> > couple of error messages.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > 
> > v6: fix sign extension bug
> > 
> > v5: tweak commit message, adjust a few more spots [Vladimir]
> > 
> > v4: split off enum changes to earlier patches [Vladimir]
> 
> [..]
> 
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1165,7 +1165,7 @@ static int nbd_negotiate_options(NBDClient *client, 
> > Error **errp)
> >           client->optlen = length;
> > 
> >           if (length > NBD_MAX_BUFFER_SIZE) {
> > -            error_setg(errp, "len (%" PRIu32" ) is larger than max len 
> > (%u)",
> > +            error_setg(errp, "len (%" PRIu32 ") is larger than max len 
> > (%u)",
> >                          length, NBD_MAX_BUFFER_SIZE);
> >               return -EINVAL;
> >           }
> > @@ -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    = (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits 
> > */
> 
> should it be "(uint64_t)" ?

No. ldl_be_p() returns an int.  Widening int to 64 bits sign-extends;
we have to first make it unsigned (by casting to uint32_t) so that we
then have an unsigned value that widens by zero-extension.

Maybe we should fix ldl_bd_p() and friends to favor unsigned values.
'git grep ldul_be' has zero hits; even though Peter just touched the
docs patch claiming it exists:

https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00645.html


> 
> > 
> >       trace_nbd_receive_request(magic, request->flags, request->type,
> >                                 request->from, request->len);
> > @@ -1899,7 +1899,7 @@ static int coroutine_fn 
> > nbd_co_send_simple_reply(NBDClient *client,
> >                                                    NBDRequest *request,
> >                                                    uint32_t error,
> >                                                    void *data,
> > -                                                 size_t len,
> > +                                                 uint64_t len,
> >                                                    Error **errp)
> >   {
> >       NBDSimpleReply reply;
> > @@ -1910,6 +1910,7 @@ static int coroutine_fn 
> > nbd_co_send_simple_reply(NBDClient *client,
> >       };
> > 
> >       assert(!len || !nbd_err);
> > +    assert(len <= NBD_MAX_STRING_SIZE);
> 
> NBD_MAX_BUFFER_SIZE ?

No. MAX_STRING_SIZE is 4k, MAX_BUFFER_SIZE is 32M.  The smaller size
is the correct bound here (an error message has to be transmitted as a
single string, and the recipient does not expect more than a 4k error
message).

> 
> with that fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 

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