Re: [PATCH v7 11/19] media: meson: vdec: Update core m2m stream state during transitions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error path in `vdec_start_streaming` flushes BOTH M2M source and destination buffer queues, causing buffers of an already-streaming queue to be silently stranded.
- [High] The teardown sequence in `vdec_stop_streaming` calls `synchronize_irq()` before stopping the hardware, leaving a race window for a Use-After-Free in the interrupt handler.
- [High] Active DMA buffers are returned to the videobuf2 framework before the hardware is stopped, potentially causing memory corruption or IOMMU faults.
--

--- Patch [11]: [PATCH v7 11/19] media: meson: vdec: Update core m2m stream state during transitions ---
commit 8309f1636cfcc48cc67621545812af232aebc2dd
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Update core m2m stream state during transitions

Explicitly update the V4L2 core Memory-to-Memory (m2m) framework's
internal streaming states when initiating or terminating streaming on
the device queues.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -299,6 +299,8 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	else
>  		sess->streamon_cap = 1;
>  
> +	v4l2_m2m_update_start_streaming_state(sess->m2m_ctx, q);
> +
>  	if (!sess->streamon_out)
>  		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
vdec_start_streaming() mistakenly strand buffers if one queue fails to start?

If the CAPTURE queue starts successfully, but the OUTPUT queue fails to start
due to the hardware being busy, the error path flushes both queues:

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
    ...
err_unlock_no_hw:
	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);
    ...
}

Because the CAPTURE queue is actively streaming, wouldn't returning these
buffers with VB2_BUF_STATE_QUEUED cause the vb2 core to leave them in the
queued list without putting them on the done list, permanently freezing
the userspace application?

[ ... ]

> @@ -529,6 +531,8 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  			sess->changed_format = 0;
>  	}
>  
> +	v4l2_m2m_update_stop_streaming_state(sess->m2m_ctx, q);
> +
>  	mutex_unlock(&core->lock);
>  }

[Severity: High]
This is a pre-existing issue, but does vdec_stop_streaming() return active
DMA buffers to the videobuf2 framework before the hardware is stopped?

When flushing buffers, it immediately returns them to the V4L2 core:

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
    ...
	/* flush buffers to kill background workqueue thread */
	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
		while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_ERROR);
		sess->streamon_out = 0;
	} else {
    ...
}

If the vb2 framework unmaps these buffers while the hardware is still running
and writing to physical memory, can this cause DMA memory corruption or IOMMU
faults before vdec_poweroff() is finally called later in the function?


[Severity: High]
This is a pre-existing issue, but does the teardown sequence in
vdec_stop_streaming() leave a race window for a use-after-free?

The synchronization happens before the hardware is halted:

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
    ...
		/* Synchronize and flush pending hardware interrupt service routines */
		synchronize_irq(core->vdec_irq);
		/* Ensure esparser ISR finishes executing */
		synchronize_irq(core->esparser_irq);

		cancel_work_sync(&sess->esparser_queue_work);
		mutex_lock(&core->lock);

		if (core->cur_sess == sess) {
			vdec_poweroff(sess);
            ...
			kfree(sess->priv);
    ...
}

If a new hardware interrupt fires immediately after the synchronization
completes, and vdec_stop_streaming() proceeds to free sess->priv,
could the concurrent ISR access the freed session data?

drivers/staging/media/meson/vdec/vdec.c:vdec_isr() {
    ...
	/* Secure an atomic acquire snapshot to protect against concurrent teardown */
	sess = smp_load_acquire(&core->cur_sess);
	if (!sess)
		return IRQ_HANDLED;

	sess->last_irq_jiffies = get_jiffies_64();

	return sess->fmt_out->codec_ops->isr(sess);
    ...
}

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

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