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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5229 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> >> >> >> --- 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.

Well, it does :-)

dev_get_drvdata(_dev) -> NULL -> kfree(NULL)

We're just leaking memory. I guess a patch like below would be best:

diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 3828c2ec8623..4dc04253da61 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 /*-------------------------------------------------------------------------*/
 
-static void gadget_release(struct device *_dev)
-{
-	struct net2280	*dev = dev_get_drvdata(_dev);
-
-	kfree(dev);
-}
-
 /* tear down the binding between this driver and the pci device */
 
 static void net2280_remove(struct pci_dev *pdev)
@@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
 	device_remove_file(&pdev->dev, &dev_attr_registers);
 
 	ep_info(dev, "unbind\n");
+
+	kfree(dev);
 }
 
 /* wrap this driver around the specified device, but
@@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (retval)
 		goto done;
 
-	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
-			gadget_release);
+	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
 	if (retval)
 		goto done;
 	return 0;

> 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.

it won't deallocate anything :-) dev_set_drvdata() was never called,
we will endup with kfree(NULL) which is safe and just silently returns.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]