[PATCH v2] media: gspca: don't complete buffers before streaming has started

[25031212351]

From: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>

gspca enables the data transfers before it knows whether start_streaming()
will succeed: gspca_init_transfer() sets gspca_dev->streaming before the
URBs are submitted, so an URB that has already been submitted can complete
and gspca_frame_add() can hand its buffer to videobuf2 while the remaining
URBs are still being submitted.

When one of those submissions fails, gspca_start_streaming() returns the
buffers that are still queued to vb2 in QUEUED state, but the buffer that
has already been completed is no longer on the driver's queue, and
vb2_buffer_done() cannot take it back (it only accepts active buffers).
vb2_start_streaming() then warns because the driver left a buffer on the
done list although it failed to start:

 ------------[ cut here ]------------
 WARNING: drivers/media/common/videobuf2/videobuf2-core.c:1821 at vb2_start_streaming+0x312/0x680
 ...
 Call Trace:
  <TASK>
  vb2_core_streamon+0x285/0x470
  __vb2_init_fileio+0xdc1/0x1120
  __vb2_perform_fileio+0xa7a/0x15e0
  vb2_fop_read+0x219/0x3e0
  v4l2_read+0x1fc/0x320
  vfs_read+0x1d9/0x980
  ksys_read+0x111/0x210
 ...
 gspca_main: set alt 0 err -19
 gspca_main: usb_submit_urb alt 1 err -2

This was observed on a camera whose USB configuration was changed from
userspace while a read() was starting the stream: the endpoint is removed,
so usb_submit_urb() fails with -ENOENT, and the driver is unbound.

Do not complete buffers before the transfers are fully set up.  A new
streaming_ready flag is set at the end of gspca_init_transfer() only when
it succeeded, so a failing start_streaming() always leaves the done list
empty.  Frames that arrive before that are dropped, just like frames that
arrive when no buffer is queued; the window lasts only as long as the
remaining URBs are still being submitted.

vb2_is_streaming() cannot be used for this check: videobuf2 sets
q->streaming before calling start_streaming() when VIDIOC_STREAMON comes in
with fewer buffers queued than min_queued_buffers, and start_streaming() is
then called later from vb2_core_qbuf().

Fixes: 1f5965c4dfd7 ("media: gspca: convert to vb2")
Cc: stable@xxxxxxxxxxxxxxx
Co-developed-by: Bin Yu <byu@xxxxxxxxxxxxx>
Signed-off-by: Bin Yu <byu@xxxxxxxxxxxxx>
Co-developed-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
Changes in v2:
 - Do not gate the completion on vb2_is_streaming(): videobuf2 sets
   q->streaming before calling start_streaming() when VIDIOC_STREAMON is
   issued with fewer buffers queued than min_queued_buffers, so that call
   site was still reachable and the warning could still be triggered.
   Track in the driver whether gspca_init_transfer() has finished setting
   up the transfers instead.

 drivers/media/usb/gspca/gspca.c | 23 +++++++++++++++++++++--
 drivers/media/usb/gspca/gspca.h |  1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 594d73e50b9f..3f5ece57d66a 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -392,8 +392,23 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
 	spin_unlock_irqrestore(&gspca_dev->qlock, flags);
 
 	if (packet_type == FIRST_PACKET) {
-		/* if there is no queued buffer, discard the whole frame */
-		if (!buf) {
+		/*
+		 * If there is no queued buffer, discard the whole frame.
+		 *
+		 * Frames are also discarded until gspca_init_transfer() has
+		 * set up the transfers: the first URB can complete while the
+		 * others are still being submitted, and if a submission then
+		 * fails, start_streaming() must leave vb2's done list empty.
+		 * A completed buffer cannot be taken back, so frames that
+		 * run into that window are dropped.
+		 *
+		 * vb2_is_streaming() cannot be used for this: videobuf2 sets
+		 * q->streaming before calling start_streaming() when
+		 * VIDIOC_STREAMON comes in with fewer buffers queued than
+		 * min_queued_buffers, and start_streaming() is only called
+		 * later from vb2_core_qbuf().
+		 */
+		if (!buf || !gspca_dev->streaming_ready) {
 			gspca_dev->last_packet_type = DISCARD_PACKET;
 			gspca_dev->sequence++;
 			return;
@@ -732,6 +747,7 @@ static int create_urbs(struct gspca_dev *gspca_dev,
 static void gspca_stream_off(struct gspca_dev *gspca_dev)
 {
 	gspca_dev->streaming = false;
+	gspca_dev->streaming_ready = false;
 	gspca_dev->usb_err = 0;
 	if (gspca_dev->sd_desc->stopN)
 		gspca_dev->sd_desc->stopN(gspca_dev);
@@ -887,6 +903,9 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		}
 	}
 out:
+	/* the transfers are set up now, buffers may be completed */
+	if (!ret)
+		gspca_dev->streaming_ready = true;
 	gspca_input_create_urb(gspca_dev);
 	return ret;
 }
diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h
index a6554d5e9e1a..3e0b9e27f4b1 100644
--- a/drivers/media/usb/gspca/gspca.h
+++ b/drivers/media/usb/gspca/gspca.h
@@ -187,6 +187,7 @@ struct gspca_dev {
 	__u8 last_packet_type;
 	__s8 empty_packet;		/* if (-1) don't check empty packets */
 	bool streaming;
+	bool streaming_ready;		/* buffers can be completed */
 
 	__u8 curr_mode;			/* current camera mode */
 	struct v4l2_pix_format pixfmt;	/* current mode parameters */
-- 
2.34.1