Re: [PATCH 03/11] drm/panthor: Fix UAF on works queued to panthor_cleanup_wq

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 20 Jul 2026 11:47:59 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> On Thursday, 25 June 2026 14:40:29 Central European Summer Time Boris Brezillon wrote:
> > If we don't drain the panthor_cleanup_wq before
> > unregistering/destroying the panthor_device, we might end up with
> > cleanup works that are executed after the device is gone.
> > 
> > Add a drain_workqueue() call in panthor_device_unplug() to prevent
> > that.
> > 
> > Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> > Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block")
> > Reported-by: sashiko-bot@kernel.org
> > Closes: https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v5-0-8836a74e0ef9@collabora.com?part=2
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index 0b25abebb803..a0774e28aa94 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -101,6 +101,11 @@ void panthor_device_unplug(struct panthor_device *ptdev)
> >  	panthor_gpu_unplug(ptdev);
> >  	panthor_pwr_unplug(ptdev);
> >  
> > +	/* Make sure works queued to panthor_cleanup_wq are executed
> > +	 * before the device is destroyed.
> > +	 */
> > +	drain_workqueue(panthor_cleanup_wq);
> > +
> >  	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
> >  	pm_runtime_put_sync_suspend(ptdev->base.dev);
> >  
> > 
> >   
> 
> I feel like nothing in the driver instance should be allowed to queue new
> work to that workqueue while the unplug mutex is being held, and
> panthor_sched_unplug should just disable_work_sync all the associated group
> release_work items as well. If there's not a strong enough relationship
> between a scheduler and the groups to ensure we get all the groups in a
> non-racey manner that might not work out cleanly though.

Release is a bit a special work though. It's something that happens
once we know the group has left the scheduler, and all it's supposed
to do is free the resources the group might hold (mostly memory).

> 
> Having a scheduler unplug be completely disconnected from the associated
> groups being gone seems counter-intuitive to me.

It's actually the other way around: unplugging the scheduler might
result in new group_release work items being queued to the cleanup_wq.
The drain_worqueue() (which should actually be a flush_worqueue) is
here to guarantee that we're not leaving objects behind that would
still have references to objects that are about to be destroyed (in the
case of group_release, that's the drm_device, which holds all the GEM
database that we need to return the GEM refs the destroyed groups need
to release).