| Message ID | 7e8f365ce447ffe9d1eba3bfb680f7708cb9ebd1.1524596473.git.paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 --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
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(-)