[PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off
@ 2019-01-09  7:10 Paul Elder
  2019-01-09  7:10 ` [PATCH v3 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt Paul Elder
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Paul Elder @ 2019-01-09  7:10 UTC (permalink / raw)
  To: laurent.pinchart, kieran.bingham
  Cc: Paul Elder, rogerq, balbi, gregkh, linux-usb, linux-kernel

Down the call stack from the ioctl handler for VIDIOC_STREAMON,
uvc_video_alloc_requests contains a BUG_ON, which in the high level,
triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF
being issued previously.

This can happen in a few ways, such as if the userspace uvc gadget
application simply doesn't issue VIDIOC_STREAMOFF. Another way is if
uvc_function_set_alt with alt 0 is called after it is called with 1 but
before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMOFF will
not be queued to userspace, and therefore userspace will never call
VIDIOC_STREAMOFF.

To fix this, add two more uvc states: starting and stopping. The
starting state is entered when uvc_function_set_alt 1 is called, and is
exited in uvc_v4l2_streamon, when the state is changed to streaming. The
stopping state is entered when uvc_function_set_alt 0 is called, and is
exited in uvc_v4l2_streamoff, when the state is changed to connected.

The status phase of the SET_INTERFACE request doesn't need to be delayed
by the uvc gadget driver, so that is removed.

Finally, there is another way to trigger the aforementioned BUG: start
streaming and (physically) disconnect usb. To fix this, call
uvcg_video_enable 0 in uvc_function_disable.

Changes in v3:

- add state guard to uvc_function_set_alt 1
- add documentation for newly added uvc states
- reorder uvc states to more or less follow the flow diagram
- add more state guards to ioctl handlers for streamon and streamoff
- added interrupt-safe uvcg_video_cancel and used instead of the
  non-interrupt-save uvcg_video_enable 0 in uvc_function_disable

Changes in v2:
	1. Remove delay usb status phase

Paul Elder (4):
  usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
  usb: gadget: uvc: don't delay the status phase of non-zero
    SET_INTERFACE requests
  usb: gadget: uvc: disable stream when disconnected
  usb: gadget: uvc: remove unused/duplicate function prototypes from
    uvc.h

 drivers/usb/gadget/function/f_uvc.c     | 23 ++++++++----
 drivers/usb/gadget/function/uvc.h       | 47 +++++++++++++++++++------
 drivers/usb/gadget/function/uvc_v4l2.c  | 28 +++++++++++----
 drivers/usb/gadget/function/uvc_video.c | 13 +++++++
 drivers/usb/gadget/function/uvc_video.h |  2 ++
 5 files changed, 91 insertions(+), 22 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
  2019-01-09  7:10 [PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off Paul Elder
@ 2019-01-09  7:10 ` Paul Elder
  2019-01-09  7:10 ` [PATCH v3 2/4] usb: gadget: uvc: don't delay the status phase of non-zero SET_INTERFACE requests Paul Elder
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Paul Elder @ 2019-01-09  7:10 UTC (permalink / raw)
  To: laurent.pinchart, kieran.bingham
  Cc: Paul Elder, rogerq, balbi, gregkh, linux-usb, linux-kernel

If the streamon ioctl is issued while the stream is already on, then the
kernel BUGs. This happens at the BUG_ON in uvc_video_alloc_requests
within the call stack from the ioctl handler for VIDIOC_STREAMON.

This could happen when uvc_function_set_alt 0 races and wins against
uvc_v4l2_streamon, or when userspace neglects to issue the
VIDIOC_STREAMOFF ioctl.

To fix this, add two more uvc states: starting and stopping. Use these
to prevent the racing, and to detect when VIDIOC_STREAMON is issued
without previously issuing VIDIOC_STREAMOFF.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v3:

- add state guard to uvc_function_set_alt 1
- add documentation for newly added uvc states
- reorder uvc states to more or less follow the flow diagram
- add more state guards to ioctl handlers for streamon and streamoff

Changes in v2: Nothing

 drivers/usb/gadget/function/f_uvc.c    | 17 ++++++++----
 drivers/usb/gadget/function/uvc.h      | 37 ++++++++++++++++++++++++++
 drivers/usb/gadget/function/uvc_v4l2.c | 26 ++++++++++++++++--
 3 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 8c99392df593..2ec3b73b2b75 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -317,26 +317,31 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 
 	switch (alt) {
 	case 0:
-		if (uvc->state != UVC_STATE_STREAMING)
+		if (uvc->state != UVC_STATE_STREAMING &&
+		    uvc->state != UVC_STATE_STARTING)
 			return 0;
 
 		if (uvc->video.ep)
 			usb_ep_disable(uvc->video.ep);
 
+		uvc->state = UVC_STATE_STOPPING;
+
 		memset(&v4l2_event, 0, sizeof(v4l2_event));
 		v4l2_event.type = UVC_EVENT_STREAMOFF;
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
 
-		uvc->state = UVC_STATE_CONNECTED;
 		return 0;
 
 	case 1:
-		if (uvc->state != UVC_STATE_CONNECTED)
-			return 0;
-
 		if (!uvc->video.ep)
 			return -EINVAL;
 
+		if (uvc->state == UVC_STATE_STOPPING)
+			return -EINVAL;
+
+		if (uvc->state != UVC_STATE_CONNECTED)
+			return 0;
+
 		uvcg_info(f, "reset UVC\n");
 		usb_ep_disable(uvc->video.ep);
 
@@ -346,6 +351,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 			return ret;
 		usb_ep_enable(uvc->video.ep);
 
+		uvc->state = UVC_STATE_STARTING;
+
 		memset(&v4l2_event, 0, sizeof(v4l2_event));
 		v4l2_event.type = UVC_EVENT_STREAMON;
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 099d650082e5..f183e349499c 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -101,10 +101,47 @@ struct uvc_video {
 	unsigned int fid;
 };
 
+/**
+ * enum uvc_state - the states of a struct uvc_device
+ * @UVC_STATE_DISCONNECTED: not connected state
+ *			    - transition to connected state on .set_alt
+ * @UVC_STATE_CONNECTED:    connected state
+ *			    - transition to disconnected state on .disable
+ *			      and alloc
+ *			    - transition to starting state on .set_alt 1
+ * @UVC_STATE_STARTING:     starting state
+ *			    - transition to streaming state on streamon ioctl
+ *			    - transition to stopping state on set_alt 0
+ * @UVC_STATE_STREAMING:    streaming state
+ *			    - transition to stopping state on .set_alt 0
+ * @UVC_STATE_STOPPING:     stopping state
+ *			    - transition to connected on streamoff ioctl
+ *
+ * Diagram of state transitions:
+ *
+ *                  disable
+ *   +---------------------------+
+ *   v                           |
+ * +--------------+  set_alt   +-----------+
+ * | DISCONNECTED | ---------> | CONNECTED |
+ * +--------------+            +-----------+
+ *                                |     ^
+ *                 set_alt 1      |     |     streamoff
+ *         +----------------------+     --------------------+
+ *         V                                                |
+ *  +----------+  streamon   +-----------+  set_alt 0   +----------+
+ *  | STARTING | ----------> | STREAMING | -----------> | STOPPING |
+ *  +----------+             +-----------+              +----------+
+ *         |                                                ^
+ *         |                   set_alt 0                    |
+ *         +------------------------------------------------+
+ */
 enum uvc_state {
 	UVC_STATE_DISCONNECTED,
 	UVC_STATE_CONNECTED,
+	UVC_STATE_STARTING,
 	UVC_STATE_STREAMING,
+	UVC_STATE_STOPPING,
 };
 
 struct uvc_device {
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index a1183eccee22..97e624214287 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -197,17 +197,24 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (type != video->queue.queue.type)
 		return -EINVAL;
 
+	if (uvc->state == UVC_STATE_STREAMING)
+		return 0;
+
+	if (uvc->state != UVC_STATE_STARTING)
+		return -EINVAL;
+
 	/* Enable UVC video. */
 	ret = uvcg_video_enable(video, 1);
 	if (ret < 0)
 		return ret;
 
+	uvc->state = UVC_STATE_STREAMING;
+
 	/*
 	 * Complete the alternate setting selection setup phase now that
 	 * userspace is ready to provide video frames.
 	 */
 	uvc_function_setup_continue(uvc);
-	uvc->state = UVC_STATE_STREAMING;
 
 	return 0;
 }
@@ -218,11 +225,26 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	struct video_device *vdev = video_devdata(file);
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_video *video = &uvc->video;
+	int ret;
 
 	if (type != video->queue.queue.type)
 		return -EINVAL;
 
-	return uvcg_video_enable(video, 0);
+	/*
+	 * Check for connected state also because we want to reset buffers
+	 * if this is called when the stream is already off.
+	 */
+	if (uvc->state != UVC_STATE_STOPPING &&
+	    uvc->state != UVC_STATE_CONNECTED)
+		return 0;
+
+	ret = uvcg_video_enable(video, 0);
+	if (ret < 0)
+		return ret;
+
+	uvc->state = UVC_STATE_CONNECTED;
+
+	return 0;
 }
 
 static int
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 2/4] usb: gadget: uvc: don't delay the status phase of non-zero SET_INTERFACE requests
  2019-01-09  7:10 [PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off Paul Elder
  2019-01-09  7:10 ` [PATCH v3 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt Paul Elder
@ 2019-01-09  7:10 ` Paul Elder
  2019-01-09  7:10 ` [PATCH v3 3/4] usb: gadget: uvc: disable stream when disconnected Paul Elder
  2019-01-09  7:10 ` [PATCH v3 4/4] usb: gadget: uvc: remove unused/duplicate function prototypes from uvc.h Paul Elder
  3 siblings, 0 replies; 5+ messages in thread
From: Paul Elder @ 2019-01-09  7:10 UTC (permalink / raw)
  To: laurent.pinchart, kieran.bingham
  Cc: Paul Elder, rogerq, balbi, gregkh, linux-usb, linux-kernel

Reception of a SET_INTERFACE request with a non-zero alternate setting
signals the start of the video stream. The gadget has to enable the
video streaming endpoint and to signal stream start to userspace, in
order to start receiving video frames to transmit over USB. As userspace
can be slow to react, the UVC function driver delays the status phase of
the SET_INTERFACE control transfer until userspace is ready.

The status phase is delayed by returning USB_GADGET_DELAYED_STATUS from
the function's .set_alt() handler. This creates a race condition as the
userspace application could process the stream start event before the
composite layer processes the USB_GADGET_DELAYED_STATUS return value.
The race has been observed in practice, and can't be solved without a
change to the USB_GADGET_DELAYED_STATUS API.

Fortunately the UVC function driver doesn't strictly require delaying
the status phase for non-zero SET_INTERFACE, as the only requirement
from a USB point of view is that the streaming endpoint must be enabled
before the status phase completes, and that is already guaranteed by the
current code. We can thus complete the status phase synchronously,
removing the race condition at the same time.

Without delaying the status phase the host will likely start issuing
isochronous transfers before we queue the first USB requests. The UDC
will reply with NAKs which should be handled properly by the host. If
this ends up causing issues another option will be to modify the status
phase delay API to fix the race condition.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v3: Nothing

Changes in v2:
	1. Remove delay usb status phase

 drivers/usb/gadget/function/f_uvc.c    | 3 ++-
 drivers/usb/gadget/function/uvc_v4l2.c | 6 ------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 2ec3b73b2b75..b407b10a95ed 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -356,7 +356,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 		memset(&v4l2_event, 0, sizeof(v4l2_event));
 		v4l2_event.type = UVC_EVENT_STREAMON;
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
-		return USB_GADGET_DELAYED_STATUS;
+
+		return 0;

 	default:
 		return -EINVAL;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 97e624214287..7816ea9886e1 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -210,12 +210,6 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)

 	uvc->state = UVC_STATE_STREAMING;

-	/*
-	 * Complete the alternate setting selection setup phase now that
-	 * userspace is ready to provide video frames.
-	 */
-	uvc_function_setup_continue(uvc);
-
 	return 0;
 }

-- 
2.20.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 3/4] usb: gadget: uvc: disable stream when disconnected
  2019-01-09  7:10 [PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off Paul Elder
  2019-01-09  7:10 ` [PATCH v3 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt Paul Elder
  2019-01-09  7:10 ` [PATCH v3 2/4] usb: gadget: uvc: don't delay the status phase of non-zero SET_INTERFACE requests Paul Elder
@ 2019-01-09  7:10 ` Paul Elder
  2019-01-09  7:10 ` [PATCH v3 4/4] usb: gadget: uvc: remove unused/duplicate function prototypes from uvc.h Paul Elder
  3 siblings, 0 replies; 5+ messages in thread
From: Paul Elder @ 2019-01-09  7:10 UTC (permalink / raw)
  To: laurent.pinchart, kieran.bingham
  Cc: Paul Elder, rogerq, balbi, gregkh, linux-usb, linux-kernel

If the streamon ioctl is issued while the stream is already on, then the
kernel BUGs. This happens at the BUG_ON in uvc_video_alloc_requests
within the call stack from the ioctl handler for VIDIOC_STREAMON.

This can also be triggered by starting the stream and then physically
disconnecting usb. To fix this, do the streamoff procedures on usb
disconnect. Since uvcg_video_enable is not interrupt-safe, add an
interrupt-safe version uvcg_video_cancel, and use that.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
v2 Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Changes in v3:

- added interrupt-safe uvcg_video_cancel and used instead of the
  non-interrupt-save uvcg_video_enable 0

Changes in v2: Nothing

 drivers/usb/gadget/function/f_uvc.c     |  3 +++
 drivers/usb/gadget/function/uvc_video.c | 13 +++++++++++++
 drivers/usb/gadget/function/uvc_video.h |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index b407b10a95ed..4134117b5f81 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -369,9 +369,12 @@ uvc_function_disable(struct usb_function *f)
 {
 	struct uvc_device *uvc = to_uvc(f);
 	struct v4l2_event v4l2_event;
+	struct uvc_video *video = &uvc->video;
 
 	uvcg_info(f, "%s()\n", __func__);
 
+	uvcg_video_cancel(video, 1);
+
 	memset(&v4l2_event, 0, sizeof(v4l2_event));
 	v4l2_event.type = UVC_EVENT_DISCONNECT;
 	v4l2_event_queue(&uvc->vdev, &v4l2_event);
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 5c042f380708..5f3ed9e0b5ad 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -348,6 +348,19 @@ int uvcg_video_pump(struct uvc_video *video)
 	return 0;
 }
 
+int uvcg_video_cancel(struct uvc_video *video, int disconnect)
+{
+	unsigned int i;
+
+	for (i = 0; i < UVC_NUM_REQUESTS; ++i)
+		if (video->req[i])
+			usb_ep_dequeue(video->ep, video->req[i]);
+
+	uvc_video_free_requests(video);
+	uvcg_queue_cancel(&video->queue, disconnect);
+	return 0;
+}
+
 /*
  * Enable or disable the video stream.
  */
diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
index 278dc52c7604..1f310fa58ce5 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -16,6 +16,8 @@ struct uvc_video;
 
 int uvcg_video_pump(struct uvc_video *video);
 
+int uvcg_video_cancel(struct uvc_video *video, int disconnect);
+
 int uvcg_video_enable(struct uvc_video *video, int enable);
 
 int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 4/4] usb: gadget: uvc: remove unused/duplicate function prototypes from uvc.h
  2019-01-09  7:10 [PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off Paul Elder
                   ` (2 preceding siblings ...)
  2019-01-09  7:10 ` [PATCH v3 3/4] usb: gadget: uvc: disable stream when disconnected Paul Elder
@ 2019-01-09  7:10 ` Paul Elder
  3 siblings, 0 replies; 5+ messages in thread
From: Paul Elder @ 2019-01-09  7:10 UTC (permalink / raw)
  To: laurent.pinchart, kieran.bingham
  Cc: Paul Elder, rogerq, balbi, gregkh, linux-usb, linux-kernel

Defined in uvc.h, uvc_endpoint_stream doesn't exist, and
uvc_function_connect, uvc_function_disconnect, and
uvc_function_setup_continue have duplicates in f_uvc.h.
Remove these four function prototypes from uvc.h

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 drivers/usb/gadget/function/uvc.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index f183e349499c..671020c8a836 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -185,14 +185,4 @@ struct uvc_file_handle {
 #define to_uvc_file_handle(handle) \
 	container_of(handle, struct uvc_file_handle, vfh)
 
-/* ------------------------------------------------------------------------
- * Functions
- */
-
-extern void uvc_function_setup_continue(struct uvc_device *uvc);
-extern void uvc_endpoint_stream(struct uvc_device *dev);
-
-extern void uvc_function_connect(struct uvc_device *uvc);
-extern void uvc_function_disconnect(struct uvc_device *uvc);
-
 #endif /* _UVC_GADGET_H_ */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-01-09  7:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09  7:10 [PATCH v3 0/4] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off Paul Elder
2019-01-09  7:10 ` [PATCH v3 1/4] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt Paul Elder
2019-01-09  7:10 ` [PATCH v3 2/4] usb: gadget: uvc: don't delay the status phase of non-zero SET_INTERFACE requests Paul Elder
2019-01-09  7:10 ` [PATCH v3 3/4] usb: gadget: uvc: disable stream when disconnected Paul Elder
2019-01-09  7:10 ` [PATCH v3 4/4] usb: gadget: uvc: remove unused/duplicate function prototypes from uvc.h Paul Elder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).