Re: [PATCH v4] usb: gadget: uvc: Fix deadlocks in uvcg_configfs attribute callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: gadget: uvc: unsynchronized refcnt check in uvcg_color_matching store
--

commit 52f34f2adcf8a7a6589075037a07a9887a733e9b
Author: Saad Varshani <varshanisaad@gmail.com>

usb: gadget: uvc: Fix deadlocks in uvcg_configfs attribute callbacks

This commit removes su_mutex locking from uvcg_configfs attribute show and
store callbacks to resolve a circular lock dependency with configfs
frag_sem, and moves the fmt->linked and color_match->refcnt checks under
opts->lock.

> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 70a1415ea4014..606c02214c8ce 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
[ ... ]
> @@ -3000,7 +2782,6 @@ static ssize_t uvcg_color_matching_##cname##_store(			\
>  	struct config_item *item, const char *page, size_t len)		\
>  {									\
>  	struct config_group *group = to_config_group(item);		\
> -	struct mutex *su_mutex = &group->cg_subsys->su_mutex;		\
>  	struct uvcg_color_matching *color_match =			\
>  		to_uvcg_color_matching(group);				\
>  	struct f_uvc_opts *opts;					\
> @@ -3012,25 +2793,21 @@ static ssize_t uvcg_color_matching_##cname##_store(			\
>  	if (ret)							\
>  		return ret;						\
>  									\
> -	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
> +	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;	\
> +	opts = to_f_uvc_opts(opts_item);				\
> +									\
> +	mutex_lock(&opts->lock);					\
>  									\
>  	if (color_match->refcnt) {					\
>  		ret = -EBUSY;						\
>  		goto unlock_su;						\
>  	}								\
>  									\
> -	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;	\
> -	opts = to_f_uvc_opts(opts_item);				\
> -									\
> -	mutex_lock(&opts->lock);					\
> -									\
>  	color_match->desc.aname = num;					\
>  	ret = len;							\
>  									\
> -	mutex_unlock(&opts->lock);					\
>  unlock_su:								\
> -	mutex_unlock(su_mutex);						\
> -									\
> +	mutex_unlock(&opts->lock);					\
>  	return ret;							\
>  }									\
>  UVC_ATTR(uvcg_color_matching_, cname, aname)

[Severity: Medium]
As reported by Sashiko on v2 [1], does moving the color_match->refcnt check
under opts->lock after dropping su_mutex in UVCG_COLOR_MATCHING_ATTR leave
color_match->refcnt and color_match->desc unsynchronized against concurrent
modifications?

Unlike fmt->linked in uvcg_frame_b_frame_index_show(), all functions that
modify color_match->refcnt execute under su_mutex without acquiring
opts->lock. Specifically, uvcg_format_allow_link() and
uvcg_format_drop_link() update color_matching_desc->refcnt while holding
only su_mutex:

drivers/usb/gadget/function/uvc_configfs.c:uvcg_format_allow_link() {
    ...
	color_matching_desc->refcnt--;

	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
	fmt->color_matching = color_matching_desc;
	color_matching_desc->refcnt++;
    ...
}

drivers/usb/gadget/function/uvc_configfs.c:uvcg_format_drop_link() {
    ...
	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
	color_matching_desc->refcnt--;

	streaming = src->ci_parent->ci_parent;
	color_matching_desc = uvcg_format_get_default_color_match(streaming);

	fmt = to_uvcg_format(src);
	fmt->color_matching = color_matching_desc;
	color_matching_desc->refcnt++;
    ...
}

Similarly, uvcg_uncompressed_make(), uvcg_mjpeg_make(), and
uvcg_framebased_make() increment the default color_match->refcnt under
su_mutex (held by configfs_mkdir()) without holding opts->lock:

drivers/usb/gadget/function/uvc_configfs.c:uvcg_uncompressed_make() {
    ...
	h->fmt.color_matching = color_match;
	color_match->refcnt++;
    ...
}

Can a concurrent symlink, unlink, or mkdir operation increment
color_match->refcnt and link color_match to a format while
uvcg_color_matching_##cname##_store() observes color_match->refcnt == 0 and
mutates color_match->desc.aname?

(Also, the error label in uvcg_color_matching_##cname##_store() is still
named unlock_su even though it now unlocks opts->lock.)

[1] https://lore.kernel.org/r/20261106020727.6055-1-varshanisaad@gmail.com

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