Re: [PATCH v3 04/14] drm/v3d: Replace spin_lock_irqsave() with spin_lock()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Removing `spin_lock_irqsave` exposes a uniprocessor race and obstructs the correct fix for an existing SMP use-after-free race.
- [Low] Mixing goto-based error handling and scope-based cleanup helpers (`scoped_guard()`) in the same function violates subsystem guidelines.
--

commit 7d41ab716d6dcf438f92d8905f5e28343e3ab169
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Replace spin_lock_irqsave() with spin_lock()

This commit replaces spin_lock_irqsave() and spin_unlock_irqrestore() with
the scoped_guard() macro using plain spinlocks in v3d_overflow_mem_work()
and v3d_bin_job_run().

> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
> index 86efaef2722c3..754a969b862b6 100644
> --- a/drivers/gpu/drm/v3d/v3d_irq.c
> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
[ ... ]
> @@ -64,17 +63,15 @@ v3d_overflow_mem_work(struct work_struct *work)
>  	 * bin job got scheduled, that's fine.  We'll just give them
>  	 * some binner pool anyway.
>  	 */
> -	spin_lock_irqsave(&queue->queue_lock, irqflags);
> -	bin_job = (struct v3d_bin_job *)queue->active_job;
> +	scoped_guard(spinlock, &queue->queue_lock) {

[Severity: High]
Does removing the IRQ-disabling protection here cement a broken locking design?

The commit message notes the hard IRQ handler doesn't take this lock, but
v3d_irq_signal_fence() modifies queue->active_job locklessly. Because of this
missing lock, an SMP use-after-free race already exists where the DRM
scheduler can free the job while v3d_overflow_mem_work() is dereferencing
bin_job->render.

By switching from spin_lock_irqsave() to spin_lock(), this exposes a
uniprocessor race where the IRQ can preempt the workqueue's critical section.
Furthermore, if someone fixes the SMP race by adding
spin_lock(&queue->queue_lock) to the IRQ handler, won't the system deadlock
when the IRQ interrupts this workqueue?

> +		bin_job = (struct v3d_bin_job *)queue->active_job;
>  
> -	if (!bin_job) {
> -		spin_unlock_irqrestore(&queue->queue_lock, irqflags);
> -		goto out;
> -	}
> +		if (!bin_job)
> +			goto out;

[Severity: Low]
Is it ok to mix goto-based error handling with scope-based cleanup helpers?

The kernel cleanup guidelines explicitly forbid mixing goto and scope-based
cleanup helpers in the same function because it creates confusing ownership
semantics.

While scoped_guard() is introduced here for lock management, the function
retains explicit goto statements like this one to reach the out label for
drm_gem_object_put().

>  
> -	drm_gem_object_get(obj);
> -	list_add_tail(&bo->unref_head, &bin_job->render->unref_list);
> -	spin_unlock_irqrestore(&queue->queue_lock, irqflags);
> +		drm_gem_object_get(obj);
> +		list_add_tail(&bo->unref_head, &bin_job->render->unref_list);
> +	}
>  
>  	v3d_mmu_flush_all(v3d);
>  
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 46f4fc09c59ec..04fd1a365576b 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
[ ... ]
> @@ -240,9 +239,9 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
>  	return fence;
>  
>  out_clean_job:
> -	spin_lock_irqsave(&queue->queue_lock, irqflags);
> -	queue->active_job = NULL;
> -	spin_unlock_irqrestore(&queue->queue_lock, irqflags);
> +	scoped_guard(spinlock, &queue->queue_lock) {
> +		queue->active_job = NULL;
> +	}
>  	return NULL;
>  }

[Severity: Low]
Does this also violate the cleanup guidelines against mixing goto and
scope-based cleanup?

Similar to v3d_overflow_mem_work(), this function introduces scoped_guard()
while still retaining the out_clean_job label that is reached via goto
statements earlier in the function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-v3d-sched-misc-fixes-v3-0-d7114bba55a0@igalia.com?part=4