Re: [PATCH v6 5/8] media: meson: vdec: Cancel esparser work during teardown

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling `cancel_work_sync(&sess->esparser_queue_work)` while holding the session lock (`sess->lock`) causes a guaranteed AB-BA deadlock if the `esparser_queue_all_src` work is executing.

Pre-existing issues:
- [Critical] Prematurely clearing the hardware owner `core->cur_sess` introduces a critical race condition that can corrupt hardware state and disable clocks for concurrent sessions.
--

commit 20f0925bb191ee052dd2907764b22c2e4a352074
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Cancel esparser work during teardown

This commit adds cancel_work_sync() calls during streaming stop, error unwind,
and device close. It aims to prevent use-after-free hazards by ensuring the
esparser workqueue is drained before releasing session state and buffers.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 698a95566ad27..4884ee04b3529 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -380,6 +380,8 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  			  sess->vififo_vaddr, sess->vififo_paddr);
>  	sess->vififo_vaddr = NULL;
>  bufs_done:
> +	cancel_work_sync(&sess->esparser_queue_work);
> +

[Severity: High]
Does this code introduce a deadlock? The V4L2 core acquires the queue lock
(sess->lock) before calling vdec_start_streaming() or vdec_stop_streaming().

If cancel_work_sync() is called here while the ioctl thread holds sess->lock,
and the worker thread is currently executing:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
    ...
    mutex_lock(&sess->lock);
    ...

Could the worker thread block indefinitely waiting for sess->lock, while the
ioctl thread blocks indefinitely in cancel_work_sync() waiting for the worker
to complete?

>  	mutex_lock(&core->lock);
>  	if (core->cur_sess == sess)
>  		core->cur_sess = NULL;

[ ... ]

> @@ -437,6 +439,8 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  	struct vb2_v4l2_buffer *buf;
>  	enum amvdec_status old_status;
>  
> +	cancel_work_sync(&sess->esparser_queue_work);
> +
>  	/*
>  	 * Safely snapshot the status and clear the hardware owner inside
>  	 * the mutex to prevent data races with concurrent STREAMON requests.
> @@ -448,7 +452,11 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  	sess->status = STATUS_STOPPED;
>  	mutex_unlock(&core->lock);

[Severity: Critical]
This isn't a bug introduced by this patch, but does prematurely clearing
core->cur_sess here introduce a race condition?

Since core->cur_sess is set to NULL under core->lock, but the hardware
teardown via vdec_poweroff() executes later after releasing the lock, could
a concurrent STREAMON from a different session acquire core->cur_sess and
execute vdec_poweron()?

This sequence would cause vdec_poweroff() of the old session and
vdec_poweron() of the new session to execute simultaneously, potentially
leading to conflicting hardware accesses and disabling clocks for the
concurrent session.

> -	/* Evaluate the hardware state using our snapshot */
> +	if (q->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +		if (old_status >= STATUS_RUNNING && codec_ops->drain)
> +			codec_ops->drain(sess);
> +	}

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