On Fri, 06 Apr 2018 17:14:19 +0200
Greg Kurz <gr...@kaod.org> 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 <gr...@kaod.org>
> ---
> 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) {
>
