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.
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) {
