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 --- Ricardo Ribalda (2): media: uvcvideo: Do not set an async control owned by other fh media: uvcvideo: Remove dangling pointers
drivers/media/usb/uvc/uvc_ctrl.c | 44 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 3 +++ 3 files changed, 47 insertions(+), 2 deletions(-) --- base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319 change-id: 20241127-uvc-fix-async-2c9d40413ad8
Best regards,
If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.
Without this patch, the first file handle will never get the event associated to that operation.
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 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..5d3a28edf7f0 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;
+ /* Other file handle is waiting a response from this async control. */ + if (ctrl->handle && ctrl->handle != handle) + return -EBUSY; + /* Clamp out of range values. */ switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_INTEGER:
Hi Ricardo,
Thank you for the patch.
On Wed, Nov 27, 2024 at 07:46:10AM +0000, Ricardo Ribalda wrote:
If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.
Without this patch, the first file handle will never get the event associated to that operation.
Please explain why that is an issue (both for the commit message and for me, as I'm not sure what you're fixing here).
What may be an issue is that ctrl->handle seem to be accessed from different contexts without proper locking :-S
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 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..5d3a28edf7f0 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;
- /* Other file handle is waiting a response from this async control. */
- if (ctrl->handle && ctrl->handle != handle)
return -EBUSY;- /* Clamp out of range values. */ switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_INTEGER:
On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
On Wed, Nov 27, 2024 at 07:46:10AM +0000, Ricardo Ribalda wrote:
If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.
Without this patch, the first file handle will never get the event associated to that operation.
Please explain why that is an issue (both for the commit message and for me, as I'm not sure what you're fixing here).
What about something like this:
Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.
What may be an issue is that ctrl->handle seem to be accessed from different contexts without proper locking :-S
Isn't it always protected by ctrl_mutex?
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 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..5d3a28edf7f0 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;
/* Other file handle is waiting a response from this async control. */if (ctrl->handle && ctrl->handle != handle)return -EBUSY;/* Clamp out of range values. */ switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_INTEGER:-- Regards,
Laurent Pinchart
On Wed, Nov 27, 2024 at 10:25:48AM +0100, Ricardo Ribalda wrote:
On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart wrote:
On Wed, Nov 27, 2024 at 07:46:10AM +0000, Ricardo Ribalda wrote:
If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.
Without this patch, the first file handle will never get the event associated to that operation.
Please explain why that is an issue (both for the commit message and for me, as I'm not sure what you're fixing here).
What about something like this:
Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.
So it's related to the userspace-visible behaviour, there are no issues with this inside the kernel ?
Applications should in any case implement timeouts, as UVC devices are fairly unreliable. What bothers me with this patch is that if the device doesn't generate the event, ctrl->handle will never be reset to NULL, and the control will never be settable again. I think the current behaviour is a lesser evil.
What may be an issue is that ctrl->handle seem to be accessed from different contexts without proper locking :-S
Isn't it always protected by ctrl_mutex?
Not that I can tell. At least uvc_ctrl_status_event_async() isn't called with that lock held. uvc_ctrl_set() seems OK (a lockedep assert at the beginning of the function wouldn't hurt).
As uvc_ctrl_status_event_async() is the URB completion handler, a spinlock would be nicer than a mutex to protect ctrl->handle.
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 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..5d3a28edf7f0 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;
/* Other file handle is waiting a response from this async control. */if (ctrl->handle && ctrl->handle != handle)return -EBUSY;/* Clamp out of range values. */ switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_INTEGER:
On Wed, 27 Nov 2024 at 10:42, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Wed, Nov 27, 2024 at 10:25:48AM +0100, Ricardo Ribalda wrote:
On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart wrote:
On Wed, Nov 27, 2024 at 07:46:10AM +0000, Ricardo Ribalda wrote:
If a file handle is waiting for a response from an async control, avoid that other file handle operate with it.
Without this patch, the first file handle will never get the event associated to that operation.
Please explain why that is an issue (both for the commit message and for me, as I'm not sure what you're fixing here).
What about something like this:
Without this patch, the first file handle will never get the event associated with that operation, which can lead to endless loops in applications. Eg: If an application A wants to change the zoom and to know when the operation has completed: it will open the video node, subscribe to the zoom event, change the control and wait for zoom to finish. If before the zoom operation finishes, another application B changes the zoom, the first app A will loop forever.
So it's related to the userspace-visible behaviour, there are no issues with this inside the kernel ?
It is also required by the granular PM patchset.
Applications should in any case implement timeouts, as UVC devices are fairly unreliable. What bothers me with this patch is that if the device doesn't generate the event, ctrl->handle will never be reset to NULL, and the control will never be settable again. I think the current behaviour is a lesser evil.
The same app can set the control as many times as it wants, and if that app closes the next patch will clean out the handle. Maybe I should invert the patches?
What may be an issue is that ctrl->handle seem to be accessed from different contexts without proper locking :-S
Isn't it always protected by ctrl_mutex?
Not that I can tell. At least uvc_ctrl_status_event_async() isn't called with that lock held. uvc_ctrl_set() seems OK (a lockedep assert at the beginning of the function wouldn't hurt).
ctrl->handle = NULL in uvc_ctrl_status_event_async() is completely redundant.
The only place we set the value of the handle is in uvc_ctrl_set, and that can only happen for controls with mappings. I am sending another patch to remove that set to make it clear.
As uvc_ctrl_status_event_async() is the URB completion handler, a spinlock would be nicer than a mutex to protect ctrl->handle.
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 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..5d3a28edf7f0 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) return -EACCES;
/* Other file handle is waiting a response from this async control. */if (ctrl->handle && ctrl->handle != handle)return -EBUSY;/* Clamp out of range values. */ switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_INTEGER:-- Regards,
Laurent Pinchart
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 | 40 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 3 +++ 3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 5d3a28edf7f0..51a53ad25e9c 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex);
handle = ctrl->handle; - ctrl->handle = NULL; + if (handle) { + ctrl->handle = NULL; + WARN_ON(!handle->pending_async_ctrls); + if (handle->pending_async_ctrls) + handle->pending_async_ctrls--; + }
list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data); @@ -2050,8 +2055,11 @@ 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) { + if (!ctrl->handle) + handle->pending_async_ctrls++; ctrl->handle = handle; + }
ctrl->dirty = 1; ctrl->modified = 1; @@ -2774,6 +2782,34 @@ 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) { + int i; + + for (i = 0; i < entity->ncontrols; ++i) { + struct uvc_control *ctrl = &entity->controls[i]; + + if (!ctrl->handle || ctrl->handle != handle) + continue; + + ctrl->handle = NULL; + if (WARN_ON(!handle->pending_async_ctrls)) + continue; + handle->pending_async_ctrls--; + } + } + + 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..2f8a9c48e32a 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -612,6 +612,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state; + unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */ };
struct uvc_driver { @@ -797,6 +798,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 Wed, Nov 27, 2024 at 07:46:11AM +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 | 40 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 3 +++ 3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 5d3a28edf7f0..51a53ad25e9c 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex); handle = ctrl->handle;
- ctrl->handle = NULL;
- if (handle) {
ctrl->handle = NULL;WARN_ON(!handle->pending_async_ctrls);if (handle->pending_async_ctrls)handle->pending_async_ctrls--;- }
There's at least one other location where ctrl->handle is reset to NULL.
list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data); @@ -2050,8 +2055,11 @@ 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) {
if (!ctrl->handle)handle->pending_async_ctrls++;
Is this protected by ctrl_mutex ?
Please be careful about locking and race conditions, taking the time to double check will help getting your patches merged faster.
- }
ctrl->dirty = 1; ctrl->modified = 1; @@ -2774,6 +2782,34 @@ 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) {
int i;
unsigned int
I wonder if these days you could event write
for (unsigned int i = 0; i < entity->ncontrols; ++i) {
for (i = 0; i < entity->ncontrols; ++i) {struct uvc_control *ctrl = &entity->controls[i];if (!ctrl->handle || ctrl->handle != handle)
Given that handle can't be null, you can write
if (ctrl->handle != handle)
continue;ctrl->handle = NULL;if (WARN_ON(!handle->pending_async_ctrls))continue;
Is this needed ? If we find more controls for this handle than pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at the end of this function to trigger, isn't that enough ?
handle->pending_async_ctrls--;}- }
- 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..2f8a9c48e32a 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -612,6 +612,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state;
- unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */
The kernel does it the other way around, it lists in the documentation of the lock what data it protects.
}; struct uvc_driver { @@ -797,6 +798,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 Wed, 27 Nov 2024 at 10:35, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
On Wed, Nov 27, 2024 at 07:46:11AM +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 | 40 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 3 +++ 3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 5d3a28edf7f0..51a53ad25e9c 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex);
handle = ctrl->handle;
ctrl->handle = NULL;
if (handle) {ctrl->handle = NULL;WARN_ON(!handle->pending_async_ctrls);if (handle->pending_async_ctrls)handle->pending_async_ctrls--;}There's at least one other location where ctrl->handle is reset to NULL.
That assignment is not needed. I added a patch to remove it in the next version.
list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data);@@ -2050,8 +2055,11 @@ 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) {if (!ctrl->handle)handle->pending_async_ctrls++; ctrl->handle = handle;Is this protected by ctrl_mutex ?
yes, uvc_ctrl_set is only called by uvc_ioctl_s_try_ext_ctrls that calls uvc_ctrl_begin
I will send another patch to add an annotation to the function to make it explicit.
Please be careful about locking and race conditions, taking the time to double check will help getting your patches merged faster.
} ctrl->dirty = 1; ctrl->modified = 1;@@ -2774,6 +2782,34 @@ 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) {int i;unsigned int
I wonder if these days you could event write
for (unsigned int i = 0; i < entity->ncontrols; ++i) {
for (i = 0; i < entity->ncontrols; ++i) {struct uvc_control *ctrl = &entity->controls[i];if (!ctrl->handle || ctrl->handle != handle)Given that handle can't be null, you can write
if (ctrl->handle != handle)
continue;ctrl->handle = NULL;if (WARN_ON(!handle->pending_async_ctrls))continue;Is this needed ? If we find more controls for this handle than pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at the end of this function to trigger, isn't that enough ?
I want to know if the warning is triggered because I have too many pending_async_ctrls or too little.
handle->pending_async_ctrls--;}}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..2f8a9c48e32a 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -612,6 +612,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state;
unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */The kernel does it the other way around, it lists in the documentation of the lock what data it protects.
};
struct uvc_driver { @@ -797,6 +798,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
-- Ricardo Ribalda
On Wed, Nov 27, 2024 at 11:23:48AM +0100, Ricardo Ribalda wrote:
On Wed, 27 Nov 2024 at 10:35, Laurent Pinchart wrote:
On Wed, Nov 27, 2024 at 07:46:11AM +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 | 40 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 3 +++ 3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 5d3a28edf7f0..51a53ad25e9c 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex);
handle = ctrl->handle;
ctrl->handle = NULL;
if (handle) {ctrl->handle = NULL;WARN_ON(!handle->pending_async_ctrls);if (handle->pending_async_ctrls)handle->pending_async_ctrls--;}There's at least one other location where ctrl->handle is reset to NULL.
That assignment is not needed. I added a patch to remove it in the next version.
list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data);@@ -2050,8 +2055,11 @@ 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) {if (!ctrl->handle)handle->pending_async_ctrls++; ctrl->handle = handle;Is this protected by ctrl_mutex ?
yes, uvc_ctrl_set is only called by uvc_ioctl_s_try_ext_ctrls that calls uvc_ctrl_begin
You're right. I think I figured out after writing this part of the review, and forgot to delete it. Sorry.
I will send another patch to add an annotation to the function to make it explicit.
Please be careful about locking and race conditions, taking the time to double check will help getting your patches merged faster.
} ctrl->dirty = 1; ctrl->modified = 1;@@ -2774,6 +2782,34 @@ 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) {int i;unsigned int
I wonder if these days you could event write
for (unsigned int i = 0; i < entity->ncontrols; ++i) {
for (i = 0; i < entity->ncontrols; ++i) {struct uvc_control *ctrl = &entity->controls[i];if (!ctrl->handle || ctrl->handle != handle)Given that handle can't be null, you can write
if (ctrl->handle != handle)
continue;ctrl->handle = NULL;if (WARN_ON(!handle->pending_async_ctrls))continue;Is this needed ? If we find more controls for this handle than pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at the end of this function to trigger, isn't that enough ?
I want to know if the warning is triggered because I have too many pending_async_ctrls or too little.
You could also print the value of pending_async_ctrls at the end, it would give you that information, and tell you how many you're missing. Not a big deal, and I don't expect that warning to be triggered.
handle->pending_async_ctrls--;}}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..2f8a9c48e32a 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -612,6 +612,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state;
unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */The kernel does it the other way around, it lists in the documentation of the lock what data it protects.
};
struct uvc_driver { @@ -797,6 +798,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 Thu, 28 Nov 2024 at 21:51, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Wed, Nov 27, 2024 at 11:23:48AM +0100, Ricardo Ribalda wrote:
On Wed, 27 Nov 2024 at 10:35, Laurent Pinchart wrote:
On Wed, Nov 27, 2024 at 07:46:11AM +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 | 40 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 3 +++ 3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 5d3a28edf7f0..51a53ad25e9c 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex);
handle = ctrl->handle;
ctrl->handle = NULL;
if (handle) {ctrl->handle = NULL;WARN_ON(!handle->pending_async_ctrls);if (handle->pending_async_ctrls)handle->pending_async_ctrls--;}There's at least one other location where ctrl->handle is reset to NULL.
That assignment is not needed. I added a patch to remove it in the next version.
list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data);@@ -2050,8 +2055,11 @@ 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) {if (!ctrl->handle)handle->pending_async_ctrls++; ctrl->handle = handle;Is this protected by ctrl_mutex ?
yes, uvc_ctrl_set is only called by uvc_ioctl_s_try_ext_ctrls that calls uvc_ctrl_begin
You're right. I think I figured out after writing this part of the review, and forgot to delete it. Sorry.
I will send another patch to add an annotation to the function to make it explicit.
Please be careful about locking and race conditions, taking the time to double check will help getting your patches merged faster.
} ctrl->dirty = 1; ctrl->modified = 1;@@ -2774,6 +2782,34 @@ 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) {int i;unsigned int
I wonder if these days you could event write
for (unsigned int i = 0; i < entity->ncontrols; ++i) {
for (i = 0; i < entity->ncontrols; ++i) {struct uvc_control *ctrl = &entity->controls[i];if (!ctrl->handle || ctrl->handle != handle)Given that handle can't be null, you can write
if (ctrl->handle != handle)
continue;ctrl->handle = NULL;if (WARN_ON(!handle->pending_async_ctrls))continue;Is this needed ? If we find more controls for this handle than pending_async_ctrls, decrementing it below 0 will case the WARN_ON() at the end of this function to trigger, isn't that enough ?
I want to know if the warning is triggered because I have too many pending_async_ctrls or too little.
You could also print the value of pending_async_ctrls at the end, it would give you that information, and tell you how many you're missing. Not a big deal, and I don't expect that warning to be triggered.
The issue is that the variable is unsigned. it will show a weird number... And making it signed seems incorrect.
I also do not expect it to be triggered. For now I will keep it as is. If you have a strong opinion against it I will change it.
handle->pending_async_ctrls--;}}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..2f8a9c48e32a 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -612,6 +612,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state;
unsigned int pending_async_ctrls; /* Protected by ctrl_mutex. */The kernel does it the other way around, it lists in the documentation of the lock what data it protects.
};
struct uvc_driver { @@ -797,6 +798,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
linux-stable-mirror@lists.linaro.org
-
Laurent Pinchart -
Ricardo Ribalda