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