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

2018-04-06 Thread Greg Kurz
On Fri, 6 Apr 2018 18:54:13 +0200
Cornelia Huck  wrote:

> On Fri, 06 Apr 2018 17:14:19 +0200
> Greg Kurz  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 :)
> 

I guess I should rather split the patch in two: a bug fix with the two
g_free calls for 2.12 and stable, and a cleanup patch for 2.13.

> > 
> > 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 
> > ---
> >  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_realiz

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

2018-04-06 Thread Cornelia Huck
On Fri, 06 Apr 2018 17:14:19 +0200
Greg Kurz  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 
> ---
>  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);
> +v