* [PATCH v2 1/2] usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind()
2026-09-23 10:39 [PATCH v2 0/2] usb: gadget: uvc: fix resource leak on video->async_wq and video->kworker Xu Yang
@ 2026-09-23 10:39 ` Xu Yang
2026-09-23 10:45 ` sashiko-bot
2026-09-23 10:39 ` [PATCH v2 2/2] usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit() Xu Yang
1 sibling, 1 reply; 5+ messages in thread
From: Xu Yang @ 2026-09-23 10:39 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kai Aizen, Michael Grzeschik
Cc: linux-usb, linux-kernel, imx, Xu Yang, Frank Li
From: Xu Yang <xu.yang_2@nxp.com>
Commit 68aa70648b62 ("usb: gadget: uvc: hold opts->lock across XU walks
in uvc_function_bind") introduced an error_unlock label to release
opts->lock on failure paths. The label is misplaced between the return
statement and v4l2_error, causing it to fall through into v4l2_error
and call v4l2_device_unregister() on a device that was never registered.
Replace the manual mutex_lock/unlock pair and the error_unlock label
with scoped_guard(mutex), removing the need for explicit lock cleanup on
error paths.
Fixes: 68aa70648b62 ("usb: gadget: uvc: hold opts->lock across XU walks in uvc_function_bind")
Assisted-by: Claude:claude-sonnet-4.6
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
drivers/usb/gadget/function/f_uvc.c | 80 +++++++++++++++++--------------------
1 file changed, 36 insertions(+), 44 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index d1bf3ea75197..a4fb2790f4ff 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -768,23 +768,17 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
- /*
- * Hold opts->lock across both the XU string-descriptor fixup below and
- * the descriptor-copy block further down. Without this, configfs
- * uvcg_extension_drop() (which takes opts->lock) can race with the
- * list_for_each_entry() walks here and inside uvc_copy_descriptors(),
- * leading to a UAF on a freed struct uvcg_extension. See
- * drivers/usb/gadget/function/uvc_configfs.c::uvcg_extension_drop().
- */
- mutex_lock(&opts->lock);
-
/*
* XUs can have an arbitrary string descriptor describing them. If they
- * have one pick up the ID.
+ * have one pick up the ID. Hold opts->lock here to avoid race with configfs
+ * uvcg_extension_make() and uvcg_extension_drop().
*/
- list_for_each_entry(xu, &opts->extension_units, list)
- if (xu->string_descriptor_index)
- xu->desc.iExtension = cdev->usb_strings[xu->string_descriptor_index].id;
+ scoped_guard(mutex, &opts->lock) {
+ list_for_each_entry(xu, &opts->extension_units, list)
+ if (xu->string_descriptor_index)
+ xu->desc.iExtension =
+ cdev->usb_strings[xu->string_descriptor_index].id;
+ }
/*
* We attach the hard-coded defaults incase the user does not provide
@@ -795,7 +789,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
ARRAY_SIZE(uvc_en_us_strings));
if (IS_ERR(us)) {
ret = PTR_ERR(us);
- goto error_unlock;
+ goto error;
}
uvc_iad.iFunction = opts->iad_index ? cdev->usb_strings[opts->iad_index].id :
@@ -809,50 +803,50 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
/* Allocate interface IDs. */
if ((ret = usb_interface_id(c, f)) < 0)
- goto error_unlock;
+ goto error;
uvc_iad.bFirstInterface = ret;
uvc_control_intf.bInterfaceNumber = ret;
uvc->control_intf = ret;
opts->control_interface = ret;
if ((ret = usb_interface_id(c, f)) < 0)
- goto error_unlock;
+ goto error;
uvc_streaming_intf_alt0.bInterfaceNumber = ret;
uvc_streaming_intf_alt1.bInterfaceNumber = ret;
uvc->streaming_intf = ret;
opts->streaming_interface = ret;
/* Copy descriptors */
- f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
- if (IS_ERR(f->fs_descriptors)) {
- ret = PTR_ERR(f->fs_descriptors);
- f->fs_descriptors = NULL;
- goto error_unlock;
- }
+ scoped_guard(mutex, &opts->lock) {
+ f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
+ if (IS_ERR(f->fs_descriptors)) {
+ ret = PTR_ERR(f->fs_descriptors);
+ f->fs_descriptors = NULL;
+ goto error;
+ }
- f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
- if (IS_ERR(f->hs_descriptors)) {
- ret = PTR_ERR(f->hs_descriptors);
- f->hs_descriptors = NULL;
- goto error_unlock;
- }
+ f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
+ if (IS_ERR(f->hs_descriptors)) {
+ ret = PTR_ERR(f->hs_descriptors);
+ f->hs_descriptors = NULL;
+ goto error;
+ }
- f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
- if (IS_ERR(f->ss_descriptors)) {
- ret = PTR_ERR(f->ss_descriptors);
- f->ss_descriptors = NULL;
- goto error_unlock;
- }
+ f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
+ if (IS_ERR(f->ss_descriptors)) {
+ ret = PTR_ERR(f->ss_descriptors);
+ f->ss_descriptors = NULL;
+ goto error;
+ }
- f->ssp_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER_PLUS);
- if (IS_ERR(f->ssp_descriptors)) {
- ret = PTR_ERR(f->ssp_descriptors);
- f->ssp_descriptors = NULL;
- goto error_unlock;
+ f->ssp_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER_PLUS);
+ if (IS_ERR(f->ssp_descriptors)) {
+ ret = PTR_ERR(f->ssp_descriptors);
+ f->ssp_descriptors = NULL;
+ goto error;
+ }
}
- mutex_unlock(&opts->lock);
-
/* Preallocate control endpoint request. */
uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0, GFP_KERNEL);
uvc->control_buf = kmalloc(UVC_MAX_REQUEST_SIZE, GFP_KERNEL);
@@ -884,8 +878,6 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
return 0;
-error_unlock:
- mutex_unlock(&opts->lock);
v4l2_error:
v4l2_device_unregister(&uvc->v4l2_dev);
error:
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2 2/2] usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit()
2026-09-23 10:39 [PATCH v2 0/2] usb: gadget: uvc: fix resource leak on video->async_wq and video->kworker Xu Yang
2026-09-23 10:39 ` [PATCH v2 1/2] usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind() Xu Yang
@ 2026-09-23 10:39 ` Xu Yang
2026-09-23 10:51 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Xu Yang @ 2026-09-23 10:39 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kai Aizen, Michael Grzeschik
Cc: linux-usb, linux-kernel, imx, Xu Yang, Frank Li
From: Xu Yang <xu.yang_2@nxp.com>
kthread_destroy_worker() was never called during unbind, leaving the
UVCG kthread running after the gadget function is unbound. Also, if
uvcg_video_init() or uvc_register_video() fails during bind, async_wq
and kworker were not cleaned up, causing resource leaks.
Both issues require the same teardown sequence: cancel the pending work,
destroy the kworker, and destroy the workqueue. Consolidate this logic
into a new uvcg_video_deinit() helper and call it from both
uvc_function_unbind() and the v4l2_error path in uvc_function_bind().
In uvc_function_unbind(), uvcg_video_deinit() is placed after
video_unregister_device() to fix the ordering. Without this ordering,
tearing down the workers before unregistering the V4L2 device could lead
to use-after-free on video device resources still accessed by those
workers.
Fixes: f0bbfbd16b3b ("usb: gadget: uvc: rework to enqueue in pump worker from encoded queue")
Assisted-by: Claude:claude-sonnet-4.6
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
drivers/usb/gadget/function/f_uvc.c | 7 ++-----
drivers/usb/gadget/function/uvc_video.c | 15 +++++++++++++++
drivers/usb/gadget/function/uvc_video.h | 1 +
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index a4fb2790f4ff..fa2f9d0e4ce0 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -879,6 +879,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
return 0;
v4l2_error:
+ uvcg_video_deinit(&uvc->video);
v4l2_device_unregister(&uvc->v4l2_dev);
error:
if (uvc->control_req) {
@@ -1028,11 +1029,6 @@ static void uvc_function_unbind(struct usb_configuration *c,
connected = uvc->func_connected;
}
- kthread_cancel_work_sync(&video->hw_submit);
-
- if (video->async_wq)
- destroy_workqueue(video->async_wq);
-
/*
* If we know we're connected via v4l2, then there should be a cleanup
* of the device from userspace either via UVC_EVENT_DISCONNECT or
@@ -1048,6 +1044,7 @@ static void uvc_function_unbind(struct usb_configuration *c,
device_remove_file(&uvc->vdev.dev, &dev_attr_function_name);
video_unregister_device(&uvc->vdev);
+ uvcg_video_deinit(video);
v4l2_device_unregister(&uvc->v4l2_dev);
scoped_guard(mutex, &uvc->lock)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 9ba09118bb74..002afca9141e 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -841,3 +841,18 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
return uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,
V4L2_BUF_TYPE_VIDEO_OUTPUT, &video->mutex);
}
+
+void uvcg_video_deinit(struct uvc_video *video)
+{
+ kthread_cancel_work_sync(&video->hw_submit);
+
+ if (!IS_ERR_OR_NULL(video->kworker)) {
+ kthread_destroy_worker(video->kworker);
+ video->kworker = NULL;
+ }
+
+ if (video->async_wq) {
+ destroy_workqueue(video->async_wq);
+ video->async_wq = NULL;
+ }
+}
diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
index 8ef6259741f1..6c5481f107f9 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -18,5 +18,6 @@ int uvcg_video_enable(struct uvc_video *video);
int uvcg_video_disable(struct uvc_video *video);
int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
+void uvcg_video_deinit(struct uvc_video *video);
#endif /* __UVC_VIDEO_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread