Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753929AbdDKHfh (ORCPT ); Tue, 11 Apr 2017 03:35:37 -0400 Received: from mga05.intel.com ([192.55.52.43]:6600 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753724AbdDKHej (ORCPT ); Tue, 11 Apr 2017 03:34:39 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,184,1488873600"; d="asc'?scan'208";a="954642769" From: Felipe Balbi To: Alan Stern Cc: Roger Quadros , vivek.gautam@codeaurora.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device In-Reply-To: References: Date: Tue, 11 Apr 2017 10:34:14 +0300 Message-ID: <87r30zcs95.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6217 Lines: 188 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Alan Stern 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_unreg= ister()=20 >> >> >>=20 >> >> >> not on the gadget API, no. >> >> >>=20 >> >> >> > call on the previous line invokes the gadget->dev.release callba= ck,=20 >> >> >> > which might deallocate gadget. If that happens, your new memset= will=20 >> >> >> > oops. >> >> >>=20 >> >> >> that won't happen. struct usb_gadget is a member of the UDC's priv= ate >> >> >> structure, like this: >> >> >>=20 >> >> >> 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= =20 >> >> > usb_gadget to control the lifetime of its private structure? >> >>=20 >> >> 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.=20=20 >> >> > Furthermore, net2280_remove() continues to reference the net2280 st= ruct >> >> > after calling usb_del_gadget_udc(), and it never does seem to do a >> >> > final put.) >> >>=20 >> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_devic= e_id *id) >> >> { >> >> struct net2280 *dev; >> >> unsigned long resource, len; >> >> void __iomem *base =3D NULL; >> >> int retval, i; >> >>=20 >> >> /* alloc, and start init */ >> >> dev =3D kzalloc(sizeof(*dev), GFP_KERNEL); >> >> if (dev =3D=3D NULL) { >> >> retval =3D -ENOMEM; >> >> goto done; >> >> } >> >>=20 >> >> 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 th= e=20 >> > driver data in dev->gadget.dev. >>=20 >> hmmm, indeed. The same is happening with other callers of >> usb_add_gadget_udc_release(). >>=20 >> I guess this should be enough? >>=20 >> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev) >>=20=20 >> static void gadget_release(struct device *_dev) >> { >> - struct net2280 *dev =3D dev_get_drvdata(_dev); >> + struct net2280 *dev =3D dev_get_drvdata(_dev->parent); >>=20=20 >> kfree(dev); >> } > > Oddly enough, yes. But it doesn't explain why this code doesn't blow=20 > 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/net2= 280.c index 3828c2ec8623..4dc04253da61 100644 =2D-- 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) =20 /*------------------------------------------------------------------------= -*/ =20 =2Dstatic void gadget_release(struct device *_dev) =2D{ =2D struct net2280 *dev =3D dev_get_drvdata(_dev); =2D =2D kfree(dev); =2D} =2D /* tear down the binding between this driver and the pci device */ =20 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); =20 ep_info(dev, "unbind\n"); + + kfree(dev); } =20 /* 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; =20 =2D retval =3D usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, =2D gadget_release); + retval =3D 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=20 > access the private data structure after calling usb_del_gadget_udc().=20= =20 > 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=20 > usb_add_gadget_udc_release() fails. At the end of that routine, the=20 > error pathway does put_device(&gadget->dev). This will invoke the=20 > release callback, deallocating the private data structure without=20 > 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. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAljshvYACgkQzL64meEa mQYcMQ/+LtFZYvOOqLWc+YjRM6rkvY8akVrj9dLn9a81vxE2nV/V/L9J1cnWjRBr YzYkhWbU5Pthlz3d6lQQwTi5WVNUVDB124qVAF6MYzElVvTC3OelhQkI/KSRLL4o XfJPhISaru82Vw3bZD+U5kO/cJJdVpvdV9DwVsgBuVWFZLpE94ock3nrvTK9kgSC zaQvc1407NnwndGVo5IHPj8fQnLEVVIrKixzXLF1AIKYnbjU5V0e1OWp/Ig7jgUq uAm4OixRn0lyyAyAMbrb/xTp3y12f2fYHhFukZeNVrohr+i9n/LLFNZJ8865YuSF asQNsCeJGbIN4ABlEIBLOAcmYg7Gp282vGJJ/BEFaZbsESoEOMeUauosDGmI7LQM AdmmmyaXpBfxEZh0MkKABBezOGPT4dRWKrXD9EyGbq52ZBCFj/v76PaeBM+cChKK 25nZUXfPCCnQuqwMcwZ+d5tKBSr8XQcNXpAhg+262kU6cztHXUHrQavE+uyHUW0R idpBcWmIOybNEw5lXXQ3Ch8B05sRDxZQiy56GKRx79LTFJUMeSiWsDSUqvfsx8Fr uC90FZ+KJhbMMgSFDPVulJ4410lSqI+h9dbebNuj3Yzjk6jR9T4DIiR9YtWVW7e9 6lH1PxMQ+IvBf8qiZDLr7h5wZoqhk5CKgdUKZNcUW2cC4DYX9Ao= =wcMs -----END PGP SIGNATURE----- --=-=-=--