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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: gadget: uvc: undeclared variables and broken parent lookup
  in format ops
- [Low] usb: gadget: uvc: missing Fixes trailer for su_mutex deadlock
  fix
--

commit 836a51f50b7ceaae4e0f814eb631463c15ac345b
Author: Saad Varshani <varshanisaad@gmail.com>

> usb: gadget: uvc: Fix deadlocks in uvcg_configfs attribute callbacks
>
> Removed su_mutex locking in the function uvcg_extension_bm_controls_store.
> Done this due to the deadlock caused by the mutex conflict in
> fs/configfs_write_iter. Validated commit with the C repro provided by
> syzbot. No other conficts observed.
>
> Also removed the su_mutex in the other attribute extension functions.
> Such as uvcg_extension_b_control_size_store. This applies to all
> uvcg_configfs attribute store/show functions.
>
> The deadlock was happening due to a circular mutex dependency between
> configfs (frag_sem) and uvcg_extension su_mutex. The reason it is safe
> to traverse item->ci_parent without su_mutex is because
> configfs_write_iter() calls flush_write_buffer() which calls the
> attribute function, holding frag_sem. Which prevents configfs_rmdir()
> from unlinking the item or its ci_parent hierarchy
>
> Also reordered fmt->linked in uvcg_frame_b_frame_index_show() to be
> under the &opts->lock. As well as reorder refcnt in uvcg_color_matching
> store to be under &opts->lock.
>
> Applied opts->lock mutex, to functions uvcg_format_allow_link(),
> uvcg_format_drop_link(), uvcg_uncompressed_make(), uvcg_mjpeg_make(),
> and uvcg_framebased_make() due to them also modifying
> color_match->refcnt, to avoid concurrency issues.
>
> ---
> v5 changes:
> 	- Synchronised color_match->refcnt changes with the opts->lock
> 	  mutex in the functions uvcg_format_allow_link(),
> 	  uvcg_format_drop_link(), uvcg_uncompressed_make(),
> 	  uvcg_mjpeg_make(), and uvcg_framebased_make()
>
> Reported-by: syzbot+300213c2b5b55c0244f4@syzkaller.appspotmail.com
> Closes: https://syzbot.org/bug?extid=300213c2b5b55c0244f4
> Signed-off-by: Saad Varshani <varshanisaad@gmail.com>

[Severity: Low]
This fixes a deadlock introduced when configfs attribute callbacks were
added to the UVC function.  Should the commit message include:

  Fixes: 46919a23ee87 ("usb: gadget: uvc: configfs support in uvc function")

> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 70a1415ea4014..4f775efe24b6d 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c

[ ... ]

> @@ -1609,6 +1485,11 @@ static int uvcg_format_allow_link(struct config_item *src, struct config_item *t
>  
>  	mutex_lock(su_mutex);
>  
> +	opts_item = f->group.cg_item.ci_parent->ci_parent->ci_parent;
> +	opts = to_f_uvc_opts(opts_item);
> +
> +	mutex_lock(&opts->lock);
> +
>  	streaming = src->ci_parent->ci_parent;
>  	color_matching = config_group_find_item(to_config_group(streaming), "color_matching");
>  	if (!color_matching || color_matching != tgt->ci_parent) {
> @@ -1637,6 +1518,7 @@ static int uvcg_format_allow_link(struct config_item *src, struct config_item *t
>  
>  out_put_cm:
>  	config_item_put(color_matching);
> +	mutex_unlock(&opts->lock);
>  	mutex_unlock(su_mutex);
>  
>  	return ret;
> @@ -1651,6 +1533,11 @@ static void uvcg_format_drop_link(struct config_item *src, struct config_item *t
>  
>  	mutex_lock(su_mutex);
>  
> +	opts_item = f->group.cg_item.ci_parent->ci_parent->ci_parent;
> +	opts = to_f_uvc_opts(opts_item);
> +
> +	mutex_lock(&opts->lock);
> +
>  	color_matching_desc = to_uvcg_color_matching(to_config_group(tgt));
>  	color_matching_desc->refcnt--;
>  
> @@ -1661,6 +1548,7 @@ static void uvcg_format_drop_link(struct config_item *src, struct config_item *t
>  	fmt->color_matching = color_matching_desc;
>  	color_matching_desc->refcnt++;
>  
> +	mutex_unlock(&opts->lock);
>  	mutex_unlock(su_mutex);
>  }

[ ... ]

> @@ -2498,12 +2330,17 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>  	h->desc.bmInterlaceFlags	= 0;
>  	h->desc.bCopyProtect		= 0;
>  
> +	opts_item = f->group.cg_item.ci_parent->ci_parent->ci_parent;
> +	opts = to_f_uvc_opts(opts_item);
> +
> +	mutex_lock(&opts->lock);
>  	INIT_LIST_HEAD(&h->fmt.frames);
>  	h->fmt.type = UVCG_UNCOMPRESSED;
>  	h->fmt.color_matching = color_match;
>  	color_match->refcnt++;
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_uncompressed_type);
> +	mutex_unlock(&opts->lock);
>  
>  	return &h->fmt.group;
>  }

[ ... ]

> @@ -2688,12 +2512,17 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>  	h->desc.bmInterlaceFlags	= 0;
>  	h->desc.bCopyProtect		= 0;
>  
> +	opts_item = f->group.cg_item.ci_parent->ci_parent->ci_parent;
> +	opts = to_f_uvc_opts(opts_item);
> +
> +	mutex_lock(&opts->lock);
>  	INIT_LIST_HEAD(&h->fmt.frames);
>  	h->fmt.type = UVCG_MJPEG;
>  	h->fmt.color_matching = color_match;
>  	color_match->refcnt++;
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_mjpeg_type);
> +	mutex_unlock(&opts->lock);
>  
>  	return &h->fmt.group;
>  }

[ ... ]

> @@ -2945,10 +2753,15 @@ static struct config_group *uvcg_framebased_make(struct config_group *group,
>  	INIT_LIST_HEAD(&h->fmt.frames);
>  	h->fmt.type = UVCG_FRAMEBASED;
>  
> +	opts_item = f->group.cg_item.ci_parent->ci_parent->ci_parent;
> +	opts = to_f_uvc_opts(opts_item);
> +
> +	mutex_lock(&opts->lock);
>  	h->fmt.color_matching = color_match;
>  	color_match->refcnt++;
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_framebased_type);
> +	mutex_unlock(&opts->lock);
>  
>  	return &h->fmt.group;
>  }

[Severity: High]
Does this break the build and use the wrong parent traversal for
opts_item?

None of uvcg_format_allow_link(), uvcg_format_drop_link(),
uvcg_uncompressed_make(), uvcg_mjpeg_make(), or uvcg_framebased_make()
declares f, opts_item, or opts, so uvc_configfs.c fails to compile.

Even if f were replaced with the newly allocated format h in the three
.make_group() callbacks, h->fmt.group.cg_item.ci_parent is still NULL
until configfs_mkdir() calls link_group() after .make_group() returns,
which would cause a NULL pointer dereference; and walking three ci_parent
levels from the parent group argument would walk past the UVC function
instance item because group is only two levels below it.  In all five
functions, opts_item can be resolved from streaming->ci_parent.

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