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@xxxxxxxxxxxxxxxxxxxxxxxxx >> Closes: https://syzkaller.appspot.com/bug?extid=5bcd7c809d365e14c4df >> Fixes: adc589d2a208 ("media: vimc: Add vimc-streamer for stream >> control") >> Cc: stable@xxxxxxxxxxxxxxx >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> >> --- >> 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
