Re: [Qemu-stable] [PATCH for-2.12] vfio-ccw: fix memory leak in vfio_ccw_realize()

[Cornelia Huck]

On Fri, 06 Apr 2018 17:14:19 +0200
Greg Kurz <address@hidden> wrote:

> The vfio_ccw_realize() function currently leaks vcdev->vdev.name if
> the subchannel is already attached or if vfio_get_device() fails.
> 
> This happens because vcdev->vdev.name is expected to be freed in
> vfio_put_device() which isn't called in this case.
> 
> Adding g_free(vcdev->vdev.name) on these two error paths would be
> enough to fix the leak, but this is unfortunate since we would then
> have three locations doing this. Also, this would be inconsistent
> with the label based rollback scheme of this function.

This is all a bit awkward :(

But I'd actually prefer adding the two g_free calls for now, as that
would be a nice, small patch that my brain can still process at this
point in the evening :)

Your patch would be a nice cleanup for 2.13, though.

Alternatively, I could be convinced to queue this if I get a R-b from
someone :)

> 
> 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 to a
> separate vfio_ccw_get_device() function, which is the counterpart
> of vfio_put_device(). While here, vfio_put_device() is renamed to
> vfio_ccw_put_device() for consistency.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  hw/vfio/ccw.c |   54 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 4e5855741a64..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,20 +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);
> -            goto out_device_err;
> -        }
> -    }
> -
> -    if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, &err)) {
> +    if (vfio_ccw_get_device(group, vcdev, &err)) {
>          goto out_device_err;
>      }
>  
> @@ -380,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:
> @@ -401,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) {
>