RE: [PATCH 4.19 093/125] usb: gadget: configfs: fix concurrent issue between composite APIs

[Peter Chen]

> >
> > We meet several NULL pointer issues if configfs_composite_unbind and
> > composite_setup (or composite_disconnect) are running together.
> > These issues occur when do the function switch stress test, the
> > configfs_compsoite_unbind is called from user mode by echo "" to
> > /sys/../UDC entry, and meanwhile, the setup interrupt or disconnect
> > interrupt occurs by hardware. The composite_setup
> 
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -61,6 +61,8 @@ struct gadget_info {
> >  	bool use_os_desc;
> >  	char b_vendor_code;
> >  	char qw_sign[OS_STRING_QW_SIGN_LEN];
> > +	spinlock_t spinlock;
> > +	bool unbind;
> >  };
> >
> >  static inline struct gadget_info *to_gadget_info(struct config_item
> > *item) @@ -1244,6 +1246,7 @@ static int configfs_composite_bind(struct
> usb_gadget *gadget,
> >  	int				ret;
> >
> >  	/* the gi->lock is hold by the caller */
> > +	gi->unbind = 0;
> >  	cdev->gadget = gadget;
> 
> Since variable is bool, I'd expect "= false" here?
> 
> > +	unsigned long flags;
> >
> >  	/* the gi->lock is hold by the caller */
> 
> "is held".
> 
> >  	cdev = get_gadget_data(gadget);
> >  	gi = container_of(cdev, struct gadget_info, cdev);
> > +	spin_lock_irqsave(&gi->spinlock, flags);
> > +	gi->unbind = 1;
> 
> = true;
> 
> > +static int configfs_composite_setup(struct usb_gadget *gadget,
> > +		const struct usb_ctrlrequest *ctrl) {
> > +	struct usb_composite_dev *cdev;
> > +	struct gadget_info *gi;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	cdev = get_gadget_data(gadget);
> > +	if (!cdev)
> > +		return 0;
> > +
> > +	gi = container_of(cdev, struct gadget_info, cdev);
> > +	spin_lock_irqsave(&gi->spinlock, flags);
> > +	cdev = get_gadget_data(gadget);
> 
> cdev already contains required value, why get it second time? (If it needs to be
> done under lock, comment might be useful...)
> 

Hi Pavel,

Thanks for comment.

The reason is the cdev may be set to NULL by configfs_composite_unbind through
another process, eg, the user wants to disable current USB gadget function, 
this patch tries to fix such kinds of concurrent issue.

Peter