| Message ID | 20220706074219.3614-16-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/15] drm/i915/gvt: fix a memory leak in intel_gvt_init_vgpu_types | expand |
On 7/6/2022 1:12 PM, Christoph Hellwig wrote: > The mdev_type already holds a reference to the parent through > mdev_types_kset, so drop the extra reference. > > Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/vfio/mdev/mdev_sysfs.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index c5cd035d591d0..e2087cac1c859 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -153,8 +153,6 @@ static void mdev_type_release(struct kobject *kobj) > struct mdev_type *type = to_mdev_type(kobj); > > pr_debug("Releasing group %s\n", kobj->name); > - /* Pairs with the get in add_mdev_supported_type() */ > - put_device(type->parent->dev); > kfree(type); > } > > @@ -170,16 +168,12 @@ static int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type) > > type->kobj.kset = parent->mdev_types_kset; > type->parent = parent; > - /* Pairs with the put in mdev_type_release() */ > - get_device(parent->dev); > > ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL, > "%s-%s", dev_driver_string(parent->dev), > type->sysfs_name); > - if (ret) { > - kobject_put(&type->kobj); This kobject_put is required here in error case, see description above kobject_init_and_add(). > + if (ret) > return ret; > - } > > type->devices_kobj = kobject_create_and_add("devices", &type->kobj); > if (!type->devices_kobj) { > @@ -191,7 +185,6 @@ static int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type) > > attr_devices_failed: > kobject_del(&type->kobj); > - kobject_put(&type->kobj); Same as above. Thanks, Kirti
On Wed, Jul 06, 2022 at 09:42:19AM +0200, Christoph Hellwig wrote: > The mdev_type already holds a reference to the parent through > mdev_types_kset, so drop the extra reference. I would drop this patch, but at least the explanation needs tweaking.. kobj's are weird things, they have a kobj.parent, but the refcount for that is dropped during kobject_del() and parent is NULL'd. vs this reference which is being put back at kobject release. So, the extra kset reference isn't contributing more protection - when mdev_unregister_device() calls kset_unregister() it still has a valid reference on the parent from the caller. If this change is safe it is because no accesses to mdev->parent are happening after mdev_unregister_device() returns, due to things like the sysfs attribute fencing and the mdev_unreg_sem fencing child devices. I didn't check everything, but it seems believable it could be true. I also suggest this patch should NULL mdev_type->parent during unregister_device as the kobj stuff does, so any use after unregister but before release could be crash, we generally shouldn't leave unref'd pointers floating around. Jason
> From: Christoph Hellwig > Sent: Wednesday, July 6, 2022 3:42 PM > > The mdev_type already holds a reference to the parent through > mdev_types_kset, so drop the extra reference. > > Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On Wed, Jul 06, 2022 at 07:18:48PM +0530, Kirti Wankhede wrote: >> - kobject_put(&type->kobj); > > This kobject_put is required here in error case, see description above > kobject_init_and_add(). And they are completely unrelated anyway, this should have never slipped in.
On Wed, Jul 06, 2022 at 11:38:33AM -0300, Jason Gunthorpe wrote: > On Wed, Jul 06, 2022 at 09:42:19AM +0200, Christoph Hellwig wrote: > > The mdev_type already holds a reference to the parent through > > mdev_types_kset, so drop the extra reference. > > I would drop this patch, but at least the explanation needs tweaking.. I'm fine with that. Alex, any preferences?
On Thu, 7 Jul 2022 15:40:52 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Wed, Jul 06, 2022 at 11:38:33AM -0300, Jason Gunthorpe wrote: > > On Wed, Jul 06, 2022 at 09:42:19AM +0200, Christoph Hellwig wrote: > > > The mdev_type already holds a reference to the parent through > > > mdev_types_kset, so drop the extra reference. > > > > I would drop this patch, but at least the explanation needs tweaking.. > > I'm fine with that. Alex, any preferences? Modulo the bogus kobject_put()s, this essentially reverts: commit 9a302449a58d45d0ef2aab686f64b35919bc604c Author: Jason Gunthorpe <jgg@ziepe.ca> Date: Tue Apr 6 16:40:30 2021 -0300 vfio/mdev: Add missing reference counting to mdev_type struct mdev_type holds a pointer to the kref'd object struct mdev_parent, but doesn't hold the kref. The lifetime of the parent becomes implicit because parent_remove_sysfs_files() is supposed to remove all the access before the parent can be freed, but this is very hard to reason about. Make it obviously correct by adding the missing get. Reviewed-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Kevin Tian <kevin.tian@intel.com> Reviewed-by: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Message-Id: <7-v2-d36939638fc6+d54-vfio2_jgg@nvidia.com> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Which we all seemed to think was a good thing 15 months ago. It is still difficult to reason when the mdev_type_ktype.release function occurs relative to the parent reference held by the kset, but without an explanation how we're safe, I'm ok with a little paranoia and explicit references. Thanks, Alex
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c index c5cd035d591d0..e2087cac1c859 100644 --- a/drivers/vfio/mdev/mdev_sysfs.c +++ b/drivers/vfio/mdev/mdev_sysfs.c @@ -153,8 +153,6 @@ static void mdev_type_release(struct kobject *kobj) struct mdev_type *type = to_mdev_type(kobj); pr_debug("Releasing group %s\n", kobj->name); - /* Pairs with the get in add_mdev_supported_type() */ - put_device(type->parent->dev); kfree(type); } @@ -170,16 +168,12 @@ static int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type) type->kobj.kset = parent->mdev_types_kset; type->parent = parent; - /* Pairs with the put in mdev_type_release() */ - get_device(parent->dev); ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL, "%s-%s", dev_driver_string(parent->dev), type->sysfs_name); - if (ret) { - kobject_put(&type->kobj); + if (ret) return ret; - } type->devices_kobj = kobject_create_and_add("devices", &type->kobj); if (!type->devices_kobj) { @@ -191,7 +185,6 @@ static int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type) attr_devices_failed: kobject_del(&type->kobj); - kobject_put(&type->kobj); return ret; }
The mdev_type already holds a reference to the parent through mdev_types_kset, so drop the extra reference. Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/vfio/mdev/mdev_sysfs.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)