[v1] media: vimc: skip .s_stream() for stopped entities

[PATCH] media: vimc: skip .s_stream() for stopped entities

Posted by Nikita Zhandarovich 9 months, 3 weeks ago

Syzbot reported [1] a warning prompted by a check in call_s_stream()
that checks whether .s_stream() operation is warranted for unstarted
or stopped subdevs.

Add a simple fix in vimc_streamer_pipeline_terminate() ensuring that
entities skip a call to .s_stream() unless they have been previously
properly started.

[1] Syzbot report:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 5933 at drivers/media/v4l2-core/v4l2-subdev.c:460 call_s_stream+0x2df/0x350 drivers/media/v4l2-core/v4l2-subdev.c:460
Modules linked in:
CPU: 0 UID: 0 PID: 5933 Comm: syz-executor330 Not tainted 6.13.0-rc2-syzkaller-00362-g2d8308bf5b67 #0
...
Call Trace:
 <TASK>
 vimc_streamer_pipeline_terminate+0x218/0x320 drivers/media/test-drivers/vimc/vimc-streamer.c:62
 vimc_streamer_pipeline_init drivers/media/test-drivers/vimc/vimc-streamer.c:101 [inline]
 vimc_streamer_s_stream+0x650/0x9a0 drivers/media/test-drivers/vimc/vimc-streamer.c:203
 vimc_capture_start_streaming+0xa1/0x130 drivers/media/test-drivers/vimc/vimc-capture.c:256
 vb2_start_streaming+0x15f/0x5a0 drivers/media/common/videobuf2/videobuf2-core.c:1789
 vb2_core_streamon+0x2a7/0x450 drivers/media/common/videobuf2/videobuf2-core.c:2348
 vb2_streamon drivers/media/common/videobuf2/videobuf2-v4l2.c:875 [inline]
 vb2_ioctl_streamon+0xf4/0x170 drivers/media/common/videobuf2/videobuf2-v4l2.c:1118
 __video_do_ioctl+0xaf0/0xf00 drivers/media/v4l2-core/v4l2-ioctl.c:3122
 video_usercopy+0x4d2/0x1620 drivers/media/v4l2-core/v4l2-ioctl.c:3463
 v4l2_ioctl+0x1ba/0x250 drivers/media/v4l2-core/v4l2-dev.c:366
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:906 [inline]
 __se_sys_ioctl fs/ioctl.c:892 [inline]
 __x64_sys_ioctl+0x190/0x200 fs/ioctl.c:892
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f2b85c01b19
...

Reported-by: syzbot+5bcd7c809d365e14c4df@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=5bcd7c809d365e14c4df
Fixes: adc589d2a208 ("media: vimc: Add vimc-streamer for stream control")
Cc: stable@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 drivers/media/test-drivers/vimc/vimc-streamer.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
index 807551a5143b..64dd7e0ea5ad 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -59,6 +59,12 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
 			continue;
 
 		sd = media_entity_to_v4l2_subdev(ved->ent);
+		/*
+		 * Do not call .s_stream() to stop an already
+		 * stopped/unstarted subdev.
+		 */
+		if (!sd->s_stream_enabled)
+			continue;
 		v4l2_subdev_call(sd, video, s_stream, 0);
 	}
 }

Re: [PATCH] media: vimc: skip .s_stream() for stopped entities

Posted by Shuah Khan 9 months, 3 weeks ago

On 2/24/25 08:49, Nikita Zhandarovich wrote:
> Syzbot reported [1] a warning prompted by a check in call_s_stream()
> that checks whether .s_stream() operation is warranted for unstarted
> or stopped subdevs.
> 
> Add a simple fix in vimc_streamer_pipeline_terminate() ensuring that
> entities skip a call to .s_stream() unless they have been previously
> properly started.
> 
> [1] Syzbot report:
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 5933 at drivers/media/v4l2-core/v4l2-subdev.c:460 call_s_stream+0x2df/0x350 drivers/media/v4l2-core/v4l2-subdev.c:460
> Modules linked in:
> CPU: 0 UID: 0 PID: 5933 Comm: syz-executor330 Not tainted 6.13.0-rc2-syzkaller-00362-g2d8308bf5b67 #0
> ...
> Call Trace:
>   <TASK>
>   vimc_streamer_pipeline_terminate+0x218/0x320 drivers/media/test-drivers/vimc/vimc-streamer.c:62
>   vimc_streamer_pipeline_init drivers/media/test-drivers/vimc/vimc-streamer.c:101 [inline]
>   vimc_streamer_s_stream+0x650/0x9a0 drivers/media/test-drivers/vimc/vimc-streamer.c:203
>   vimc_capture_start_streaming+0xa1/0x130 drivers/media/test-drivers/vimc/vimc-capture.c:256
>   vb2_start_streaming+0x15f/0x5a0 drivers/media/common/videobuf2/videobuf2-core.c:1789
>   vb2_core_streamon+0x2a7/0x450 drivers/media/common/videobuf2/videobuf2-core.c:2348
>   vb2_streamon drivers/media/common/videobuf2/videobuf2-v4l2.c:875 [inline]
>   vb2_ioctl_streamon+0xf4/0x170 drivers/media/common/videobuf2/videobuf2-v4l2.c:1118
>   __video_do_ioctl+0xaf0/0xf00 drivers/media/v4l2-core/v4l2-ioctl.c:3122
>   video_usercopy+0x4d2/0x1620 drivers/media/v4l2-core/v4l2-ioctl.c:3463
>   v4l2_ioctl+0x1ba/0x250 drivers/media/v4l2-core/v4l2-dev.c:366
>   vfs_ioctl fs/ioctl.c:51 [inline]
>   __do_sys_ioctl fs/ioctl.c:906 [inline]
>   __se_sys_ioctl fs/ioctl.c:892 [inline]
>   __x64_sys_ioctl+0x190/0x200 fs/ioctl.c:892
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f2b85c01b19
> ...
> 
> Reported-by: syzbot+5bcd7c809d365e14c4df@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=5bcd7c809d365e14c4df
> Fixes: adc589d2a208 ("media: vimc: Add vimc-streamer for stream control")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>   drivers/media/test-drivers/vimc/vimc-streamer.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index 807551a5143b..64dd7e0ea5ad 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -59,6 +59,12 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>   			continue;
>   
>   		sd = media_entity_to_v4l2_subdev(ved->ent);
> +		/*
> +		 * Do not call .s_stream() to stop an already
> +		 * stopped/unstarted subdev.
> +		 */
> +		if (!sd->s_stream_enabled)

The right interface to call is v4l2_subdev_is_streaming() instead
of checking s_stream_enabled directly.

> +			continue;
>   		v4l2_subdev_call(sd, video, s_stream, 0);
>   	}
>   }

Would it better to change vimc_streamer_pipeline_init() to not
call vimc_streamer_pipeline_terminate() instead here?

ret = v4l2_subdev_call(sd, video, s_stream, 1);
                         if (ret && ret != -ENOIOCTLCMD) {
                                 dev_err(ved->dev, "subdev_call error %s\n",
                                         ved->ent->name);
                                 vimc_streamer_pipeline_terminate(stream);
                                 return ret;
                         }


thanks,
-- Shuah

Re: [PATCH] media: vimc: skip .s_stream() for stopped entities

Posted by Nikita Zhandarovich 9 months, 3 weeks ago

Hi,

On 2/27/25 19:36, Shuah Khan wrote:
> On 2/24/25 08:49, Nikita Zhandarovich wrote:
>> Syzbot reported [1] a warning prompted by a check in call_s_stream()
>> that checks whether .s_stream() operation is warranted for unstarted
>> or stopped subdevs.
>>
>> Add a simple fix in vimc_streamer_pipeline_terminate() ensuring that
>> entities skip a call to .s_stream() unless they have been previously
>> properly started.
>>
>> [1] Syzbot report:
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 5933 at drivers/media/v4l2-core/v4l2-
>> subdev.c:460 call_s_stream+0x2df/0x350 drivers/media/v4l2-core/v4l2-
>> subdev.c:460
>> Modules linked in:
>> CPU: 0 UID: 0 PID: 5933 Comm: syz-executor330 Not tainted 6.13.0-rc2-
>> syzkaller-00362-g2d8308bf5b67 #0
>> ...
>> Call Trace:
>>   <TASK>
>>   vimc_streamer_pipeline_terminate+0x218/0x320 drivers/media/test-
>> drivers/vimc/vimc-streamer.c:62
>>   vimc_streamer_pipeline_init drivers/media/test-drivers/vimc/vimc-
>> streamer.c:101 [inline]
>>   vimc_streamer_s_stream+0x650/0x9a0 drivers/media/test-drivers/vimc/
>> vimc-streamer.c:203
>>   vimc_capture_start_streaming+0xa1/0x130 drivers/media/test-drivers/
>> vimc/vimc-capture.c:256
>>   vb2_start_streaming+0x15f/0x5a0 drivers/media/common/videobuf2/
>> videobuf2-core.c:1789
>>   vb2_core_streamon+0x2a7/0x450 drivers/media/common/videobuf2/
>> videobuf2-core.c:2348
>>   vb2_streamon drivers/media/common/videobuf2/videobuf2-v4l2.c:875
>> [inline]
>>   vb2_ioctl_streamon+0xf4/0x170 drivers/media/common/videobuf2/
>> videobuf2-v4l2.c:1118
>>   __video_do_ioctl+0xaf0/0xf00 drivers/media/v4l2-core/v4l2-ioctl.c:3122
>>   video_usercopy+0x4d2/0x1620 drivers/media/v4l2-core/v4l2-ioctl.c:3463
>>   v4l2_ioctl+0x1ba/0x250 drivers/media/v4l2-core/v4l2-dev.c:366
>>   vfs_ioctl fs/ioctl.c:51 [inline]
>>   __do_sys_ioctl fs/ioctl.c:906 [inline]
>>   __se_sys_ioctl fs/ioctl.c:892 [inline]
>>   __x64_sys_ioctl+0x190/0x200 fs/ioctl.c:892
>>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>   do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>> RIP: 0033:0x7f2b85c01b19
>> ...
>>
>> Reported-by: syzbot+5bcd7c809d365e14c4df@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=5bcd7c809d365e14c4df
>> Fixes: adc589d2a208 ("media: vimc: Add vimc-streamer for stream
>> control")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>> ---
>>   drivers/media/test-drivers/vimc/vimc-streamer.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/
>> drivers/media/test-drivers/vimc/vimc-streamer.c
>> index 807551a5143b..64dd7e0ea5ad 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
>> @@ -59,6 +59,12 @@ static void
>> vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
>>               continue;
>>             sd = media_entity_to_v4l2_subdev(ved->ent);
>> +        /*
>> +         * Do not call .s_stream() to stop an already
>> +         * stopped/unstarted subdev.
>> +         */
>> +        if (!sd->s_stream_enabled)
> 
> The right interface to call is v4l2_subdev_is_streaming() instead
> of checking s_stream_enabled directly.
> 

Agreed, thank you for your suggestion, I'll fix it in v2.

>> +            continue;
>>           v4l2_subdev_call(sd, video, s_stream, 0);
>>       }
>>   }
> 
> Would it better to change vimc_streamer_pipeline_init() to not
> call vimc_streamer_pipeline_terminate() instead here?
> 
> ret = v4l2_subdev_call(sd, video, s_stream, 1);
>                         if (ret && ret != -ENOIOCTLCMD) {
>                                 dev_err(ved->dev, "subdev_call error
> %s\n",
>                                         ved->ent->name);
>                                 vimc_streamer_pipeline_terminate(stream);
>                                 return ret;
>                         }
> 

Correct me if I'm wrong but since we go through several entities in
vimc_streamer_pipeline_init(), some of them may have had streaming
enabled before we trigger an error here. And if we decide to exit early
due to said error, this is the time to stop all previously started
entities.

> 
> thanks,
> -- Shuah

Regards,
Nikita