Re: [PATCH v4 2/3] media: meson: vdec: Add error handling for recycle thread creation

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added error path in `vdec_start_streaming()` leaks `sess->priv` when `kthread_run()` fails.

Pre-existing issues:
- [Critical] Race condition between hardware power-on and `core->cur_sess` initialization leads to a NULL pointer dereference in the IRQ handler.
- [High] Returning buffers for both source and destination queues upon single-queue failure orphans active queue buffers.
- [High] Concurrent sessions can bypass the hardware exclusivity check, leading to simultaneous hardware programming.
--

commit 6ac3ef9e0ba2b15ab6c09a7f5386537c4ce45927
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Add error handling for recycle thread creation

This commit adds error handling for kthread_run() in vdec_start_streaming()
to prevent a kernel panic when thread creation fails. It unwinds resources
and correctly reorders the error path to clear streamon flags before emptying
M2M buffers.

Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 9244fb09eb36b..8615a935e86de 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -337,29 +337,37 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  
>  	sess->sequence_cap = 0;
>  	sess->sequence_out = 0;
> -	if (vdec_codec_needs_recycle(sess))
> +	if (vdec_codec_needs_recycle(sess)) {
>  		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
>  						   "vdec_recycle");
> +		if (IS_ERR(sess->recycle_thread)) {
> +			ret = PTR_ERR(sess->recycle_thread);
> +			sess->recycle_thread = NULL;
> +			goto err_cleanup;

[Severity: High]
Does this error path leak sess->priv?

The vdec_poweron() function invoked earlier allocates dynamic memory for
the codec context and assigns it to sess->priv. When kthread_run() fails,
this new error path calls vdec_poweroff() which stops the hardware but
doesn't free sess->priv.

Because kthread_run() failing prevents sess->status from being updated to
STATUS_INIT, any subsequent call to vdec_stop_streaming() will fail its
status check and bypass the entire cleanup block, permanently leaking
sess->priv and any associated DMA memory.

> +		}
> +	}
>  
>  	sess->status = STATUS_INIT;
>  	core->cur_sess = sess;

[Severity: Critical]
This is a pre-existing issue, but can an early hardware interrupt cause a
NULL pointer dereference here?

Because vdec_poweron() enables the decoding hardware and unmasks hardware
interrupts before core->cur_sess is assigned, if a hardware interrupt
triggers in between, the interrupt handler vdec_isr() will read a NULL
core->cur_sess. It then executes sess->last_irq_jiffies = get_jiffies_64(),
leading to a kernel panic. Adding the thread creation logic above widens
this vulnerable window.

[Severity: High]
This is a pre-existing issue, but can concurrent sessions bypass the hardware
exclusivity check and corrupt hardware state?

The exclusivity check at the start of vdec_start_streaming():

    if (core->cur_sess && core->cur_sess != sess) {

is executed without holding the core-level lock (core->lock). Because the
standard V4L2 queue lock (q->lock) is allocated per-session, two separate
processes opening the device concurrently can enter vdec_start_streaming()
at the same time, evaluate core->cur_sess == NULL as true, and concurrently
invoke vdec_poweron(sess), triggering simultaneous writes to the same
hardware registers.

>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
> +err_cleanup:
> +	vdec_poweroff(sess);
>  vififo_free:
>  	dma_free_coherent(sess->core->dev, sess->vififo_size,
>  			  sess->vififo_vaddr, sess->vififo_paddr);
>  bufs_done:
> -	while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
> -		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> -	while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
> -		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> -
>  	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>  		sess->streamon_out = 0;
>  	else
>  		sess->streamon_cap = 0;
>  
> +	while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
> +		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> +	while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
> +		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);

[Severity: High]
This is a pre-existing issue, but will returning buffers for both queues upon
a single-queue failure orphan active queue buffers?

If vdec_start_streaming() fails (e.g., for the OUTPUT queue), the error path
unconditionally removes all buffers from both the source (OUTPUT) and
destination (CAPTURE) M2M queues and returns them with VB2_BUF_STATE_QUEUED.

If the CAPTURE queue was successfully started in a previous call, returning
its buffers puts them back into the vb2 queued list while the driver
discards its references. Because the CAPTURE queue remains active,
userspace calling DQBUF will hang indefinitely waiting for frames that
the driver will never process.

> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521073449.10057-1-linux.amoon@gmail.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic