On Wed, 2020-07-22 at 22:56 +0300, Evgeny Novikov wrote: > Hi Alan, > > I have neither an appropriate hardware nor an experience to deal with > issues that you mentioned. Our framework does not allow to detect > them as well at the moment. At last, it seems that rather many > drivers can suffer from these issues. So, it would be much better if > somebody else will suggest necessary fixes and test them carefully. > > BTW, you have already discussed the race within net2280_remove() with > my colleague about 3 years ago. But you did not achieve a consensus > at that time and no fixes were made after all. > > Anyway, one can consider both issues independently on the one fixed > by the patch. FYI. It looks like I'm likely to resume my work on that driver in the next few weeks in which case I could probably look into these Alan. Cheers, Ben. > -- > Evgeny Novikov > Linux Verification Center, ISP RAS > http://linuxtesting.org > > 22.07.2020, 17:17, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx>: > > On Tue, Jul 21, 2020 at 11:15:58PM +0300, Evgeny Novikov wrote: > > > Driver does not release memory for device on error handling > > > paths in > > > net2280_probe() when gadget_release() is not registered yet. > > > > > > The patch fixes the bug like in other similar drivers. > > > > > > Found by Linux Driver Verification project (linuxtesting.org). > > > > > > Signed-off-by: Evgeny Novikov <novikov@xxxxxxxxx> > > > --- > > > drivers/usb/gadget/udc/net2280.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/gadget/udc/net2280.c > > > b/drivers/usb/gadget/udc/net2280.c > > > index 5eff85eeaa5a..d5fe071b2db2 100644 > > > --- a/drivers/usb/gadget/udc/net2280.c > > > +++ b/drivers/usb/gadget/udc/net2280.c > > > @@ -3781,8 +3781,10 @@ static int net2280_probe(struct pci_dev > > > *pdev, const struct pci_device_id *id) > > > return 0; > > > > > > done: > > > - if (dev) > > > + if (dev) { > > > net2280_remove(pdev); > > > + kfree(dev); > > > + } > > > return retval; > > > } > > > > This patch seems to be the tip of an iceberg. Following through its > > implications led to a couple of discoveries. > > > > usb_del_gadget_udc() calls device_unregister(&gadget->dev). Once > > this > > call returns, gadget has to be regarded as a stale pointer. But the > > very next line of code does: > > > > memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > > > for no apparent reason. I'm amazed this hasn't caused problems > > already. > > Is there any justification for keeping this memset? It's hard to > > imagine that it does any good. > > > > Similarly, net2280_remove() calls usb_del_gadget_udc(&dev->gadget) > > at > > its start, and so dev must be a stale pointer for the entire > > remainder > > of the routine. But it gets used repeatedly. Surely we ought to > > have > > a device_get() and device_put() in there. > > > > Alan Stern
