The stop_streaming callback is called with the queue lock held. But some drivers (vivid being one of them) need to stop a kernel thread in stop_streaming, and if that kernel thread takes the same queue lock, then stop_streaming may have to unlock the queue lock, stop the thread, and lock it again.
However, if you do that, then you must ensure that no other operations can take place that can change the list of buffers queued to the driver, specifically: QBUF, STREAMON/OFF, REQBUFS, CREATE_BUFS. __vb2_wait_for_done_vb() also checks for this and won't try to wait for new buffers if in_stop_streaming is true. This issue caused this syzbot report: https://syzkaller.appspot.com/bug?extid=736c3aae4af7b50d9683 Signed-off-by: Hans Verkuil <hverkuil-ci...@xs4all.nl> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com --- diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 4489744fbbd9..7c70bb9f6cb8 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -677,6 +677,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, return -EBUSY; } + if (q->in_stop_streaming) { + dprintk(1, "reqbufs while the stream is being stopped\n"); + return -EBUSY; + } + if (q->waiting_in_dqbuf && *count) { dprintk(1, "another dup()ped fd is waiting for a buffer\n"); return -EBUSY; @@ -811,6 +816,11 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, unsigned plane_sizes[VB2_MAX_PLANES] = { }; int ret; + if (q->in_stop_streaming) { + dprintk(1, "create_bufs while the stream is being stopped\n"); + return -EBUSY; + } + if (q->num_buffers == VB2_MAX_FRAME) { dprintk(1, "maximum number of buffers already allocated\n"); return -ENOBUFS; @@ -1514,6 +1524,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, struct vb2_buffer *vb; int ret; + if (q->in_stop_streaming) { + dprintk(1, "qbuf while the stream is being stopped\n"); + return -EBUSY; + } + if (q->error) { dprintk(1, "fatal error occurred on queue\n"); return -EIO; @@ -1675,6 +1690,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) return -EBUSY; } + if (q->in_stop_streaming) { + dprintk(1, "the stream is being stopped, will not wait for buffers\n"); + return -EINVAL; + } + if (!q->streaming) { dprintk(1, "streaming off, will not wait for buffers\n"); return -EINVAL; @@ -1716,7 +1736,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) dprintk(3, "will sleep waiting for buffers\n"); ret = wait_event_interruptible(q->done_wq, !list_empty(&q->done_list) || !q->streaming || - q->error); + q->in_stop_streaming || q->error); /* * We need to reevaluate both conditions again after reacquiring @@ -1866,12 +1886,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q) { unsigned int i; + if (WARN_ON(q->in_stop_streaming)) + return; + /* * 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 @@ -1975,6 +2001,11 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type) return -EINVAL; } + if (q->in_stop_streaming) { + dprintk(1, "streamon while the stream is being stopped\n"); + return -EBUSY; + } + if (q->streaming) { dprintk(3, "already streaming\n"); return 0; @@ -2026,6 +2057,11 @@ int vb2_core_streamoff(struct vb2_queue *q, unsigned int type) return -EINVAL; } + if (q->in_stop_streaming) { + dprintk(1, "streamoff while the stream is being stopped\n"); + return -EBUSY; + } + /* * Cancel will pause streaming and remove all buffers from the driver * and videobuf, effectively returning control over them to userspace. diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 640aabe69450..c4b7edd25e13 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -535,6 +535,9 @@ struct vb2_buf_ops { * @streaming: current streaming state * @start_streaming_called: @start_streaming was called successfully and we * started streaming. + * @in_stop_streaming: set when calling @stop_streaming. While 1, no new buffers + * can be queued or created, and neither is it possible to start or + * stop streaming. * @error: a fatal error occurred on the queue * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for * buffers. Only set for capture queues if qbuf has not yet been @@ -595,6 +598,7 @@ struct vb2_queue { unsigned int streaming:1; unsigned int start_streaming_called:1; + unsigned int in_stop_streaming:1; unsigned int error:1; unsigned int waiting_for_buffers:1; unsigned int waiting_in_dqbuf:1;
