Re: [PATCH v3] staging: media: atomisp: add missing mutex lock in atomisp_s_fmt_cap

[Hans de Goede <hansg@kernel.org>]

Hi,

On 17-Jul-25 2:42 PM, Abdelrahman Fekry wrote:
> The function atomisp_set_fmt() modifies shared device state and expects
> callers to hold the isp->mutex for synchronization. While most internal
> callers correctly lock the mutex before invoking atomisp_set_fmt(), the
> V4L2 ioctl handler atomisp_s_fmt_cap() does not.
> 
> This results in an unsafe execution path for VIDIOC_S_FMT ioctls
> (e.g. via v4l2-ctl), where shared structures such as pipe->pix and
> pipe->frame_info may be modified concurrently without proper protection.
> 
> - Fix this by explicitly locking isp->mutex in atomisp_s_fmt_cap().

Thank you for your patch, but I'm afraid that this stems from
not understanding how v4l2 drivers work.

isp->mutex is set as the mutex for the /dev/video# node in:
drivers/staging/media/atomisp/pci/atomisp_v4l2.c:

int atomisp_video_init(struct atomisp_video_pipe *video)
{
	...
	video->vdev.lock = &video->isp->mutex;

This guarantees that any ioctl handlers will be called with
isp->mutex already held.

The suggested change here will result in trying to lock
the mutex a second time leading to a deadlock.

So NACK for this patch.

What would be useful is adding:

	lockdep_assert_held(&isp->mutex);

to functions which expect the mutex to be held. I regularly
test the atomisp code with lockdep enabled kernels so this
will help catch any paths where we indeed are not locking
the mutex while we should lock it.

Regards.

Hans






> 
> Fixes: 4bdab80981ca ("media: atomisp: Make it possible to call atomisp_set_fmt() without a file handle")
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> ---
> v3:
> - Use guard(mutex) as suggested by Dan Carpenter and Andy Shevchenko
> - Remove extra blank line after variable declaration
> - Fix include order as requested by Andy Shevchenko
> 
> v2: https://lore.kernel.org/all/20250717013003.20936-1-abdelrahmanfekry375@gmail.com/
> - Add Fixes tag
> - use cleanup.h micros instead of explicitly calling mutex_lock/unlock
> 
> v1: https://lore.kernel.org/all/20250716014225.15279-1-abdelrahmanfekry375@gmail.com/
> ---
>  drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index bb8b2f2213b0..2690c05afff3 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -7,6 +7,7 @@
>   * Copyright (c) 2010 Silicon Hive www.siliconhive.com.
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/pci.h>
>  
> @@ -416,7 +417,9 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
>  			     struct v4l2_format *f)
>  {
>  	struct video_device *vdev = video_devdata(file);
> +	struct atomisp_device *isp = video_get_drvdata(vdev);
>  
> +	guard(mutex)(&isp->mutex);
>  	return atomisp_set_fmt(vdev, f);
>  }
>