Re: [PATCH v4 3/3] media: meson: vdec: Cancel esparser work in error and stop paths

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] AB-BA Deadlock in vdec_stop_streaming() and vdec_start_streaming().
- [High] Workqueue race condition and unexpected rescheduling in vdec_stop_streaming().

Pre-existing issues:
- [Critical] Use-After-Free in vdec_close() due to incorrect work cancellation order.
--

commit 68d67ae586f216ae4b5eab6d7931a8e345aebd15
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Cancel esparser work in error and stop paths

This commit attempts to fix a use-after-free issue by cancelling the
esparser_queue_work before freeing the session context. It adds
cancel_work_sync() calls in the error paths of vdec_start_streaming(),
in vdec_stop_streaming(), and in vdec_close().

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
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -358,6 +358,8 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	dma_free_coherent(sess->core->dev, sess->vififo_size,
>  			  sess->vififo_vaddr, sess->vififo_paddr);
>  bufs_done:
> +	cancel_work_sync(&sess->esparser_queue_work);
> +

[Severity: Critical]
Does this cancel_work_sync() call introduce a deadlock?

These are V4L2 queue callbacks, which are invoked by the V4L2 core with
the queue lock held. Specifically, __video_do_ioctl acquires &sess->lock
before calling the VIDIOC_STREAMOFF or VIDIOC_STREAMON ioctls.

However, the worker thread esparser_queue_all_src() begins execution by
attempting to acquire the exact same lock with mutex_lock(&sess->lock).

If a user calls VIDIOC_STREAMOFF while the worker thread is executing,
the ioctl thread will block indefinitely on cancel_work_sync() waiting
for the worker to finish, while the worker is permanently blocked waiting
for sess->lock held by the ioctl thread.

>  	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>  		sess->streamon_out = 0;

[ ... ]

> @@ -415,6 +417,7 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  		if (vdec_codec_needs_recycle(sess))
>  			kthread_stop(sess->recycle_thread);
>  
> +		cancel_work_sync(&sess->esparser_queue_work);
>  		vdec_poweroff(sess);

[Severity: High]
Could this placement of cancel_work_sync() leave the background work
pending after the stream is fully stopped?

The call to vdec_poweroff(sess) waits for the hardware to become inactive.
During this time, the hardware is still running and can complete a buffer,
triggering a codec interrupt.

The interrupt handler calls amvdec_dst_buf_done(), which explicitly calls
schedule_work(&sess->esparser_queue_work). Since the work is scheduled
after cancel_work_sync() has already returned, the work could be unexpectedly
left pending.

>  		vdec_free_canvas(sess);
>  		dma_free_coherent(sess->core->dev, sess->vififo_size,

[ ... ]

> @@ -937,6 +940,7 @@ static int vdec_close(struct file *file)
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  	v4l2_m2m_release(sess->m2m_dev);
>  	v4l2_ctrl_handler_free(&sess->ctrl_handler);
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: Critical]
This isn't a bug introduced by this patch, but the patch's attempt to fix it
is incomplete.

Could placing cancel_work_sync() after v4l2_m2m_ctx_release() lead to
a use-after-free?

The function v4l2_m2m_ctx_release() explicitly frees sess->m2m_ctx. If
the background work is rescheduled before this final cancellation (for
example, due to the late hardware interrupt during vdec_stop_streaming()
described above), the worker thread esparser_queue_all_src() can execute.

The worker thread iterates over buffers using the freed context, which
dereferences sess->m2m_ctx before cancel_work_sync() can stop it. Should
the work be cancelled before releasing the context?

>  	v4l2_fh_del(&sess->fh, file);
>  	v4l2_fh_exit(&sess->fh);

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

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