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

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

On 10.05.2022 11:48, Tvrtko Ursulin wrote:
>
> On 10/05/2022 10:39, Andrzej Hajda wrote:
>> On 10.05.2022 10:18, Tvrtko Ursulin wrote:
>>>
>>> On 10/05/2022 08:58, Andrzej Hajda wrote:
>>>> Hi Tvrtko,
>>>>
>>>> On 10.05.2022 09:28, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 29/04/2022 20:56, 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.
>>>>>>
>>>>>> v2: s/gtn/gt/ (Andi)
>>>>>>
>>>>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>>> Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface")
>>>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/gt/intel_gt.c       |  1 +
>>>>>>   drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 29 
>>>>>> ++++++++++--------------
>>>>>>   drivers/gpu/drm/i915/gt/intel_gt_sysfs.h |  6 +----
>>>>>>   drivers/gpu/drm/i915/gt/intel_gt_types.h |  3 +++
>>>>>>   drivers/gpu/drm/i915/i915_sysfs.c        |  2 ++
>>>>>>   5 files changed, 19 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>>>>>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>>>>>> index 92394f13b42f..9aede288eb86 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>>>>> @@ -785,6 +785,7 @@ void intel_gt_driver_unregister(struct 
>>>>>> intel_gt *gt)
>>>>>>   {
>>>>>>       intel_wakeref_t wakeref;
>>>>>>   +    intel_gt_sysfs_unregister(gt);
>>>>>>       intel_rps_driver_unregister(&gt->rps);
>>>>>>       intel_gsc_fini(&gt->gsc);
>>>>>>   diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c 
>>>>>> b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
>>>>>> index 8ec8bc660c8c..9e4ebf53379b 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
>>>>>> @@ -24,7 +24,7 @@ bool is_object_gt(struct kobject *kobj)
>>>>>>     static struct intel_gt *kobj_to_gt(struct kobject *kobj)
>>>>>>   {
>>>>>> -    return container_of(kobj, struct kobj_gt, base)->gt;
>>>>>> +    return container_of(kobj, struct intel_gt, sysfs_gt);
>>>>>>   }
>>>>>>     struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
>>>>>> @@ -72,9 +72,9 @@ static struct attribute *id_attrs[] = {
>>>>>>   };
>>>>>>   ATTRIBUTE_GROUPS(id);
>>>>>>   +/* A kobject needs a release() method even if it does nothing */
>>>>>>   static void kobj_gt_release(struct kobject *kobj)
>>>>>>   {
>>>>>> -    kfree(kobj);
>>>>>>   }
>>>>>>     static struct kobj_type kobj_gt_type = {
>>>>>> @@ -85,8 +85,6 @@ static struct kobj_type kobj_gt_type = {
>>>>>>     void intel_gt_sysfs_register(struct intel_gt *gt)
>>>>>>   {
>>>>>> -    struct kobj_gt *kg;
>>>>>> -
>>>>>>       /*
>>>>>>        * We need to make things right with the
>>>>>>        * ABI compatibility. The files were originally
>>>>>> @@ -98,25 +96,22 @@ void intel_gt_sysfs_register(struct intel_gt 
>>>>>> *gt)
>>>>>>       if (gt_is_root(gt))
>>>>>>           intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));
>>>>>>   -    kg = kzalloc(sizeof(*kg), GFP_KERNEL);
>>>>>> -    if (!kg)
>>>>>> +    /* init and xfer ownership to sysfs tree */
>>>>>> +    if (kobject_init_and_add(&gt->sysfs_gt, &kobj_gt_type,
>>>>>> +                 gt->i915->sysfs_gt, "gt%d", gt->info.id))
>>>>>
>>>>> Was there closure/agreement on the matter of whether or not there 
>>>>> is a potential race between "kfree(gt)" and sysfs access (last put 
>>>>> from sysfs that is)? I've noticed Andrzej and Ashutosh were 
>>>>> discussing it but did not read all the details.
>>>>>
>>>>
>>>> Not really :)
>>>> IMO docs are against this practice, Ashutosh shows examples of this 
>>>> practice in code and according to his analysis it is safe.
>>>> I gave up looking for contradictions :) Either it is OK, kobject is 
>>>> not fully shared object, docs are obsolete and needs update, either 
>>>> the patch is wrong.
>>>> Anyway finally I tend to accept this solution, I failed to prove it 
>>>> is wrong :)
>>>
>>> Like a question of whether hotunplug can be triggered while 
>>> userspace is sitting in a sysfs hook? Final kfree then has to be 
>>> delayed until userspace exists.
>>>
>>> Btw where is the "kfree(gt)" for the tiles on the PCI remove path? I 
>>> can't find it.. Do we have a leak?
>>
>> intel_gt_tile_cleanup ?
>
> Called from intel_gt_release_all, whose only caller is the failure 
> path of i915_driver_probe. Feels like something is missing?

This is final proof this patch is safe - no kfree, no UAF :)

Apparently it is broken in internal branch as well.
Should I take care of it?

Regards
Andrzej


>
> Regards,
>
> Tvrtko