This patchset fixes two bugs with the async controls for the uvc driver.
They were found while implementing the granular PM, but I am sending them as a separate patches, so they can be reviewed sooner. They fix real issues in the driver that need to be taken care.
Signed-off-by: Ricardo Ribalda ribalda@chromium.org --- Changes in v3: - change again! order of patches. - Introduce uvc_ctrl_set_handle. - Do not change ctrl->handle if it is not NULL.
Changes in v2: - Annotate lockdep - ctrl->handle != handle - Change order of patches - Move documentation of mutex - Link to v1: https://lore.kernel.org/r/20241127-uvc-fix-async-v1-0-eb8722531b8c@chromium....
--- Ricardo Ribalda (4): media: uvcvideo: Do not replace the handler of an async ctrl media: uvcvideo: Remove dangling pointers media: uvcvideo: Annotate lock requirements for uvc_ctrl_set media: uvcvideo: Remove redundant NULL assignment
drivers/media/usb/uvc/uvc_ctrl.c | 52 +++++++++++++++++++++++++++++++++++----- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 14 +++++++++-- 3 files changed, 60 insertions(+), 8 deletions(-) --- base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319 change-id: 20241127-uvc-fix-async-2c9d40413ad8
Best regards,
ctrl->handle was used to keep a reference to the last fh that changed an asynchronous control.
But what we need instead, is to keep a reference to the originator of an uncompleted operation.
We use that handle to filter control events. Under some situations, the originator of an event shall not be notified.
In the current implementation, we unconditionally replace the handle pointer, which can result in an invalid notification to the real originator of the operation.
Lets fix that.
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 | 2 +- drivers/media/usb/uvc/uvcvideo.h | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..88ef8fdc2be2 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2046,7 +2046,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
- if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS && !ctrl->handle) ctrl->handle = handle;
ctrl->dirty = 1; diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..ce688b80e986 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -150,7 +150,10 @@ struct uvc_control {
u8 *uvc_data;
- struct uvc_fh *handle; /* File handle that last changed the control. */ + struct uvc_fh *handle; /* + * File handle that initially changed the + * async control. + */ };
/*
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 | 44 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 9 +++++++- 3 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 88ef8fdc2be2..0a79a3def017 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1579,6 +1579,23 @@ 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_set_handle(struct uvc_control *ctrl, struct uvc_fh *handle) +{ + /* NOTE: We must own the chain->ctrl_mutex to run this function. */ + + if (handle) { + if (WARN_ON(ctrl->handle)) + return; + handle->pending_async_ctrls++; + ctrl->handle = handle; + } else if (ctrl->handle) { + 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 +1606,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex);
handle = ctrl->handle; - ctrl->handle = NULL; + uvc_ctrl_set_handle(ctrl, NULL);
list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data); @@ -2047,7 +2064,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS && !ctrl->handle) - ctrl->handle = handle; + uvc_ctrl_set_handle(ctrl, handle);
ctrl->dirty = 1; ctrl->modified = 1; @@ -2770,6 +2787,29 @@ 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) { + struct uvc_control *ctrl = &entity->controls[i]; + + if (ctrl->handle != handle) + continue; + + uvc_ctrl_set_handle(ctrl, NULL); + } + } + + 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 ce688b80e986..e0e4f099a210 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -340,7 +340,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 */ @@ -615,6 +619,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 { @@ -800,6 +805,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 Fri, 29 Nov 2024 at 20:25, Ricardo Ribalda ribalda@chromium.org 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 | 44 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 9 +++++++- 3 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 88ef8fdc2be2..0a79a3def017 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1579,6 +1579,23 @@ 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_set_handle(struct uvc_control *ctrl, struct uvc_fh *handle) +{
/* NOTE: We must own the chain->ctrl_mutex to run this function. */
if (handle) {
if (WARN_ON(ctrl->handle))
return;
handle->pending_async_ctrls++;
ctrl->handle = handle;
} else if (ctrl->handle) {
ctrl->handle = NULL;
if (WARN_ON(!handle->pending_async_ctrls))
return;
handle->pending_async_ctrls--;
This is wrong, handle is NULL here. Sorry about that.
Uploading a new version.
}
+}
void uvc_ctrl_status_event(struct uvc_video_chain *chain, struct uvc_control *ctrl, const u8 *data) { @@ -1589,7 +1606,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex);
handle = ctrl->handle;
ctrl->handle = NULL;
uvc_ctrl_set_handle(ctrl, NULL); list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data);
@@ -2047,7 +2064,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS && !ctrl->handle)
ctrl->handle = handle;
uvc_ctrl_set_handle(ctrl, handle); ctrl->dirty = 1; ctrl->modified = 1;
@@ -2770,6 +2787,29 @@ 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) {
struct uvc_control *ctrl = &entity->controls[i];
if (ctrl->handle != handle)
continue;
uvc_ctrl_set_handle(ctrl, NULL);
}
}
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 ce688b80e986..e0e4f099a210 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -340,7 +340,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 */
@@ -615,6 +619,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 { @@ -800,6 +805,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);
-- 2.47.0.338.g60cca15819-goog
linux-stable-mirror@lists.linaro.org