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.
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 +- .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 141 +++++++++--------- 2 files changed, 70 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_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c index c5467f34c51f..20fdf0578119 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,71 @@ 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. + * Removes all handlers from the IRQ tables under the spinlock, cancels + * pending work items, and deallocates all handler data. */ 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"); + 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 +485,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 +497,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,9 +586,12 @@ 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; + + DM_IRQ_TABLE_LOCK(adev, irq_table_flags); if (list_empty(handler_list)) - return; + goto out_unlock; list_for_each_entry(handler_data, handler_list, list) { if (queue_work(system_highpri_wq, &handler_data->work)) { @@ -617,7 +609,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 +631,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 +673,9 @@ 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; - 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
