Re: [PULL V2 19/25] vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs

[Jason Wang]

在 2022/7/29 22:08, Peter Maydell 写道:
> On Wed, 20 Jul 2022 at 10:04, Jason Wang <jasowang@redhat.com> wrote:
> > From: Eugenio Pérez <eperezma@redhat.com>
> >
> > To know the device features is needed for CVQ SVQ, so SVQ knows if it
> > can handle all commands or not. Extract from
> > vhost_vdpa_get_max_queue_pairs so we can reuse it.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> Hi; this change introduces a resource leak in the new
> error-exit return path in net_init_vhost_vdpa(). Spotted
> by Coverity, CID 1490785.
>
> > @@ -517,10 +521,11 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
> > *name,
> >                           NetClientState *peer, Error **errp)
> >   {
> >       const NetdevVhostVDPAOptions *opts;
> > +    uint64_t features;
> >       int vdpa_device_fd;
> >       g_autofree NetClientState **ncs = NULL;
> >       NetClientState *nc;
> > -    int queue_pairs, i, has_cvq = 0;
> > +    int queue_pairs, r, i, has_cvq = 0;
> >
> >       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >       opts = &netdev->u.vhost_vdpa;
> > @@ -534,7 +539,12 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
> > *name,
> >           return -errno;
> >       }
> >
> > -    queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd,
> > +    r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
> > +    if (unlikely(r < 0)) {
> > +        return r;
> At this point in the code we have allocated the file descriptor
> vdpa_device_fd, but this return path fails to close it.


Exactly.


> > +    }
> > +
> > +    queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, features,
> >                                                    &has_cvq, errp);
> >       if (queue_pairs < 0) {
> >           qemu_close(vdpa_device_fd);
> Compare this pre-existing error-exit path, which correctly
> calls qemu_close() on the fd.
>
> Related question: is this function supposed to return -1 on
> failure, or negative-errno ?


Kind of either:

  if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) 
< 0) {
        /* FIXME drop when all init functions store an Error */
        if (errp && !*errp) {
            error_setg(errp, "Device '%s' could not be initialized",
                       NetClientDriver_str(netdev->type));
        }
        return -1;
    }


>   At the moment it has a mix
> of both. I think that the sole caller only really wants "<0 on
> error", in which case the error-exit code paths could probably
> be tidied up so that instead of explicitly calling
> qemu_close() and returning r, queue_pairs, or whatever
> they got back from the function they just called, they
> could just 'goto err_svq' which will do the "close the fd
> and return -1" work. Better still, by initializing 'i'
> to 0 at the top of the function (and naming it something
> clearer, ideally), all the code paths after the initial
> qemu_open() succeeds could be made to use 'goto err'
> for the error-exit case.


Yes, having a consistent goto based error handling seems much better.

Eugenio, please post patch to fix this.

Thanks


> thanks
> -- PMM