Message-ID: <5ce98154b3f16c09b2d9b48493e88a4c6916281e.camel@mailbox.org>
Date: Wed, 23 Jul 2025 16:41:00 +0200
From: Philipp Stanner <phasta@...lbox.org>
To: James <bold.zone2373@...tmail.com>, phasta@...nel.org, 
 matthew.brost@...el.com, dakr@...nel.org, Christian
 König <ckoenig.leichtzumerken@...il.com>,
 maarten.lankhorst@...ux.intel.com,  mripard@...nel.org,
 tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,  Shuah Khan
 <skhan@...uxfoundation.org>
Cc: 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.

Hello,

On Tue, 2025-07-22 at 13:05 -0700, James wrote:
> On Mon, Jul 21, 2025, at 1:16 AM, Philipp Stanner wrote:
> > On Mon, 2025-07-21 at 09:52 +0200, Philipp Stanner wrote:
> > > +Cc Tvrtko, who's currently reworking FIFO and RR.
> > > 
> > > On Sun, 2025-07-20 at 16:56 -0700, James Flowers wrote:
> > > > Fixes an issue where entities are added to the run queue in
> > > > drm_sched_rq_update_fifo_locked after being killed, causing a
> > > > slab-use-after-free error.
> > > > 
> > > > Signed-off-by: James Flowers <bold.zone2373@...tmail.com>
> > > > ---
> > > > This issue was detected by syzkaller running on a Steam Deck OLED.
> > > > Unfortunately I don't have a reproducer for it. I've
> > > 
> > > Well, now that's kind of an issue – if you don't have a reproducer, how
> > > can you know that your patch is correct? How can we?
> > > 
> > > It would certainly be good to know what the fuzz testing framework
> > > does.
> > > 
> > > > included the KASAN reports below:
> > > 
> > > 
> > > Anyways, KASAN reports look interesting. But those might be many
> > > different issues. Again, would be good to know what the fuzzer has been
> > > testing. Can you maybe split this fuzz test into sub-tests? I suspsect
> > > those might be different faults.
> > > 
> > > 
> > > Anyways, taking a first look…
> > > 
> > > 
> > > > 
> > > > ==================================================================
> > > > BUG: KASAN: slab-use-after-free in rb_next+0xda/0x160 lib/rbtree.c:505
> > > > Read of size 8 at addr ffff8881805085e0 by task kworker/u32:12/192

[SNIP]

> > > > 
> > > >  drivers/gpu/drm/scheduler/sched_main.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > 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.
> > > 
> > > Could you test adding spin_lock(&entity->lock) to drm_sched_fini()?
> > 
> > Ah no, forget about that.
> > 
> > In drm_sched_fini(), you'd have to take the locks in reverse order as
> > in drm_sched_entity_push/pop_job(), thereby replacing race with
> > deadlock.
> > 
> > I suspect that this is an issue in amdgpu. But let's wait for
> > Christian.
> > 
> > 
> > P.
> > 
> > 
> > > 
> > > Would be cool if Tvrtko and Christian take a look. Maybe we even have a
> > > fundamental design issue.
> > > 
> > > 
> > > Regards
> > > P.
> > > 
> > > 
> > > >  }
> > > >  
> > > >  /**
> > > 
> 
> Thanks for taking a look at this. I did try to get a reproducer using syzkaller, without success. I can attempt it myself but I expect it will take me some time, if I'm able to at all with this bug. I did run some of the igt-gpu-tools tests (amdgpu and drm ones), and there was no difference after the changes on my system. After this change I wasn't running into the UAF errors after 100k+ executions but I see what you mean, Philipp - perhaps it's missing the root issue. 
> 
> FYI, as an experiment I forced the use of RR with "drm_sched_policy = DRM_SCHED_POLICY_RR", and I'm not seeing any slab-use-after-frees, so maybe the problem is with the FIFO implementation? 

I can't imagine that. The issue your encountering is most likely a race
caused by the driver tearing down entities after the scheduler, so
different scheduler runtime behavior might hide ("fix") the race
(that's the nature of races, actually: sometimes they're there,
sometimes not). RR running with different time patterns than FIFO
doesn't mean that FIFO has a bug.

> 
> For now, the closest thing to a reproducer I can provide is my syzkaller config, in case anyone else is able to try this with a Steam Deck OLED. I've included this below along with an example program run by syzkaller (in generated C code and a Syz language version).

Thanks for investigating this.

My recommendation for now is that you write a reproducer program,
possibly inspired by the syzkaller code you showed.

Reproduce it cleanly and (optionally) try a fix. Then another mail
would be good, especially with the amdgpu maintainers on Cc since I
suspect that this is a driver issue.

Don't get me wrong, a UAF definitely needs to be fixed; but since it's
not occurring outside of fuzzing currently and as we can't reproduce
it, we can't really do much about it until that's the case.

I will in the mean time provide a patch pimping up the memory life time
documentation for scheduler objects.

Thx
P.

Powered by blists - more mailing lists