Re: [PATCH 05/17] media: v4l2-mem2mem: support running multiple jobs in parallel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Lockless list traversal of `job_queue` in `v4l2_m2m_suspend` leads to list corruption and kernel panic.
- [High] Unlocked dereference of `first_ctx` fields in `v4l2_m2m_get_curr_priv` leads to a Use-After-Free.
- [Medium] `v4l2_m2m_get_curr_priv` breaks interrupt mapping for drivers utilizing parallel jobs.
--

commit b3a2fe91efda298e9f5953581aac80b6a8ccdefb
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: v4l2-mem2mem: support running multiple jobs in parallel

Add support for running multiple jobs in parallel for SoCs containing
multiple identical devices. An example is the Rockchip RK3588 SoC,
which contains two identical RGA3 devices.

> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index a65cbb124cfe0..14ac9c85803d1 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c

[ ... ]

> @@ -229,14 +235,22 @@ EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_by_idx);
>  void *v4l2_m2m_get_curr_priv(struct v4l2_m2m_dev *m2m_dev)
>  {
>  	unsigned long flags;
> -	void *ret = NULL;
> +	struct v4l2_m2m_ctx *first_ctx;
>  
>  	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
> -	if (m2m_dev->curr_ctx)
> -		ret = m2m_dev->curr_ctx->priv;
> +	if (list_empty(&m2m_dev->job_queue)) {
> +		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> +		return NULL;
> +	}
> +
> +	first_ctx = list_first_entry(&m2m_dev->job_queue,
> +				     struct v4l2_m2m_ctx, queue);
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>  
> -	return ret;
> +	if (first_ctx->job_flags & TRANS_RUNNING)
> +		return first_ctx->priv;

[Severity: High]
Does this introduce a use-after-free?

The job_spinlock is dropped right before reading first_ctx->job_flags
and first_ctx->priv. If the job is concurrently canceled, it could be
removed from the list and its context freed before these reads occur.

[Severity: Medium]
Will this return the correct context when multiple jobs are running in
parallel?

If max_parallel_jobs > 1, multiple contexts could be marked with
TRANS_RUNNING.  Because this function indiscriminately returns the priv
pointer of the first element in the queue, a driver servicing an interrupt
for a newer concurrent job might mistakenly receive the priv of the oldest
running job instead.

> +	else
> +		return NULL;
>  }
>  EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);

[ ... ]

> @@ -544,16 +572,19 @@ EXPORT_SYMBOL(v4l2_m2m_buf_done_and_job_finish);
>  void v4l2_m2m_suspend(struct v4l2_m2m_dev *m2m_dev)
>  {
>  	unsigned long flags;
> -	struct v4l2_m2m_ctx *curr_ctx;
> +	struct v4l2_m2m_ctx *ctx;
> +	struct v4l2_m2m_ctx *ctx_safe;
>  
>  	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
>  	m2m_dev->job_queue_flags |= QUEUE_PAUSED;
> -	curr_ctx = m2m_dev->curr_ctx;
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>  
> -	if (curr_ctx)
> -		wait_event(curr_ctx->finished,
> -			   !(curr_ctx->job_flags & TRANS_RUNNING));
> +	list_for_each_entry_safe(ctx, ctx_safe, &m2m_dev->job_queue, queue) {
> +		if (!(ctx->job_flags & TRANS_RUNNING))
> +			break;
> +
> +		wait_event(ctx->finished, !(ctx->job_flags & TRANS_RUNNING));

[Severity: High]
Could this result in list corruption?

This loop traverses m2m_dev->job_queue without holding the
m2m_dev->job_spinlock.  When wait_event() blocks, a concurrent job
completion could call _v4l2_m2m_job_finish() and remove ctx_safe from the
list via list_del().

When the loop resumes, dereferencing the poisoned ctx_safe pointer would
likely cause a crash.

> +	}
>  }
>  EXPORT_SYMBOL(v4l2_m2m_suspend);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-spu-rga3multicore-v1-0-3ec2b15675f7@pengutronix.de?part=5