[Qemu-devel] [PATCH v2 0/2] vfio-ccw: fix memory leak + cleanup

[Qemu-devel] [PATCH v2 0/2] vfio-ccw: fix memory leak + cleanup

[Greg Kurz]

This series fixes two leaks in vfio_ccw_realize(). The v2 is basically
the same as the initial series, split in two patches: a trivial bugfix
suitable for 2.12 and stable, and a follow-up with more cleanup for
2.13.

--
Greg

---

Greg Kurz (2):
      vfio-ccw: fix memory leaks in vfio_ccw_realize()
      vfio-ccw: introduce vfio_ccw_get_device()


 hw/vfio/ccw.c |   54 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 18 deletions(-)

[Qemu-devel] [PATCH v2 1/2] vfio-ccw: fix memory leaks in vfio_ccw_realize()

[Greg Kurz]

If the subchannel is already attached or if vfio_get_device() fails, the
code jumps to the 'out_device_err' label and doesn't free the string it
has just allocated.

The code should be reworked so that vcdev->vdev.name only gets set when
the device has been attached, and freed when it is about to be detached.
This could be achieved  with the addition of a vfio_ccw_get_device()
function that would be the counterpart of vfio_put_device(). But this is
a more elaborate cleanup that should be done in a follow-up. For now,
let's just add calls to g_free() on the buggy error paths.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/vfio/ccw.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 4e5855741a64..fe34b507699f 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -357,11 +357,13 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         if (strcmp(vbasedev->name, vcdev->vdev.name) == 0) {
             error_setg(&err, "vfio: subchannel %s has already been attached",
                        vcdev->vdev.name);
+            g_free(vcdev->vdev.name);
             goto out_device_err;
         }
     }
 
     if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, &err)) {
+        g_free(vcdev->vdev.name);
         goto out_device_err;
     }
 

[Qemu-devel] [PATCH v2 2/2] vfio-ccw: introduce vfio_ccw_get_device()

[Greg Kurz]

A recent patch fixed leaks of the dynamically allocated vcdev->vdev.name
field in vfio_ccw_realize(), but we now have three freeing sites for it.
This is unfortunate and seems to indicate something is wrong with its
life cycle.

The root issue is that vcdev->vdev.name is set before vfio_get_device()
is called, which theoretically prevents to call vfio_put_device() to
do the freeing. Well actually, we could call it anyway  because
vfio_put_base_device() is a nop if the device isn't attached, but this
would be confusing.

This patch hence moves all the logic of attaching the device, including
the "already attached" check, to a separate vfio_ccw_get_device() function,
counterpart of vfio_put_device(). While here, vfio_put_device() is renamed
to vfio_ccw_put_device() for consistency.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/vfio/ccw.c |   56 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index fe34b507699f..49ae986d288d 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -292,12 +292,44 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
     g_free(vcdev->io_region);
 }
 
-static void vfio_put_device(VFIOCCWDevice *vcdev)
+static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
 {
     g_free(vcdev->vdev.name);
     vfio_put_base_device(&vcdev->vdev);
 }
 
+static int vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
+                               Error **errp)
+{
+    char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
+                                 vcdev->cdev.hostid.ssid,
+                                 vcdev->cdev.hostid.devid);
+    VFIODevice *vbasedev;
+
+    QLIST_FOREACH(vbasedev, &group->device_list, next) {
+        if (strcmp(vbasedev->name, name) == 0) {
+            error_setg(errp, "vfio: subchannel %s has already been attached",
+                       name);
+            goto out_err;
+        }
+    }
+
+    if (vfio_get_device(group, vcdev->cdev.mdevid, &vcdev->vdev, errp)) {
+        goto out_err;
+    }
+
+    vcdev->vdev.ops = &vfio_ccw_ops;
+    vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
+    vcdev->vdev.name = name;
+    vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj;
+
+    return 0;
+
+out_err:
+    g_free(name);
+    return -1;
+}
+
 static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
 {
     char *tmp, group_path[PATH_MAX];
@@ -327,7 +359,6 @@ static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
 
 static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 {
-    VFIODevice *vbasedev;
     VFIOGroup *group;
     CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev);
     S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev);
@@ -348,22 +379,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         goto out_group_err;
     }
 
-    vcdev->vdev.ops = &vfio_ccw_ops;
-    vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
-    vcdev->vdev.name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
-                                       cdev->hostid.ssid, cdev->hostid.devid);
-    vcdev->vdev.dev = dev;
-    QLIST_FOREACH(vbasedev, &group->device_list, next) {
-        if (strcmp(vbasedev->name, vcdev->vdev.name) == 0) {
-            error_setg(&err, "vfio: subchannel %s has already been attached",
-                       vcdev->vdev.name);
-            g_free(vcdev->vdev.name);
-            goto out_device_err;
-        }
-    }
-
-    if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, &err)) {
-        g_free(vcdev->vdev.name);
+    if (vfio_ccw_get_device(group, vcdev, &err)) {
         goto out_device_err;
     }
 
@@ -382,7 +398,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 out_notifier_err:
     vfio_ccw_put_region(vcdev);
 out_region_err:
-    vfio_put_device(vcdev);
+    vfio_ccw_put_device(vcdev);
 out_device_err:
     vfio_put_group(group);
 out_group_err:
@@ -403,7 +419,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
 
     vfio_ccw_unregister_io_notifier(vcdev);
     vfio_ccw_put_region(vcdev);
-    vfio_put_device(vcdev);
+    vfio_ccw_put_device(vcdev);
     vfio_put_group(group);
 
     if (cdc->unrealize) {

Re: [Qemu-devel] [PATCH v2 1/2] vfio-ccw: fix memory leaks in vfio_ccw_realize()

[Cornelia Huck]

On Sat, 07 Apr 2018 16:43:46 +0200
Greg Kurz <groug@kaod.org> wrote:

> If the subchannel is already attached or if vfio_get_device() fails, the
> code jumps to the 'out_device_err' label and doesn't free the string it
> has just allocated.
> 
> The code should be reworked so that vcdev->vdev.name only gets set when
> the device has been attached, and freed when it is about to be detached.
> This could be achieved  with the addition of a vfio_ccw_get_device()
> function that would be the counterpart of vfio_put_device(). But this is
> a more elaborate cleanup that should be done in a follow-up. For now,
> let's just add calls to g_free() on the buggy error paths.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/vfio/ccw.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 4e5855741a64..fe34b507699f 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -357,11 +357,13 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>          if (strcmp(vbasedev->name, vcdev->vdev.name) == 0) {
>              error_setg(&err, "vfio: subchannel %s has already been attached",
>                         vcdev->vdev.name);
> +            g_free(vcdev->vdev.name);
>              goto out_device_err;
>          }
>      }
>  
>      if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, &err)) {
> +        g_free(vcdev->vdev.name);
>          goto out_device_err;
>      }
>  
> 

Thanks, applied to s390-fixes.

Re: [Qemu-devel] [PATCH v2 2/2] vfio-ccw: introduce vfio_ccw_get_device()

[Cornelia Huck]

On Sat, 07 Apr 2018 16:44:01 +0200
Greg Kurz <groug@kaod.org> wrote:

> A recent patch fixed leaks of the dynamically allocated vcdev->vdev.name
> field in vfio_ccw_realize(), but we now have three freeing sites for it.
> This is unfortunate and seems to indicate something is wrong with its
> life cycle.
> 
> The root issue is that vcdev->vdev.name is set before vfio_get_device()
> is called, which theoretically prevents to call vfio_put_device() to
> do the freeing. Well actually, we could call it anyway  because
> vfio_put_base_device() is a nop if the device isn't attached, but this
> would be confusing.
> 
> This patch hence moves all the logic of attaching the device, including
> the "already attached" check, to a separate vfio_ccw_get_device() function,
> counterpart of vfio_put_device(). While here, vfio_put_device() is renamed
> to vfio_ccw_put_device() for consistency.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/vfio/ccw.c |   56 ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index fe34b507699f..49ae986d288d 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -292,12 +292,44 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
>      g_free(vcdev->io_region);
>  }
>  
> -static void vfio_put_device(VFIOCCWDevice *vcdev)
> +static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
>  {
>      g_free(vcdev->vdev.name);
>      vfio_put_base_device(&vcdev->vdev);
>  }
>  
> +static int vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
> +                               Error **errp)

Make this one void, as you already pass an error object?

> +{
> +    char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
> +                                 vcdev->cdev.hostid.ssid,
> +                                 vcdev->cdev.hostid.devid);
> +    VFIODevice *vbasedev;
> +
> +    QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +        if (strcmp(vbasedev->name, name) == 0) {
> +            error_setg(errp, "vfio: subchannel %s has already been attached",
> +                       name);
> +            goto out_err;
> +        }
> +    }
> +
> +    if (vfio_get_device(group, vcdev->cdev.mdevid, &vcdev->vdev, errp)) {
> +        goto out_err;
> +    }
> +
> +    vcdev->vdev.ops = &vfio_ccw_ops;
> +    vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
> +    vcdev->vdev.name = name;
> +    vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj;
> +
> +    return 0;
> +
> +out_err:
> +    g_free(name);
> +    return -1;

That -1 return code looks artificial, so I really think this should be
void :) The only caller already defines a local error object anyway, so
it can just check for err.

> +}
> +
>  static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
>  {
>      char *tmp, group_path[PATH_MAX];