[Intel-gfx] [PATCH 7/9] drm/i915/gt: Fix memory leaks in per-gt sysfs

[Dixit, Ashutosh]

On Sun, 24 Apr 2022 15:36:23 -0700, Andi Shyti wrote:
>
> Hi Andrzej and Ashutosh,
>
> > > > > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > index 937b2e1a305e..4c72b4f983a6 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > > @@ -222,6 +222,9 @@ struct intel_gt {
> > > > >	} mocs;
> > > > >		struct intel_pxp pxp;
> > > > > +
> > > > > +	/* gt/gtN sysfs */
> > > > > +	struct kobject sysfs_gtn;
> > > > If you put kobject as a part of intel_gt what assures you that lifetime of
> > > > kobject is shorter than intel_gt? Ie its refcounter is 0 on removal of
> > > > intel_gt?
> > > Because we are explicitly doing a kobject_put() in
> > > intel_gt_sysfs_unregister(). Which is exactly what we are *not* doing in
> > > the previous code.
> > >
> > > Let me explain a bit about the previous code (but feel free to skip since
> > > the patch should speak for itself):
> > > * Previously we kzalloc a 'struct kobj_gt'
> > > * But we don't save a pointer to the 'struct kobj_gt' so we don't have the
> > >    pointer to the kobject to be able to do a kobject_put() on it later
> > > * Therefore we need to store the pointer in 'struct intel_gt'
> > > * But if we have to put the pointer in 'struct intel_gt' we might as well
> > >    put the kobject as part of 'struct intel_gt' and that also removes the
> > >    need to have a 'struct kobj_gt' (kobj_to_gt() can just use container_of()
> > >    to get gt from kobj).
> > > * So I think this patch simpler/cleaner than the original code if you take
> > >    the requirement for kobject_put() into account.
>
> This is my oversight. This was something I completely forgot to
> fix but it was my intention to do and actually I had some fixes
> ongoing. But because this patch took too long to get in I
> completely forgot about it (Sujaritha was actually the first who
> pointed this out).
>
> Thanks, Ashutosh for taking this.
>
> > I fully agree that previous code is incorrect but I am not convinced current
> > code is correct.
> > If some objects are kref-counted it means usually they can have multiple
> > concurrent users and kobject_put does not work as traditional
> > destructor/cleanup/unregister.
> > So in this particular case after calling kobject_init_and_add sysfs core can
> > get multiple references on the object. Later, during driver unregistration
> > kobject_put is called, but if the object is still in use by sysfs core, the
> > object will not be destroyed/released. If the driver unregistration
> > continues memory will be freed, leaving sysfs-core (or other users) with
> > dangling pointers. Unless there is some additional synchronization mechanism
> > I am not aware of.
>
> Thanks Andrzej for summarizing this and what you said is actually
> what happens. I had a similar solution developed and I had wrong
> pointer reference happening.

Hi Andrzej/Andi,

I did do some research into kobject's and such before writing this patch
and based on that I believe the patch is correct. Presenting some evidence
below.

The patch is verified by:

a. Putting a printk in the release() method when it exists (it does for
   sysfs_gtn kobject)
b. Enabling dynamic prints for lib/kobject.c

For example, with the following:

# echo 'file kobject.c +p' > /sys/kernel/debug/dynamic_debug/control
# echo -n "0000:03:00.0" > /sys/bus/pci/drivers/i915/unbind

We see this in dmesg (see kobject_cleanup() called from kobject_put()):

[ 1034.930007] kobject: '.defaults' (ffff88817130a640): kobject_cleanup, parent ffff8882262b5778
[ 1034.930020] kobject: '.defaults' (ffff88817130a640): auto cleanup kobject_del
[ 1034.930336] kobject: '.defaults' (ffff88817130a640): calling ktype release
[ 1034.930340] kobject: (ffff88817130a640): dynamic_kobj_release
[ 1034.930354] kobject: '.defaults': free name
[ 1034.930366] kobject: 'gt0' (ffff8882262b5778): kobject_cleanup, parent ffff88817130a240
[ 1034.930371] kobject: 'gt0' (ffff8882262b5778): auto cleanup kobject_del
[ 1034.931930] kobject: 'gt0' (ffff8882262b5778): calling ktype release
[ 1034.931936] kobject: 'gt0': free name
[ 1034.958004] kobject: 'i915_0000_03_00.0' (ffff88810e1f8800): fill_kobj_path: path = '/devices/i915_0000_03_00.0'
[ 1034.958155] kobject: 'i915_0000_03_00.0' (ffff88810e1f8800): kobject_cleanup, parent 0000000000000000
[ 1034.958162] kobject: 'i915_0000_03_00.0' (ffff88810e1f8800): calling ktype release
[ 1034.958188] kobject: 'i915_0000_03_00.0': free name
[ 1034.958729] kobject: 'gt' (ffff88817130a240): kobject_cleanup, parent ffff8881160c5000
[ 1034.958736] kobject: 'gt' (ffff88817130a240): auto cleanup kobject_del
[ 1034.958762] kobject: 'gt' (ffff88817130a240): calling ktype release
[ 1034.958767] kobject: (ffff88817130a240): dynamic_kobj_release
[ 1034.958778] kobject: 'gt': free name

We have the following directory structure (one of the patches is creating
/sys/class/drm/card0/gt/gt0/.defaults):

      /sys/class/drm/card0/gt
                           |-gt0
                              |-.defaults

And we see from dmesg .defaults, gt0 and gt kobjects being cleaned up in
that order.

Looking at lib/kobject.c there are several interesting things:

* Three subsystems are involved: kobject, sysfs and kernfs.

* A child kobject takes a reference on the parent, so we must do a
  kobject_put() on the child before doing kobject_put() on the parent
  (creating a child kobject creates a corresponding sub-directory in sysfs).

* Adding files to a sysfs directory does not take a reference on the
  kobject, only on the parent kernfs_node.

* Since we do call sysfs_create_group() (for RC6) ordinarily we will need
  to call sysfs_remove_group() but this does not seem to be needed because
  we are not creating a directory for the group (by providing a name for
  the group). So sysfs_create_group() is equivalent to sysfs_create_files().
  So it seems we don't need sysfs_remove_group().

* Similarly it appears files created by sysfs_create_files() do not need to
  be removed by sysfs_remove_files() because __kobject_del() and
  sysfs_remove_dir() called from kobject_cleanup() do that for us (the
  comment in kobject_cleanup() says "remove from sysfs if the caller did
  not do it").

Based on the above it is clear that no one except a child kobject takes a
reference on the parent kobject and as long as we kobject_put() them in the
correct order (as we seem to be doing based on dmesg trace above) we should
be ok.

Also what is followed in this patch is a fairly standard coding
pattern. Further, in case of any errors we generally see failure to unload
the module etc. and none of these things are being observed, module reload
works fine.

I hope these points are helpful in completing review of the patch.

Thanks.
--
Ashutosh