Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753826AbdDJPR2 (ORCPT ); Mon, 10 Apr 2017 11:17:28 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:45768 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753411AbdDJPR0 (ORCPT ); Mon, 10 Apr 2017 11:17:26 -0400 Date: Mon, 10 Apr 2017 11:17:25 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Felipe Balbi cc: Roger Quadros , , , Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device In-Reply-To: <87tw5wefx2.fsf@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3651 Lines: 104 On Mon, 10 Apr 2017, Felipe Balbi wrote: > Hi, > > Alan Stern 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