Re: [PATCH] drm/amdgpu: initialize irq.lock spinlock earlier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The IH work queues (ih1_work, ih2_work, ih_soft_work) are not cancelled before freeing the IH ring buffers during hardware teardown, leading to a potential use-after-free or NULL pointer dereference.
--

commit d96f9c222586fdd4fb9ac2385c6edc335ba11a51
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

drm/amdgpu: initialize irq.lock spinlock earlier

If there is an early failure during amdgpu probe, it will end up calling
amdgpu_irq_disable_all, which takes irq.lock spinlock without it being
initialized. Initializing irq.lock earlier at amdgpu_device_init fixes
the issue.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 82bc6d657e5a0..836ba07713649 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -309,8 +309,6 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  	unsigned int irq, flags;
>  	int r;
>  
> -	spin_lock_init(&adev->irq.lock);
> -

[Severity: High]
This isn't a bug introduced by this patch, but looking at the driver
teardown, are the IH work queues ever cancelled?

In amdgpu_irq_init(), ih1_work, ih2_work, and ih_soft_work are initialized
and can be scheduled by the interrupt handler. However, during teardown:

drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:amdgpu_irq_fini_hw() {
	if (adev->irq.installed) {
		free_irq(adev->irq.irq, adev_to_drm(adev));
		adev->irq.installed = false;
...
	amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
	amdgpu_ih_ring_fini(adev, &adev->irq.ih);
	amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
	amdgpu_ih_ring_fini(adev, &adev->irq.ih2);
}

The driver frees the IRQ and the IH ring buffers, but does not explicitly
cancel the pending work items using cancel_work_sync().

If a scheduled work item executes concurrently with or after teardown, could
it dereference the freed adev or ih->ring pointers, leading to a
use-after-free or NULL pointer dereference?

Should the work items be cancelled synchronously before the ring buffers are
freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-amdgpu-mutex-fix-2-v1-1-1d850180ed19@igalia.com?part=1