| Message ID | 20241202-uvc-fix-async-v5-2-6658c1fe312b@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Two +1 fixes for async controls | expand |
Hi, On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote: > When an async control is written, we copy a pointer to the file handle > that started the operation. That pointer will be used when the device is > done. Which could be anytime in the future. > > If the user closes that file descriptor, its structure will be freed, > and there will be one dangling pointer per pending async control, that > the driver will try to use. > > Clean all the dangling pointers during release(). > > To avoid adding a performance penalty in the most common case (no async > operation), a counter has been introduced with some logic to make sure > that it is properly handled. > > Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++-- > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- > 3 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 9a80a7d8e73a..af1e38f5c6e9 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > } > > +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > +{ > + lockdep_assert_held(&handle->chain->ctrl_mutex); > + > + if (ctrl->handle) > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > + "UVC non compliance: Setting an async control with a pending operation."); > + > + if (handle == ctrl->handle) > + return; > + > + if (ctrl->handle) > + ctrl->handle->pending_async_ctrls--; > + > + ctrl->handle = handle; > + handle->pending_async_ctrls++; > +} Maybe simplify this to: static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) { lockdep_assert_held(&handle->chain->ctrl_mutex); if (!ctrl->handle) handle->pending_async_ctrls++; else dev_warn_ratelimited(&handle->stream->dev->udev->dev, "UVC non compliance: Setting an async control with a pending operation."); ctrl->handle = handle; } ? Otherwise the patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > +{ > + lockdep_assert_held(&handle->chain->ctrl_mutex); > + > + if (ctrl->handle != handle) /* Nothing to do here.*/ > + return; > + > + ctrl->handle = NULL; > + if (WARN_ON(!handle->pending_async_ctrls)) > + return; > + handle->pending_async_ctrls--; > +} > + > void uvc_ctrl_status_event(struct uvc_video_chain *chain, > struct uvc_control *ctrl, const u8 *data) > { > @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > mutex_lock(&chain->ctrl_mutex); > > handle = ctrl->handle; > - ctrl->handle = NULL; > + if (handle) > + uvc_ctrl_put_handle(handle, ctrl); > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > s32 value = __uvc_ctrl_get_value(mapping, data); > @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > if (!rollback && handle && > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > - ctrl->handle = handle; > + uvc_ctrl_get_handle(handle, ctrl); > } > > return 0; > @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > return 0; > } > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > +{ > + struct uvc_entity *entity; > + > + guard(mutex)(&handle->chain->ctrl_mutex); > + > + if (!handle->pending_async_ctrls) > + return; > + > + list_for_each_entry(entity, &handle->chain->dev->entities, list) > + for (unsigned int i = 0; i < entity->ncontrols; ++i) > + uvc_ctrl_put_handle(handle, &entity->controls[i]); > + > + WARN_ON(handle->pending_async_ctrls); > +} > + > /* > * Cleanup device controls. > */ > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 97c5407f6603..b425306a3b8c 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > + uvc_ctrl_cleanup_fh(handle); > + > /* Only free resources if this is a privileged handle. */ > if (uvc_has_privileges(handle)) > uvc_queue_release(&stream->queue); > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 07f9921d83f2..92ecdd188587 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -337,7 +337,11 @@ struct uvc_video_chain { > struct uvc_entity *processing; /* Processing unit */ > struct uvc_entity *selector; /* Selector unit */ > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > + struct mutex ctrl_mutex; /* > + * Protects ctrl.info, > + * ctrl.handle and > + * uvc_fh.pending_async_ctrls > + */ > > struct v4l2_prio_state prio; /* V4L2 priority state */ > u32 caps; /* V4L2 chain-wide caps */ > @@ -612,6 +616,7 @@ struct uvc_fh { > struct uvc_video_chain *chain; > struct uvc_streaming *stream; > enum uvc_handle_state state; > + unsigned int pending_async_ctrls; > }; > > struct uvc_driver { > @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry); > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > + > /* Utility functions */ > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > u8 epaddr); >
Hi Hans On Mon, 2 Dec 2024 at 15:44, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote: > > When an async control is written, we copy a pointer to the file handle > > that started the operation. That pointer will be used when the device is > > done. Which could be anytime in the future. > > > > If the user closes that file descriptor, its structure will be freed, > > and there will be one dangling pointer per pending async control, that > > the driver will try to use. > > > > Clean all the dangling pointers during release(). > > > > To avoid adding a performance penalty in the most common case (no async > > operation), a counter has been introduced with some logic to make sure > > that it is properly handled. > > > > Cc: stable@vger.kernel.org > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++-- > > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > > drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- > > 3 files changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 9a80a7d8e73a..af1e38f5c6e9 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > > } > > > > +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > > +{ > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > + > > + if (ctrl->handle) > > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > > + "UVC non compliance: Setting an async control with a pending operation."); > > + > > + if (handle == ctrl->handle) > > + return; > > + > > + if (ctrl->handle) > > + ctrl->handle->pending_async_ctrls--; > > + > > + ctrl->handle = handle; > > + handle->pending_async_ctrls++; > > +} > > Maybe simplify this to: I do not think that we can do that simplification. If we do that, the original file handle `pending_async_ctrls` value will be out of sync. Let me know if you do not agree to delete your reviewed-by Thanks! > > static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > { > lockdep_assert_held(&handle->chain->ctrl_mutex); > > if (!ctrl->handle) > handle->pending_async_ctrls++; > else > dev_warn_ratelimited(&handle->stream->dev->udev->dev, > "UVC non compliance: Setting an async control with a pending operation."); > > ctrl->handle = handle; > } > > ? > > Otherwise the patch looks good to me: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Regards, > > Hans > > > > > > +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > > +{ > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > + > > + if (ctrl->handle != handle) /* Nothing to do here.*/ > > + return; > > + > > + ctrl->handle = NULL; > > + if (WARN_ON(!handle->pending_async_ctrls)) > > + return; > > + handle->pending_async_ctrls--; > > +} > > + > > void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > struct uvc_control *ctrl, const u8 *data) > > { > > @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > mutex_lock(&chain->ctrl_mutex); > > > > handle = ctrl->handle; > > - ctrl->handle = NULL; > > + if (handle) > > + uvc_ctrl_put_handle(handle, ctrl); > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > s32 value = __uvc_ctrl_get_value(mapping, data); > > @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > > > if (!rollback && handle && > > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > - ctrl->handle = handle; > > + uvc_ctrl_get_handle(handle, ctrl); > > } > > > > return 0; > > @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > > return 0; > > } > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > +{ > > + struct uvc_entity *entity; > > + > > + guard(mutex)(&handle->chain->ctrl_mutex); > > + > > + if (!handle->pending_async_ctrls) > > + return; > > + > > + list_for_each_entry(entity, &handle->chain->dev->entities, list) > > + for (unsigned int i = 0; i < entity->ncontrols; ++i) > > + uvc_ctrl_put_handle(handle, &entity->controls[i]); > > + > > + WARN_ON(handle->pending_async_ctrls); > > +} > > + > > /* > > * Cleanup device controls. > > */ > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 97c5407f6603..b425306a3b8c 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > > > + uvc_ctrl_cleanup_fh(handle); > > + > > /* Only free resources if this is a privileged handle. */ > > if (uvc_has_privileges(handle)) > > uvc_queue_release(&stream->queue); > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 07f9921d83f2..92ecdd188587 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -337,7 +337,11 @@ struct uvc_video_chain { > > struct uvc_entity *processing; /* Processing unit */ > > struct uvc_entity *selector; /* Selector unit */ > > > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > > + struct mutex ctrl_mutex; /* > > + * Protects ctrl.info, > > + * ctrl.handle and > > + * uvc_fh.pending_async_ctrls > > + */ > > > > struct v4l2_prio_state prio; /* V4L2 priority state */ > > u32 caps; /* V4L2 chain-wide caps */ > > @@ -612,6 +616,7 @@ struct uvc_fh { > > struct uvc_video_chain *chain; > > struct uvc_streaming *stream; > > enum uvc_handle_state state; > > + unsigned int pending_async_ctrls; > > }; > > > > struct uvc_driver { > > @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > struct uvc_xu_control_query *xqry); > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > > + > > /* Utility functions */ > > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > > u8 epaddr); > > >
Hi, On 2-Dec-24 3:49 PM, Ricardo Ribalda wrote: > Hi Hans > > > On Mon, 2 Dec 2024 at 15:44, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote: >>> When an async control is written, we copy a pointer to the file handle >>> that started the operation. That pointer will be used when the device is >>> done. Which could be anytime in the future. >>> >>> If the user closes that file descriptor, its structure will be freed, >>> and there will be one dangling pointer per pending async control, that >>> the driver will try to use. >>> >>> Clean all the dangling pointers during release(). >>> >>> To avoid adding a performance penalty in the most common case (no async >>> operation), a counter has been introduced with some logic to make sure >>> that it is properly handled. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> --- >>> drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++-- >>> drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ >>> drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- >>> 3 files changed, 60 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >>> index 9a80a7d8e73a..af1e38f5c6e9 100644 >>> --- a/drivers/media/usb/uvc/uvc_ctrl.c >>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >>> @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, >>> uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); >>> } >>> >>> +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) >>> +{ >>> + lockdep_assert_held(&handle->chain->ctrl_mutex); >>> + >>> + if (ctrl->handle) >>> + dev_warn_ratelimited(&handle->stream->dev->udev->dev, >>> + "UVC non compliance: Setting an async control with a pending operation."); >>> + >>> + if (handle == ctrl->handle) >>> + return; >>> + >>> + if (ctrl->handle) >>> + ctrl->handle->pending_async_ctrls--; >>> + >>> + ctrl->handle = handle; >>> + handle->pending_async_ctrls++; >>> +} >> >> Maybe simplify this to: > > I do not think that we can do that simplification. > > If we do that, the original file handle `pending_async_ctrls` value > will be out of sync. Ah good point, I missed that the -- and ++ are done on 2 potentially different handles. Regards, Hans >> static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) >> { >> lockdep_assert_held(&handle->chain->ctrl_mutex); >> >> if (!ctrl->handle) >> handle->pending_async_ctrls++; >> else >> dev_warn_ratelimited(&handle->stream->dev->udev->dev, >> "UVC non compliance: Setting an async control with a pending operation."); >> >> ctrl->handle = handle; >> } >> >> ? >> >> Otherwise the patch looks good to me: >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > >> >> Regards, >> >> Hans >> >> >> >> >>> +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl) >>> +{ >>> + lockdep_assert_held(&handle->chain->ctrl_mutex); >>> + >>> + if (ctrl->handle != handle) /* Nothing to do here.*/ >>> + return; >>> + >>> + ctrl->handle = NULL; >>> + if (WARN_ON(!handle->pending_async_ctrls)) >>> + return; >>> + handle->pending_async_ctrls--; >>> +} >>> + >>> void uvc_ctrl_status_event(struct uvc_video_chain *chain, >>> struct uvc_control *ctrl, const u8 *data) >>> { >>> @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, >>> mutex_lock(&chain->ctrl_mutex); >>> >>> handle = ctrl->handle; >>> - ctrl->handle = NULL; >>> + if (handle) >>> + uvc_ctrl_put_handle(handle, ctrl); >>> >>> list_for_each_entry(mapping, &ctrl->info.mappings, list) { >>> s32 value = __uvc_ctrl_get_value(mapping, data); >>> @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, >>> >>> if (!rollback && handle && >>> ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) >>> - ctrl->handle = handle; >>> + uvc_ctrl_get_handle(handle, ctrl); >>> } >>> >>> return 0; >>> @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) >>> return 0; >>> } >>> >>> +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) >>> +{ >>> + struct uvc_entity *entity; >>> + >>> + guard(mutex)(&handle->chain->ctrl_mutex); >>> + >>> + if (!handle->pending_async_ctrls) >>> + return; >>> + >>> + list_for_each_entry(entity, &handle->chain->dev->entities, list) >>> + for (unsigned int i = 0; i < entity->ncontrols; ++i) >>> + uvc_ctrl_put_handle(handle, &entity->controls[i]); >>> + >>> + WARN_ON(handle->pending_async_ctrls); >>> +} >>> + >>> /* >>> * Cleanup device controls. >>> */ >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c >>> index 97c5407f6603..b425306a3b8c 100644 >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c >>> @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) >>> >>> uvc_dbg(stream->dev, CALLS, "%s\n", __func__); >>> >>> + uvc_ctrl_cleanup_fh(handle); >>> + >>> /* Only free resources if this is a privileged handle. */ >>> if (uvc_has_privileges(handle)) >>> uvc_queue_release(&stream->queue); >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h >>> index 07f9921d83f2..92ecdd188587 100644 >>> --- a/drivers/media/usb/uvc/uvcvideo.h >>> +++ b/drivers/media/usb/uvc/uvcvideo.h >>> @@ -337,7 +337,11 @@ struct uvc_video_chain { >>> struct uvc_entity *processing; /* Processing unit */ >>> struct uvc_entity *selector; /* Selector unit */ >>> >>> - struct mutex ctrl_mutex; /* Protects ctrl.info */ >>> + struct mutex ctrl_mutex; /* >>> + * Protects ctrl.info, >>> + * ctrl.handle and >>> + * uvc_fh.pending_async_ctrls >>> + */ >>> >>> struct v4l2_prio_state prio; /* V4L2 priority state */ >>> u32 caps; /* V4L2 chain-wide caps */ >>> @@ -612,6 +616,7 @@ struct uvc_fh { >>> struct uvc_video_chain *chain; >>> struct uvc_streaming *stream; >>> enum uvc_handle_state state; >>> + unsigned int pending_async_ctrls; >>> }; >>> >>> struct uvc_driver { >>> @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, >>> int uvc_xu_ctrl_query(struct uvc_video_chain *chain, >>> struct uvc_xu_control_query *xqry); >>> >>> +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); >>> + >>> /* Utility functions */ >>> struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, >>> u8 epaddr); >>> >> > >
Hi Ricardo, Thank you for the patch. On Mon, Dec 02, 2024 at 02:24:36PM +0000, Ricardo Ribalda wrote: > When an async control is written, we copy a pointer to the file handle > that started the operation. That pointer will be used when the device is > done. Which could be anytime in the future. > > If the user closes that file descriptor, its structure will be freed, > and there will be one dangling pointer per pending async control, that > the driver will try to use. > > Clean all the dangling pointers during release(). > > To avoid adding a performance penalty in the most common case (no async > operation), a counter has been introduced with some logic to make sure > that it is properly handled. > > Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++-- > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- > 3 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 9a80a7d8e73a..af1e38f5c6e9 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > } > > +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > +{ > + lockdep_assert_held(&handle->chain->ctrl_mutex); > + > + if (ctrl->handle) > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > + "UVC non compliance: Setting an async control with a pending operation."); > + > + if (handle == ctrl->handle) > + return; > + > + if (ctrl->handle) > + ctrl->handle->pending_async_ctrls--; > + > + ctrl->handle = handle; > + handle->pending_async_ctrls++; > +} > + > +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > +{ > + lockdep_assert_held(&handle->chain->ctrl_mutex); > + > + if (ctrl->handle != handle) /* Nothing to do here.*/ > + return; > + > + ctrl->handle = NULL; > + if (WARN_ON(!handle->pending_async_ctrls)) > + return; > + handle->pending_async_ctrls--; > +} get/put have strong connotations in the kernel, related to acquiring a reference to a given object, and releasing it. The usage here is different, and I think it makes the usage below confusing. I prefer the original single function. > + > void uvc_ctrl_status_event(struct uvc_video_chain *chain, > struct uvc_control *ctrl, const u8 *data) > { > @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > mutex_lock(&chain->ctrl_mutex); > > handle = ctrl->handle; > - ctrl->handle = NULL; > + if (handle) > + uvc_ctrl_put_handle(handle, ctrl); > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > s32 value = __uvc_ctrl_get_value(mapping, data); > @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > if (!rollback && handle && > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > - ctrl->handle = handle; > + uvc_ctrl_get_handle(handle, ctrl); > } > > return 0; > @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > return 0; > } > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > +{ > + struct uvc_entity *entity; > + > + guard(mutex)(&handle->chain->ctrl_mutex); > + > + if (!handle->pending_async_ctrls) > + return; > + > + list_for_each_entry(entity, &handle->chain->dev->entities, list) list_for_each_entry(entity, &handle->chain->dev->entities, list) { > + for (unsigned int i = 0; i < entity->ncontrols; ++i) > + uvc_ctrl_put_handle(handle, &entity->controls[i]); } > + > + WARN_ON(handle->pending_async_ctrls); > +} > + > /* > * Cleanup device controls. > */ > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 97c5407f6603..b425306a3b8c 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > + uvc_ctrl_cleanup_fh(handle); > + > /* Only free resources if this is a privileged handle. */ > if (uvc_has_privileges(handle)) > uvc_queue_release(&stream->queue); > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 07f9921d83f2..92ecdd188587 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -337,7 +337,11 @@ struct uvc_video_chain { > struct uvc_entity *processing; /* Processing unit */ > struct uvc_entity *selector; /* Selector unit */ > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > + struct mutex ctrl_mutex; /* > + * Protects ctrl.info, > + * ctrl.handle and > + * uvc_fh.pending_async_ctrls > + */ > > struct v4l2_prio_state prio; /* V4L2 priority state */ > u32 caps; /* V4L2 chain-wide caps */ > @@ -612,6 +616,7 @@ struct uvc_fh { > struct uvc_video_chain *chain; > struct uvc_streaming *stream; > enum uvc_handle_state state; > + unsigned int pending_async_ctrls; > }; > > struct uvc_driver { > @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry); > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > + > /* Utility functions */ > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > u8 epaddr); >
On Tue, 3 Dec 2024 at 21:36, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Mon, Dec 02, 2024 at 02:24:36PM +0000, Ricardo Ribalda wrote: > > When an async control is written, we copy a pointer to the file handle > > that started the operation. That pointer will be used when the device is > > done. Which could be anytime in the future. > > > > If the user closes that file descriptor, its structure will be freed, > > and there will be one dangling pointer per pending async control, that > > the driver will try to use. > > > > Clean all the dangling pointers during release(). > > > > To avoid adding a performance penalty in the most common case (no async > > operation), a counter has been introduced with some logic to make sure > > that it is properly handled. > > > > Cc: stable@vger.kernel.org > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++-- > > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > > drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- > > 3 files changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 9a80a7d8e73a..af1e38f5c6e9 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > > } > > > > +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > > +{ > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > + > > + if (ctrl->handle) > > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > > + "UVC non compliance: Setting an async control with a pending operation."); > > + > > + if (handle == ctrl->handle) > > + return; > > + > > + if (ctrl->handle) > > + ctrl->handle->pending_async_ctrls--; > > + > > + ctrl->handle = handle; > > + handle->pending_async_ctrls++; > > +} > > + > > +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > > +{ > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > + > > + if (ctrl->handle != handle) /* Nothing to do here.*/ > > + return; > > + > > + ctrl->handle = NULL; > > + if (WARN_ON(!handle->pending_async_ctrls)) > > + return; > > + handle->pending_async_ctrls--; > > +} > > get/put have strong connotations in the kernel, related to acquiring a > reference to a given object, and releasing it. The usage here is > different, and I think it makes the usage below confusing. I prefer the > original single function. I just uploaded a new version... not sure if it looks nicer though. Regards! > > > + > > void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > struct uvc_control *ctrl, const u8 *data) > > { > > @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > mutex_lock(&chain->ctrl_mutex); > > > > handle = ctrl->handle; > > - ctrl->handle = NULL; > > + if (handle) > > + uvc_ctrl_put_handle(handle, ctrl); > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > s32 value = __uvc_ctrl_get_value(mapping, data); > > @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > > > if (!rollback && handle && > > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > - ctrl->handle = handle; > > + uvc_ctrl_get_handle(handle, ctrl); > > } > > > > return 0; > > @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > > return 0; > > } > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > +{ > > + struct uvc_entity *entity; > > + > > + guard(mutex)(&handle->chain->ctrl_mutex); > > + > > + if (!handle->pending_async_ctrls) > > + return; > > + > > + list_for_each_entry(entity, &handle->chain->dev->entities, list) > > list_for_each_entry(entity, &handle->chain->dev->entities, list) { > > > + for (unsigned int i = 0; i < entity->ncontrols; ++i) > > + uvc_ctrl_put_handle(handle, &entity->controls[i]); > > } > > > + > > + WARN_ON(handle->pending_async_ctrls); > > +} > > + > > /* > > * Cleanup device controls. > > */ > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 97c5407f6603..b425306a3b8c 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > > > + uvc_ctrl_cleanup_fh(handle); > > + > > /* Only free resources if this is a privileged handle. */ > > if (uvc_has_privileges(handle)) > > uvc_queue_release(&stream->queue); > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 07f9921d83f2..92ecdd188587 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -337,7 +337,11 @@ struct uvc_video_chain { > > struct uvc_entity *processing; /* Processing unit */ > > struct uvc_entity *selector; /* Selector unit */ > > > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > > + struct mutex ctrl_mutex; /* > > + * Protects ctrl.info, > > + * ctrl.handle and > > + * uvc_fh.pending_async_ctrls > > + */ > > > > struct v4l2_prio_state prio; /* V4L2 priority state */ > > u32 caps; /* V4L2 chain-wide caps */ > > @@ -612,6 +616,7 @@ struct uvc_fh { > > struct uvc_video_chain *chain; > > struct uvc_streaming *stream; > > enum uvc_handle_state state; > > + unsigned int pending_async_ctrls; > > }; > > > > struct uvc_driver { > > @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > struct uvc_xu_control_query *xqry); > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > > + > > /* Utility functions */ > > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > > u8 epaddr); > > > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 9a80a7d8e73a..af1e38f5c6e9 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); } +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl) +{ + lockdep_assert_held(&handle->chain->ctrl_mutex); + + if (ctrl->handle) + dev_warn_ratelimited(&handle->stream->dev->udev->dev, + "UVC non compliance: Setting an async control with a pending operation."); + + if (handle == ctrl->handle) + return; + + if (ctrl->handle) + ctrl->handle->pending_async_ctrls--; + + ctrl->handle = handle; + handle->pending_async_ctrls++; +} + +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl) +{ + lockdep_assert_held(&handle->chain->ctrl_mutex); + + if (ctrl->handle != handle) /* Nothing to do here.*/ + return; + + ctrl->handle = NULL; + if (WARN_ON(!handle->pending_async_ctrls)) + return; + handle->pending_async_ctrls--; +} + void uvc_ctrl_status_event(struct uvc_video_chain *chain, struct uvc_control *ctrl, const u8 *data) { @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex); handle = ctrl->handle; - ctrl->handle = NULL; + if (handle) + uvc_ctrl_put_handle(handle, ctrl); list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data); @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, if (!rollback && handle && ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) - ctrl->handle = handle; + uvc_ctrl_get_handle(handle, ctrl); } return 0; @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev) return 0; } +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) +{ + struct uvc_entity *entity; + + guard(mutex)(&handle->chain->ctrl_mutex); + + if (!handle->pending_async_ctrls) + return; + + list_for_each_entry(entity, &handle->chain->dev->entities, list) + for (unsigned int i = 0; i < entity->ncontrols; ++i) + uvc_ctrl_put_handle(handle, &entity->controls[i]); + + WARN_ON(handle->pending_async_ctrls); +} + /* * Cleanup device controls. */ diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 97c5407f6603..b425306a3b8c 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) uvc_dbg(stream->dev, CALLS, "%s\n", __func__); + uvc_ctrl_cleanup_fh(handle); + /* Only free resources if this is a privileged handle. */ if (uvc_has_privileges(handle)) uvc_queue_release(&stream->queue); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..92ecdd188587 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -337,7 +337,11 @@ struct uvc_video_chain { struct uvc_entity *processing; /* Processing unit */ struct uvc_entity *selector; /* Selector unit */ - struct mutex ctrl_mutex; /* Protects ctrl.info */ + struct mutex ctrl_mutex; /* + * Protects ctrl.info, + * ctrl.handle and + * uvc_fh.pending_async_ctrls + */ struct v4l2_prio_state prio; /* V4L2 priority state */ u32 caps; /* V4L2 chain-wide caps */ @@ -612,6 +616,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state; + unsigned int pending_async_ctrls; }; struct uvc_driver { @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, int uvc_xu_ctrl_query(struct uvc_video_chain *chain, struct uvc_xu_control_query *xqry); +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); + /* Utility functions */ struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, u8 epaddr);
When an async control is written, we copy a pointer to the file handle that started the operation. That pointer will be used when the device is done. Which could be anytime in the future. If the user closes that file descriptor, its structure will be freed, and there will be one dangling pointer per pending async control, that the driver will try to use. Clean all the dangling pointers during release(). To avoid adding a performance penalty in the most common case (no async operation), a counter has been introduced with some logic to make sure that it is properly handled. Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- 3 files changed, 60 insertions(+), 3 deletions(-)