Re: [PATCH 2/2] usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `uvcg_video_deinit()` in `uvc_function_unbind()` before waiting for V4L2 file release completion leaves it exposed to concurrent in-flight ioctls, causing use-after-free or NULL pointer dereferences.
--

--- Patch [2]: [PATCH 2/2] usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit() ---
commit 4d7b46b3698b9db70da6cefc8230b5f7e4eb66ce
Author: Xu Yang <[email protected]>

usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit()

This commit refactors the cleanup logic for UVC video into a new 
uvcg_video_deinit() helper to fix resource leaks in error paths and missing 
kworker destruction during unbind. It also reorders the teardown in 
uvc_function_unbind() to happen after video_unregister_device().

> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index a4fb2790f4ffe..fa2f9d0e4ce0b 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
[ ... ]
> @@ -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);

[Severity: High]
This is a pre-existing issue, but could this ordering lead to a NULL
pointer dereference if there are in-flight ioctls?

If a userspace app is calling an ioctl like VIDIOC_QBUF concurrently while
the gadget is unbound, video_unregister_device() clears the registered
flag but doesn't wait for in-flight ioctls to finish.

Then uvcg_video_deinit() destroys video->async_wq and sets it to NULL:

drivers/usb/gadget/function/uvc_video.c:uvcg_video_deinit() {
	...
	if (video->async_wq) {
		destroy_workqueue(video->async_wq);
		video->async_wq = NULL;
	}
}

An in-flight uvc_v4l2_qbuf() could then resume and try to use the workqueue
by dereferencing the NULL pointer:

drivers/usb/gadget/function/uvc_v4l2.c:uvc_v4l2_qbuf() {
	...
	if (uvc->state == UVC_STATE_STREAMING)
		queue_work(video->async_wq, &video->pump);
}

Should uvcg_video_deinit() be deferred until after 
wait_for_completion(&vdev_release_done) when the video device is fully 
released and no more ioctls can be executing?

>  	v4l2_device_unregister(&uvc->v4l2_dev);
>  
>  	scoped_guard(mutex, &uvc->lock)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2