[PATCH] media: i2c: ov5648: Fix a data race on state->streaming

[PATCH] media: i2c: ov5648: Fix a data race on state->streaming

[Ginger Li]

ov5648_s_stream() updates state->streaming after it has released
sensor->mutex, while ov5648_resume() reads that field with sensor->mutex
held.  A concurrent resume can therefore observe a stale streaming state and
leave the sensor in standby while the software believes that it is streaming,
or the other way around.

Keep the update inside the sensor->mutex protected section that already
programs the standby mode.

Fixes: e43ccb0a045f ("media: i2c: Add support for the OV5648 image sensor")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/media/i2c/ov5648.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
--- a/drivers/media/i2c/ov5648.c
+++ b/drivers/media/i2c/ov5648.c
@@ -2145,12 +2145,12 @@ static int ov5648_s_stream(struct v4l2_subdev *subdev,
 
 	mutex_lock(&sensor->mutex);
 	ret = ov5648_sw_standby(sensor, !enable);
+	if (!ret)
+		state->streaming = !!enable;
 	mutex_unlock(&sensor->mutex);
 
 	if (ret)
 		return ret;
-
-	state->streaming = !!enable;
 
 	if (!enable)
 		pm_runtime_put(sensor->dev);
-- 
2.43.0

Re: [PATCH] media: i2c: ov5648: Fix a data race on state->streaming

[Kieran Bingham]

Quoting Ginger Li (2026-09-23 02:56:31)
> ov5648_s_stream() updates state->streaming after it has released
> sensor->mutex, while ov5648_resume() reads that field with sensor->mutex
> held.  A concurrent resume can therefore observe a stale streaming state and
> leave the sensor in standby while the software believes that it is streaming,
> or the other way around.
> 
> Keep the update inside the sensor->mutex protected section that already
> programs the standby mode.
> 
> Fixes: e43ccb0a045f ("media: i2c: Add support for the OV5648 image sensor")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> ---
>  drivers/media/i2c/ov5648.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
> --- a/drivers/media/i2c/ov5648.c
> +++ b/drivers/media/i2c/ov5648.c
> @@ -2145,12 +2145,12 @@ static int ov5648_s_stream(struct v4l2_subdev *subdev,
>  
>         mutex_lock(&sensor->mutex);
>         ret = ov5648_sw_standby(sensor, !enable);
> +       if (!ret)
> +               state->streaming = !!enable;

I see streaming is accessed in ov5648_suspend and ov5648_resume with the
lock held indeed, so I think this makes sense:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         mutex_unlock(&sensor->mutex);
>  
>         if (ret)
>                 return ret;
> -
> -       state->streaming = !!enable;
>  
>         if (!enable)
>                 pm_runtime_put(sensor->dev);
> -- 
> 2.43.0
>