linux-kernel - Re: [PATCH] drm/sched: Fix amdgpu crash upon suspend/resume

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 for Android: free password hash cracker in your pocket

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

Message-ID: <582e10673bb749f18ebf8a18f46ca573df396576.camel@redhat.com>
Date: Mon, 13 Jan 2025 09:43:12 +0100
From: Philipp Stanner <pstanner@...hat.com>
To: Christian König <christian.koenig@....com>, Philipp
 Reisner <philipp.reisner@...bit.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, Simona
 Vetter <simona@...ll.ch>, Danilo Krummrich <dakr@...nel.org>, Philipp
 Stanner <phasta@...nel.org>
Subject: Re: [PATCH] drm/sched: Fix amdgpu crash upon suspend/resume

+cc Danilo
+cc myself

On Wed, 2025-01-08 at 09:19 +0100, Christian König wrote:
> Am 07.01.25 um 16:21 schrieb Philipp Reisner:
> > [...]
> > > > The OOPS happens because the rq member of entity is NULL in
> > > > drm_sched_job_arm() after the call to
> > > > drm_sched_entity_select_rq().
> > > > 
> > > > In drm_sched_entity_select_rq(), the code considers that
> > > > drb_sched_pick_best() might return a NULL value. When NULL, it
> > > > assigns
> > > > NULL to entity->rq even if it had a non-NULL value before.
> > > > 
> > > > drm_sched_job_arm() does not deal with entities having a rq of
> > > > NULL.
> > > > 
> > > > Fix this by leaving the entity on the engine it was instead of
> > > > assigning a NULL to its run queue member.
> > > Well that is clearly not the correct approach to fixing this. So
> > > clearly
> > > a NAK from my side.
> > > 
> > > The real question is why is amdgpu_cs_ioctl() called when all of
> > > userspace should be frozen?
> > > 
> > > Regards,
> > > Christian.
> > > 
> > Could the OOPS happen at resume time? Might it be that the kernel
> > activates user-space
> > before all the components of the GPU finished their wakeup?
> > 
> > Maybe drm_sched_pick_best() returns NULL since no scheduler is
> > ready yet?
> 
> Yeah that is exactly what I meant. It looks like either the suspend
> or 
> the resume order is somehow messed up.
> 
> In other words either some application tries to submit GPU work while
> it 
> should already been stopped, or it tries to submit GPU work before it
> is 
> started.
> 
> > Apart from whether amdgpu_cs_ioctl() should run at this point, I
> > still think the
> > suggested change improves the code. drm_sched_pick_best() can
> > return NULL.
> > drm_sched_entity_select_rq() can handle the NULL (partially).
> > 
> > drm_sched_job_arm() crashes on an entity that has rq set to NULL.
> 
> Which is actually not the worst outcome :)
> 
> With your patch applied we don't immediately crash any more in the 
> submission path, but the whole system could then later deadlock
> because 
> the core memory management waits for a GPU submission which never
> returns.
> 
> That is an even worse situation because you then can't pinpoint any
> more 
> where that is coming from.
> 
> > The handling of NULL values is half-baked.
> > 
> > In my opinion, you should define if drm_sched_pick_best() may put a
> > NULL into
> > rq. If your answer is yes, it might put a NULL there; then, there
> > should be a
> > BUG_ON(!entity->rq) after the invocation of
> > drm_sched_entity_select_rq().
> > If your answer is no, the BUG_ON() should be in
> > drm_sched_pick_best().
> 
> Yeah good point.
> 
> We might not want a BUG_ON(), that is only justified when we prevent 
> further damage (e.g. random data corruption or similar).
> 
> I suggest using a WARN(!shed, "Submission without activated
> sheduler!"). 
> This way the system has at least a chance of survival should the 
> scheduler become ready later on.
> 
> On the other hand the BUG_ON() or the NULL pointer deref should only 
> kill the application thread which is submitting something before the 
> driver is resumed. So that might help to pinpoint where the actually 
> issue is.

As I see it the BUG_ON() would just be a more pretty NULL pointer
deref. If we agree that this is effectively a misuse of the scheduler
API we probably want to add it to make it more pretty, though?

@Philipp:
BTW, I only just discovered this thread by coincidence. Please use
get_maintainer. The scheduler currently has 4 maintainers, and none of
them is on CC.

Danke,
P.

> 
> Regards,
> Christian.
> 
> > 
> > That helps guys with zero domain knowledge, like me, to figure out
> > how
> > this is all
> > supposed to work.
> > 
> > best regards,
> >   Philipp
>