On 2026-07-09 7:38, Leo Li wrote:
>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
Hi Leo,
Thanks, I agree.
The irq_fini_in_progress flag is not needed for the handler-list UAF
case if all users of irq_handler_list_low/high_tab are serialized by
DM_IRQ_TABLE_LOCK.
With the fini path moving the handlers off the tables under the lock,
a concurrent IRQ path should either see the old list before the splice,
queue/run the handler, and then be covered by cancel_work_sync(), or see
the empty list after the splice and return without touching any handler
data.
I will remove this and submit a v2 shortly.
>
>> +
>> 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]);
