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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 10 May 2022 03:41:57 -0700, Andrzej Hajda wrote:
> 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:
> >>>>>
> >>>>> 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?

See Daniele's comment here:

https://patchwork.freedesktop.org/patch/478856/?series=101551&rev=1

We clean up the gt sysfs during PCI device remove (i915_driver_remove ->
i915_driver_unregister -> intel_gt_driver_unregister ->
intel_gt_sysfs_unregister (added in this patch)). But from Daniele's mail
it appears that "kfree(gt)" can only be done from i915_driver_release().

So as long as i915_driver_release() always happens after
i915_driver_remove() (which seems to be the case though I couldn't figure
out why (i.e. who is putting the final reference of the drm device)) there
is no UAF and no race. Thanks!