linux-kernel - Re: [PATCH] drm/sched: Prevent stopped entities from being added to the run queue.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DBHNK2XQHUIW.TQHV41LR5D8I@kernel.org>
Date: Mon, 21 Jul 2025 12:14:31 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Philipp Stanner" <phasta@...lbox.org>
Cc: <phasta@...nel.org>, "James Flowers" <bold.zone2373@...tmail.com>,
 <matthew.brost@...el.com>, <ckoenig.leichtzumerken@...il.com>,
 <maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
 <tzimmermann@...e.de>, <airlied@...il.com>, <simona@...ll.ch>,
 <skhan@...uxfoundation.org>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>, <linux-kernel-mentees@...ts.linux.dev>,
 "Tvrtko Ursulin" <tvrtko.ursulin@...lia.com>
Subject: Re: [PATCH] drm/sched: Prevent stopped entities from being added to
 the run queue.

On Mon Jul 21, 2025 at 10:16 AM CEST, Philipp Stanner wrote:
> On Mon, 2025-07-21 at 09:52 +0200, Philipp Stanner wrote:
>> On Sun, 2025-07-20 at 16:56 -0700, James Flowers wrote:
>> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> > index bfea608a7106..997a2cc1a635 100644
>> > --- a/drivers/gpu/drm/scheduler/sched_main.c
>> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> > @@ -172,8 +172,10 @@ void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>> >  
>> >  	entity->oldest_job_waiting = ts;
>> >  
>> > -	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
>> > -		      drm_sched_entity_compare_before);
>> > +	if (!entity->stopped) {
>> > +		rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
>> > +			      drm_sched_entity_compare_before);
>> > +	}
>> 
>> If this is a race, then this patch here is broken, too, because you're
>> checking the 'stopped' boolean as the callers of that function do, too
>> – just later. :O
>> 
>> Could still race, just less likely.
>> 
>> The proper way to fix it would then be to address the issue where the
>> locking is supposed to happen. Let's look at, for example,
>> drm_sched_entity_push_job():
>> 
>> 
>> void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>> {
>> 	(Bla bla bla)
>> 
>>  	…………
>> 
>> 	/* first job wakes up scheduler */
>> 	if (first) {
>> 		struct drm_gpu_scheduler *sched;
>> 		struct drm_sched_rq *rq;
>> 
>> 		/* Add the entity to the run queue */
>> 		spin_lock(&entity->lock);
>> 		if (entity->stopped) {                  <---- Aha!
>> 			spin_unlock(&entity->lock);
>> 
>> 			DRM_ERROR("Trying to push to a killed entity\n");
>> 			return;
>> 		}
>> 
>> 		rq = entity->rq;
>> 		sched = rq->sched;
>> 
>> 		spin_lock(&rq->lock);
>> 		drm_sched_rq_add_entity(rq, entity);
>> 
>> 		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> 			drm_sched_rq_update_fifo_locked(entity, rq, submit_ts); <---- bumm!
>> 
>> 		spin_unlock(&rq->lock);
>> 		spin_unlock(&entity->lock);
>> 
>> But the locks are still being hold. So that "shouldn't be happening"(tm).
>> 
>> Interesting. AFAICS only drm_sched_entity_kill() and drm_sched_fini()
>> stop entities. The former holds appropriate locks, but drm_sched_fini()
>> doesn't. So that looks like a hot candidate to me. Opinions?
>> 
>> On the other hand, aren't drivers prohibited from calling
>> drm_sched_entity_push_job() after calling drm_sched_fini()? If the
>> fuzzer does that, then it's not the scheduler's fault.

Exactly, this is the first question to ask.

And I think it's even more restrictive:

In drm_sched_fini()

	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
		struct drm_sched_rq *rq = sched->sched_rq[i];

		spin_lock(&rq->lock);
		list_for_each_entry(s_entity, &rq->entities, list)
			/*
			 * Prevents reinsertion and marks job_queue as idle,
			 * it will be removed from the rq in drm_sched_entity_fini()
			 * eventually
			 */
			s_entity->stopped = true;
		spin_unlock(&rq->lock);
		kfree(sched->sched_rq[i]);
	}

In drm_sched_entity_kill()

	static void drm_sched_entity_kill(struct drm_sched_entity *entity)
	{
		struct drm_sched_job *job;
		struct dma_fence *prev;

		if (!entity->rq)
			return;

		spin_lock(&entity->lock);
		entity->stopped = true;
		drm_sched_rq_remove_entity(entity->rq, entity);
		spin_unlock(&entity->lock);

		[...]
	}

If this runs concurrently, this is a UAF as well.

Personally, I have always been working with the assupmtion that entites have to
be torn down *before* the scheduler, but those lifetimes are not documented
properly.

There are two solutions:

  (1) Strictly require all entities to be torn down before drm_sched_fini(),
      i.e. stick to the natural ownership and lifetime rules here (see below).

  (2) Actually protect *any* changes of the relevent fields of the entity
      structure with the entity lock.

While (2) seems rather obvious, we run into lock inversion with this approach,
as you note below as well. And I think drm_sched_fini() should not mess with
entities anyways.

The ownership here seems obvious:

The scheduler *owns* a resource that is used by entities. Consequently, entities
are not allowed to out-live the scheduler.

Surely, the current implementation to just take the resource away from the
entity under the hood can work as well with appropriate locking, but that's a
mess.

If the resource *really* needs to be shared for some reason (which I don't see),
shared ownership, i.e. reference counting, is much less error prone.