On Fri, 6 Apr 2018 18:54:13 +0200
Cornelia Huck <coh...@redhat.com> wrote:
> 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 :)
>
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 <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) {
> >
>
