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

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

[Abdelrahman Fekry]

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().

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);
 }
 
-- 
2.25.1

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

[Andy Shevchenko]

On Thu, Jul 17, 2025 at 3:43 PM Abdelrahman Fekry
<abdelrahmanfekry375@gmail.com> 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().

Now LGTM, FWIW,
Reviewed-by: Andy Shevchneko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Carpenter]

On Thu, Jul 17, 2025 at 03:42:34PM +0300, 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().
> 
> Fixes: 4bdab80981ca ("media: atomisp: Make it possible to call atomisp_set_fmt() without a file handle")
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> ---

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

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

[Hans de Goede]

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);
>  }
>  

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

[Abdelrahman Fekry]

Hi Hans,
Thanks for the feedback.

On Tue, Aug 5, 2025 at 12:00 PM Hans de Goede <hansg@kernel.org> wrote:
>
> 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.
great, now i understand.
>
> 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.
will look into that.
Could you please check my other patch series if you have time
https://lore.kernel.org/all/20250712191325.132666-1-abdelrahmanfekry375@gmail.com/
thank you !
>
> 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);
> >  }
> >
>