[PATCH 0/2] vb2: fix syzkaller race conditions

[PATCH 0/2] vb2: fix syzkaller race conditions

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

These two patches fix syzkaller race conditions. The basic problem is
the same for both: in some specific cases an ioctl can release the
core serialization mutex, thus allowing another thread to access the
same vb2 queue through a dup()ped filehandle.

This can happen in VIDIOC_DQBUF and read()/write() (this calls dqbuf
internally, so it is really the same problem): if no new buffer is
available the DQBUF ioctl will block and wait for a new
buffer to arrive. Before it waits it releases the serialization lock,
and afterwards it reacquires it. This is intentional, since you do not
want to block other ioctls while waiting for a buffer.

But this means that you need to flag that you are waiting for a buffer
and check the flag in the appropriate places.

The same can happen in the stop_streaming operation: the driver may
have to release the serialization lock while waiting for streaming to
stop (vivid does this). The same approach is used to prevent new
read()s/write()s or QBUF ioctls while it is in stop_streaming.

These flags are always checked/set with the serialization mutex locked.

Regards,

	Hans

Hans Verkuil (2):
  vb2: add waiting_in_dqbuf flag
  vb2: don't allow queueing buffers when canceling queue

 .../media/common/videobuf2/videobuf2-core.c   | 32 ++++++++++++++++++-
 include/media/videobuf2-core.h                |  2 ++
 2 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.19.1

[PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Hans Verkuil]

Calling VIDIOC_DQBUF can release the core serialization lock pointed to
by vb2_queue->lock if it has to wait for a new buffer to arrive.

However, if userspace dup()ped the video device filehandle, then it is
possible to read or call DQBUF from two filehandles at the same time.

It is also possible to call REQBUFS from one filehandle while the other
is waiting for a buffer. This will remove all the buffers and reallocate
new ones. Removing all the buffers isn't the problem here (that's already
handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
aware that the buffers have changed.

This is fixed by setting a flag whenever the lock is released while waiting
for a buffer to arrive. And checking the flag where needed so we can return
-EBUSY.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 .../media/common/videobuf2/videobuf2-core.c    | 18 ++++++++++++++++++
 include/media/videobuf2-core.h                 |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 03954c13024c..138223af701f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 		return -EBUSY;
 	}
 
+	if (q->waiting_in_dqbuf && *count) {
+		dprintk(1, "another dup()ped fd is waiting for a buffer\n");
+		return -EBUSY;
+	}
+
 	if (*count == 0 || q->num_buffers != 0 ||
 	    (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
 		/*
@@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 	for (;;) {
 		int ret;
 
+		if (q->waiting_in_dqbuf) {
+			dprintk(1, "another dup()ped fd is waiting for a buffer\n");
+			return -EBUSY;
+		}
+
 		if (!q->streaming) {
 			dprintk(1, "streaming off, will not wait for buffers\n");
 			return -EINVAL;
@@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 			return -EAGAIN;
 		}
 
+		q->waiting_in_dqbuf = 1;
 		/*
 		 * We are streaming and blocking, wait for another buffer to
 		 * become ready or for streamoff. Driver's lock is released to
@@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 		 * the locks or return an error if one occurred.
 		 */
 		call_void_qop(q, wait_finish, q);
+		q->waiting_in_dqbuf = 0;
 		if (ret) {
 			dprintk(1, "sleep was interrupted\n");
 			return ret;
@@ -2547,6 +2559,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	if (!data)
 		return -EINVAL;
 
+	if (q->waiting_in_dqbuf) {
+		dprintk(3, "another dup()ped fd is %s\n",
+			read ? "reading" : "writing");
+		return -EBUSY;
+	}
+
 	/*
 	 * Initialize emulator on first call.
 	 */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..613f22910174 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -584,6 +584,7 @@ struct vb2_queue {
 	unsigned int			start_streaming_called:1;
 	unsigned int			error:1;
 	unsigned int			waiting_for_buffers:1;
+	unsigned int			waiting_in_dqbuf:1;
 	unsigned int			is_multiplanar:1;
 	unsigned int			is_output:1;
 	unsigned int			copy_timestamp:1;
-- 
2.19.1

[PATCH 2/2] vb2: don't allow queueing buffers when canceling queue

[Hans Verkuil]

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.

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.

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@xs4all.nl>
Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
---
 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

Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue

[Tomasz Figa]

Hi Hans,

On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl>
> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
> ---
>  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
>

Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue

[Hans Verkuil]

On 11/16/2018 09:34 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl>
>> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
>> ---
>>  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
>>

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Tomasz Figa]

Hi Hans,

On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>
> However, if userspace dup()ped the video device filehandle, then it is
> possible to read or call DQBUF from two filehandles at the same time.
>

What side effects would reading have?

As for another DQBUF in parallel, perhaps that's actually a valid
operation that should be handled? I can imagine that one could want to
have multiple threads dequeuing buffers as they become available, so
that no dispatch thread is needed.

> It is also possible to call REQBUFS from one filehandle while the other
> is waiting for a buffer. This will remove all the buffers and reallocate
> new ones. Removing all the buffers isn't the problem here (that's already
> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
> aware that the buffers have changed.
>
> This is fixed by setting a flag whenever the lock is released while waiting
> for a buffer to arrive. And checking the flag where needed so we can return
> -EBUSY.

Maybe it would make more sense to actually handle those side effects?
Such waiting DQBUF would then just fail in the same way as if it
couldn't get a buffer (or if it's blocking, just retry until a correct
buffer becomes available?).

Best regards,
Tomasz

>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  .../media/common/videobuf2/videobuf2-core.c    | 18 ++++++++++++++++++
>  include/media/videobuf2-core.h                 |  1 +
>  2 files changed, 19 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 03954c13024c..138223af701f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>                 return -EBUSY;
>         }
>
> +       if (q->waiting_in_dqbuf && *count) {
> +               dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> +               return -EBUSY;
> +       }
> +
>         if (*count == 0 || q->num_buffers != 0 ||
>             (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
>                 /*
> @@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>         for (;;) {
>                 int ret;
>
> +               if (q->waiting_in_dqbuf) {
> +                       dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> +                       return -EBUSY;
> +               }
> +
>                 if (!q->streaming) {
>                         dprintk(1, "streaming off, will not wait for buffers\n");
>                         return -EINVAL;
> @@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>                         return -EAGAIN;
>                 }
>
> +               q->waiting_in_dqbuf = 1;
>                 /*
>                  * We are streaming and blocking, wait for another buffer to
>                  * become ready or for streamoff. Driver's lock is released to
> @@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>                  * the locks or return an error if one occurred.
>                  */
>                 call_void_qop(q, wait_finish, q);
> +               q->waiting_in_dqbuf = 0;
>                 if (ret) {
>                         dprintk(1, "sleep was interrupted\n");
>                         return ret;
> @@ -2547,6 +2559,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>         if (!data)
>                 return -EINVAL;
>
> +       if (q->waiting_in_dqbuf) {
> +               dprintk(3, "another dup()ped fd is %s\n",
> +                       read ? "reading" : "writing");
> +               return -EBUSY;
> +       }
> +
>         /*
>          * Initialize emulator on first call.
>          */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e86981d615ae..613f22910174 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -584,6 +584,7 @@ struct vb2_queue {
>         unsigned int                    start_streaming_called:1;
>         unsigned int                    error:1;
>         unsigned int                    waiting_for_buffers:1;
> +       unsigned int                    waiting_in_dqbuf:1;
>         unsigned int                    is_multiplanar:1;
>         unsigned int                    is_output:1;
>         unsigned int                    copy_timestamp:1;
> --
> 2.19.1
>

Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue

[Tomasz Figa]

On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl> 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@xs4all.nl>
> >> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
> >> ---
> >>  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
> >>
>

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Hans Verkuil]

On 11/16/2018 09:43 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>
>> However, if userspace dup()ped the video device filehandle, then it is
>> possible to read or call DQBUF from two filehandles at the same time.
>>
> 
> What side effects would reading have?
> 
> As for another DQBUF in parallel, perhaps that's actually a valid
> operation that should be handled? I can imagine that one could want to
> have multiple threads dequeuing buffers as they become available, so
> that no dispatch thread is needed.

I think parallel DQBUFs can be done, but it has never been tested, nor
has vb2 been designed with that in mind. I also don't see the use-case
since if you have, say, two DQBUFs in parallel, then it will be random
which DQBUF gets which frame.

If we ever see a need for this, then that needs to be designed and tested
properly.

> 
>> It is also possible to call REQBUFS from one filehandle while the other
>> is waiting for a buffer. This will remove all the buffers and reallocate
>> new ones. Removing all the buffers isn't the problem here (that's already
>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>> aware that the buffers have changed.
>>
>> This is fixed by setting a flag whenever the lock is released while waiting
>> for a buffer to arrive. And checking the flag where needed so we can return
>> -EBUSY.
> 
> Maybe it would make more sense to actually handle those side effects?
> Such waiting DQBUF would then just fail in the same way as if it
> couldn't get a buffer (or if it's blocking, just retry until a correct
> buffer becomes available?).

That sounds like a good idea, but it isn't.

With this patch you can't call REQBUFS to reallocate buffers while a thread
is waiting for a buffer.

If I allow this, then the problem moves to when the thread that called REQBUFS
calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
that only the DQBUF that relied on the old buffers is stopped?

It sounds nice, but the more I think about it, the more problems I see with it.

I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
thread is still waiting for a buffer.

That said, I think one test is missing in vb2_core_create_bufs: there too it
should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.

Admittedly, that would require some extremely unfortunate scheduling, but
it is easy enough to check this.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>>
>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>> ---
>>  .../media/common/videobuf2/videobuf2-core.c    | 18 ++++++++++++++++++
>>  include/media/videobuf2-core.h                 |  1 +
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 03954c13024c..138223af701f 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>                 return -EBUSY;
>>         }
>>
>> +       if (q->waiting_in_dqbuf && *count) {
>> +               dprintk(1, "another dup()ped fd is waiting for a buffer\n");
>> +               return -EBUSY;
>> +       }
>> +
>>         if (*count == 0 || q->num_buffers != 0 ||
>>             (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
>>                 /*
>> @@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>>         for (;;) {
>>                 int ret;
>>
>> +               if (q->waiting_in_dqbuf) {
>> +                       dprintk(1, "another dup()ped fd is waiting for a buffer\n");
>> +                       return -EBUSY;
>> +               }
>> +
>>                 if (!q->streaming) {
>>                         dprintk(1, "streaming off, will not wait for buffers\n");
>>                         return -EINVAL;
>> @@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>>                         return -EAGAIN;
>>                 }
>>
>> +               q->waiting_in_dqbuf = 1;
>>                 /*
>>                  * We are streaming and blocking, wait for another buffer to
>>                  * become ready or for streamoff. Driver's lock is released to
>> @@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
>>                  * the locks or return an error if one occurred.
>>                  */
>>                 call_void_qop(q, wait_finish, q);
>> +               q->waiting_in_dqbuf = 0;
>>                 if (ret) {
>>                         dprintk(1, "sleep was interrupted\n");
>>                         return ret;
>> @@ -2547,6 +2559,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>>         if (!data)
>>                 return -EINVAL;
>>
>> +       if (q->waiting_in_dqbuf) {
>> +               dprintk(3, "another dup()ped fd is %s\n",
>> +                       read ? "reading" : "writing");
>> +               return -EBUSY;
>> +       }
>> +
>>         /*
>>          * Initialize emulator on first call.
>>          */
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index e86981d615ae..613f22910174 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -584,6 +584,7 @@ struct vb2_queue {
>>         unsigned int                    start_streaming_called:1;
>>         unsigned int                    error:1;
>>         unsigned int                    waiting_for_buffers:1;
>> +       unsigned int                    waiting_in_dqbuf:1;
>>         unsigned int                    is_multiplanar:1;
>>         unsigned int                    is_output:1;
>>         unsigned int                    copy_timestamp:1;
>> --
>> 2.19.1
>>

Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue

[Hans Verkuil]

On 11/16/2018 09:45 AM, Tomasz Figa wrote:
> On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl> 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?

Ah, I misunderstood your question. Your question was: why should stop_streaming
have to release the lock when it waits for buffers to finish.

In this case (vivid) the thread generating the image takes the main lock, which
is the same as queue->lock. So stop_streaming (which is called with the same
lock taken) has to unlock it, stop the thread, then retake it.

I thought this would be more common, but after analyzing other usages of kthread
it appears to be vivid specific. So I agree that it is better to fix vivid
instead of messing about with vb2.

Regards,

	Hans

> 
>>
>>>
>>>>
>>>> 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@xs4all.nl>
>>>> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
>>>> ---
>>>>  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
>>>>
>>

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Tomasz Figa]

On Fri, Nov 16, 2018 at 6:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/16/2018 09:43 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
> >> by vb2_queue->lock if it has to wait for a new buffer to arrive.
> >>
> >> However, if userspace dup()ped the video device filehandle, then it is
> >> possible to read or call DQBUF from two filehandles at the same time.
> >>
> >
> > What side effects would reading have?
> >
> > As for another DQBUF in parallel, perhaps that's actually a valid
> > operation that should be handled? I can imagine that one could want to
> > have multiple threads dequeuing buffers as they become available, so
> > that no dispatch thread is needed.
>
> I think parallel DQBUFs can be done, but it has never been tested, nor
> has vb2 been designed with that in mind. I also don't see the use-case
> since if you have, say, two DQBUFs in parallel, then it will be random
> which DQBUF gets which frame.
>

Any post processing that operates only on single frame data would be
able to benefit from multiple threads, with results ordered after the
processing, based on timestamps.

Still, if that's not something we've ever claimed as supported and
couldn't work correctly with current code, it sounds fair to
completely forbid it for now.

> If we ever see a need for this, then that needs to be designed and tested
> properly.
>
> >
> >> It is also possible to call REQBUFS from one filehandle while the other
> >> is waiting for a buffer. This will remove all the buffers and reallocate
> >> new ones. Removing all the buffers isn't the problem here (that's already
> >> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
> >> aware that the buffers have changed.
> >>
> >> This is fixed by setting a flag whenever the lock is released while waiting
> >> for a buffer to arrive. And checking the flag where needed so we can return
> >> -EBUSY.
> >
> > Maybe it would make more sense to actually handle those side effects?
> > Such waiting DQBUF would then just fail in the same way as if it
> > couldn't get a buffer (or if it's blocking, just retry until a correct
> > buffer becomes available?).
>
> That sounds like a good idea, but it isn't.
>
> With this patch you can't call REQBUFS to reallocate buffers while a thread
> is waiting for a buffer.
>
> If I allow this, then the problem moves to when the thread that called REQBUFS
> calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
> mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
> that only the DQBUF that relied on the old buffers is stopped?
>
> It sounds nice, but the more I think about it, the more problems I see with it.
>
> I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
> thread is still waiting for a buffer.
>
> That said, I think one test is missing in vb2_core_create_bufs: there too it
> should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
> REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
> buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
>
> Admittedly, that would require some extremely unfortunate scheduling, but
> it is easy enough to check this.

I thought a bit more about this and I agree with you. We should keep
things as simple as possible.

Another thing that came to my mind is that the problematic scenario
described in the commit message can happen only if queue->lock ==
dev->lock. I wonder how likely it would be to mandate queue->lock !=
dev->lock?

Best regards,
Tomasz

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Hans Verkuil]

On 11/19/2018 06:27 AM, Tomasz Figa wrote:
> On Fri, Nov 16, 2018 at 6:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/16/2018 09:43 AM, Tomasz Figa wrote:
>>> Hi Hans,
>>>
>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>>>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>>>
>>>> However, if userspace dup()ped the video device filehandle, then it is
>>>> possible to read or call DQBUF from two filehandles at the same time.
>>>>
>>>
>>> What side effects would reading have?
>>>
>>> As for another DQBUF in parallel, perhaps that's actually a valid
>>> operation that should be handled? I can imagine that one could want to
>>> have multiple threads dequeuing buffers as they become available, so
>>> that no dispatch thread is needed.
>>
>> I think parallel DQBUFs can be done, but it has never been tested, nor
>> has vb2 been designed with that in mind. I also don't see the use-case
>> since if you have, say, two DQBUFs in parallel, then it will be random
>> which DQBUF gets which frame.
>>
> 
> Any post processing that operates only on single frame data would be
> able to benefit from multiple threads, with results ordered after the
> processing, based on timestamps.
> 
> Still, if that's not something we've ever claimed as supported and
> couldn't work correctly with current code, it sounds fair to
> completely forbid it for now.
> 
>> If we ever see a need for this, then that needs to be designed and tested
>> properly.
>>
>>>
>>>> It is also possible to call REQBUFS from one filehandle while the other
>>>> is waiting for a buffer. This will remove all the buffers and reallocate
>>>> new ones. Removing all the buffers isn't the problem here (that's already
>>>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>>>> aware that the buffers have changed.
>>>>
>>>> This is fixed by setting a flag whenever the lock is released while waiting
>>>> for a buffer to arrive. And checking the flag where needed so we can return
>>>> -EBUSY.
>>>
>>> Maybe it would make more sense to actually handle those side effects?
>>> Such waiting DQBUF would then just fail in the same way as if it
>>> couldn't get a buffer (or if it's blocking, just retry until a correct
>>> buffer becomes available?).
>>
>> That sounds like a good idea, but it isn't.
>>
>> With this patch you can't call REQBUFS to reallocate buffers while a thread
>> is waiting for a buffer.
>>
>> If I allow this, then the problem moves to when the thread that called REQBUFS
>> calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
>> mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
>> that only the DQBUF that relied on the old buffers is stopped?
>>
>> It sounds nice, but the more I think about it, the more problems I see with it.
>>
>> I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
>> thread is still waiting for a buffer.
>>
>> That said, I think one test is missing in vb2_core_create_bufs: there too it
>> should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
>> REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
>> buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
>>
>> Admittedly, that would require some extremely unfortunate scheduling, but
>> it is easy enough to check this.
> 
> I thought a bit more about this and I agree with you. We should keep
> things as simple as possible.
> 
> Another thing that came to my mind is that the problematic scenario
> described in the commit message can happen only if queue->lock ==
> dev->lock. I wonder how likely it would be to mandate queue->lock !=
> dev->lock?

My plan is to switch vivid to that model. Expect patches for that today.
One thing I noticed is that there is an issue with calling queue_setup
in that case. I have a separate patch for that, so just read it when I
post it.

Regards,

	Hans

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Hans Verkuil]

On 11/19/2018 09:44 AM, Hans Verkuil wrote:
> On 11/19/2018 06:27 AM, Tomasz Figa wrote:
>> On Fri, Nov 16, 2018 at 6:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>> On 11/16/2018 09:43 AM, Tomasz Figa wrote:
>>>> Hi Hans,
>>>>
>>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>
>>>>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>>>>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>>>>
>>>>> However, if userspace dup()ped the video device filehandle, then it is
>>>>> possible to read or call DQBUF from two filehandles at the same time.
>>>>>
>>>>
>>>> What side effects would reading have?
>>>>
>>>> As for another DQBUF in parallel, perhaps that's actually a valid
>>>> operation that should be handled? I can imagine that one could want to
>>>> have multiple threads dequeuing buffers as they become available, so
>>>> that no dispatch thread is needed.
>>>
>>> I think parallel DQBUFs can be done, but it has never been tested, nor
>>> has vb2 been designed with that in mind. I also don't see the use-case
>>> since if you have, say, two DQBUFs in parallel, then it will be random
>>> which DQBUF gets which frame.
>>>
>>
>> Any post processing that operates only on single frame data would be
>> able to benefit from multiple threads, with results ordered after the
>> processing, based on timestamps.
>>
>> Still, if that's not something we've ever claimed as supported and
>> couldn't work correctly with current code, it sounds fair to
>> completely forbid it for now.
>>
>>> If we ever see a need for this, then that needs to be designed and tested
>>> properly.
>>>
>>>>
>>>>> It is also possible to call REQBUFS from one filehandle while the other
>>>>> is waiting for a buffer. This will remove all the buffers and reallocate
>>>>> new ones. Removing all the buffers isn't the problem here (that's already
>>>>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>>>>> aware that the buffers have changed.
>>>>>
>>>>> This is fixed by setting a flag whenever the lock is released while waiting
>>>>> for a buffer to arrive. And checking the flag where needed so we can return
>>>>> -EBUSY.
>>>>
>>>> Maybe it would make more sense to actually handle those side effects?
>>>> Such waiting DQBUF would then just fail in the same way as if it
>>>> couldn't get a buffer (or if it's blocking, just retry until a correct
>>>> buffer becomes available?).
>>>
>>> That sounds like a good idea, but it isn't.
>>>
>>> With this patch you can't call REQBUFS to reallocate buffers while a thread
>>> is waiting for a buffer.
>>>
>>> If I allow this, then the problem moves to when the thread that called REQBUFS
>>> calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
>>> mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
>>> that only the DQBUF that relied on the old buffers is stopped?
>>>
>>> It sounds nice, but the more I think about it, the more problems I see with it.
>>>
>>> I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
>>> thread is still waiting for a buffer.
>>>
>>> That said, I think one test is missing in vb2_core_create_bufs: there too it
>>> should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
>>> REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
>>> buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
>>>
>>> Admittedly, that would require some extremely unfortunate scheduling, but
>>> it is easy enough to check this.
>>
>> I thought a bit more about this and I agree with you. We should keep
>> things as simple as possible.
>>
>> Another thing that came to my mind is that the problematic scenario
>> described in the commit message can happen only if queue->lock ==
>> dev->lock. I wonder how likely it would be to mandate queue->lock !=
>> dev->lock?
> 
> My plan is to switch vivid to that model. Expect patches for that today.
> One thing I noticed is that there is an issue with calling queue_setup
> in that case. I have a separate patch for that, so just read it when I
> post it.

Note that this specific scenario can happen regardless of whether
queue->lock == dev->lock or not.

Regards,

	Hans

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Tomasz Figa]

On Mon, Nov 19, 2018 at 6:54 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/19/2018 09:44 AM, Hans Verkuil wrote:
> > On 11/19/2018 06:27 AM, Tomasz Figa wrote:
> >> On Fri, Nov 16, 2018 at 6:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>
> >>> On 11/16/2018 09:43 AM, Tomasz Figa wrote:
> >>>> Hi Hans,
> >>>>
> >>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>
> >>>>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
> >>>>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
> >>>>>
> >>>>> However, if userspace dup()ped the video device filehandle, then it is
> >>>>> possible to read or call DQBUF from two filehandles at the same time.
> >>>>>
> >>>>
> >>>> What side effects would reading have?
> >>>>
> >>>> As for another DQBUF in parallel, perhaps that's actually a valid
> >>>> operation that should be handled? I can imagine that one could want to
> >>>> have multiple threads dequeuing buffers as they become available, so
> >>>> that no dispatch thread is needed.
> >>>
> >>> I think parallel DQBUFs can be done, but it has never been tested, nor
> >>> has vb2 been designed with that in mind. I also don't see the use-case
> >>> since if you have, say, two DQBUFs in parallel, then it will be random
> >>> which DQBUF gets which frame.
> >>>
> >>
> >> Any post processing that operates only on single frame data would be
> >> able to benefit from multiple threads, with results ordered after the
> >> processing, based on timestamps.
> >>
> >> Still, if that's not something we've ever claimed as supported and
> >> couldn't work correctly with current code, it sounds fair to
> >> completely forbid it for now.
> >>
> >>> If we ever see a need for this, then that needs to be designed and tested
> >>> properly.
> >>>
> >>>>
> >>>>> It is also possible to call REQBUFS from one filehandle while the other
> >>>>> is waiting for a buffer. This will remove all the buffers and reallocate
> >>>>> new ones. Removing all the buffers isn't the problem here (that's already
> >>>>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
> >>>>> aware that the buffers have changed.
> >>>>>
> >>>>> This is fixed by setting a flag whenever the lock is released while waiting
> >>>>> for a buffer to arrive. And checking the flag where needed so we can return
> >>>>> -EBUSY.
> >>>>
> >>>> Maybe it would make more sense to actually handle those side effects?
> >>>> Such waiting DQBUF would then just fail in the same way as if it
> >>>> couldn't get a buffer (or if it's blocking, just retry until a correct
> >>>> buffer becomes available?).
> >>>
> >>> That sounds like a good idea, but it isn't.
> >>>
> >>> With this patch you can't call REQBUFS to reallocate buffers while a thread
> >>> is waiting for a buffer.
> >>>
> >>> If I allow this, then the problem moves to when the thread that called REQBUFS
> >>> calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
> >>> mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
> >>> that only the DQBUF that relied on the old buffers is stopped?
> >>>
> >>> It sounds nice, but the more I think about it, the more problems I see with it.
> >>>
> >>> I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
> >>> thread is still waiting for a buffer.
> >>>
> >>> That said, I think one test is missing in vb2_core_create_bufs: there too it
> >>> should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
> >>> REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
> >>> buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
> >>>
> >>> Admittedly, that would require some extremely unfortunate scheduling, but
> >>> it is easy enough to check this.
> >>
> >> I thought a bit more about this and I agree with you. We should keep
> >> things as simple as possible.
> >>
> >> Another thing that came to my mind is that the problematic scenario
> >> described in the commit message can happen only if queue->lock ==
> >> dev->lock. I wonder how likely it would be to mandate queue->lock !=
> >> dev->lock?
> >
> > My plan is to switch vivid to that model. Expect patches for that today.
> > One thing I noticed is that there is an issue with calling queue_setup
> > in that case. I have a separate patch for that, so just read it when I
> > post it.
>
> Note that this specific scenario can happen regardless of whether
> queue->lock == dev->lock or not.

Ah, good point. Somehow I assumed that only QBUF/DQBUF would use
queue->lock, while anything else would use dev->lock, but that's not
the case.

Then I can't find any simpler and/or more general fix for now, so I'm
okay with this.

Another note, don't we need similar error in case of REQBUFS(0), while
DQBUF() is waiting? Current patch seems to add one only for count !=
0.

Best regards,
Tomasz

Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag

[Hans Verkuil]

On 11/19/2018 11:32 AM, Tomasz Figa wrote:
> On Mon, Nov 19, 2018 at 6:54 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/19/2018 09:44 AM, Hans Verkuil wrote:
>>> On 11/19/2018 06:27 AM, Tomasz Figa wrote:
>>>> On Fri, Nov 16, 2018 at 6:45 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>
>>>>> On 11/16/2018 09:43 AM, Tomasz Figa wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>
>>>>>>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>>>>>>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>>>>>>
>>>>>>> However, if userspace dup()ped the video device filehandle, then it is
>>>>>>> possible to read or call DQBUF from two filehandles at the same time.
>>>>>>>
>>>>>>
>>>>>> What side effects would reading have?
>>>>>>
>>>>>> As for another DQBUF in parallel, perhaps that's actually a valid
>>>>>> operation that should be handled? I can imagine that one could want to
>>>>>> have multiple threads dequeuing buffers as they become available, so
>>>>>> that no dispatch thread is needed.
>>>>>
>>>>> I think parallel DQBUFs can be done, but it has never been tested, nor
>>>>> has vb2 been designed with that in mind. I also don't see the use-case
>>>>> since if you have, say, two DQBUFs in parallel, then it will be random
>>>>> which DQBUF gets which frame.
>>>>>
>>>>
>>>> Any post processing that operates only on single frame data would be
>>>> able to benefit from multiple threads, with results ordered after the
>>>> processing, based on timestamps.
>>>>
>>>> Still, if that's not something we've ever claimed as supported and
>>>> couldn't work correctly with current code, it sounds fair to
>>>> completely forbid it for now.
>>>>
>>>>> If we ever see a need for this, then that needs to be designed and tested
>>>>> properly.
>>>>>
>>>>>>
>>>>>>> It is also possible to call REQBUFS from one filehandle while the other
>>>>>>> is waiting for a buffer. This will remove all the buffers and reallocate
>>>>>>> new ones. Removing all the buffers isn't the problem here (that's already
>>>>>>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>>>>>>> aware that the buffers have changed.
>>>>>>>
>>>>>>> This is fixed by setting a flag whenever the lock is released while waiting
>>>>>>> for a buffer to arrive. And checking the flag where needed so we can return
>>>>>>> -EBUSY.
>>>>>>
>>>>>> Maybe it would make more sense to actually handle those side effects?
>>>>>> Such waiting DQBUF would then just fail in the same way as if it
>>>>>> couldn't get a buffer (or if it's blocking, just retry until a correct
>>>>>> buffer becomes available?).
>>>>>
>>>>> That sounds like a good idea, but it isn't.
>>>>>
>>>>> With this patch you can't call REQBUFS to reallocate buffers while a thread
>>>>> is waiting for a buffer.
>>>>>
>>>>> If I allow this, then the problem moves to when the thread that called REQBUFS
>>>>> calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
>>>>> mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
>>>>> that only the DQBUF that relied on the old buffers is stopped?
>>>>>
>>>>> It sounds nice, but the more I think about it, the more problems I see with it.
>>>>>
>>>>> I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
>>>>> thread is still waiting for a buffer.
>>>>>
>>>>> That said, I think one test is missing in vb2_core_create_bufs: there too it
>>>>> should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
>>>>> REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
>>>>> buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
>>>>>
>>>>> Admittedly, that would require some extremely unfortunate scheduling, but
>>>>> it is easy enough to check this.
>>>>
>>>> I thought a bit more about this and I agree with you. We should keep
>>>> things as simple as possible.
>>>>
>>>> Another thing that came to my mind is that the problematic scenario
>>>> described in the commit message can happen only if queue->lock ==
>>>> dev->lock. I wonder how likely it would be to mandate queue->lock !=
>>>> dev->lock?
>>>
>>> My plan is to switch vivid to that model. Expect patches for that today.
>>> One thing I noticed is that there is an issue with calling queue_setup
>>> in that case. I have a separate patch for that, so just read it when I
>>> post it.
>>
>> Note that this specific scenario can happen regardless of whether
>> queue->lock == dev->lock or not.
> 
> Ah, good point. Somehow I assumed that only QBUF/DQBUF would use
> queue->lock, while anything else would use dev->lock, but that's not
> the case.
> 
> Then I can't find any simpler and/or more general fix for now, so I'm
> okay with this.
> 
> Another note, don't we need similar error in case of REQBUFS(0), while
> DQBUF() is waiting? Current patch seems to add one only for count !=
> 0.

It's OK to delete since that will set q->streaming to false, and DQBUF will
return an error.

Regards,

	Hans