Re: [PATCH 1/4] media: v4l2-mem2mem: serialize REMOVE_BUFS with job execution

Le samedi 15 août 2026 à 22:56 +0800, Junrui Luo via B4 Relay a écrit :
> From: Junrui Luo <[email protected]>
> 
> VIDIOC_REMOVE_BUFS can free a buffer that an in-flight job is still
> decoding from, giving a use-after-free of the reference frame.
> 
> For stateless decoders a reference frame sits in VB2_BUF_STATE_DEQUEUED,
> which is exactly what REMOVE_BUFS frees: userspace dequeues the decoded
> capture buffer and names its timestamp as the reference for the next
> frame. device_run() resolves that timestamp with vb2_find_buffer() and
> keeps the vb2_plane_vaddr() result for the whole decode without holding a
> reference. Nothing serializes the two paths: REMOVE_BUFS runs under
> m2m_ctx->q_lock and q->mmap_lock, while the job path holds neither and
> reaches device_run() from schedule_work() outside any ioctl lock.
> 
> Hold the instance off the job queue and wait for its in-flight job before
> removing the buffers. The pause is per-instance rather than device-wide
> because the buffers belong to that instance's own queues and the caller
> is inside one of its own ioctls, so the context cannot be released while
> the wait is in progress. A losing race degrades to vb2_find_buffer()
> returning NULL and the job failing with -EINVAL, which is already
> handled.

I recognize this is a bug, but the fix waits, where it should just fail (EBUSY).
We have flags to track the buffers owned by the driver, and that should be
usable here and entirely done at vb2 level.

Once this solved, you should check if this isn't actually a vicodec bug (there
is other related bug  report iirc) rather then a m2m framework issues as you
subject state.

Please don't just prompt this back, read and understand.

Nicolas

> 
> Reproduced on vicodec under KASAN:
> 
>   BUG: KASAN: vmalloc-out-of-bounds in add_deltas+0x450/0xcc0
>   Read of size 1 at addr ffffc90000a77bc0 by task trigger_bin/71
>   Call Trace:
>    add_deltas+0x450/0xcc0
>    decode_plane+0x1799/0x34b0
>    fwht_decode_frame+0x173/0x620
>    v4l2_fwht_decode+0x595/0xea0
>    device_run+0x6bb/0x1850
>    media_request_ioctl+0x2fb/0x450
>    __se_sys_ioctl+0xb1/0x110
>    do_syscall_64+0x12c/0x360
> 
> Fixes: 2f2419502f69 ("media: v4l2: Add mem2mem helpers for REMOVE_BUFS ioctl")
> Reported-by: Yuhao Jiang <[email protected]>
> Assisted-by: Claude:claude-opus-5
> Cc: [email protected]
> Signed-off-by: Junrui Luo <[email protected]>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 60 +++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index a65cbb124cfe..10a0a74f782c 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -41,6 +41,8 @@ module_param(debug, bool, 0644);
>  #define TRANS_RUNNING		(1 << 1)
>  /* Instance is currently aborting */
>  #define TRANS_ABORT		(1 << 2)
> +/* Instance must not be scheduled while its buffers are being removed */
> +#define TRANS_PAUSED		(1 << 3)
>  
>  
>  /* The job queue is not running new jobs */
> @@ -309,9 +311,9 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
>  
>  	spin_lock_irqsave(&m2m_dev->job_spinlock, flags_job);
>  
> -	/* If the context is aborted then don't schedule it */
> -	if (m2m_ctx->job_flags & TRANS_ABORT) {
> -		dprintk("Aborted context\n");
> +	/* If the context is aborted or paused then don't schedule it */
> +	if (m2m_ctx->job_flags & (TRANS_ABORT | TRANS_PAUSED)) {
> +		dprintk("Aborted or paused context\n");
>  		goto job_unlock;
>  	}
>  
> @@ -1388,16 +1390,66 @@ int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);
>  
> +/*
> + * Keep @m2m_ctx off the job queue and wait for its in-flight job, if any.
> + *
> + * Only this instance is held back: the buffers about to be removed belong to
> + * its own queues, so no other instance can be reading them. Waiting on this
> + * instance is also what makes the wait safe -- the caller is inside one of its
> + * ioctls, so v4l2_m2m_ctx_release() cannot free it here.
> + */
> +static void v4l2_m2m_pause_ctx(struct v4l2_m2m_ctx *m2m_ctx)
> +{
> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
> +	m2m_ctx->job_flags |= TRANS_PAUSED;
> +	if (m2m_ctx->job_flags & TRANS_RUNNING) {
> +		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> +		wait_event(m2m_ctx->finished,
> +			   !(m2m_ctx->job_flags & TRANS_RUNNING));
> +		return;
> +	}
> +	if (m2m_ctx->job_flags & TRANS_QUEUED) {
> +		list_del(&m2m_ctx->queue);
> +		m2m_ctx->job_flags &= ~TRANS_QUEUED;
> +	}
> +	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> +}
> +
> +static void v4l2_m2m_resume_ctx(struct v4l2_m2m_ctx *m2m_ctx)
> +{
> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
> +	m2m_ctx->job_flags &= ~TRANS_PAUSED;
> +	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> +
> +	v4l2_m2m_try_schedule(m2m_ctx);
> +}
> +
>  int v4l2_m2m_ioctl_remove_bufs(struct file *file, void *priv,
>  			       struct v4l2_remove_buffers *remove)
>  {
>  	struct v4l2_fh *fh = file_to_v4l2_fh(file);
>  	struct vb2_queue *q = v4l2_m2m_get_vq(fh->m2m_ctx, remove->type);
> +	int ret;
>  
>  	if (q->type != remove->type)
>  		return -EINVAL;
>  
> -	return vb2_core_remove_bufs(q, remove->index, remove->count);
> +	/*
> +	 * Removal is only allowed for DEQUEUED buffers, but that is exactly
> +	 * the state a stateless decoder's reference frame is in while a job
> +	 * resolves it by timestamp and reads its memory.
> +	 */
> +	v4l2_m2m_pause_ctx(fh->m2m_ctx);
> +	ret = vb2_core_remove_bufs(q, remove->index, remove->count);
> +	v4l2_m2m_resume_ctx(fh->m2m_ctx);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_remove_bufs);
>

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTvDVKBFcTDwhoEbxLZQZRRKWBy9AUCaoNTWwAKCRDZQZRRKWBy
9A+TAQDzT19jvMHuEkO79vd5IoN/64y4cPUOs4LQKpVLeLe8/AEA/NknG6GW+Z4O
odGAQjeIJtsvbcYPUflJfer+oqHksAo=
=gTE2
-----END PGP SIGNATURE-----