Re: [Libguestfs] [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

[Eric Blake]

On Thu, Oct 05, 2023 at 05:33:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > +static int
> > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> > +                                 Error **errp)
> > +{
> > +    int payload_len = request->len;
> 
> payload_len should be uint64_t
> 
> > +    g_autofree char *buf = NULL;
> > +    size_t count, i, nr_bitmaps;
> > +    uint32_t id;
> > +
> 
> otherwise, we may do something unexpected here, when reqeuest->len is too big 
> for int:
> 
> > +    if (payload_len > NBD_MAX_BUFFER_SIZE) {
> > +        error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
> > +                   request->len, NBD_MAX_BUFFER_SIZE);
> > +        return -EINVAL;
> > +    }

Oh, it looks like I introduced that same type mismatch in commit
8db7e2d6 as well, although it appears to have a latent effect until
this series enables the ability for request->length to actually exceed
32 bits.  I'll reply on 1/12 with another squash I'm making there.

> > +
> > +    assert(client->contexts.exp == client->exp);
> > +    nr_bitmaps = client->exp->nr_export_bitmaps;
> > +    request->contexts = g_new0(NBDMetaContexts, 1);
> > +    request->contexts->exp = client->exp;
> > +
> > +    if (payload_len % sizeof(uint32_t) ||
> > +        payload_len < sizeof(NBDBlockStatusPayload) ||
> > +        payload_len > (sizeof(NBDBlockStatusPayload) +
> > +                       sizeof(id) * client->contexts.count)) {
> > +        goto skip;
> > +    }
> 
> [..]
> 
> >    * connection right away, -EAGAIN to indicate we were interrupted and the
> > @@ -2505,7 +2593,18 @@ static int coroutine_fn 
> > nbd_co_receive_request(NBDRequestData *req,
> >           break;
> > 
> >       case NBD_CMD_BLOCK_STATUS:
> > -        request->contexts = &client->contexts;
> > +        if (extended_with_payload) {
> > +            ret = nbd_co_block_status_payload_read(client, request, errp);
> > +            if (ret < 0) {
> > +                return ret;
> > +            }
> > +            /* payload now consumed */
> > +            check_length = extended_with_payload = false;
> 
> why set extended_with_payload to false? it's a bit misleading. And you don't 
> do this for WRITE request.

Indeed; it doesn't make any different to later in the function.  Will drop.

> 
> > +            payload_len = 0;
> > +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> > +        } else {
> > +            request->contexts = &client->contexts;
> > +        }
> >           valid_flags |= NBD_CMD_FLAG_REQ_ONE;
> >           break;
> > 
> 
> [..]
> 
> 
> with payload_len changed to uint64_t, your squash-in applied and s/>/>=/ 
> fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks for the careful review.

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