Re: [PATCH] usb: gadget: uvc: Fix deadlock in uvcg_extension_bm_controls_store

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: uvc: incomplete su_mutex deadlock fix across sibling configfs attributes
--

commit 352b1b2da8cde195be417fe376d9490d718f3b87
Author: Saad Varshani <varshanisaad@gmail.com>

usb: gadget: uvc: Fix deadlock in uvcg_extension_bm_controls_store

This commit removes the acquisition of group->cg_subsys->su_mutex from
uvcg_extension_bm_controls_store() to avoid a circular locking dependency
reported by syzbot in configfs_write_iter().

> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 70a1415ea4014..78588366f656f 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1114,16 +1114,12 @@ static ssize_t uvcg_extension_bm_controls_show(struct config_item *item,
>  static ssize_t uvcg_extension_bm_controls_store(struct config_item *item,
>  						const char *page, size_t len)
>  {
> -	struct config_group *group = to_config_group(item->ci_parent);
> -	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
>  	struct uvcg_extension *xu = to_uvcg_extension(item);
>  	struct config_item *opts_item;
>  	struct f_uvc_opts *opts;
>  	u8 *bm_controls, *iter;
>  	int ret, n = 0;
>  
> -	mutex_lock(su_mutex);
> -
>  	opts_item = item->ci_parent->ci_parent->ci_parent;
>  	opts = to_f_uvc_opts(opts_item);
>  
> @@ -1157,7 +1153,6 @@ static ssize_t uvcg_extension_bm_controls_store(struct config_item *item,
>  
>  unlock:
>  	mutex_unlock(&opts->lock);
> -	mutex_unlock(su_mutex);

[Severity: High]
Does removing su_mutex only from uvcg_extension_bm_controls_store() leave the
exact same deadlock present in all sibling configfs attribute callbacks in
drivers/usb/gadget/function/uvc_configfs.c?

When configfs_write_iter() -> flush_write_buffer() or configfs_read_iter() ->
fill_read_buffer() invokes an attribute callback, it already holds
buffer->mutex and down_read(&frag->frag_sem), which prevents configfs_rmdir()
from unlinking the item or its ci_parent hierarchy.

However, the sibling store and show callbacks on struct uvcg_extension still
acquire group->cg_subsys->su_mutex before opts->lock in the same way:

drivers/usb/gadget/function/uvc_configfs.c:uvcg_extension_ba_source_id_store() {
	struct config_group *group = to_config_group(item->ci_parent);
	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
	...
	mutex_lock(su_mutex);

	opts_item = item->ci_parent->ci_parent->ci_parent;
	opts = to_f_uvc_opts(opts_item);

	mutex_lock(&opts->lock);
}

The same locking pattern remains in uvcg_extension_b_control_size_store(),
uvcg_extension_b_nr_in_pins_store(), uvcg_extension_b_num_controls_store(),
uvcg_extension_guid_extension_code_store(), uvcg_extension_bm_controls_show(),
uvcg_extension_ba_source_id_show(), uvcg_extension_guid_extension_code_show(),
and the other configfs attribute show/store callbacks throughout
drivers/usb/gadget/function/uvc_configfs.c (such as
uvcg_default_processing_bm_controls_store(),
uvcg_default_camera_bm_controls_store(), and
uvcg_frame_dw_frame_interval_store()).

Should su_mutex be removed consistently across these configfs attribute
callbacks, and could the commit message also describe the lock dependency
chain and why traversing item->ci_parent is safe without su_mutex?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105053900.20082-1-varshanisaad@gmail.com?part=1