Re: [PATCH] drm/scheduler: set entity to NULL in drm_sched_entity_pop_job()

[Luben Tuikov]

On 2023-04-11 14:13, Danilo Krummrich wrote:
> On 4/5/23 19:39, Luben Tuikov wrote:
>> On 2023-03-31 01:59, Christian König wrote:
>>> Am 31.03.23 um 02:06 schrieb Danilo Krummrich:
>>>> It already happend a few times that patches slipped through which
>>>> implemented access to an entity through a job that was already removed
>>>> from the entities queue. Since jobs and entities might have different
>>>> lifecycles, this can potentially cause UAF bugs.
>>>>
>>>> In order to make it obvious that a jobs entity pointer shouldn't be
>>>> accessed after drm_sched_entity_pop_job() was called successfully, set
>>>> the jobs entity pointer to NULL once the job is removed from the entity
>>>> queue.
>>>>
>>>> Moreover, debugging a potential NULL pointer dereference is way easier
>>>> than potentially corrupted memory through a UAF.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
>>>
>>> In general "YES PLEASE!", but I fear that this will break amdgpus reset
>>> sequence.
>>>
>>> On the other hand when amdgpu still relies on that pointer it's clearly
>>> a bug (which I pointed out tons of times before).
>>>
>>> Luben any opinion on that? Could you drive cleaning that up as well?
>>
>> I didn't find any references to scheduling entity after the job
>> is submitted to the hardware. (I commented the same in the other
>> thread, we just need to decide which way to go.)
> 
> AFAICS from the other mail thread it seems to be consensus to not 
> ref-count entities and handle job statistics differently.
> 
> Should we go ahead and take this patch then? Maybe it also makes sense 
> to send a V2 additionally adding a comment to the drm_sched_job 
> structure mentioning that .entity must not be used after the job was 
> taken from the entities queue.

Yes, please send a v2, but instead of mentioning (or in addition to)
that the job was taken from the "entities queue", mention that
once the job is pushed to the hardware, i.e. the pending queue,
from then on, the "entity" should not be referenced anymore. IOW, we
want to mention that it is going from X to Y, as opposed to just
that it's taken from X, because the latter begs the question "Where
is it going to then?".

For the record, I think that using kref would give us the most
stability, even if we skipped "entity" and let the scheduler, or
even the controller keep a kref on submitted commands down to
the GPU. On reset, we block command submission to the GPU from
the outermost boundary, and then start peeling the layers from
the innermost boundary.

Using kref also forces us to think objectively and set explicit
(clear) dependencies from the outset--i.e. who gets freed and when
and under what circumstances.

And using kref might even expose the wrong dependencies, thus
prompting a redesign and thus a fix.
-- 
Regards,
Luben