On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> On 11/16/2018 09:34 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >>
> >> Calling the stop_streaming op can release the core serialization lock
> >> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
> >> An example of that behavior is the vivid driver.
> >
> > Why would vb2_queue->lock have to be released to wait for buffer to
> > finish? The drivers I worked with never had to do anything like that.
>
> Actually, they all do. It's done through the wait_prepare/finish callbacks
> and by setting those to vb2_ops_wait_prepare/finish.
>
> If you don't, then while one thread is waiting for a buffer to arrive,
> another thread cannot queue a new buffer since it will be serialized by
> queue->lock.
>
> v4l2-compliance even tests for this.
Why would you need the userspace to queue more buffers when you're
stopping the queue?
>
> >
> >>
> >> However, if userspace dup()ped the video device filehandle, then it is
> >> possible to stop streaming on one filehandle and call read/write or
> >> VIDIOC_QBUF from the other.
> >
> > How about other ioctls? I can imagine at least STREAMON could be
> > called at the same time too, but not sure if it would have any side
> > effects.
>
> STREAMON would return an error since q->streaming is still set while
> in the stop_streaming callback.
>
> So that combination is safe.
>
Okay, thanks. I'm still slightly worried that this approach with a
flag makes it possible to miss some non-trivial cases, though...
> Regards,
>
> Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> This is fixed by setting a flag whenever stop_streaming is called and
> >> checking the flag where needed so we can return -EBUSY.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil@xxxxxxxxx>
> >> Reported-by: syzbot+736c3aae4af7b50d9683@xxxxxxxxxxxxxxxxxxxxxxxxx
> >> ---
> >> drivers/media/common/videobuf2/videobuf2-core.c | 14 +++++++++++++-
> >> include/media/videobuf2-core.h | 1 +
> >> 2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 138223af701f..560577321fe7 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> >> dprintk(1, "fatal error occurred on queue\n");
> >> return -EIO;
> >> }
> >> + if (q->in_stop_streaming) {
> >> + dprintk(1, "stop_streaming is called\n");
> >> + return -EBUSY;
> >> + }
> >>
> >> vb = q->bufs[index];
> >>
> >> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >> * Tell driver to stop all transactions and release all queued
> >> * buffers.
> >> */
> >> - if (q->start_streaming_called)
> >> + if (q->start_streaming_called) {
> >> + q->in_stop_streaming = 1;
> >> call_void_qop(q, stop_streaming, q);
> >> + q->in_stop_streaming = 0;
> >> + }
> >>
> >> /*
> >> * If you see this warning, then the driver isn't cleaning up properly
> >> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> >> return -EBUSY;
> >> }
> >>
> >> + if (q->in_stop_streaming) {
> >> + dprintk(3, "stop_streaming is called\n");
> >> + return -EBUSY;
> >> + }
> >> +
> >> /*
> >> * Initialize emulator on first call.
> >> */
> >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> >> index 613f22910174..5a3d3ada5940 100644
> >> --- a/include/media/videobuf2-core.h
> >> +++ b/include/media/videobuf2-core.h
> >> @@ -585,6 +585,7 @@ struct vb2_queue {
> >> unsigned int error:1;
> >> unsigned int waiting_for_buffers:1;
> >> unsigned int waiting_in_dqbuf:1;
> >> + unsigned int in_stop_streaming:1;
> >> unsigned int is_multiplanar:1;
> >> unsigned int is_output:1;
> >> unsigned int copy_timestamp:1;
> >> --
> >> 2.19.1
> >>
>
