Re: [PATCH 1/2] media: amd: isp4: hold buf_list_lock while sending buffers at stream start

[Bin Du]

Reviewed-by: Bin Du <bin.du@xxxxxxx>

On 10/27/2026 2:24 AM, Seifelden Abdelmonem wrote:
> 
> isp4vid_qops_start_streaming() walks isp_vdev->buf_list without
> buf_list_lock and passes each buffer to isp4sd_ioc_send_img_buf().
> Every other access to buf_list holds that lock.
> 
> vb2 serialises the walk against buf_queue(), but not against the
> firmware response thread. Once ISP4SD_MIN_BUF_CNT_BEF_START_STREAM
> buffers have been sent, isp4sd_ioc_send_img_buf() also sends
> START_STREAM, so the firmware can complete buffers while the walk is
> still running. isp4vid_handle_frame_done() then list_del()s the
> completed buffer from buf_list. If that buffer is the current entry of
> the walk, list_for_each_entry() follows the poisoned next pointer:
> 
>   Oops: general protection fault, maybe for address 0xffff8e6944d97060
>   RIP: 0010:isp4if_alloc_buffer_node+0x37/0x50 [amd_isp4_capture]
>   RBX: dead0000000000b8 ... RSI: dead0000000000b8
>   Call Trace:
>    isp4sd_ioc_send_img_buf
>    isp4vid_qops_start_streaming
>    vb2_start_streaming
>    vb2_core_streamon
> 
> The oopsing task holds the queue lock, so the video device can no
> longer be closed and the module cannot be unloaded until reboot.
> 
> This was hit on a board that is not supported upstream yet, with an
> out-of-tree change that made one isp4sd_ioc_send_img_buf() call take
> over a second. With the stock driver the window is much shorter, but
> the list is still modified concurrently without the lock.
> 
> Hold buf_list_lock across the walk. This adds the lock order
> buf_list_lock -> ops_mutex. No path takes them in the reverse order:
> isp4vid_handle_frame_done() runs in the response thread without
> ops_mutex. isp4sd_ioc_send_img_buf() only queues commands and does not
> wait for a firmware response, so the response thread is at most
> delayed until the initial buffers have been sent.
> 
> Fixes: 2ccf48af2270 ("media: platform: amd: isp4 video node and buffers handling added")
> Signed-off-by: Seifelden Abdelmonem <saif.elden.hatim@xxxxxxxxx>
> ---
> Testing and AI use: see the cover letter.
> 
>  drivers/media/platform/amd/isp4/isp4_video.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/amd/isp4/isp4_video.c b/drivers/media/platform/amd/isp4/isp4_video.c
> index 856a2a0b4..58ea8d6ac 100644
> --- a/drivers/media/platform/amd/isp4/isp4_video.c
> +++ b/drivers/media/platform/amd/isp4/isp4_video.c
> @@ -567,9 +567,11 @@ static int isp4vid_qops_start_streaming(struct vb2_queue *vq,
>                 }
>         }
> 
> -       list_for_each_entry(isp4vid_buf, &isp_vdev->buf_list, list)
> -               isp4sd_ioc_send_img_buf(isp_vdev->isp_sdev,
> -                                       &isp4vid_buf->img_buf);
> +       scoped_guard(mutex, &isp_vdev->buf_list_lock) {
> +               list_for_each_entry(isp4vid_buf, &isp_vdev->buf_list, list)
> +                       isp4sd_ioc_send_img_buf(isp_vdev->isp_sdev,
> +                                               &isp4vid_buf->img_buf);
> +       }
> 
>         isp_vdev->stream_started = true;
> 
> --
> 2.56.0