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

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 20.04.2022 18:12, Dixit, Ashutosh wrote:
> On Wed, 20 Apr 2022 05:17:57 -0700, Andrzej Hajda wrote:
>> Hi Ashutosh,
> Hi Andrzej,
>
>> On 20.04.2022 07:21, Ashutosh Dixit wrote:
>>> All kmalloc'd kobjects need a kobject_put() to free memory. For example in
>>> previous code, kobj_gt_release() never gets called. The requirement of
>>> kobject_put() now results in a slightly different code organization.
>>>
>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface")
> /snip/
>
>>> +void intel_gt_sysfs_unregister(struct intel_gt *gt)
>>> +{
>>> +	kobject_put(&gt->sysfs_gtn);
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
>>> index 9471b26752cf..a99aa7e8b01a 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
>>> @@ -13,11 +13,6 @@
>>>      struct intel_gt;
>>>    -struct kobj_gt {
>>> -	struct kobject base;
>>> -	struct intel_gt *gt;
>>> -};
>>> -
>>>    bool is_object_gt(struct kobject *kobj);
>>>      struct drm_i915_private *kobj_to_i915(struct kobject *kobj);
>>> @@ -28,6 +23,7 @@ intel_gt_create_kobj(struct intel_gt *gt,
>>> 		     const char *name);
>>>      void intel_gt_sysfs_register(struct intel_gt *gt);
>>> +void intel_gt_sysfs_unregister(struct intel_gt *gt);
>>>    struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
>>> 					    const char *name);
>>>    diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> 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.

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.

Regards
Andrzej

> Thanks.
> --
> Ashutosh