vdpa: Fix file descriptor leak on get features error

[PATCH] vdpa: Fix file descriptor leak on get features error

Posted by Eugenio Pérez 1 year, 8 months ago

File descriptor vdpa_device_fd is not free in the case of returning
error from vhost_vdpa_get_features. Fixing it by making all errors go to
the same error path.

Resolves: Coverity CID 1490785
Fixes: 8170ab3f43 ("vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs")

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 6abad276a6..303447a68e 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     g_autofree NetClientState **ncs = NULL;
     g_autoptr(VhostIOVATree) iova_tree = NULL;
     NetClientState *nc;
-    int queue_pairs, r, i, has_cvq = 0;
+    int queue_pairs, r, i = 0, has_cvq = 0;
 
     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
     opts = &netdev->u.vhost_vdpa;
@@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
 
     r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
     if (unlikely(r < 0)) {
-        return r;
+        goto err;
     }
 
     queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, features,
-- 
2.31.1

Re: [PATCH] vdpa: Fix file descriptor leak on get features error

Posted by Laurent Vivier 1 year, 8 months ago

On 01/08/2022 16:47, Eugenio Pérez wrote:
> File descriptor vdpa_device_fd is not free in the case of returning
> error from vhost_vdpa_get_features. Fixing it by making all errors go to
> the same error path.
> 
> Resolves: Coverity CID 1490785
> Fixes: 8170ab3f43 ("vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs")
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   net/vhost-vdpa.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 6abad276a6..303447a68e 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>       g_autofree NetClientState **ncs = NULL;
>       g_autoptr(VhostIOVATree) iova_tree = NULL;
>       NetClientState *nc;
> -    int queue_pairs, r, i, has_cvq = 0;
> +    int queue_pairs, r, i = 0, has_cvq = 0;
>   
>       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>       opts = &netdev->u.vhost_vdpa;
> @@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>   
>       r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
>       if (unlikely(r < 0)) {
> -        return r;
> +        goto err;

Why don't you use "goto err_svq"?

Thanks,
Laurent

Re: [PATCH] vdpa: Fix file descriptor leak on get features error

Posted by Peter Maydell 1 year, 8 months ago

On Mon, 1 Aug 2022 at 19:31, Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 01/08/2022 16:47, Eugenio Pérez wrote:
> > File descriptor vdpa_device_fd is not free in the case of returning
> > error from vhost_vdpa_get_features. Fixing it by making all errors go to
> > the same error path.
> >
> > Resolves: Coverity CID 1490785
> > Fixes: 8170ab3f43 ("vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs")
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   net/vhost-vdpa.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 6abad276a6..303447a68e 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> >       g_autofree NetClientState **ncs = NULL;
> >       g_autoptr(VhostIOVATree) iova_tree = NULL;
> >       NetClientState *nc;
> > -    int queue_pairs, r, i, has_cvq = 0;
> > +    int queue_pairs, r, i = 0, has_cvq = 0;
> >
> >       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >       opts = &netdev->u.vhost_vdpa;
> > @@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> >
> >       r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
> >       if (unlikely(r < 0)) {
> > -        return r;
> > +        goto err;
>
> Why don't you use "goto err_svq"?

This patch is kind of half of the idea discussed in the thread where
this problem was reported, which is that by setting i = 0 you can
then consistently have all the error handling be 'goto err' and that
frees everything that needs to be freed regardless of whether it's
called after or before the initialization of the ncs[] entries. But it
doesn't do the other half of the job, which is making all the other
error handling code in the function also use 'goto err', so it looks
a bit odd as it stands.

-- PMM

Re: [PATCH] vdpa: Fix file descriptor leak on get features error

Posted by Eugenio Perez Martin 1 year, 8 months ago

On Mon, Aug 1, 2022 at 8:34 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 1 Aug 2022 at 19:31, Laurent Vivier <lvivier@redhat.com> wrote:
> >
> > On 01/08/2022 16:47, Eugenio Pérez wrote:
> > > File descriptor vdpa_device_fd is not free in the case of returning
> > > error from vhost_vdpa_get_features. Fixing it by making all errors go to
> > > the same error path.
> > >
> > > Resolves: Coverity CID 1490785
> > > Fixes: 8170ab3f43 ("vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs")
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   net/vhost-vdpa.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 6abad276a6..303447a68e 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > >       g_autofree NetClientState **ncs = NULL;
> > >       g_autoptr(VhostIOVATree) iova_tree = NULL;
> > >       NetClientState *nc;
> > > -    int queue_pairs, r, i, has_cvq = 0;
> > > +    int queue_pairs, r, i = 0, has_cvq = 0;
> > >
> > >       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > >       opts = &netdev->u.vhost_vdpa;
> > > @@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > >
> > >       r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
> > >       if (unlikely(r < 0)) {
> > > -        return r;
> > > +        goto err;
> >
> > Why don't you use "goto err_svq"?
>
> This patch is kind of half of the idea discussed in the thread where
> this problem was reported, which is that by setting i = 0 you can
> then consistently have all the error handling be 'goto err' and that
> frees everything that needs to be freed regardless of whether it's
> called after or before the initialization of the ncs[] entries. But it
> doesn't do the other half of the job, which is making all the other
> error handling code in the function also use 'goto err', so it looks
> a bit odd as it stands.
>

That's right, I thought just fixing the issue about the leaked file
descriptor was the right thing to do in the hard feature freeze, and
that other part should be left for the next dev phase. Is the unified
error handling code acceptable for this period? I can send a patch
either on top of this one or squashed if so for sure.

Thanks!

Re: [PATCH] vdpa: Fix file descriptor leak on get features error

Posted by Michael S. Tsirkin 1 year, 8 months ago

On Mon, Aug 01, 2022 at 08:48:41PM +0200, Eugenio Perez Martin wrote:
> On Mon, Aug 1, 2022 at 8:34 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 1 Aug 2022 at 19:31, Laurent Vivier <lvivier@redhat.com> wrote:
> > >
> > > On 01/08/2022 16:47, Eugenio Pérez wrote:
> > > > File descriptor vdpa_device_fd is not free in the case of returning
> > > > error from vhost_vdpa_get_features. Fixing it by making all errors go to
> > > > the same error path.
> > > >
> > > > Resolves: Coverity CID 1490785
> > > > Fixes: 8170ab3f43 ("vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs")
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >   net/vhost-vdpa.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 6abad276a6..303447a68e 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > > >       g_autofree NetClientState **ncs = NULL;
> > > >       g_autoptr(VhostIOVATree) iova_tree = NULL;
> > > >       NetClientState *nc;
> > > > -    int queue_pairs, r, i, has_cvq = 0;
> > > > +    int queue_pairs, r, i = 0, has_cvq = 0;
> > > >
> > > >       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > > >       opts = &netdev->u.vhost_vdpa;
> > > > @@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > > >
> > > >       r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
> > > >       if (unlikely(r < 0)) {
> > > > -        return r;
> > > > +        goto err;
> > >
> > > Why don't you use "goto err_svq"?
> >
> > This patch is kind of half of the idea discussed in the thread where
> > this problem was reported, which is that by setting i = 0 you can
> > then consistently have all the error handling be 'goto err' and that
> > frees everything that needs to be freed regardless of whether it's
> > called after or before the initialization of the ncs[] entries. But it
> > doesn't do the other half of the job, which is making all the other
> > error handling code in the function also use 'goto err', so it looks
> > a bit odd as it stands.
> >
> 
> That's right, I thought just fixing the issue about the leaked file
> descriptor was the right thing to do in the hard feature freeze, and
> that other part should be left for the next dev phase. Is the unified
> error handling code acceptable for this period? I can send a patch
> either on top of this one or squashed if so for sure.
> 
> Thanks!

Yea I'd prefer a minimal patch, unless others object strongly.


-- 
MST