Re: [PATCH v6 3/8] media: meson: vdec: Handle kthread failure and free codec state

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Double-free of DMA buffers in the new err_cleanup path due to a redundant codec_ops->stop(sess) call.

Pre-existing issues:
- [Critical] Clearing the hardware ownership token `core->cur_sess` before executing `vdec_poweroff(sess)` creates a critical data race with concurrent sessions.
- [High] The concurrent locking patch destroys the STATUS_NEEDS_RESUME state, leading to massive memory leaks during dynamic resolution changes.
--

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

media: meson: vdec: Handle kthread failure and free codec state
    
vdec_start_streaming() launches a recycle thread when required by the
codec. If kthread_run() fails, the previous error path only powered off
the hardware, leaving sess->priv and codec state allocated. This caused
a permanent leak of the codec context and associated DMA buffers.
    
Fix this by introducing a dedicated err_cleanup path:

- Call codec_ops->stop() to release the codec context and clear sess->priv.
- Power off the hardware before freeing buffers to avoid DMA faults.
- Free canvas IDs explicitly and set sess->vififo_vaddr = NULL after
  dma_free_coherent() to guard against double‑free in fallback stop paths.
- Reset core->cur_sess and sess->status to prevent stale references.

Following change closes the memory leak on kthread_run() failure and ensures
robust cleanup of codec resources in both error and stop paths.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 7233000e22323..8a5bf1a96830c 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -352,16 +354,31 @@ 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;
> +		}
> +	}
>  
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
> +err_cleanup:
> +	vdec_free_canvas(sess);
> +	vdec_poweroff(sess);
> +	if (codec_ops && codec_ops->stop && sess->priv) {
> +		codec_ops->stop(sess);

[Severity: Critical]
Does this explicitly call codec_ops->stop() a second time, resulting in a
double-free of the codec DMA memory?

Looking at vdec_poweroff() called just above, it internally delegates to
vdec_ops->stop(), which invokes codec_ops->stop(). Codec stop callbacks
free their DMA buffers via dma_free_coherent() but do not set the
pointers to NULL. If it is called again here, wouldn't it attempt to free
them a second time?

> +		kfree(sess->priv);
> +		sess->priv = NULL;
> +	}
>  vififo_free:
>  	dma_free_coherent(sess->core->dev, sess->vififo_size,
>  			  sess->vififo_vaddr, sess->vififo_paddr);
> +	sess->vififo_vaddr = NULL;
>  bufs_done:

[ ... ]

> @@ -441,8 +458,11 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  
>  		vdec_poweroff(sess);

[Severity: Critical]
This is a pre-existing issue, but could clearing the hardware ownership token
before executing vdec_poweroff() create a data race with concurrent sessions?

Looking earlier in vdec_stop_streaming(), core->cur_sess = NULL is set
inside the mutex, but vdec_poweroff(sess) is executed here after the mutex
is dropped:

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

This allows a concurrent vdec_start_streaming() from another context to
acquire the lock, claim ownership, and call vdec_poweron(). Since
vdec_poweron() and vdec_poweroff() would execute concurrently on different
sessions, is it possible they will read and write to the exact same global
hardware registers and corrupt the shared hardware state?

>  		vdec_free_canvas(sess);
> -		dma_free_coherent(sess->core->dev, sess->vififo_size,
> -				  sess->vififo_vaddr, sess->vififo_paddr);
> +		if (sess->vififo_vaddr) {
> +			dma_free_coherent(sess->core->dev, sess->vififo_size,
> +					  sess->vififo_vaddr,
> +					  sess->vififo_paddr);
> +		}
>  		vdec_reset_timestamps(sess);
>  		vdec_reset_bufs_recycle(sess);
>  		kfree(sess->priv);

[Severity: High]
This isn't a bug introduced by this patch, but does the recent change to
concurrent locking destroy the STATUS_NEEDS_RESUME state?

Earlier in vdec_stop_streaming(), sess->status is unconditionally overwritten
to STATUS_STOPPED inside the mutex lock:

	mutex_lock(&core->lock);
	old_status = sess->status;
	sess->status = STATUS_STOPPED;
	mutex_unlock(&core->lock);

When the CAPTURE queue is stopped during a source resolution change, the
hardware cleanup block is intentionally skipped because old_status is
STATUS_NEEDS_RESUME. However, when vdec_start_streaming() is later called to
resume the CAPTURE queue, it expects sess->status == STATUS_NEEDS_RESUME.
Since it was forced to STATUS_STOPPED, will the resume check fail and cause
the driver to incorrectly fall through to full initialization, executing
dma_alloc_coherent() over the existing sess->vififo_vaddr and leaking
memory?

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