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); + 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
