On Mon, Jun 29, 2026 at 12:51 PM Geoffrey McRae <[email protected]> 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]>
Series looks correct to me, but I'm not an expert on the display code. So ideally we'd get some feedback from Harry or Leo. Series is: Acked-by: Alex Deucher <[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); > + > 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]); > -- > 2.43.0 >
