Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Mon, 10 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> > On Wed, 5 Apr 2017, Felipe Balbi wrote:
> >
> >> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >> >> >>  	flush_work(&gadget->work);
> >> >> >>  	device_unregister(&udc->dev);
> >> >> >>  	device_unregister(&gadget->dev);
> >> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >> >> >>  }
> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >> >
> >> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> >> >> 
> >> >> not on the gadget API, no.
> >> >> 
> >> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> >> > oops.
> >> >> 
> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> >> structure, like this:
> >> >> 
> >> >> struct dwc3 {
> >> >> 	[...]
> >> >> 	struct usb_gadget	gadget;
> >> >> 	struct usb_gadget_driver *gadget_driver;
> >> >> 	[...]
> >> >> };
> >> >
> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> >> > usb_gadget to control the lifetime of its private structure?
> >> 
> >> nope, not being used. At least not yet.
> >
> > I'm not convinced (yet)...
> >
> >> > (By the way, can you tell what's going on in net2280.c?  I must be
> >> > missing something; it looks like gadget_release() would quickly run
> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
> >> > final put.)
> >> 
> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> {
> >> 	struct net2280		*dev;
> >> 	unsigned long		resource, len;
> >> 	void			__iomem *base = NULL;
> >> 	int			retval, i;
> >> 
> >> 	/* alloc, and start init */
> >> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >> 	if (dev == NULL) {
> >> 		retval = -ENOMEM;
> >> 		goto done;
> >> 	}
> >> 
> >> 	pci_set_drvdata(pdev, dev);
> >> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > That sets the driver data in the struct pci_dev, not in
> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> > driver data in dev->gadget.dev.
> 
> hmmm, indeed. The same is happening with other callers of
> usb_add_gadget_udc_release().
> 
> I guess this should be enough?
> 
> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  static void gadget_release(struct device *_dev)
>  {
> -	struct net2280	*dev = dev_get_drvdata(_dev);
> +	struct net2280	*dev = dev_get_drvdata(_dev->parent);
>  
>  	kfree(dev);
>  }

Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
up every time it gets called, in its current form.

And it doesn't help with the fact that net2280_remove() continues to 
access the private data structure after calling usb_del_gadget_udc().  
Strictly speaking, that routine should do

	get_device(&dev->gadget.dev);

at the start, with a corresponding put_device() at the end.

There's another problem.  Suppose a call to 
usb_add_gadget_udc_release() fails.  At the end of that routine, the 
error pathway does put_device(&gadget->dev).  This will invoke the 
release callback, deallocating the private data structure without 
giving the caller (i.e., the UDC driver) a chance to clean up.

Alan Stern