The current unplug logic is broken in multiple ways. This is an attempt
at addressing the various problems found along the way (some were
reported by Sashiko, others have been found while trying to address
Sashiko's concerns).

Sending a new version even though v2 didn't receive any human review
just to try and address the new stuff pointed out by Sashiko.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v3:
- Fix a race in the reset reschedule logic we added to
  panthor_device_resume() (missing smp_mb__after_atomic())
- Fix a VM leak when reset and suspend are racing with each other
- Add missing drm_dev_enter/exit() sections
- Insert the groups in the user_owned list even if the group creation
  happens during a reset
- Try to document why some of the issues pointed out by Sashiko are
  either not real issues, or are expected (either fixed in a later
  commits, or just expected behavior)
- Fix a race between panthor_device_unplug() and vm_prep_for_cleanup()
  (introduced in v2)
- Link to v2: https://patch.msgid.link/20260811-panthor-unplug-fixes-v2-0-6b583e37f9ae@collabora.com

Changes in v2:
- Fix UAFs caused by deferred cleanup works
- Fix UAFs caused by open FDs closed after unplug
- Fix deadlock when device_unplug() is called from the reset work
- Make sure reset requests are not lost in the resume and post_reset
  paths
- Fix a deadlock in the suspend path
- Fix a clk prepare_enable leak in the unplug path
- Don't use a drmm_action to flush the cleanup queue (this could cause
  UAFs)
- Drop the now unused panthor_vm::unusable field
- Keep track of user owned resources to prevent leaks and/or UAFs
- Link to v1: https://patch.msgid.link/20260804-panthor-unplug-fixes-v1-0-abbbd2d41b13@collabora.com

---
Boris Brezillon (17):
      drm/panthor: Disable reset work before unplug
      drm/panthor: Further delay reset work enablement
      drm/panthor: Make sure reset requests in the resume path are not lost
      drm/panthor: Make sure reset requests in the post reset path are not lost
      drm/panthor: Flush the cleanup_wq in the unplug path
      drm/panthor: Drop unused vm argument passed to panthor_vm_prepare_sync_only_op_ctx()
      drm/panthor: Move the debugfs initialization to panthor_device.c
      drm/panthor: Split panthor_vm
      drm/panthor: Add fine-grained restrictions on VMs
      drm/panthor: Check AS state before disabling
      drm/panthor: Don't pre-allocate VMAs or page tables when preparing a full VM unmap
      drm/panthor: Make the VM cleanup path more robust against UAF
      drm/panthor: Track user owned VMs
      drm/panthor: Track user owned groups
      drm/panthor: Fix the unplug logic
      drm/panthor: Add a debugfs knob to simulate unplug failures
      drm/panthor: Add a debugfs knobs to simulate reset failures

 drivers/gpu/drm/panthor/panthor_device.c |  189 +++-
 drivers/gpu/drm/panthor/panthor_device.h |   38 +
 drivers/gpu/drm/panthor/panthor_drv.c    |  132 ++-
 drivers/gpu/drm/panthor/panthor_fw.c     |    9 +-
 drivers/gpu/drm/panthor/panthor_mmu.c    | 1493 ++++++++++++++++++------------
 drivers/gpu/drm/panthor/panthor_mmu.h    |    4 +-
 drivers/gpu/drm/panthor/panthor_sched.c  |  132 ++-
 7 files changed, 1344 insertions(+), 653 deletions(-)
---
base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
change-id: 20260804-panthor-unplug-fixes-7927b3ddc2f9

Best regards,
--  
Boris Brezillon <boris.brezillon@collabora.com>

+Danilo, since you worked on the 'bound lifetime stuff in rust, and I
feel this is related to the problem I'm trying to fix here.

On Thu, 13 Aug 2026 12:56:58 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> The current unplug logic is broken in multiple ways. This is an attempt
> at addressing the various problems found along the way (some were
> reported by Sashiko, others have been found while trying to address
> Sashiko's concerns).
> 
> Sending a new version even though v2 didn't receive any human review
> just to try and address the new stuff pointed out by Sashiko.

Just a note I forgot to add to my cover letter. I've already spent way
more time than I wanted on this, not just because Sashiko keeps finding
new issues at each of my attempt, but also because the whole idea of
pretending a device on a platform bus is unplugged and can't harm us is
doomed. This is not an hot-pluggable bus, and the device is still there,
so, unless we can be absolutely sure it's inactive (which a RESET can
provide, but RESETs are fallible) we just have two options:

1. prevent the device from going away until we managed to properly
   shutdown the GPU

2. make sure all resources the HW might have its hands on at the time
   the failure of RESET in the unplug path happened are leaked

Option 1 is no longer possible since platform_driver::remove() can't
return an error. That leaves options 2, which is basically what this
patchset is doing, but the whole idea of leaking resources when the
final RESET in the unplug path fails has various nasty implications,
like the fact we end up with dangling drm_device (drm_gpuvm retains a
ref, and each GPU mapping we kept alive in the gpuvm is what keeps the
gpuvm and the BOs alive). In practice, there should be no one
triggering operations on this drm_device, because all the user-facing
interfaces have been shutdown by drm_dev_unregister() (which is called
by drm_dev_unplug()), but as things stand now, this drm_device still
has access to module-specific vtables, and there's nothing retaining
the module either.

TLDR; this is all super fragile stuff, on the other hand the current
situation is probably even worse. so if anyone has any idea how to
handle this properly (or at least a bit better than we do), please let
me know. I know a lot of this stuff is currently being considered as
part of the drm-rust abstractions, so hopefully we have a long-term
solution for rust drivers, but I'd really like a short-term solution
for panthor that doesn't involve nasty tricks or overly complex
refactoring.

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v3:
> - Fix a race in the reset reschedule logic we added to
>   panthor_device_resume() (missing smp_mb__after_atomic())
> - Fix a VM leak when reset and suspend are racing with each other
> - Add missing drm_dev_enter/exit() sections
> - Insert the groups in the user_owned list even if the group creation
>   happens during a reset
> - Try to document why some of the issues pointed out by Sashiko are
>   either not real issues, or are expected (either fixed in a later
>   commits, or just expected behavior)
> - Fix a race between panthor_device_unplug() and vm_prep_for_cleanup()
>   (introduced in v2)
> - Link to v2: https://patch.msgid.link/20260811-panthor-unplug-fixes-v2-0-6b583e37f9ae@collabora.com
> 
> Changes in v2:
> - Fix UAFs caused by deferred cleanup works
> - Fix UAFs caused by open FDs closed after unplug
> - Fix deadlock when device_unplug() is called from the reset work
> - Make sure reset requests are not lost in the resume and post_reset
>   paths
> - Fix a deadlock in the suspend path
> - Fix a clk prepare_enable leak in the unplug path
> - Don't use a drmm_action to flush the cleanup queue (this could cause
>   UAFs)
> - Drop the now unused panthor_vm::unusable field
> - Keep track of user owned resources to prevent leaks and/or UAFs
> - Link to v1: https://patch.msgid.link/20260804-panthor-unplug-fixes-v1-0-abbbd2d41b13@collabora.com
> 
> ---
> Boris Brezillon (17):
>       drm/panthor: Disable reset work before unplug
>       drm/panthor: Further delay reset work enablement
>       drm/panthor: Make sure reset requests in the resume path are not lost
>       drm/panthor: Make sure reset requests in the post reset path are not lost
>       drm/panthor: Flush the cleanup_wq in the unplug path
>       drm/panthor: Drop unused vm argument passed to panthor_vm_prepare_sync_only_op_ctx()
>       drm/panthor: Move the debugfs initialization to panthor_device.c
>       drm/panthor: Split panthor_vm
>       drm/panthor: Add fine-grained restrictions on VMs
>       drm/panthor: Check AS state before disabling
>       drm/panthor: Don't pre-allocate VMAs or page tables when preparing a full VM unmap
>       drm/panthor: Make the VM cleanup path more robust against UAF
>       drm/panthor: Track user owned VMs
>       drm/panthor: Track user owned groups
>       drm/panthor: Fix the unplug logic
>       drm/panthor: Add a debugfs knob to simulate unplug failures
>       drm/panthor: Add a debugfs knobs to simulate reset failures
> 
>  drivers/gpu/drm/panthor/panthor_device.c |  189 +++-
>  drivers/gpu/drm/panthor/panthor_device.h |   38 +
>  drivers/gpu/drm/panthor/panthor_drv.c    |  132 ++-
>  drivers/gpu/drm/panthor/panthor_fw.c     |    9 +-
>  drivers/gpu/drm/panthor/panthor_mmu.c    | 1493 ++++++++++++++++++------------
>  drivers/gpu/drm/panthor/panthor_mmu.h    |    4 +-
>  drivers/gpu/drm/panthor/panthor_sched.c  |  132 ++-
>  7 files changed, 1344 insertions(+), 653 deletions(-)
> ---
> base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
> change-id: 20260804-panthor-unplug-fixes-7927b3ddc2f9
> 
> Best regards,
> --  
> Boris Brezillon <boris.brezillon@collabora.com>
>

On Thu Aug 13, 2026 at 1:23 PM CEST, Boris Brezillon wrote:
> Just a note I forgot to add to my cover letter. I've already spent way
> more time than I wanted on this, not just because Sashiko keeps finding
> new issues at each of my attempt, but also because the whole idea of
> pretending a device on a platform bus is unplugged and can't harm us is
> doomed. This is not an hot-pluggable bus, and the device is still there,
> so, unless we can be absolutely sure it's inactive (which a RESET can
> provide, but RESETs are fallible) we just have two options:

I probably need a bit more context about which exact problem(s) you are trying
to solve.

> 1. prevent the device from going away until we managed to properly
>    shutdown the GPU

I'm not exactly sure what you mean with "device going away". If you mean
"prevent the device from being unbound from the driver" this is essentially what
you do by waiting for the completion of some HW teardown operation in remove().

In general, the implementation of remove() should ensure that on the one hand
the device it torn down (or reset), so it does not mess with system resources
anymore (e.g. attempt to do any DMA transfers) and behaves correctly on a
subsequent probe of this or another driver.

And on the other hand, the driver must release all device assoicated resources,
such as DMA mappings, IRQs, I/O memory mappings, etc. and it should also ensure
that no more driver code is reachable from any asynchronous paths, such as
workqueues, IOCTLs, timers, etc.

The latter obviously also depends on the subsystem and whether the lifetime of
userspace structurs and their associated driver private data is cleanly
decoupled (e.g. struct drm_file and ->driver_priv).

Since you also mention hot-unplug; those rules are universial regardless of
whether remove is triggered by a hot-unplug event or because the driver is
unbound for a different reason. The DRM API is a bit misleading about this,
because with drm_dev_unregister() there is no way to prevent DRM IOCTLs from
running after remove(), which wrongly suggests that this is not a potential
issue.

Not summarizing this because I think you are not aware already, but it may
provide a good entry point for you to point out where exactly things are getting
tricky.

> 2. make sure all resources the HW might have its hands on at the time
>    the failure of RESET in the unplug path happened are leaked

I'm not sure what you mean by this. But it suggests that the problem you try to
deal with is a misbehaving device that fails to reset?

Also, what do you mean with leaking the device resources?

On Thu, 13 Aug 2026 20:50:22 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Thu Aug 13, 2026 at 1:23 PM CEST, Boris Brezillon wrote:
> > Just a note I forgot to add to my cover letter. I've already spent way
> > more time than I wanted on this, not just because Sashiko keeps finding
> > new issues at each of my attempt, but also because the whole idea of
> > pretending a device on a platform bus is unplugged and can't harm us is
> > doomed. This is not an hot-pluggable bus, and the device is still there,
> > so, unless we can be absolutely sure it's inactive (which a RESET can
> > provide, but RESETs are fallible) we just have two options:  
> 
> I probably need a bit more context about which exact problem(s) you are trying
> to solve.

Sorry. You can find more context in patch 15.

> 
> > 1. prevent the device from going away until we managed to properly
> >    shutdown the GPU  
> 
> I'm not exactly sure what you mean with "device going away". If you mean
> "prevent the device from being unbound from the driver" this is essentially what
> you do by waiting for the completion of some HW teardown operation in remove().
> 
> In general, the implementation of remove() should ensure that on the one hand
> the device it torn down (or reset), so it does not mess with system resources
> anymore (e.g. attempt to do any DMA transfers) and behaves correctly on a
> subsequent probe of this or another driver.

So, that's the ideal situation, were a SOFT_RESET works. But because
SOFT_RESET is a GPU command that has to be acknowledged by the GPU, you
have no guarantee that this reset actually worked.

> 
> And on the other hand, the driver must release all device assoicated resources,
> such as DMA mappings, IRQs, I/O memory mappings, etc. and it should also ensure
> that no more driver code is reachable from any asynchronous paths, such as
> workqueues, IOCTLs, timers, etc.

Yep, we also take care of that in the nominal case (AKA RESET worked,
and we know the HW is off).

> 
> The latter obviously also depends on the subsystem and whether the lifetime of
> userspace structurs and their associated driver private data is cleanly
> decoupled (e.g. struct drm_file and ->driver_priv).
> 
> Since you also mention hot-unplug; those rules are universial regardless of
> whether remove is triggered by a hot-unplug event or because the driver is
> unbound for a different reason.

The API doesn't change, but the implication of such a removal do
change: on an hot-pluggable bus, the device is physically gone, so it
can't do any harm. On a platform bus, the device is there, and it might
remain clocked and powered even after the platform_device has been
unbound, because power domains and clks can be shared across devices,
so even the clk_disable_unprepare() & co we have in the remove path
won't guarantee that the GPU is inactive.

> The DRM API is a bit misleading about this,
> because with drm_dev_unregister() there is no way to prevent DRM IOCTLs from
> running after remove(), which wrongly suggests that this is not a potential
> issue.
> 
> Not summarizing this because I think you are not aware already, but it may
> provide a good entry point for you to point out where exactly things are getting
> tricky.

Things get tricky when we diverge from the nominal case: SOFT_RESET
didn't work, and we're either stuck in an infinite RESET loop waiting
for it to eventually work, or we just take the hit and leak any
resource the HW had access to at the time the RESET command was issued,
because we can't know for sure that the GPU is in such a bad state it
can't access memory anymore. All we know is that it's in a bad enough
state to no longer acknowledge RESET requests.

> 
> > 2. make sure all resources the HW might have its hands on at the time
> >    the failure of RESET in the unplug path happened are leaked  
> 
> I'm not sure what you mean by this. But it suggests that the problem you try to
> deal with is a misbehaving device that fails to reset?

Yes, this.

> 
> Also, what do you mean with leaking the device resources?

I mean leaking all the memory that the GPU had access to (page tables
and memory pointed by those page tables), so that it's never returned
to the system with a risk of UAF.

On Thu, Aug 13, 2026 at 01:23:20PM +0200, Boris Brezillon wrote:
> +Danilo, since you worked on the 'bound lifetime stuff in rust, and I
> feel this is related to the problem I'm trying to fix here.
> 
> On Thu, 13 Aug 2026 12:56:58 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > The current unplug logic is broken in multiple ways. This is an attempt
> > at addressing the various problems found along the way (some were
> > reported by Sashiko, others have been found while trying to address
> > Sashiko's concerns).
> > 
> > Sending a new version even though v2 didn't receive any human review
> > just to try and address the new stuff pointed out by Sashiko.

Sorry, I was on holiday at the beginning of the week, back today.

> 
> Just a note I forgot to add to my cover letter. I've already spent way
> more time than I wanted on this, not just because Sashiko keeps finding
> new issues at each of my attempt, but also because the whole idea of
> pretending a device on a platform bus is unplugged and can't harm us is
> doomed. This is not an hot-pluggable bus, and the device is still there,
> so, unless we can be absolutely sure it's inactive (which a RESET can
> provide, but RESETs are fallible) we just have two options:
> 
> 1. prevent the device from going away until we managed to properly
>    shutdown the GPU

That's going to be event harder with the upcoming HW where the GPU
slice can be made inaccessible by an arbiter.

> 
> 2. make sure all resources the HW might have its hands on at the time
>    the failure of RESET in the unplug path happened are leaked

There is another option which is to make sure that the HW can only
access the dummy pages. We're still in control of the MMU and the page
tables, once we update those and flush them we should be safe in the
knowledge that the HW cannot access live resources.


> 
> Option 1 is no longer possible since platform_driver::remove() can't
> return an error. That leaves options 2, which is basically what this
> patchset is doing, but the whole idea of leaking resources when the
> final RESET in the unplug path fails has various nasty implications,
> like the fact we end up with dangling drm_device (drm_gpuvm retains a
> ref, and each GPU mapping we kept alive in the gpuvm is what keeps the
> gpuvm and the BOs alive). In practice, there should be no one
> triggering operations on this drm_device, because all the user-facing
> interfaces have been shutdown by drm_dev_unregister() (which is called
> by drm_dev_unplug()), but as things stand now, this drm_device still
> has access to module-specific vtables, and there's nothing retaining
> the module either.
> 
> TLDR; this is all super fragile stuff, on the other hand the current
> situation is probably even worse. so if anyone has any idea how to
> handle this properly (or at least a bit better than we do), please let
> me know. I know a lot of this stuff is currently being considered as
> part of the drm-rust abstractions, so hopefully we have a long-term
> solution for rust drivers, but I'd really like a short-term solution
> for panthor that doesn't involve nasty tricks or overly complex
> refactoring.

I think some of the pain we're suffering comes from the overlap (that
you've tried to address in this series) between the resources that
are visible to the HW and the ones that are visible to user space. The
split of AS and VM is the right thing to do.

My proposal for handling the unplugging would be to have race as quick
as possible to the MMU unplug and then free up all BOs and VMs that
were allocated at the request of user space, then go back and free
the kernel BOs. Then hopefully we should be in a position where there
are no GPU mappings and we can unplug the drm_gpuvm.

Best regards,
Liviu

> 
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Changes in v3:
> > - Fix a race in the reset reschedule logic we added to
> >   panthor_device_resume() (missing smp_mb__after_atomic())
> > - Fix a VM leak when reset and suspend are racing with each other
> > - Add missing drm_dev_enter/exit() sections
> > - Insert the groups in the user_owned list even if the group creation
> >   happens during a reset
> > - Try to document why some of the issues pointed out by Sashiko are
> >   either not real issues, or are expected (either fixed in a later
> >   commits, or just expected behavior)
> > - Fix a race between panthor_device_unplug() and vm_prep_for_cleanup()
> >   (introduced in v2)
> > - Link to v2: https://patch.msgid.link/20260811-panthor-unplug-fixes-v2-0-6b583e37f9ae@collabora.com
> > 
> > Changes in v2:
> > - Fix UAFs caused by deferred cleanup works
> > - Fix UAFs caused by open FDs closed after unplug
> > - Fix deadlock when device_unplug() is called from the reset work
> > - Make sure reset requests are not lost in the resume and post_reset
> >   paths
> > - Fix a deadlock in the suspend path
> > - Fix a clk prepare_enable leak in the unplug path
> > - Don't use a drmm_action to flush the cleanup queue (this could cause
> >   UAFs)
> > - Drop the now unused panthor_vm::unusable field
> > - Keep track of user owned resources to prevent leaks and/or UAFs
> > - Link to v1: https://patch.msgid.link/20260804-panthor-unplug-fixes-v1-0-abbbd2d41b13@collabora.com
> > 
> > ---
> > Boris Brezillon (17):
> >       drm/panthor: Disable reset work before unplug
> >       drm/panthor: Further delay reset work enablement
> >       drm/panthor: Make sure reset requests in the resume path are not lost
> >       drm/panthor: Make sure reset requests in the post reset path are not lost
> >       drm/panthor: Flush the cleanup_wq in the unplug path
> >       drm/panthor: Drop unused vm argument passed to panthor_vm_prepare_sync_only_op_ctx()
> >       drm/panthor: Move the debugfs initialization to panthor_device.c
> >       drm/panthor: Split panthor_vm
> >       drm/panthor: Add fine-grained restrictions on VMs
> >       drm/panthor: Check AS state before disabling
> >       drm/panthor: Don't pre-allocate VMAs or page tables when preparing a full VM unmap
> >       drm/panthor: Make the VM cleanup path more robust against UAF
> >       drm/panthor: Track user owned VMs
> >       drm/panthor: Track user owned groups
> >       drm/panthor: Fix the unplug logic
> >       drm/panthor: Add a debugfs knob to simulate unplug failures
> >       drm/panthor: Add a debugfs knobs to simulate reset failures
> > 
> >  drivers/gpu/drm/panthor/panthor_device.c |  189 +++-
> >  drivers/gpu/drm/panthor/panthor_device.h |   38 +
> >  drivers/gpu/drm/panthor/panthor_drv.c    |  132 ++-
> >  drivers/gpu/drm/panthor/panthor_fw.c     |    9 +-
> >  drivers/gpu/drm/panthor/panthor_mmu.c    | 1493 ++++++++++++++++++------------
> >  drivers/gpu/drm/panthor/panthor_mmu.h    |    4 +-
> >  drivers/gpu/drm/panthor/panthor_sched.c  |  132 ++-
> >  7 files changed, 1344 insertions(+), 653 deletions(-)
> > ---
> > base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
> > change-id: 20260804-panthor-unplug-fixes-7927b3ddc2f9
> > 
> > Best regards,
> > --  
> > Boris Brezillon <boris.brezillon@collabora.com>
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

On Thu, 13 Aug 2026 16:11:37 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Thu, Aug 13, 2026 at 01:23:20PM +0200, Boris Brezillon wrote:
> > +Danilo, since you worked on the 'bound lifetime stuff in rust, and I
> > feel this is related to the problem I'm trying to fix here.
> > 
> > On Thu, 13 Aug 2026 12:56:58 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > The current unplug logic is broken in multiple ways. This is an attempt
> > > at addressing the various problems found along the way (some were
> > > reported by Sashiko, others have been found while trying to address
> > > Sashiko's concerns).
> > > 
> > > Sending a new version even though v2 didn't receive any human review
> > > just to try and address the new stuff pointed out by Sashiko.  
> 
> Sorry, I was on holiday at the beginning of the week, back today.
> 
> > 
> > Just a note I forgot to add to my cover letter. I've already spent way
> > more time than I wanted on this, not just because Sashiko keeps finding
> > new issues at each of my attempt, but also because the whole idea of
> > pretending a device on a platform bus is unplugged and can't harm us is
> > doomed. This is not an hot-pluggable bus, and the device is still there,
> > so, unless we can be absolutely sure it's inactive (which a RESET can
> > provide, but RESETs are fallible) we just have two options:
> > 
> > 1. prevent the device from going away until we managed to properly
> >    shutdown the GPU  
> 
> That's going to be event harder with the upcoming HW where the GPU
> slice can be made inaccessible by an arbiter.
> 
> > 
> > 2. make sure all resources the HW might have its hands on at the time
> >    the failure of RESET in the unplug path happened are leaked  
> 
> There is another option which is to make sure that the HW can only
> access the dummy pages. We're still in control of the MMU and the page
> tables, once we update those and flush them we should be safe in the
> knowledge that the HW cannot access live resources.

That's more for an "active device" situation though. Active as in,
device is probed and ready to accept user requests, even if it might be
temporarily inaccessible because of RESETs (or access-window loss
on new gens).

The thing I'm trying to fix here is the unplug logic: device is going
away, we just need to make sure it's either

- off

or

- the resources it had access to are leaked

or

- we prevent the removal until we're sure it's off (retry the SOFT_RESET
  indefinitely?)

> 
> 
> > 
> > Option 1 is no longer possible since platform_driver::remove() can't
> > return an error. That leaves options 2, which is basically what this
> > patchset is doing, but the whole idea of leaking resources when the
> > final RESET in the unplug path fails has various nasty implications,
> > like the fact we end up with dangling drm_device (drm_gpuvm retains a
> > ref, and each GPU mapping we kept alive in the gpuvm is what keeps the
> > gpuvm and the BOs alive). In practice, there should be no one
> > triggering operations on this drm_device, because all the user-facing
> > interfaces have been shutdown by drm_dev_unregister() (which is called
> > by drm_dev_unplug()), but as things stand now, this drm_device still
> > has access to module-specific vtables, and there's nothing retaining
> > the module either.
> > 
> > TLDR; this is all super fragile stuff, on the other hand the current
> > situation is probably even worse. so if anyone has any idea how to
> > handle this properly (or at least a bit better than we do), please let
> > me know. I know a lot of this stuff is currently being considered as
> > part of the drm-rust abstractions, so hopefully we have a long-term
> > solution for rust drivers, but I'd really like a short-term solution
> > for panthor that doesn't involve nasty tricks or overly complex
> > refactoring.  
> 
> I think some of the pain we're suffering comes from the overlap (that
> you've tried to address in this series) between the resources that
> are visible to the HW and the ones that are visible to user space. The
> split of AS and VM is the right thing to do.

Yeah, that definitely makes things harder to disconnect when the device
goes away. But even with this split, there's still the problem that the
"unplug" we have is not HW based (unlike a PCI bus), so the HW still has
access to the memory we shared with it (for its MMU page table, and the
pages those point to).

> 
> My proposal for handling the unplugging would be to have race as quick
> as possible to the MMU unplug and then free up all BOs and VMs that
> were allocated at the request of user space, then go back and free
> the kernel BOs. Then hopefully we should be in a position where there
> are no GPU mappings and we can unplug the drm_gpuvm.

I mean, that's basically what this patchset is doing. To be accurate,
what the unplug logic does at the end of this patchset is:

1. RESET the GPU, so the HW is inactive => basically faking a real
   unplug on an hot-pluggable bus
2. unplug each component, and make sure the unplug logic doesn't
   interact with the HW. It just acts as a janitor releasing all the
   objects that were left behind at the moment the unplug happens. The
   only thing left are the user-facing objects (panthor_file) so that
   DRM FDs can be closed after the unplug, but all other operations
   IOCTLs fail with ENODEV. panthor_device also stays around a bit
   longer, but it's mostly here to keep the drm_device around until the
   last ref is dropped
3. if and only if the RESET failed in step 1, the MMU unplug logic leaks
   the GPU mappings of the resident AS instead of releasing them. This
   leak retains the gpuvm which retains the drm_device/panthor_device

Step 3 is only here to cover for failures in step 1 (in a normal
situation, there's no leak and everything is released as expected),
and that's the problematic part. I don't mind refactor the code to
isolate objects containing HW resource from the user-facing objects,
but that won't solve the fact that, on a RESET failure, we either leak
memory, or we expose ourselves to HW UAFs. If you tell me HARD_RESET is
not fallible and is safe, I can go for that. But last I looked, I've
read that it could leave the memory bus in a bad state, with the risk of
impacting the rest of the system.

On Thu, Aug 13, 2026 at 05:55:22PM +0200, Boris Brezillon wrote:
> On Thu, 13 Aug 2026 16:11:37 +0100
> Liviu Dudau <liviu.dudau@arm.com> wrote:
> 
> > On Thu, Aug 13, 2026 at 01:23:20PM +0200, Boris Brezillon wrote:
> > > +Danilo, since you worked on the 'bound lifetime stuff in rust, and I
> > > feel this is related to the problem I'm trying to fix here.
> > > 
> > > On Thu, 13 Aug 2026 12:56:58 +0200
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > >   
> > > > The current unplug logic is broken in multiple ways. This is an attempt
> > > > at addressing the various problems found along the way (some were
> > > > reported by Sashiko, others have been found while trying to address
> > > > Sashiko's concerns).
> > > > 
> > > > Sending a new version even though v2 didn't receive any human review
> > > > just to try and address the new stuff pointed out by Sashiko.  
> > 
> > Sorry, I was on holiday at the beginning of the week, back today.
> > 
> > > 
> > > Just a note I forgot to add to my cover letter. I've already spent way
> > > more time than I wanted on this, not just because Sashiko keeps finding
> > > new issues at each of my attempt, but also because the whole idea of
> > > pretending a device on a platform bus is unplugged and can't harm us is
> > > doomed. This is not an hot-pluggable bus, and the device is still there,
> > > so, unless we can be absolutely sure it's inactive (which a RESET can
> > > provide, but RESETs are fallible) we just have two options:
> > > 
> > > 1. prevent the device from going away until we managed to properly
> > >    shutdown the GPU  
> > 
> > That's going to be event harder with the upcoming HW where the GPU
> > slice can be made inaccessible by an arbiter.
> > 
> > > 
> > > 2. make sure all resources the HW might have its hands on at the time
> > >    the failure of RESET in the unplug path happened are leaked  
> > 
> > There is another option which is to make sure that the HW can only
> > access the dummy pages. We're still in control of the MMU and the page
> > tables, once we update those and flush them we should be safe in the
> > knowledge that the HW cannot access live resources.
> 
> That's more for an "active device" situation though. Active as in,
> device is probed and ready to accept user requests, even if it might be
> temporarily inaccessible because of RESETs (or access-window loss
> on new gens).
> 
> The thing I'm trying to fix here is the unplug logic: device is going
> away, we just need to make sure it's either
> 
> - off
> 
> or
> 
> - the resources it had access to are leaked
> 
> or
> 
> - we prevent the removal until we're sure it's off (retry the SOFT_RESET
>   indefinitely?)
> 
> > 
> > 
> > > 
> > > Option 1 is no longer possible since platform_driver::remove() can't
> > > return an error. That leaves options 2, which is basically what this
> > > patchset is doing, but the whole idea of leaking resources when the
> > > final RESET in the unplug path fails has various nasty implications,
> > > like the fact we end up with dangling drm_device (drm_gpuvm retains a
> > > ref, and each GPU mapping we kept alive in the gpuvm is what keeps the
> > > gpuvm and the BOs alive). In practice, there should be no one
> > > triggering operations on this drm_device, because all the user-facing
> > > interfaces have been shutdown by drm_dev_unregister() (which is called
> > > by drm_dev_unplug()), but as things stand now, this drm_device still
> > > has access to module-specific vtables, and there's nothing retaining
> > > the module either.
> > > 
> > > TLDR; this is all super fragile stuff, on the other hand the current
> > > situation is probably even worse. so if anyone has any idea how to
> > > handle this properly (or at least a bit better than we do), please let
> > > me know. I know a lot of this stuff is currently being considered as
> > > part of the drm-rust abstractions, so hopefully we have a long-term
> > > solution for rust drivers, but I'd really like a short-term solution
> > > for panthor that doesn't involve nasty tricks or overly complex
> > > refactoring.  
> > 
> > I think some of the pain we're suffering comes from the overlap (that
> > you've tried to address in this series) between the resources that
> > are visible to the HW and the ones that are visible to user space. The
> > split of AS and VM is the right thing to do.
> 
> Yeah, that definitely makes things harder to disconnect when the device
> goes away. But even with this split, there's still the problem that the
> "unplug" we have is not HW based (unlike a PCI bus), so the HW still has
> access to the memory we shared with it (for its MMU page table, and the
> pages those point to).

There is no copy of the MMU page tables that the FW or the hardware own.
Panthor is in charge of the page tables and it can force change them if it
wants to be sure that HW doesn't access memory we don't want to. If it
does, HW will get a bus access violation and halt.

> 
> > 
> > My proposal for handling the unplugging would be to have race as quick
> > as possible to the MMU unplug and then free up all BOs and VMs that
> > were allocated at the request of user space, then go back and free
> > the kernel BOs. Then hopefully we should be in a position where there
> > are no GPU mappings and we can unplug the drm_gpuvm.
> 
> I mean, that's basically what this patchset is doing. To be accurate,
> what the unplug logic does at the end of this patchset is:
> 
> 1. RESET the GPU, so the HW is inactive => basically faking a real
>    unplug on an hot-pluggable bus
> 2. unplug each component, and make sure the unplug logic doesn't
>    interact with the HW. It just acts as a janitor releasing all the
>    objects that were left behind at the moment the unplug happens. The
>    only thing left are the user-facing objects (panthor_file) so that
>    DRM FDs can be closed after the unplug, but all other operations
>    IOCTLs fail with ENODEV. panthor_device also stays around a bit
>    longer, but it's mostly here to keep the drm_device around until the
>    last ref is dropped
> 3. if and only if the RESET failed in step 1, the MMU unplug logic leaks
>    the GPU mappings of the resident AS instead of releasing them. This
>    leak retains the gpuvm which retains the drm_device/panthor_device
> 
> Step 3 is only here to cover for failures in step 1 (in a normal
> situation, there's no leak and everything is released as expected),
> and that's the problematic part. I don't mind refactor the code to
> isolate objects containing HW resource from the user-facing objects,
> but that won't solve the fact that, on a RESET failure, we either leak
> memory, or we expose ourselves to HW UAFs. If you tell me HARD_RESET is
> not fallible and is safe, I can go for that. But last I looked, I've
> read that it could leave the memory bus in a bad state, with the risk of
> impacting the rest of the system.

My suggestion would be to do step 1, 3 and then 2. But on step 3 I would
not leak the GPU mappings, but replace them with the dummy pages and release
the resident AS.

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

On Thu, 13 Aug 2026 18:06:13 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Thu, Aug 13, 2026 at 05:55:22PM +0200, Boris Brezillon wrote:
> > On Thu, 13 Aug 2026 16:11:37 +0100
> > Liviu Dudau <liviu.dudau@arm.com> wrote:
> >   
> > > On Thu, Aug 13, 2026 at 01:23:20PM +0200, Boris Brezillon wrote:  
> > > > +Danilo, since you worked on the 'bound lifetime stuff in rust, and I
> > > > feel this is related to the problem I'm trying to fix here.
> > > > 
> > > > On Thu, 13 Aug 2026 12:56:58 +0200
> > > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > > >     
> > > > > The current unplug logic is broken in multiple ways. This is an attempt
> > > > > at addressing the various problems found along the way (some were
> > > > > reported by Sashiko, others have been found while trying to address
> > > > > Sashiko's concerns).
> > > > > 
> > > > > Sending a new version even though v2 didn't receive any human review
> > > > > just to try and address the new stuff pointed out by Sashiko.    
> > > 
> > > Sorry, I was on holiday at the beginning of the week, back today.
> > >   
> > > > 
> > > > Just a note I forgot to add to my cover letter. I've already spent way
> > > > more time than I wanted on this, not just because Sashiko keeps finding
> > > > new issues at each of my attempt, but also because the whole idea of
> > > > pretending a device on a platform bus is unplugged and can't harm us is
> > > > doomed. This is not an hot-pluggable bus, and the device is still there,
> > > > so, unless we can be absolutely sure it's inactive (which a RESET can
> > > > provide, but RESETs are fallible) we just have two options:
> > > > 
> > > > 1. prevent the device from going away until we managed to properly
> > > >    shutdown the GPU    
> > > 
> > > That's going to be event harder with the upcoming HW where the GPU
> > > slice can be made inaccessible by an arbiter.
> > >   
> > > > 
> > > > 2. make sure all resources the HW might have its hands on at the time
> > > >    the failure of RESET in the unplug path happened are leaked    
> > > 
> > > There is another option which is to make sure that the HW can only
> > > access the dummy pages. We're still in control of the MMU and the page
> > > tables, once we update those and flush them we should be safe in the
> > > knowledge that the HW cannot access live resources.  
> > 
> > That's more for an "active device" situation though. Active as in,
> > device is probed and ready to accept user requests, even if it might be
> > temporarily inaccessible because of RESETs (or access-window loss
> > on new gens).
> > 
> > The thing I'm trying to fix here is the unplug logic: device is going
> > away, we just need to make sure it's either
> > 
> > - off
> > 
> > or
> > 
> > - the resources it had access to are leaked
> > 
> > or
> > 
> > - we prevent the removal until we're sure it's off (retry the SOFT_RESET
> >   indefinitely?)
> >   
> > > 
> > >   
> > > > 
> > > > Option 1 is no longer possible since platform_driver::remove() can't
> > > > return an error. That leaves options 2, which is basically what this
> > > > patchset is doing, but the whole idea of leaking resources when the
> > > > final RESET in the unplug path fails has various nasty implications,
> > > > like the fact we end up with dangling drm_device (drm_gpuvm retains a
> > > > ref, and each GPU mapping we kept alive in the gpuvm is what keeps the
> > > > gpuvm and the BOs alive). In practice, there should be no one
> > > > triggering operations on this drm_device, because all the user-facing
> > > > interfaces have been shutdown by drm_dev_unregister() (which is called
> > > > by drm_dev_unplug()), but as things stand now, this drm_device still
> > > > has access to module-specific vtables, and there's nothing retaining
> > > > the module either.
> > > > 
> > > > TLDR; this is all super fragile stuff, on the other hand the current
> > > > situation is probably even worse. so if anyone has any idea how to
> > > > handle this properly (or at least a bit better than we do), please let
> > > > me know. I know a lot of this stuff is currently being considered as
> > > > part of the drm-rust abstractions, so hopefully we have a long-term
> > > > solution for rust drivers, but I'd really like a short-term solution
> > > > for panthor that doesn't involve nasty tricks or overly complex
> > > > refactoring.    
> > > 
> > > I think some of the pain we're suffering comes from the overlap (that
> > > you've tried to address in this series) between the resources that
> > > are visible to the HW and the ones that are visible to user space. The
> > > split of AS and VM is the right thing to do.  
> > 
> > Yeah, that definitely makes things harder to disconnect when the device
> > goes away. But even with this split, there's still the problem that the
> > "unplug" we have is not HW based (unlike a PCI bus), so the HW still has
> > access to the memory we shared with it (for its MMU page table, and the
> > pages those point to).  
> 
> There is no copy of the MMU page tables that the FW or the hardware own.
> Panthor is in charge of the page tables and it can force change them if it
> wants to be sure that HW doesn't access memory we don't want to. If it
> does, HW will get a bus access violation and halt.

Nope, that's not true: AS commands can fail. When that happens, we keep
the page-table resident until the next RESET, which tells us it's safe
to assume the HW doesn't have access to this page table. But this relies
on the assumption the SOFT_RESET command is not fallible.

In the unplug path, it's pretty much the same, except it's the end of
the road, so there's no chance for us to keep the AS (and its page
table) alive and accessible if the reset fails. We either leak it, or
we release everything and expose ourselves to HW UAFs.

> 
> >   
> > > 
> > > My proposal for handling the unplugging would be to have race as quick
> > > as possible to the MMU unplug and then free up all BOs and VMs that
> > > were allocated at the request of user space, then go back and free
> > > the kernel BOs. Then hopefully we should be in a position where there
> > > are no GPU mappings and we can unplug the drm_gpuvm.  
> > 
> > I mean, that's basically what this patchset is doing. To be accurate,
> > what the unplug logic does at the end of this patchset is:
> > 
> > 1. RESET the GPU, so the HW is inactive => basically faking a real
> >    unplug on an hot-pluggable bus
> > 2. unplug each component, and make sure the unplug logic doesn't
> >    interact with the HW. It just acts as a janitor releasing all the
> >    objects that were left behind at the moment the unplug happens. The
> >    only thing left are the user-facing objects (panthor_file) so that
> >    DRM FDs can be closed after the unplug, but all other operations
> >    IOCTLs fail with ENODEV. panthor_device also stays around a bit
> >    longer, but it's mostly here to keep the drm_device around until the
> >    last ref is dropped
> > 3. if and only if the RESET failed in step 1, the MMU unplug logic leaks
> >    the GPU mappings of the resident AS instead of releasing them. This
> >    leak retains the gpuvm which retains the drm_device/panthor_device
> > 
> > Step 3 is only here to cover for failures in step 1 (in a normal
> > situation, there's no leak and everything is released as expected),
> > and that's the problematic part. I don't mind refactor the code to
> > isolate objects containing HW resource from the user-facing objects,
> > but that won't solve the fact that, on a RESET failure, we either leak
> > memory, or we expose ourselves to HW UAFs. If you tell me HARD_RESET is
> > not fallible and is safe, I can go for that. But last I looked, I've
> > read that it could leave the memory bus in a bad state, with the risk of
> > impacting the rest of the system.  
> 
> My suggestion would be to do step 1, 3 and then 2. But on step 3 I would
> not leak the GPU mappings, but replace them with the dummy pages and release
> the resident AS.

That's not possible: if you can't interact with the MMU to tell it to
flush its TLB, you can't just setup the dummy page in the page table,
because the TLB might have entries pointing to memory that was returned
to the system. Again, HW unresponsive != HW not doing mem access using
the old mappings. Also, 3 is actually a sub-case of 2 in the mmu_unplug
logic, it's not really a separate step.