Re: [PATCH v4 1/3] drm/xe: Don't unload the driver until all drm devices are freed

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

On Tue, 2026-09-29 at 14:41 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [High] xe: module unload race causing instruction page fault in
> xe_device_destroy()
> --
> 
> commit 4a4203d8cd4165e1a99345416f131e7eafe530d1
> Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> 
> drm/xe: Don't unload the driver until all drm devices are freed
> 
> This commit introduces a device count and a module exit hook to wait
> until all bare drm_device references are freed before unloading the
> xe
> module. It uses a 20-second timeout with wait_var_event_timeout() to
> block rmmod from unloading while devices remain active.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > b/drivers/gpu/drm/xe/xe_device.c
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> 
> [ ... ]
> 
> > @@ -391,6 +394,9 @@ static void xe_device_destroy(struct drm_device
> > *dev, void *dummy)
> >  		destroy_workqueue(xe->destroy_wq);
> >  
> >  	ttm_device_fini(&xe->ttm);
> > +
> > +	if (atomic_dec_and_test(&xe_device_count))
> > +		wake_up_var(&xe_device_count);
> 
> [Severity: High]
> Does this wake up the rmmod thread while still executing inside
> module text?
> If the thread executing xe_device_destroy() is preempted immediately
> after
> wake_up_var(), rmmod can complete the module exit sequence and unmap
> the
> xe module text. When the preempted thread resumes, it might attempt
> to
> execute the function epilogue in unmapped memory, potentially causing
> an
> instruction page fault.

This warning is a consequence of removing the first patch. But it's a
false positive. If the drm_dev_put() is executed within an xe module
workqueue we wait for that work item to complete later in the unload
sequence. If called from a driver hook the caller holds a module
reference around the drm_device reference.

/Thomas



> 
> >  }
> >  
> >  /**
> 
> [ ... ]
> 
> > @@ -1501,3 +1508,25 @@ struct xe_vm *xe_device_asid_to_vm(struct
> > xe_device *xe, u32 asid)
> >  
> >  	return vm;
> >  }
> > +
> > +/**
> > + * xe_device_exit() - Device subsystem exit function.
> > + *
> > + * Exit function to be called at module unload time.
> > + */
> > +void xe_device_exit(void)
> > +{
> > +	/*
> > +	 * Wait for all devices to be freed. 20s is well above the
> > typical
> > +	 * maximum dma_fence signalling time, so warn and keep
> > waiting if
> > +	 * we're still not done by then, since it may indicate a
> > leaked
> > +	 * xe_device reference is stalling module unload.
> > +	 */
> > +	if (!wait_var_event_timeout(&xe_device_count,
> > +				   
> > !atomic_read(&xe_device_count),
> > +				    HZ * 20)) {
> 
> [Severity: High]
> Is there a risk that the rmmod thread waiting here proceeds to exit
> and unmap
> the module before the waker has safely exited xe_device_destroy()?
> Once
> awoken, this thread is unaware that the waker is still executing in
> module
> text.
> 
> > +		pr_warn("%s: Waiting for %d xe device(s) to be
> > freed before unloading.\n",
> > +			DRIVER_NAME,
> > atomic_read(&xe_device_count));
> > +		wait_var_event(&xe_device_count,
> > !atomic_read(&xe_device_count));
> > +	}
> > +}