From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Matthew Brost <matthew.brost@intel.com>
Cc: Arvind Yadav <arvind.yadav@intel.com>,
<intel-xe@lists.freedesktop.org>,
<himal.prasad.ghimiray@intel.com>,
<thomas.hellstrom@linux.intel.com>, <tejas.upadhyay@intel.com>
Subject: Re: [PATCH v7] drm/xe/guc: Hold device ref until queue teardown completes
Date: Fri, 3 Jul 2026 14:27:34 -0400 [thread overview]
Message-ID: <akf_FtBIU-4IhIId@intel.com> (raw)
In-Reply-To: <ajtaGUaavUkXWVnI@gsse-cloud1.jf.intel.com>
On Tue, Jun 23, 2026 at 09:16:25PM -0700, Matthew Brost wrote:
> On Wed, Jun 24, 2026 at 09:40:29AM +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 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)
> >
> > Fixes: 2d2be279f1ca ("drm/xe: fix UAF around queue destruction")
> > Cc: Matthew Brost <matthew.brost@intel.com>
>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
I was going to push this patch, but have you guys seen this:
https://sashiko.dev/#/patchset/20260624041029.2357470-1-arvind.yadav%40intel.com
?
>
> > 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>
> > Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_device.c | 6 +--
> > drivers/gpu/drm/xe/xe_device_types.h | 3 --
> > 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 | 50 +++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_module.h | 5 +++
> > drivers/gpu/drm/xe/xe_pci.c | 6 +++
> > drivers/gpu/drm/xe/xe_svm.c | 3 +-
> > 8 files changed, 101 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 089872507bdd..0d8848e66d6b 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -469,9 +469,6 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
> > if (xe->unordered_wq)
> > destroy_workqueue(xe->unordered_wq);
> >
> > - if (xe->destroy_wq)
> > - destroy_workqueue(xe->destroy_wq);
> > -
> > ttm_device_fini(&xe->ttm);
> > }
> >
> > @@ -580,9 +577,8 @@ 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);
> > if (!xe->ordered_wq || !xe->unordered_wq ||
> > - !xe->preempt_fence_wq || !xe->destroy_wq) {
> > + !xe->preempt_fence_wq) {
> > /*
> > * Cleanup done in xe_device_destroy via
> > * drmm_add_action_or_reset register above
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 32dd2ffbc796..1ed993dffb52 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -355,9 +355,6 @@ 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 */
> > - struct workqueue_struct *destroy_wq;
> > -
> > /** @tiles: device tiles */
> > struct xe_tile tiles[XE_MAX_TILES_PER_DEVICE];
> >
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index afe5d99cdd8b..982356513f3c 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);
> > }
> >
> > @@ -1685,6 +1674,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;
> > @@ -1703,36 +1693,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)
> > @@ -1927,6 +1933,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;
> > @@ -1938,6 +1945,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);
> > @@ -2014,6 +2023,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 4cb578182912..473be717e2c4 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>
> >
> > @@ -91,6 +92,51 @@ 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-destroy-wq",
> > + WQ_UNBOUND | WQ_MEM_RECLAIM, 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 async teardown
> > + * work that can outlive a single xe_device, such as GuC exec queue and SVM
> > + * pagemap destroy.
> > + *
> > + * 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);
> > @@ -112,6 +158,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 79cb9639c0f3..e8e54f701cf3 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 force_execlist;
> > @@ -27,5 +29,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 78fc2e4dcfc6..dc35f8a8c4b8 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -1049,6 +1049,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 e1651e70c8f0..6ff99310fdb4 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.c
> > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > @@ -1732,10 +1732,9 @@ static void xe_pagemap_destroy_work(struct work_struct *work)
> > static void xe_pagemap_destroy(struct drm_pagemap *dpagemap, bool from_atomic_or_reclaim)
> > {
> > struct xe_pagemap *xpagemap = container_of(dpagemap, typeof(*xpagemap), dpagemap);
> > - struct xe_device *xe = to_xe_device(dpagemap->drm);
> >
> > if (from_atomic_or_reclaim)
> > - queue_work(xe->destroy_wq, &xpagemap->destroy_work);
> > + xe_destroy_wq_queue(&xpagemap->destroy_work);
> > else
> > xe_pagemap_destroy_work(&xpagemap->destroy_work);
> > }
> > --
> > 2.43.0
> >
next prev parent reply other threads:[~2026-07-03 18:27 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-24 4:10 [PATCH v7] drm/xe/guc: Hold device ref until queue teardown completes Arvind Yadav
2026-06-24 4:16 ` Matthew Brost
2026-07-03 18:27 ` Rodrigo Vivi [this message]
2026-07-03 22:28 ` Matthew Brost
2026-06-24 4:17 ` ✓ CI.KUnit: success for drm/xe/guc: Hold device ref until queue teardown completes (rev5) Patchwork
2026-06-24 4:59 ` ✓ Xe.CI.BAT: " Patchwork
2026-06-24 7:32 ` ✓ Xe.CI.FULL: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=akf_FtBIU-4IhIId@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=arvind.yadav@intel.com \
--cc=himal.prasad.ghimiray@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=matthew.brost@intel.com \
--cc=tejas.upadhyay@intel.com \
--cc=thomas.hellstrom@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox