On 2026-06-29 12:51, Geoffrey McRae wrote:
> DM IRQ teardown can race with interrupt handling and low-context work.
> The IRQ handler can still walk the DM IRQ handler tables while the
> teardown path removes and frees entries. Low-context work can also
> remain queued after its handler has been removed, leading to a possible
> use-after-free when the work item later runs.
>
> Add an irq_fini_in_progress flag and set it before the IRQ tables are
> torn down. Check the flag in the ISR and work scheduling paths so they
> do not access the handler tables or queue new work once teardown has
> started.
>
> Rework amdgpu_dm_irq_fini() to detach all low and high context handlers
> from the IRQ tables under the table lock, then cancel pending
> low-context work outside the lock before freeing the handlers. Also
> cancel low-context work in remove_irq_handler() before freeing an
> individual handler.
>
> Fix the suspend path by disabling HPD and HPD RX hardware interrupts
> under the IRQ table lock before flushing pending low-context work,
> avoiding a TOCTOU window where new work could be queued after the list
> check.
>
> Finally, call amdgpu_dm_irq_fini() from amdgpu_dm_fini() before DC is
> destroyed, so IRQ teardown happens while the display core state is still
> valid.
>
> Signed-off-by: Geoffrey McRae <[email protected]>
> Cc: Harry Wentland <[email protected]>
> Cc: Leo Li <[email protected]>
> Cc: Alex Deucher <[email protected]>
> Cc: Christian König <[email protected]>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +-
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 164 ++++++++++--------
> 3 files changed, 96 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b97ceabe6173..9c5e963337cc 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1010,14 +1010,11 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
> adev->dm.hpd_rx_offload_wq = NULL;
> }
>
> + amdgpu_dm_irq_fini(adev);
> +
> /* DC Destroy TODO: Replace destroy DAL */
> if (adev->dm.dc)
> dc_destroy(&adev->dm.dc);
> - /*
> - * TODO: pageflip, vlank interrupt
> - *
> - * amdgpu_dm_irq_fini(adev);
> - */
>
> if (adev->dm.cgs_device) {
> amdgpu_cgs_destroy_device(adev->dm.cgs_device);
> @@ -1523,7 +1520,6 @@ static int dm_hw_fini(struct amdgpu_ip_block *ip_block)
>
> amdgpu_dm_hpd_fini(adev);
>
> - amdgpu_dm_irq_fini(adev);
> amdgpu_dm_fini(adev);
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 909ee71d6d59..88687a7e01a5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -340,6 +340,8 @@ struct hpd_rx_irq_offload_work {
> * @dmcub_trace_event_en: enable dmcub trace events
> * @dmub_outbox_params: DMUB Outbox parameters
> * @num_of_edps: number of backlight eDPs
> + * @irq_fini_in_progress: Set during IRQ teardown to prevent interrupt
> handlers
> + * from accessing the IRQ tables during cleanup
> * @disable_hpd_irq: disables all HPD and HPD RX interrupt handling in the
> * driver when true
> * @dmub_aux_transfer_done: struct completion used to indicate when DMUB
> @@ -634,6 +636,7 @@ struct amdgpu_display_manager {
> */
> struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
> bool force_timing_sync;
> + bool irq_fini_in_progress;
> bool disable_hpd_irq;
> bool dmcub_trace_event_en;
> /**
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> index c5467f34c51f..3a5de9364ed1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> @@ -195,6 +195,9 @@ static struct list_head *remove_irq_handler(struct
> amdgpu_device *adev,
> return NULL;
> }
>
> + if (int_params->int_context == INTERRUPT_LOW_IRQ_CONTEXT)
> + cancel_work_sync(&handler->work);
> +
> kfree(handler);
>
> DRM_DEBUG_KMS(
> @@ -204,55 +207,6 @@ static struct list_head *remove_irq_handler(struct
> amdgpu_device *adev,
> return hnd_list;
> }
>
> -/**
> - * unregister_all_irq_handlers() - Cleans up handlers from the DM IRQ table
> - * @adev: The base driver device containing the DM device
> - *
> - * Go through low and high context IRQ tables and deallocate handlers.
> - */
> -static void unregister_all_irq_handlers(struct amdgpu_device *adev)
> -{
> - struct list_head *hnd_list_low;
> - struct list_head *hnd_list_high;
> - struct list_head *entry, *tmp;
> - struct amdgpu_dm_irq_handler_data *handler;
> - unsigned long irq_table_flags;
> - int i;
> -
> - DM_IRQ_TABLE_LOCK(adev, irq_table_flags);
> -
> - for (i = 0; i < DAL_IRQ_SOURCES_NUMBER; i++) {
> - hnd_list_low = &adev->dm.irq_handler_list_low_tab[i];
> - hnd_list_high = &adev->dm.irq_handler_list_high_tab[i];
> -
> - list_for_each_safe(entry, tmp, hnd_list_low) {
> -
> - handler = list_entry(entry, struct
> amdgpu_dm_irq_handler_data,
> - list);
> -
> - if (handler == NULL || handler->handler == NULL)
> - continue;
> -
> - list_del(&handler->list);
> - kfree(handler);
> - }
> -
> - list_for_each_safe(entry, tmp, hnd_list_high) {
> -
> - handler = list_entry(entry, struct
> amdgpu_dm_irq_handler_data,
> - list);
> -
> - if (handler == NULL || handler->handler == NULL)
> - continue;
> -
> - list_del(&handler->list);
> - kfree(handler);
> - }
> - }
> -
> - DM_IRQ_TABLE_UNLOCK(adev, irq_table_flags);
> -}
> -
> static bool
> validate_irq_registration_params(struct dc_interrupt_params *int_params,
> void (*ih)(void *))
> @@ -459,38 +413,84 @@ EXPORT_IF_KUNIT(amdgpu_dm_irq_init);
> * amdgpu_dm_irq_fini() - Tear down DM IRQ management
> * @adev: The base driver device containing the DM device
> *
> - * Flush all work within the low context IRQ table.
> + * Prevents any new interrupt handler scheduling, removes all handlers from
> + * the IRQ tables, cancels pending work items, and deallocates all handler
> + * data. The irq_fini_in_progress flag ensures the ISR and work scheduler
> + * do not access the handler lists during teardown.
> */
> void amdgpu_dm_irq_fini(struct amdgpu_device *adev)
> {
> int src;
> - struct list_head *lh;
> + LIST_HEAD(low_handlers);
> + LIST_HEAD(high_handlers);
> struct list_head *entry, *tmp;
> struct amdgpu_dm_irq_handler_data *handler;
> unsigned long irq_table_flags;
>
> DRM_DEBUG_KMS("DM_IRQ: releasing resources.\n");
> +
> + /*
> + * Set the fini flag before tearing down the IRQ tables. This ensures
> + * that any concurrent ISR (amdgpu_dm_irq_handler()) or work scheduler
> + * (amdgpu_dm_irq_schedule_work()) will bail out early rather than
> + * accessing handler data that is about to be freed.
> + *
> + * smp_store_release() pairs with the READ_ONCE() in the ISR and work
> + * scheduler paths to guarantee visibility across CPUs.
> + */
> + smp_store_release(&adev->dm.irq_fini_in_progress, true);
Hi Geoffrey, thanks for the patches.
Do we need irq_fini_in_progress if we're clearing the
irq_hander_list_low/high_tab under the DM_IRQ_TABLE_LOCK?
It looks to me that any concurrent amdgpu_dm_irq_immediate_work/schedule_work
will -- because of DM_IRQ_TABLE_LOCK maintaining serialization -- either:
A) run before list_splice and handle the IRQ, which is OK. In the
case of schedule_work(), where queued work can be pending, the
cance_work_sync() ensures they flush before continuing. Or,
B) run after list clear and early return since the list is empty, which
prevents any use after free.
Generally I'm a little hesitant of using mbs unless absolutely necessary, since
it's difficult to understand their correctness. If we have spinlocks handling
acquire/releases already, I'd prefer to just use those.
Thanks,
Leo
> +
> for (src = 0; src < DAL_IRQ_SOURCES_NUMBER; src++) {
> DM_IRQ_TABLE_LOCK(adev, irq_table_flags);
> - /* The handler was removed from the table,
> - * it means it is safe to flush all the 'work'
> - * (because no code can schedule a new one).
> +
> + /*
> + * Move all handlers from the low and high context tables to
> + * temporary lists under the lock. This prevents the ISR from
> + * finding them while we process them outside the lock.
> */
> - lh = &adev->dm.irq_handler_list_low_tab[src];
> + list_splice_init(&adev->dm.irq_handler_list_low_tab[src],
> + &low_handlers);
> + list_splice_init(&adev->dm.irq_handler_list_high_tab[src],
> + &high_handlers);
> +
> DM_IRQ_TABLE_UNLOCK(adev, irq_table_flags);
>
> - if (!list_empty(lh)) {
> - list_for_each_safe(entry, tmp, lh) {
> - handler = list_entry(
> - entry,
> - struct amdgpu_dm_irq_handler_data,
> - list);
> - flush_work(&handler->work);
> - }
> + /*
> + * Cancel all pending work for the low-context handlers
> + * outside the lock. cancel_work_sync() may sleep and waits
> + * until any running work completes, preventing UAF.
> + */
> + list_for_each_safe(entry, tmp, &low_handlers) {
> + handler = list_entry(entry,
> + struct amdgpu_dm_irq_handler_data,
> + list);
> + cancel_work_sync(&handler->work);
> }
> +
> + /*
> + * High-context handlers are executed synchronously within ISR
> + * context (see amdgpu_dm_irq_immediate_work()) and have no
> + * work_struct, so there is no pending work to cancel here.
> + * They will be freed along with low_handlers after the loop.
> + */
> + }
> +
> + /* Deallocate all handlers. */
> + list_for_each_safe(entry, tmp, &low_handlers) {
> + handler = list_entry(entry,
> + struct amdgpu_dm_irq_handler_data,
> + list);
> + list_del(&handler->list);
> + kfree(handler);
> + }
> +
> + list_for_each_safe(entry, tmp, &high_handlers) {
> + handler = list_entry(entry,
> + struct amdgpu_dm_irq_handler_data,
> + list);
> + list_del(&handler->list);
> + kfree(handler);
> }
> - /* Deallocate handlers from the table. */
> - unregister_all_irq_handlers(adev);
> }
> EXPORT_IF_KUNIT(amdgpu_dm_irq_fini);
>
> @@ -498,7 +498,6 @@ void amdgpu_dm_irq_suspend(struct amdgpu_device *adev)
> {
> struct drm_device *dev = adev_to_drm(adev);
> int src;
> - struct list_head *hnd_list_h;
> struct list_head *hnd_list_l;
> unsigned long irq_table_flags;
> struct list_head *entry, *tmp;
> @@ -511,12 +510,15 @@ void amdgpu_dm_irq_suspend(struct amdgpu_device *adev)
> /**
> * Disable HW interrupt for HPD and HPDRX only since FLIP and VBLANK
> * will be disabled from manage_dm_interrupts on disable CRTC.
> + *
> + * Disable the HW interrupt first, then flush any pending work. Since
> + * the HW interrupt is disabled under the lock, no new IRQ can be
> + * generated after the disable completes. Any work already queued by an
> + * in-flight ISR will be flushed below.
> */
> for (src = DC_IRQ_SOURCE_HPD1; src <= DC_IRQ_SOURCE_HPD6RX; src++) {
> hnd_list_l = &adev->dm.irq_handler_list_low_tab[src];
> - hnd_list_h = &adev->dm.irq_handler_list_high_tab[src];
> - if (!list_empty(hnd_list_l) || !list_empty(hnd_list_h))
> - dc_interrupt_set(adev->dm.dc, src, false);
> + dc_interrupt_set(adev->dm.dc, src, false);
>
> DM_IRQ_TABLE_UNLOCK(adev, irq_table_flags);
>
> @@ -597,10 +599,20 @@ static void amdgpu_dm_irq_schedule_work(struct
> amdgpu_device *adev,
> struct list_head *handler_list =
> &adev->dm.irq_handler_list_low_tab[irq_source];
> struct amdgpu_dm_irq_handler_data *handler_data;
> bool work_queued = false;
> + unsigned long irq_table_flags;
>
> - if (list_empty(handler_list))
> + /*perform a lockless check first*/
> + if (READ_ONCE(adev->dm.irq_fini_in_progress))
> return;
>
> + DM_IRQ_TABLE_LOCK(adev, irq_table_flags);
> +
> + if (READ_ONCE(adev->dm.irq_fini_in_progress))
> + goto out_unlock;
> +
> + if (list_empty(handler_list))
> + goto out_unlock;
> +
> list_for_each_entry(handler_data, handler_list, list) {
> if (queue_work(system_highpri_wq, &handler_data->work)) {
> work_queued = true;
> @@ -617,7 +629,7 @@ static void amdgpu_dm_irq_schedule_work(struct
> amdgpu_device *adev,
> handler_data_add = kzalloc(sizeof(*handler_data), GFP_ATOMIC);
> if (!handler_data_add) {
> DRM_ERROR("DM_IRQ: failed to allocate irq handler!\n");
> - return;
> + goto out_unlock;
> }
>
> /*copy new amdgpu_dm_irq_handler_data members from
> handler_data*/
> @@ -639,6 +651,9 @@ static void amdgpu_dm_irq_schedule_work(struct
> amdgpu_device *adev,
> "from display for IRQ source %d\n",
> irq_source);
> }
> +
> +out_unlock:
> + DM_IRQ_TABLE_UNLOCK(adev, irq_table_flags);
> }
>
> /*
> @@ -678,9 +693,12 @@ static int amdgpu_dm_irq_handler(struct amdgpu_device
> *adev,
> struct amdgpu_irq_src *source,
> struct amdgpu_iv_entry *entry)
> {
> + enum dc_irq_source src;
> +
> + if (READ_ONCE(adev->dm.irq_fini_in_progress))
> + return 0;
>
> - enum dc_irq_source src =
> - dc_interrupt_to_irq_source(
> + src = dc_interrupt_to_irq_source(
> adev->dm.dc,
> entry->src_id,
> entry->src_data[0]);
