Re: [PATCH v5 6/6] 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:
- [Critical] Dropping the V4L2 queue lock inside `vdec_stop_streaming()` breaks videobuf2 synchronization, allowing concurrent ioctls to cause use-after-free and double-frees.
- [High] Unbalanced mutex unlock/lock in `vdec_stop_streaming()` when called during file release causes kernel panic and lock state corruption.
- [Low] The `cancel_work_sync()` call added to `vdec_close()` is racy and ineffective because it is called before hardware interrupts are disabled.
--

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

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

This commit adds cancel_work_sync() calls to the stop and close paths to
prevent the esparser workqueue from running after session teardown. It
drops and reacquires the session lock around the cancellation in the
stop_streaming callback.

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 925537bd4d0b3..296b387f3667b 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -430,6 +434,9 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  			kthread_stop(sess->recycle_thread);
>  
>  		vdec_poweroff(sess);
> +		mutex_unlock(&sess->lock);
> +		cancel_work_sync(&sess->esparser_queue_work);
> +		mutex_lock(&sess->lock);

[Severity: Critical]
Does dropping the queue lock here break videobuf2 synchronization?

When the v4l2 core processes a streamoff ioctl, it enters
vb2_core_streamoff() holding this queue lock and eventually calls
q->ops->stop_streaming().

If we unlock it here, could a concurrent streamoff ioctl on the same
queue acquire the lock and re-enter vb2_core_streamoff()?

Since q->start_streaming_called is only cleared after the
stop_streaming callback returns, the second thread might also invoke
vdec_stop_streaming() concurrently. Because sess->status hasn't been
updated to STATUS_STOPPED yet, both threads could execute this teardown
block, potentially leading to double-frees of sess->vififo_vaddr and
sess->priv, or concurrent kthread_stop() calls.

[Severity: High]
Is it guaranteed that sess->lock is held when vdec_stop_streaming() is
called?

When a file descriptor is closed without explicitly stopping the stream
first, the call chain looks like this:

vdec_close()
  v4l2_m2m_ctx_release()
    vb2_queue_release()
      __vb2_queue_cancel()
        q->ops->stop_streaming()

In this path, the v4l2 core does not hold the queue lock. Calling
mutex_unlock() on an unheld mutex will cause lock state corruption.

Furthermore, this code will then acquire the mutex and return, leaving it
locked when vdec_close() subsequently calls mutex_destroy() on it.

[ ... ]

> @@ -948,6 +955,8 @@ static int vdec_close(struct file *file)
>  {
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
>  
> +	cancel_work_sync(&sess->esparser_queue_work);
> +

[Severity: Low]
Can the workqueue be re-armed immediately after this cancellation?

At this point in vdec_close(), the hardware hasn't been powered off yet.
If a hardware interrupt fires right after cancel_work_sync() returns,
could the interrupt service routine call schedule_work() and re-queue it?

While this might not lead to a use-after-free because it gets cancelled
again in vdec_stop_streaming() before any memory is freed, it seems to
leave the workqueue active longer than intended.

>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  	v4l2_m2m_release(sess->m2m_dev);
>  	v4l2_fh_del(&sess->fh, file);

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

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