| Message ID | 20220720090313.55169-20-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,V2,01/25] vhost: move descriptor translation to vhost_svq_vring_write_descs | expand |
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. > + } > + > + 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 ? 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. thanks -- PMM
在 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 >
On Mon, Aug 1, 2022 at 5:29 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 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. > Right, I'll fix. > > > > >> + } > >> + > >> + 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; > } > > We can write errno to errp then, and consistently use the goto for error handling as you propose. I'll post a fix in a moment. Thanks! > > 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 > > >
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 502f6f9..6e3e9f3 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -474,20 +474,24 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, return nc; } -static int vhost_vdpa_get_max_queue_pairs(int fd, int *has_cvq, Error **errp) +static int vhost_vdpa_get_features(int fd, uint64_t *features, Error **errp) +{ + int ret = ioctl(fd, VHOST_GET_FEATURES, features); + if (unlikely(ret < 0)) { + error_setg_errno(errp, errno, + "Fail to query features from vhost-vDPA device"); + } + return ret; +} + +static int vhost_vdpa_get_max_queue_pairs(int fd, uint64_t features, + int *has_cvq, Error **errp) { unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); g_autofree struct vhost_vdpa_config *config = NULL; __virtio16 *max_queue_pairs; - uint64_t features; int ret; - ret = ioctl(fd, VHOST_GET_FEATURES, &features); - if (ret) { - error_setg(errp, "Fail to query features from vhost-vDPA device"); - return ret; - } - if (features & (1 << VIRTIO_NET_F_CTRL_VQ)) { *has_cvq = 1; } else { @@ -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; + } + + queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, features, &has_cvq, errp); if (queue_pairs < 0) { qemu_close(vdpa_device_fd);