[PATCH v2 0/2] vb2: check for events before checking for buffers

[PATCH v2 0/2] vb2: check for events before checking for buffers

[Michael Tretter]

The patches fix a race condition in the poll functions of v4l2 devices.

Whenever a driver returns a buffer with the V4L2_BUF_FLAG_LAST flag set, it
must also return a V4L2_EVENT_EOS. Checking for events before checking for
buffers creates a race condition where drivers can return the buffer and event
between the checks and thus only signal the buffer without the event.
Reordering the checks avoids the race condition.

As suggested by Hans, I renamed __v4l2_m2m_poll() to v4l2_m2m_poll_for_data()
in patch 2/2.

Michael

Changelog:

v1 -> v2:
- rename __v4l2_m2m_poll() to v4l2_m2m_poll_for_data

Michael Tretter (2):
  media: vb2: reorder checks in vb2_poll()
  media: v4l2-mem2mem: reorder checks in v4l2_m2m_poll()

 .../media/common/videobuf2/videobuf2-v4l2.c   |  8 ++--
 drivers/media/v4l2-core/v4l2-mem2mem.c        | 47 +++++++++++--------
 2 files changed, 32 insertions(+), 23 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] media: vb2: reorder checks in vb2_poll()

[Michael Tretter]

When reaching the end of stream, V4L2 clients may expect the
V4L2_EOS_EVENT before being able to dequeue the last buffer, which has
the V4L2_BUF_FLAG_LAST flag set.

If the vb2_poll() function first checks for events and afterwards if
buffers are available, a driver can queue the V4L2_EOS_EVENT event and
return the buffer after the check for events but before the check for
buffers. This causes vb2_poll() to signal that the buffer with
V4L2_BUF_FLAG_LAST can be read without the V4L2_EOS_EVENT being
available.

First, check for available buffers and afterwards for events to ensure
that if vb2_poll() signals POLLIN | POLLRDNORM for the
V4L2_BUF_FLAG_LAST buffer, it also signals POLLPRI for the
V4L2_EOS_EVENT.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
v1 -> v2:
- none
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 40d76eb4c2fe..5a9ba3846f0a 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -872,17 +872,19 @@ EXPORT_SYMBOL_GPL(vb2_queue_release);
 __poll_t vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 {
 	struct video_device *vfd = video_devdata(file);
-	__poll_t res = 0;
+	__poll_t res;
+
+	res = vb2_core_poll(q, file, wait);
 
 	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
 		struct v4l2_fh *fh = file->private_data;
 
 		poll_wait(file, &fh->wait, wait);
 		if (v4l2_event_pending(fh))
-			res = EPOLLPRI;
+			res |= EPOLLPRI;
 	}
 
-	return res | vb2_core_poll(q, file, wait);
+	return res;
 }
 EXPORT_SYMBOL_GPL(vb2_poll);
 
-- 
2.20.1

[PATCH v2 2/2] media: v4l2-mem2mem: reorder checks in v4l2_m2m_poll()

[Michael Tretter]

When reaching the end of stream, V4L2 m2m clients may expect the
V4L2_EOS_EVENT. Although the V4L2_EOS_EVENT is deprecated behavior,
drivers must signal that event before dequeuing the buffer that has the
V4L2_BUF_FLAG_LAST flag set.

If a driver queues the V4L2_EOS_EVENT event and returns the buffer after
the check for events but before the check for buffers, vb2_m2m_poll()
will signal that the buffer with V4L2_BUF_FLAG_LAST can be read but not
that the V4L2_EOS_EVENT is available.

Split the check for buffers into a separate function and check for
available buffers before checking for events. This ensures that if
vb2_m2m_poll() signals POLLIN | POLLRDNORM for the V4L2_BUF_FLAG_LAST
buffer, it signals POLLPRI for the V4L2_EOS_EVENT, too.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
v1 -> v2:
- rename __v4l2_m2m_poll() to v4l2_m2m_poll_for_data()
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 47 +++++++++++++++-----------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 4f5176702937..e21aa05245dc 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -603,11 +603,10 @@ int v4l2_m2m_streamoff(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_streamoff);
 
-__poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
-			   struct poll_table_struct *wait)
+static __poll_t v4l2_m2m_poll_for_data(struct file *file,
+				       struct v4l2_m2m_ctx *m2m_ctx,
+				       struct poll_table_struct *wait)
 {
-	struct video_device *vfd = video_devdata(file);
-	__poll_t req_events = poll_requested_events(wait);
 	struct vb2_queue *src_q, *dst_q;
 	struct vb2_buffer *src_vb = NULL, *dst_vb = NULL;
 	__poll_t rc = 0;
@@ -619,16 +618,6 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 	poll_wait(file, &src_q->done_wq, wait);
 	poll_wait(file, &dst_q->done_wq, wait);
 
-	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
-		struct v4l2_fh *fh = file->private_data;
-
-		poll_wait(file, &fh->wait, wait);
-		if (v4l2_event_pending(fh))
-			rc = EPOLLPRI;
-		if (!(req_events & (EPOLLOUT | EPOLLWRNORM | EPOLLIN | EPOLLRDNORM)))
-			return rc;
-	}
-
 	/*
 	 * There has to be at least one buffer queued on each queued_list, which
 	 * means either in driver already or waiting for driver to claim it
@@ -637,10 +626,8 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 	if ((!src_q->streaming || src_q->error ||
 	     list_empty(&src_q->queued_list)) &&
 	    (!dst_q->streaming || dst_q->error ||
-	     list_empty(&dst_q->queued_list))) {
-		rc |= EPOLLERR;
-		goto end;
-	}
+	     list_empty(&dst_q->queued_list)))
+		return EPOLLERR;
 
 	spin_lock_irqsave(&dst_q->done_lock, flags);
 	if (list_empty(&dst_q->done_list)) {
@@ -650,7 +637,7 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 		 */
 		if (dst_q->last_buffer_dequeued) {
 			spin_unlock_irqrestore(&dst_q->done_lock, flags);
-			return rc | EPOLLIN | EPOLLRDNORM;
+			return EPOLLIN | EPOLLRDNORM;
 		}
 	}
 	spin_unlock_irqrestore(&dst_q->done_lock, flags);
@@ -673,7 +660,27 @@ __poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 		rc |= EPOLLIN | EPOLLRDNORM;
 	spin_unlock_irqrestore(&dst_q->done_lock, flags);
 
-end:
+	return rc;
+}
+
+__poll_t v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+			   struct poll_table_struct *wait)
+{
+	struct video_device *vfd = video_devdata(file);
+	__poll_t req_events = poll_requested_events(wait);
+	__poll_t rc = 0;
+
+	if (req_events & (EPOLLOUT | EPOLLWRNORM | EPOLLIN | EPOLLRDNORM))
+		rc = v4l2_m2m_poll_for_data(file, m2m_ctx, wait);
+
+	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
+		struct v4l2_fh *fh = file->private_data;
+
+		poll_wait(file, &fh->wait, wait);
+		if (v4l2_event_pending(fh))
+			rc |= EPOLLPRI;
+	}
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_poll);
-- 
2.20.1