Re: [PATCH v3 0/3] drm, drm/xe: Protect against premature module unloads

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Mon, 2026-09-28 at 15:03 +0200, Danilo Krummrich wrote:
> On Mon Sep 28, 2026 at 2:13 PM CEST, Thomas Hellström wrote:
> > On Mon, 2026-09-28 at 12:13 +0200, Danilo Krummrich wrote:
> > > On Mon Sep 28, 2026 at 10:46 AM CEST, Thomas Hellström wrote:
> > > > On Fri, 2026-09-25 at 18:18 +0200, Danilo Krummrich wrote:
> > > > > On Fri Sep 25, 2026 at 3:33 PM CEST, Thomas Hellström wrote:
> > > > > > Driver and shared DRM helper code is increasingly relying
> > > > > > on
> > > > > > bare
> > > > > > drm_device references (drm_dev_get()/drm_dev_put()) to keep
> > > > > > a
> > > > > > device's
> > > > > > software state around, without also pairing that with a
> > > > > > reference
> > > > > > on
> > > > > > the owning kernel module. Xe itself does this in several
> > > > > > places,
> > > > > > and
> > > > > > so does drm_gpuvm for the lifetime of a GPU VM. None of
> > > > > > these
> > > > > > references currently prevent the owning module from being
> > > > > > unloaded
> > > > > > while they, or the teardown work they can still trigger,
> > > > > > are
> > > > > > outstanding, meaning driver code can end up executing after
> > > > > > its
> > > > > > own
> > > > > > module's text has already been freed.
> > > > > 
> > > > > Since you mention DRM GPUVM in a couple of places, how can
> > > > > this
> > > > > ever
> > > > > happen? It
> > > > > wouldn't make sense to keep a VM alive beyond driver unbind.
> > > > > I.e.
> > > > > it
> > > > > can't make
> > > > > its drm_device reference count reach module unload in the
> > > > > first
> > > > > place.
> > > > 
> > > > There seems to be a bit of misunderstanding here.
> > > > 
> > > > Driver unbind removes the struct device from the driver,
> > > > triggers
> > > > device unplug, and eventually the devres release actions. IIRC
> > > > the
> > > > last
> > > > devres action removes a *single reference* on the struct
> > > > drm_device.
> > > 
> > > Correct.
> > > 
> > > > Hence if there exists other reference holders on the struct
> > > > drm_device
> > > > (open files, exported dma-bufs, exported drm_pagemaps as an
> > > > example),
> > > > the drm_device will survive the driver unbind. So will open
> > > > files
> > > > and
> > > > thus drm_gpuvms until user-space decides to remove them.
> > > 
> > > There's two lifetimes we have to deal with in drivers: the
> > > lifetime
> > > of (bus /
> > > physical) device resources, which are managed by the driver and
> > > the
> > > software
> > > state that is represented through the class device to userspace
> > > (e.g.
> > > file
> > > handles).
> > > 
> > > The former is bounded to the scope where the driver is bound to
> > > the
> > > device and
> > > the latter is unbounded and indeed depends on userspace.
> > > 
> > > Either the subsystem or the driver has to decouple those
> > > lifetimes.
> > > I.e. if the
> > > driver is unbound it should clean up all GPUVMs as they represent
> > > the
> > > GPU's
> > > virtual address space and hence are associated with the hardware.
> > > However, the
> > > driver should not operated the hardware anymore after driver
> > > unbind.
> > 
> > I disagree here. At unbind time we decouple the HW and SW state,
> > The
> > device no longer uses it's pointers to the page-table so, for
> > example
> > VRAM page-tables can be torn down, system page-tables lose their
> > dma-
> > mappings, but in xe we don't tear down the page-table structure
> > itself.
> > 
> > If HW accesses are properly protected by drm_dev_enter() /
> > drm_dev_exit(), Hw won't be accessed after unbind.
> > 
> > 
> > > 
> > > So, in your case it seems that file lifetime and VM lifetime are
> > > conflated
> > > although they should be separate.
> > 
> > I view the VM as software state, page-table pointers, dma-mappings
> > and
> > VRAM storage as HW state.
> > 
> > It seems like what we're not agreeing on is where to separate
> > those. I
> > see no reason as to why we would complicate the driver to remove
> > more
> > than necessary at unbind time?
> 
> This can certainly be done, correct. But, the VM itself represents a
> GPU's
> virtual address space and takes ownership of the corresponding
> hardware
> resources.
> 
> We can indeed revoke the hardware resources from the VM
> implementation and
> leave it in place. But this messes with the ownership model within
> the VM
> implementation:
> 
> Because now, and you say this a couple of times below, we need to
> guard all
> relevant entry points into the VM code with guards, such as
> drm_dev_{enter/exit}().
> 
> IOW, it creates partially uninitialized structures with stale
> pointers that we
> now have to guard against.
> 
> It makes much more sense to tear down everything that owns device
> resources on
> driver unbind. I.e. why keep structures with stale pointers around
> that we have
> to guard against in the first place?
> 
> It also gets us rid of the module unload issue as it allows us to
> prevent
> callbacks into the driver code after driver unbind on the subsystem
> level.
> 
> > > We can't have userspace to decide when we drop device resources,
> > > such
> > > as DMA
> > > mappings, I/O memory mappings, etc.
> > 
> > We don't (Unless we have bugs, and you may have stumbled on those
> > below?) Those should be removed at unbind time. We should also
> > revoke
> > dma-buf mappings and SVM migrates all dma-buf mappings to system.
> > 
> > > 
> > > > Files, dma-bufs and drm_pagemaps all hold a driver module
> > > > reference
> > > > until they have successfully released the drm_device. The
> > > > requirement
> > > > is "If a drm_device reference is held, a module reference of
> > > > the
> > > > driver
> > > > providing the drm_device must also be held, or if it's held by
> > > > the
> > > > driver itself, it must ensure at driver unload time that any
> > > > drm_device
> > > > references it holds are released and drmm release callbacks
> > > > have
> > > > finished executing."
> > > > 
> > > > What this series in effect does is to change this to to "The
> > > > driver
> > > > won't unload until all drm_device references are gone, and all
> > > > drmm
> > > > release callbacks have finished executing."
> > > > 
> > > > I agree that the use of drm_gpuvm in the documentation is a bit
> > > > unfair.
> > > > Since the code calling drm_dev_get() and drm_dev_put() is
> > > > intended
> > > > to
> > > > be called from the driver, the reference in effect becomes the
> > > > driver's
> > > > responsibility, but if someone would, in the future change that
> > > > so
> > > > that
> > > > those references are put from a worker from within the driver
> > > > or
> > > > even
> > > > within drm_gpuvm itself, things would break. If a future code
> > > > reviewer,
> > > > developer or AI agent knows about the new drm_device reference
> > > > guarantee, then that will lessen the review scope and code will
> > > > become
> > > > more rubost.
> > > > 
> > > > > 
> > > > > Besides that, can you please remind me whether there are any
> > > > > other
> > > > > reasons than
> > > > > the release() callback why a DRM device must not outlive
> > > > > module
> > > > > unload?
> > > > 
> > > > The drmm release callbacks.
> > > 
> > > Right, I forgot about them for a second. However, they are
> > > similar to
> > > the
> > > release() callbacks as in they are the wrong cleanup model for
> > > driver
> > > private
> > > structures.
> > > 
> > > drmm is a great tool for common subsystem structures that
> > > lifetime
> > > wise tie to
> > > the drm_device. But it is the wrong lifetime model for stuff that
> > > is
> > > used to
> > > operate the device, as this should be torn down on device unbind.
> > 
> > The current model used by xe (and amdgpu AFACT, that also ties vm
> > lifetime to file lifetime) is to block all hardware access and dma
> > at
> > unbind time. The rest is state that doesn't necessarily need to be
> > torn
> > down at unbind time. I believe the current separation is mostly
> > done
> > with drm_dev_enter() / drm_dev_exit() and why should we enforce a
> > change of that? I'd say the drivers should be free to release
> > what's
> > convenient.
> 
> See the reasons above, it is not a good layer for the lifetime
> decoupling.
> 
> > Also if drm_gpuvms are designed to not outlive the struct device,
> > why
> > do they need to take a struct drm_device reference in the first
> > place,
> > I mean I brought this problem up then and IIRC I think you argued
> > the
> > reference was needed and punted any problems it caused to the
> > drivers?
> 
> The lifetime of the hardware resources that are owned by a GPUVM
> implementation
> is restricted by the underlying bus device (e.g. PCI) being bound to
> the driver.
> 
> The lifetime of the DRM device as a class device is technically
> independent, it
> can live longer (which is likely), but it could technically also be
> shorter
> lived (which drivers don't do in practice). But even though drivers
> don't do
> this in practice, the dependency should be expressed: if GPUVM stores
> a pointer
> to a DRM device, it has to take a reference count.

It doesn't have to in a model where all users are removed before the
drm device is freed. Following your argument, shouldn't that drm_device
reference should also be accompanied by a module reference? Except that
will block rmmod?


> 
> > > I had a quick look at Xe and found this for instance:
> > > 
> > > 	 drmm_add_action_or_reset(&xe->drm, control_fini_action,
> > > gt)
> > > 
> > > control_fini_action() stops a worker that writes device
> > > registeres,
> > > which must
> > > not be done after driver unbind anymore.
> > > 
> > > Now, there's two options, either after driver unbind this work is
> > > never running
> > > (which would be correct), but then this could have been
> > > devm_add_action_or_reset(), or it does actually run after driver
> > > unbind, but
> > > this would violate the driver model.
> > 
> > Agreed, Unless there is something protecting the hardware access
> > after
> > unplug in that control subsystem, that's a genuine bug, but that's
> > separate from this discussion
> 
> I argue that it is related; surely, we can keep everything around
> until the DRM
> device is destroyed and just guard every single (callback) entry
> point.
> 
> But, that's far more complicated and error prone than just shutting
> down the hardware
> on driver unbind and tear down all entry points on the subsystem
> level; it
> messes with the ownership model of structures leaving stale pointers
> behind.
> 
> As mentioned, this is what subsystems commonly do in the kernel, and
> I don't see
> why DRM would be special in this regard.

Without knowing for sure, I think this was the route taken with
hotplugging.

> 
> > > > > I don't think the correct solution is to constrain module
> > > > > unload.
> > > > > The
> > > > > release()
> > > > > callback shouldn't really do anything other than free the
> > > > > memory
> > > > > of
> > > > > the
> > > > > drm_device allocation. All other resources a driver may have
> > > > > should
> > > > > be released
> > > > > on driver unbind.
> > > > 
> > > > That is not true. See above.
> > > 
> > > I know it is not true in practice, but we are doing the wrong
> > > thing.
> > > We are
> > > conflating the unbounded userspace lifetime with the bounded
> > > lifetime
> > > from the
> > > driver model.
> > 
> > I don't think we are. As long as all HW access is given up or
> > blocked,
> > we're fine.
> 
> Right, what you describe above works, but it leaves stale pointers
> and invalid
> structures behind that we then need to guard against.

But isn't essentially what you describe a design where we release all
dma_buf, file- and drm_pagemap references of struct drm_device at
module_unload time. That would replace their references with SRCU
protecting the drm_device pointer, falling back to a stub behaviour
when unbind has been called. So the "Can I access hardware?" would be
replaced by a "Can I access the DRM device?". 

That's an interesting idea, but would probably need careful work so
fence waits etc. doesn't block the SRCU read sections. And ofc to avoid
user-space regressing.

> 
> Don't get me wrong, I don't object to this patch series. Well, at
> least not too
> much, requiring drivers to call drm_dev_release_barrier() in module
> unload is
> pretty ugly, and I don't know any other subsystem that has such a
> guard, because
> they do structurally prevent callbacks into drivers after driver
> unbind and
> drivers hence tie the lifetime of structures to driver unbind.

FWIW, IIRC that SRCU is only strictly needed when non-driver code
(pagemap, files and dma-buf) drops the last drm_device reference
without having a module reference. And there is no such code (yet)
AFAIK, so I can drop that patch to when we think it's necessary and we
have other driver buy-ins.


Thanks,
Thomas