public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [Regression] drm/scheduler: track GPU active time per entity
@ 2023-03-28  0:57 Danilo Krummrich
  2023-03-28  8:54 ` Lucas Stach
  0 siblings, 1 reply; 33+ messages in thread
From: Danilo Krummrich @ 2023-03-28  0:57 UTC (permalink / raw)
  To: daniel, Dave Airlie, luben.tuikov, l.stach, Bagas Sanjaya,
	andrey.grodzovsky, Christian König, dri-devel, linux-kernel

Hi all,

Commit df622729ddbf ("drm/scheduler: track GPU active time per entity") 
tries to track the accumulated time that a job was active on the GPU 
writing it to the entity through which the job was deployed to the 
scheduler originally. This is done within drm_sched_get_cleanup_job() 
which fetches a job from the schedulers pending_list.

Doing this can result in a race condition where the entity is already 
freed, but the entity's newly added elapsed_ns field is still accessed 
once the job is fetched from the pending_list.

After drm_sched_entity_destroy() being called it should be safe to free 
the structure that embeds the entity. However, a job originally handed 
over to the scheduler by this entity might still reside in the 
schedulers pending_list for cleanup after drm_sched_entity_destroy() 
already being called and the entity being freed. Hence, we can run into 
a UAF.

In my case it happened that a job, as explained above, was just picked 
from the schedulers pending_list after the entity was freed due to the 
client application exiting. Meanwhile this freed up memory was already 
allocated for a subsequent client applications job structure again. 
Hence, the new jobs memory got corrupted. Luckily, I was able to 
reproduce the same corruption over and over again by just using 
deqp-runner to run a specific set of VK test cases in parallel.

Fixing this issue doesn't seem to be very straightforward though (unless 
I miss something), which is why I'm writing this mail instead of sending 
a fix directly.

Spontaneously, I see three options to fix it:

1. Rather than embedding the entity into driver specific structures 
(e.g. tied to file_priv) we could allocate the entity separately and 
reference count it, such that it's only freed up once all jobs that were 
deployed through this entity are fetched from the schedulers pending list.

2. Somehow make sure drm_sched_entity_destroy() does block until all 
jobs deployed through this entity were fetched from the schedulers 
pending list. Though, I'm pretty sure that this is not really desirable.

3. Just revert the change and let drivers implement tracking of GPU 
active times themselves.

In the case of just reverting the change I'd propose to also set a jobs 
entity pointer to NULL  once the job was taken from the entity, such 
that in case of a future issue we fail where the actual issue resides 
and to make it more obvious that the field shouldn't be used anymore 
after the job was taken from the entity.

I'm happy to implement the solution we agree on. However, it might also 
make sense to revert the change until we have a solution in place. I'm 
also happy to send a revert with a proper description of the problem. 
Please let me know what you think.

- Danilo


^ permalink raw reply	[flat|nested] 33+ messages in thread
[parent not found: <e3e07282-136f-5239-96ae-4aeb2d3c95d7.ref@yahoo.de>]

end of thread, other threads:[~2023-04-06 22:36 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-28  0:57 [Regression] drm/scheduler: track GPU active time per entity Danilo Krummrich
2023-03-28  8:54 ` Lucas Stach
2023-04-04  4:31   ` Luben Tuikov
2023-04-05 14:05     ` Danilo Krummrich
2023-04-05 16:09       ` Luben Tuikov
2023-04-05 16:23         ` Danilo Krummrich
2023-04-06  8:22         ` Christian König
2023-04-06  8:27           ` Daniel Vetter
2023-04-06  9:05             ` Asahi Lina
2023-04-06 10:09               ` Daniel Vetter
2023-04-06 12:21                 ` Asahi Lina
2023-04-06 13:37                   ` Daniel Vetter
2023-04-06 14:43                     ` Asahi Lina
2023-04-06 15:30                       ` Daniel Vetter
2023-04-06 17:11                         ` Asahi Lina
2023-04-06 17:42                           ` Daniel Vetter
2023-04-06 17:46                             ` Daniel Vetter
2023-04-06 10:45             ` Lucas Stach
2023-04-06 12:09               ` Daniel Vetter
2023-04-06 12:19                 ` Lucas Stach
2023-04-06 13:56                   ` Daniel Vetter
2023-04-06 14:21               ` Christian König
2023-04-06 15:24                 ` Lucas Stach
2023-04-06 15:33                   ` Christian König
2023-04-06 15:59                     ` Daniel Vetter
2023-04-06 16:23                       ` Daniel Vetter
2023-04-06 22:36               ` Luben Tuikov
2023-04-06 22:30           ` Luben Tuikov
2023-04-05 17:44     ` Lucas Stach
2023-04-05 20:44       ` Luben Tuikov
2023-04-06 15:58         ` Lucas Stach
2023-04-06 22:06           ` Luben Tuikov
     [not found] <e3e07282-136f-5239-96ae-4aeb2d3c95d7.ref@yahoo.de>
     [not found] ` <e3e07282-136f-5239-96ae-4aeb2d3c95d7@yahoo.de>
2023-03-28 10:29   ` Christian König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox