diff mbox

[v2,1/3] usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt

Message ID 7e8f365ce447ffe9d1eba3bfb680f7708cb9ebd1.1524596473.git.paul.elder@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Elder April 24, 2018, 8:59 p.m. UTC
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 could be triggered by uvc_function_set_alt 0 racing and
winning against uvc_v4l2_streamon, or by userspace neglecting 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 v2: Nothing

 drivers/usb/gadget/function/f_uvc.c    |  8 ++++++--
 drivers/usb/gadget/function/uvc.h      |  2 ++
 drivers/usb/gadget/function/uvc_v4l2.c | 19 +++++++++++++++++--
 3 files changed, 25 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart May 21, 2018, 8:07 a.m. UTC | #1
Hi Paul,

Thank you for the patch.

On Tuesday, 24 April 2018 23:59:34 EEST Paul Elder wrote:
> 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 could be triggered by uvc_function_set_alt 0 racing and
> winning against uvc_v4l2_streamon, or by userspace neglecting 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 v2: Nothing
> 
>  drivers/usb/gadget/function/f_uvc.c    |  8 ++++++--
>  drivers/usb/gadget/function/uvc.h      |  2 ++
>  drivers/usb/gadget/function/uvc_v4l2.c | 19 +++++++++++++++++--
>  3 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c index 439eba660e95..9b63b28a1ee3
> 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -325,17 +325,19 @@ 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)

Indentation is weird here, uvc should be aligned on the two lines.

>  			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:
> @@ -354,6 +356,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 a64e07e61f8c..afb2eac1f337 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -131,6 +131,8 @@ enum uvc_state {
>  	UVC_STATE_DISCONNECTED,
>  	UVC_STATE_CONNECTED,
>  	UVC_STATE_STREAMING,
> +	UVC_STATE_STARTING,
> +	UVC_STATE_STOPPING,

Let's order the states as theyr should normally occur, STARTING should come 
before STREAMING.

>  };
> 
>  struct uvc_device {
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c
> b/drivers/usb/gadget/function/uvc_v4l2.c index 9a9019625496..fdf02b6987c0
> 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -193,6 +193,9 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) struct uvc_video *video = &uvc->video;
>  	int ret;
> 
> +	if (uvc->state != UVC_STATE_STARTING)
> +		return 0;

I would move this check after the next one, as the VIDIOC_STREAMON ioctl 
should fail if the type isn't valid, even if we're already streaming.

Furthermore, shouldn't we silently ignore the UVC_STATE_STREAMING only ? For 
other states, I think we should return an error, as starting the stream isn't 
valid for instance when the state is UVC_STATE_DISCONNECTED.

>  	if (type != video->queue.queue.type)
>  		return -EINVAL;
> 
> @@ -201,12 +204,13 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) 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;
>  }
> @@ -217,11 +221,22 @@ 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 (uvc->state != UVC_STATE_STOPPING)
> +		return 0;

Same comment here, this should go after the next check.

While I think extending the state machine this way makes sense, I believe 
you're introducing race conditions. The VIDIOC_STREAMON and VIDIOC_STREAMOFF 
ioctls can be issued by userspace at any time, completely asynchronously to 
the reception of SET_INTERFACE requests. You will need a lock to protect this. 
I recommend trying to draw sequence diagrams to see how the different events 
can race each other.

>  	if (type != video->queue.queue.type)
>  		return -EINVAL;
> 
> -	return uvcg_video_enable(video, 0);
> +	ret = uvcg_video_enable(video, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	uvc->state = UVC_STATE_CONNECTED;
> +
> +	return 0;
> +
>  }
> 
>  static int
diff mbox

Patch

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 439eba660e95..9b63b28a1ee3 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -325,17 +325,19 @@  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:
@@ -354,6 +356,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 a64e07e61f8c..afb2eac1f337 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -131,6 +131,8 @@  enum uvc_state {
 	UVC_STATE_DISCONNECTED,
 	UVC_STATE_CONNECTED,
 	UVC_STATE_STREAMING,
+	UVC_STATE_STARTING,
+	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 9a9019625496..fdf02b6987c0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -193,6 +193,9 @@  uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	struct uvc_video *video = &uvc->video;
 	int ret;
 
+	if (uvc->state != UVC_STATE_STARTING)
+		return 0;
+
 	if (type != video->queue.queue.type)
 		return -EINVAL;
 
@@ -201,12 +204,13 @@  uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	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;
 }
@@ -217,11 +221,22 @@  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 (uvc->state != UVC_STATE_STOPPING)
+		return 0;
 
 	if (type != video->queue.queue.type)
 		return -EINVAL;
 
-	return uvcg_video_enable(video, 0);
+	ret = uvcg_video_enable(video, 0);
+	if (ret < 0)
+		return ret;
+
+	uvc->state = UVC_STATE_CONNECTED;
+
+	return 0;
+
 }
 
 static int