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