Re: [PATCH v10] drm/xe/guc: Hold device ref until queue teardown completes

["Yadav, Arvind" <arvind.yadav@intel.com>]

On 10-07-2026 12:04, Thomas Hellström wrote:
> On Fri, 2026-07-10 at 08:31 +0530, Arvind Yadav wrote:
>> GuC exec queue destruction can run asynchronously. If the final
>> device
>> put happens from a destroy worker, drmm cleanup can end up draining
>> the same workqueue and deadlock.
>>
>> Hold a drm_device reference for the queue lifetime and drop it after
>> queue teardown completes. This keeps drmm cleanup from running while
>> async destroy work is still pending.
>>
>> Move GuC destroy work to a module-lifetime Xe workqueue and flush it
>> on PCI remove so hot-unbind/rebind still waits for pending destroy
>> work.
>>
>> With queue-held device refs, guc_submit_sw_fini() cannot run with
>> live
>> GuC IDs. Replace the fini wait with an assertion and remove the
>> unused
>> fini_wq.
>>
>> v2:
>>    - Rebase
>>
>> v3:
>>    - Switch to queue-lifetime drm_dev_get()/drm_dev_put() model.
>> (Matt)
>>    - Queue async teardown on system_dfl_wq instead of xe->destroy_wq.
>> (Matt)
>>    - Drop separate deferred drm_dev_put worker.
>>    - Remove stale drain_workqueue(xe->destroy_wq) from
>> guc_submit_sw_fini().
>>
>> v4:
>>    - Replace the guc_submit_sw_fini() wait with an assertion and
>> remove
>>      the now-unused fini_wq. (sashiko)
>>
>> v5:
>>    - Move destroy work to a module-lifetime Xe workqueue instead of
>>      system_dfl_wq. (Matt)
>>    - Flush the module-lifetime destroy workqueue during PCI remove to
>>      preserve the old device-remove wait semantics.
>>
>> v6:
>>    - Keep SVM pagemap destroy work on the per-device destroy_wq to
>> avoid
>>      letting it outlive the xe_device/drm_device. (Sashiko)
>>    - Use WQ_MEM_RECLAIM for xe->destroy_wq because SVM pagemap destroy
>> work
>>      can be queued from the reclaim path.
>>
>> v7:
>>    - Drop the per-device xe->destroy_wq and use the module-level
>> destroy WQ
>>      for SVM pagemap destroy as well. (Matt)
>>    - Rename xe_exec_queue_destroy_wq_*() helpers to xe_destroy_wq_*()
>>      helpers because the WQ is no longer exec-queue specific. (Matt)
>>
>> v8:
>>    - Rebase.
>>
>> v9:
>>    - Keep SVM pagemap destroy work on the per-device WQ_MEM_RECLAIM
>>      destroy_wq because it can be queued from reclaim and embeds
>>      the dev_pagemap used by devres teardown. (Sashiko)
>>    - Keep the module-level destroy WQ GuC-only and drop WQ_MEM_RECLAIM
>>      from it.
>>    - Update the module-WQ kdoc to document the GuC/SVM split.
>>
>> v10:
>>    - Keep xe->destroy_wq per-cpu while adding WQ_MEM_RECLAIM to fix
>> the
>>      workqueue allocation warning.
> If *all* work items that release the drm_device refcount are flushed at
> device removal (unplug) time, Why do we need the drm_device reference
> in the first place? Since IIRC the devres, that runs after unplug,
> keeps a drm_device reference?


The flush only covers work queued at unplug/remove time. 
"xe_device_remove()" does not close clients or destroy user queues, so a 
queue can outlive the remove-time flush. That's why the queue-held 
"drm_dev_get()" is still needed to keep "drm_dev_release()" and drmm 
cleanup from running until queue teardown completes.


> Similarly, if we flush all outstanding per-device GuC work at device
> removal time, why do we need this additional module-wide workqueue?
> Wouldn't it be equivalent to keep the GuC work items on the per-device
> destroy wq, and flush it at unplug / pci removal, the latter being the
> key?


The PCI-remove flush only waits for destroy work already queued at that 
time. A user queue can outlive unplug and queue its destroy work later 
when the fd closes. That is why the per-device WQ is still unsafe for 
GuC teardown, the worker can drop the final drm_device ref, enter drmm 
cleanup and destroy the same WQ it is running on. So the module-lifetime 
WQ is still needed.

Please let me know if I have missed something in this reasoning.


>
>> Fixes: 2d2be279f1ca ("drm/xe: fix UAF around queue destruction")
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_device.c       |  2 +-
>>   drivers/gpu/drm/xe/xe_device_types.h |  2 +-
>>   drivers/gpu/drm/xe/xe_guc_submit.c   | 66 ++++++++++++++++----------
>> --
>>   drivers/gpu/drm/xe/xe_guc_types.h    |  2 -
>>   drivers/gpu/drm/xe/xe_module.c       | 49 +++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_module.h       |  5 +++
>>   drivers/gpu/drm/xe/xe_pci.c          |  6 +++
>>   drivers/gpu/drm/xe/xe_svm.c          |  5 +++
>>   8 files changed, 105 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c
>> b/drivers/gpu/drm/xe/xe_device.c
>> index ad7f3e61d457..47630e9f7410 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -581,7 +581,7 @@ int xe_device_init_early(struct xe_device *xe)
>>   						
>> WQ_MEM_RECLAIM);
>>   	xe->ordered_wq = alloc_ordered_workqueue("xe-ordered-wq",
>> 0);
>>   	xe->unordered_wq = alloc_workqueue("xe-unordered-wq",
>> WQ_PERCPU, 0);
>> -	xe->destroy_wq = alloc_workqueue("xe-destroy-wq", WQ_PERCPU,
>> 0);
>> +	xe->destroy_wq = alloc_workqueue("xe-destroy-wq", WQ_PERCPU
>> | WQ_MEM_RECLAIM, 0);
>>   	if (!xe->ordered_wq || !xe->unordered_wq ||
>>   	    !xe->preempt_fence_wq || !xe->destroy_wq) {
>>   		/*
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>> b/drivers/gpu/drm/xe/xe_device_types.h
>> index 022e08205897..56c17cca79c0 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -355,7 +355,7 @@ struct xe_device {
>>   	/** @unordered_wq: used to serialize unordered work */
>>   	struct workqueue_struct *unordered_wq;
>>   
>> -	/** @destroy_wq: used to serialize user destroy work, like
>> queue */
>> +	/** @destroy_wq: used to serialize SVM pagemap destroy work
>> */
>>   	struct workqueue_struct *destroy_wq;
>>   
>>   	/** @tiles: device tiles */
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
>> b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 12416bfa3255..9b9434d7452a 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/circ_buf.h>
>>   #include <linux/dma-fence-array.h>
>>   
>> +#include <drm/drm_drv.h>
>>   #include <drm/drm_managed.h>
>>   
>>   #include "abi/guc_actions_abi.h"
>> @@ -37,6 +38,7 @@
>>   #include "xe_macros.h"
>>   #include "xe_map.h"
>>   #include "xe_mocs.h"
>> +#include "xe_module.h"
>>   #include "xe_pm.h"
>>   #include "xe_ring_ops_types.h"
>>   #include "xe_sched_job.h"
>> @@ -232,17 +234,9 @@ static bool
>> exec_queue_killed_or_banned_or_wedged(struct xe_exec_queue *q)
>>   static void guc_submit_sw_fini(struct drm_device *drm, void *arg)
>>   {
>>   	struct xe_guc *guc = arg;
>> -	struct xe_device *xe = guc_to_xe(guc);
>>   	struct xe_gt *gt = guc_to_gt(guc);
>> -	int ret;
>> -
>> -	ret = wait_event_timeout(guc->submission_state.fini_wq,
>> -				 xa_empty(&guc-
>>> submission_state.exec_queue_lookup),
>> -				 HZ * 5);
>>   
>> -	drain_workqueue(xe->destroy_wq);
>> -
>> -	xe_gt_assert(gt, ret);
>> +	xe_gt_assert(gt, xa_empty(&guc-
>>> submission_state.exec_queue_lookup));
>>   
>>   	xa_destroy(&guc->submission_state.exec_queue_lookup);
>>   }
>> @@ -319,8 +313,6 @@ int xe_guc_submit_init(struct xe_guc *guc,
>> unsigned int num_ids)
>>   
>>   	xa_init(&guc->submission_state.exec_queue_lookup);
>>   
>> -	init_waitqueue_head(&guc->submission_state.fini_wq);
>> -
>>   	primelockdep(guc);
>>   
>>   	guc->submission_state.initialized = true;
>> @@ -411,9 +403,6 @@ static void __release_guc_id(struct xe_guc *guc,
>> struct xe_exec_queue *q,
>>   	xe_guc_id_mgr_release_locked(&guc->submission_state.idm,
>>   				     q->guc->id, q->width);
>>   
>> -	if (xa_empty(&guc->submission_state.exec_queue_lookup))
>> -		wake_up(&guc->submission_state.fini_wq);
>> -
>>   	mutex_unlock(&guc->submission_state.lock);
>>   }
>>   
>> @@ -1686,6 +1675,7 @@ static void guc_exec_queue_fini(struct
>> xe_exec_queue *q)
>>   {
>>   	struct xe_guc_exec_queue *ge = q->guc;
>>   	struct xe_guc *guc = exec_queue_to_guc(q);
>> +	struct drm_device *drm = &guc_to_xe(guc)->drm;
>>   
>>   	if (xe_exec_queue_is_multi_queue_secondary(q)) {
>>   		struct xe_exec_queue_group *group = q-
>>> multi_queue.group;
>> @@ -1704,36 +1694,52 @@ static void guc_exec_queue_fini(struct
>> xe_exec_queue *q)
>>   	 * (timeline name).
>>   	 */
>>   	kfree_rcu(ge, rcu);
>> +
>> +	drm_dev_put(drm);
>>   }
>>   
>> -static void __guc_exec_queue_destroy_async(struct work_struct *w)
>> +static void guc_exec_queue_do_destroy(struct xe_exec_queue *q)
>>   {
>> -	struct xe_guc_exec_queue *ge =
>> -		container_of(w, struct xe_guc_exec_queue,
>> destroy_async);
>> -	struct xe_exec_queue *q = ge->q;
>> +	struct xe_guc_exec_queue *ge = q->guc;
>>   	struct xe_guc *guc = exec_queue_to_guc(q);
>> +	struct xe_device *xe = guc_to_xe(guc);
>> +	struct drm_device *drm = &xe->drm;
>>   
>> -	guard(xe_pm_runtime)(guc_to_xe(guc));
>> -	trace_xe_exec_queue_destroy(q);
>> +	/*
>> +	 * guc_exec_queue_fini() drops the queue's drm_device ref.
>> +	 * Keep the device alive until the PM-runtime guard unwinds.
>> +	 */
>> +	drm_dev_get(drm);
>>   
>> -	/* Confirm no work left behind accessing device structures
>> */
>> -	cancel_delayed_work_sync(&ge->sched.base.work_tdr);
>> +	scoped_guard(xe_pm_runtime, xe) {
>> +		trace_xe_exec_queue_destroy(q);
>>   
>> -	xe_exec_queue_fini(q);
>> +		/* Confirm no work left behind accessing device
>> structures */
>> +		cancel_delayed_work_sync(&ge->sched.base.work_tdr);
>> +
>> +		xe_exec_queue_fini(q);
>> +	}
>> +
>> +	drm_dev_put(drm);
>>   }
>>   
>> -static void guc_exec_queue_destroy_async(struct xe_exec_queue *q)
>> +static void __guc_exec_queue_destroy_async(struct work_struct *w)
>>   {
>> -	struct xe_guc *guc = exec_queue_to_guc(q);
>> -	struct xe_device *xe = guc_to_xe(guc);
>> +	struct xe_guc_exec_queue *ge =
>> +		container_of(w, struct xe_guc_exec_queue,
>> destroy_async);
>> +
>> +	guc_exec_queue_do_destroy(ge->q);
>> +}
>>   
>> +static void guc_exec_queue_destroy_async(struct xe_exec_queue *q)
>> +{
>>   	INIT_WORK(&q->guc->destroy_async,
>> __guc_exec_queue_destroy_async);
>>   
>>   	/* We must block on kernel engines so slabs are empty on
>> driver unload */
>>   	if (q->flags & EXEC_QUEUE_FLAG_PERMANENT ||
>> exec_queue_wedged(q))
>> -		__guc_exec_queue_destroy_async(&q->guc-
>>> destroy_async);
>> +		guc_exec_queue_do_destroy(q);
>>   	else
>> -		queue_work(xe->destroy_wq, &q->guc->destroy_async);
>> +		xe_destroy_wq_queue(&q->guc->destroy_async);
>>   }
>>   
>>   static void __guc_exec_queue_destroy(struct xe_guc *guc, struct
>> xe_exec_queue *q)
>> @@ -1928,6 +1934,7 @@ static int guc_exec_queue_init(struct
>> xe_exec_queue *q)
>>   {
>>   	struct xe_gpu_scheduler *sched;
>>   	struct xe_guc *guc = exec_queue_to_guc(q);
>> +	struct drm_device *drm = &guc_to_xe(guc)->drm;
>>   	struct workqueue_struct *submit_wq = NULL;
>>   	struct xe_guc_exec_queue *ge;
>>   	long timeout;
>> @@ -1939,6 +1946,8 @@ static int guc_exec_queue_init(struct
>> xe_exec_queue *q)
>>   	if (!ge)
>>   		return -ENOMEM;
>>   
>> +	drm_dev_get(drm);
>> +
>>   	q->guc = ge;
>>   	ge->q = q;
>>   	init_rcu_head(&ge->rcu);
>> @@ -2015,6 +2024,7 @@ static int guc_exec_queue_init(struct
>> xe_exec_queue *q)
>>   	release_guc_id(guc, q);
>>   err_free:
>>   	kfree(ge);
>> +	drm_dev_put(drm);
>>   
>>   	return err;
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_guc_types.h
>> b/drivers/gpu/drm/xe/xe_guc_types.h
>> index c7b9642b41ba..31a2acb63ac3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_types.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_types.h
>> @@ -100,8 +100,6 @@ struct xe_guc {
>>   		 * even initialized - before that not even the lock
>> is valid
>>   		 */
>>   		bool initialized;
>> -		/** @submission_state.fini_wq: submit fini wait
>> queue */
>> -		wait_queue_head_t fini_wq;
>>   	} submission_state;
>>   
>>   	/** @hwconfig: Hardware config state */
>> diff --git a/drivers/gpu/drm/xe/xe_module.c
>> b/drivers/gpu/drm/xe/xe_module.c
>> index 39e4fc85f019..848d65265443 100644
>> --- a/drivers/gpu/drm/xe/xe_module.c
>> +++ b/drivers/gpu/drm/xe/xe_module.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/init.h>
>>   #include <linux/module.h>
>> +#include <linux/workqueue.h>
>>   
>>   #include <drm/drm_module.h>
>>   
>> @@ -88,6 +89,50 @@ static int xe_check_nomodeset(void)
>>   	return 0;
>>   }
>>   
>> +static struct workqueue_struct *xe_destroy_wq;
>> +
>> +static int __init xe_destroy_wq_module_init(void)
>> +{
>> +	xe_destroy_wq = alloc_workqueue("xe-guc-destroy-wq",
>> WQ_UNBOUND, 0);
>> +	if (!xe_destroy_wq)
>> +		return -ENOMEM;
>> +	return 0;
>> +}
>> +
>> +static void xe_destroy_wq_module_exit(void)
>> +{
>> +	if (xe_destroy_wq)
>> +		destroy_workqueue(xe_destroy_wq);
>> +	xe_destroy_wq = NULL;
>> +}
>> +
>> +/**
>> + * xe_destroy_wq_queue() - Queue work on the destroy workqueue
>> + * @work: work item to queue
>> + *
>> + * The destroy workqueue has module lifetime and is used for GuC
>> exec queue
>> + * teardown that can outlive a single xe_device. SVM pagemap destroy
>> uses the
>> + * per-device xe->destroy_wq instead.
>> + *
>> + * Return: %true if @work was queued, %false if it was already
>> pending.
>> + */
>> +bool xe_destroy_wq_queue(struct work_struct *work)
>> +{
>> +	return queue_work(xe_destroy_wq, work);
>> +}
>> +
>> +/**
>> + * xe_destroy_wq_flush() - Flush the destroy workqueue
>> + *
>> + * Drains all pending destroy work. Called from PCI remove to ensure
>> + * teardown ordering before the device is destroyed.
>> + */
>> +void xe_destroy_wq_flush(void)
>> +{
>> +	if (xe_destroy_wq)
>> +		flush_workqueue(xe_destroy_wq);
>> +}
>> +
>>   struct init_funcs {
>>   	int (*init)(void);
>>   	void (*exit)(void);
>> @@ -109,6 +154,10 @@ static const struct init_funcs init_funcs[] = {
>>   		.init = xe_sched_job_module_init,
>>   		.exit = xe_sched_job_module_exit,
>>   	},
>> +	{
>> +		.init = xe_destroy_wq_module_init,
>> +		.exit = xe_destroy_wq_module_exit,
>> +	},
>>   	{
>>   		.init = xe_register_pci_driver,
>>   		.exit = xe_unregister_pci_driver,
>> diff --git a/drivers/gpu/drm/xe/xe_module.h
>> b/drivers/gpu/drm/xe/xe_module.h
>> index c75153471248..a0eb7db07770 100644
>> --- a/drivers/gpu/drm/xe/xe_module.h
>> +++ b/drivers/gpu/drm/xe/xe_module.h
>> @@ -8,6 +8,8 @@
>>   
>>   #include <linux/types.h>
>>   
>> +struct work_struct;
>> +
>>   /* Module modprobe variables */
>>   struct xe_modparam {
>>   	bool probe_display;
>> @@ -26,5 +28,8 @@ struct xe_modparam {
>>   
>>   extern struct xe_modparam xe_modparam;
>>   
>> +bool xe_destroy_wq_queue(struct work_struct *work);
>> +void xe_destroy_wq_flush(void);
>> +
>>   #endif
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c
>> b/drivers/gpu/drm/xe/xe_pci.c
>> index 08c14a0202d9..b0764eb90834 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -1099,6 +1099,12 @@ static void xe_pci_remove(struct pci_dev
>> *pdev)
>>   		return;
>>   
>>   	xe_device_remove(xe);
>> +
>> +	/*
>> +	 * Preserve remove-time flush after moving destroy work to
>> module
>> +	 * lifetime.
>> +	 */
>> +	xe_destroy_wq_flush();
>>   	xe_pm_fini(xe);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_svm.c
>> b/drivers/gpu/drm/xe/xe_svm.c
>> index e0f0c23d172d..2cb17497d950 100644
>> --- a/drivers/gpu/drm/xe/xe_svm.c
>> +++ b/drivers/gpu/drm/xe/xe_svm.c
>> @@ -1732,6 +1732,11 @@ static void xe_pagemap_destroy(struct
>> drm_pagemap *dpagemap, bool from_atomic_or
>>   	struct xe_pagemap *xpagemap = container_of(dpagemap,
>> typeof(*xpagemap), dpagemap);
>>   	struct xe_device *xe = to_xe_device(dpagemap->drm);
>>   
>> +	/*
>> +	 * SVM pagemap destroy can be queued from reclaim and
>> xpagemap embeds
>> +	 * the dev_pagemap used by devres teardown. Keep it on the
>> per-device
>> +	 * WQ_MEM_RECLAIM destroy_wq, not the module-level GuC
>> destroy WQ.
>> +	 */
> I think this comment can be dropped. Outside the context of this patch
> it will be confusing?


Agreed, I will remove this comment.

Thanks,
Arvind

>
>>   	if (from_atomic_or_reclaim)
>>   		queue_work(xe->destroy_wq, &xpagemap->destroy_work);
>>   	else
> Thanks,
> Thomas