[PATCH] Fix USB panics

[M. Warner Losh]

[[ redirected to hackers only ]]

In message: <20040908150943.GA1924 at straylight.m.ringlet.net>
            Peter Pentchev <roam at ringlet.net> writes:
: A couple of days ago I started experimenting with a new USB device,
: which has no FreeBSD driver, and within the first minute of playing with
: it, my kernel panicked - something that I hadn't seen for about a month,
: even with 5.x :)

Hmmmm.  I touched this code recently.

: So here's a How to Panic a RELENG_5 Kernel In Five Easy Steps :)
: 
: 1.  Make sure usbd is not running so it cannot load drivers on demand.
: 
: 2.  kldload usb, and do not load *any* USB device drivers, not even ugen.
: 
: 3.  Plug a device, any device.
: 3a. usbd_probe_and_attach() looks for a specific driver, and fails.
: 3b. usbd_probe_and_attach() looks for a generic driver, and fails.
: 3c. usbd_probe_and_attach() leaves the newly-allocated device_t
:     structure in place (and added to the parent bus), but removes any
:     subdevs traces in the usbd_device_handle.

Leaving it in place is the right thing to do.  It looks like the
second half of this isn't.

: 4.  Unplug the device.
: 4a. usb_disconnect_port() does not find the device_t structure, since it
:     has been removed from usbd_device_handle's subdevs.
: 4b. usb_disconnect_port() frees the usbd_device_handle.

Ah.  Now that's a problem...  that's the root cause of woe.

: 5.  kldload ugen
: 5a. busdma walks the buses, looking for devices that this driver should
:     attach to.
: 5b. busdma calls ugen_attach() on the somewhat orphaned device_t.
: 5c. ugen_attach() calls usbd_devinfo() on the usbd_device_handle pointer
:     extracted from the device_t's softc - which was kinda freed in step
:     4b above :)
: 5d. Congratulations, you have reached kdb's panic handler! :)

And here's where the woe comes home to roost.

: So.. what should be done about it?  Basically, the problem is that
: usbd_probe_and_attach() leaves an orphaned device_t pointing to the
: usbd_device_handle that will be freed when the hardware device is
: physically unplugged.

I'd use different terms to describe it, but what you are saying is
essentially correct. 

: The attached patch seems to solve the problem - and a couple of related
: others - at least for me.  The #ifdef's are effectively #if 0's, I've
: just left them in to keep a perspective on the original code.  The idea
: is to keep the device_t pointer in the usbd_device_handle, but take care
: to check if the device is actually attached before dereferencing it.

Hmmmm

: Also, uhub had to be taught not to remove the device_t pointer on driver
: unload, since the hardware is still physically attached to the machine
: and the device_t is still attached to the bus, even though there is no
: driver for it.  This makes uhub's child_detached handler mostly a no-op,
: with the exception of the panic if the uhub itself is not initialized
: yet; should the whole handler be removed, since the only thing it does
: ought to be handled by usb_disconnect_port() already?

Yes.  I think so

: Is this even the right direction?  IMHO, this situation ought to be
: resolved one way or another before 5.3 hits the shelves, even if the
: solution has nothing to do with my proposed patch :)

Yes.  I don't like parts of it, but they are the parts that you
yourself don't like either :-(.

: Index: dev/usb/uhub.c
: ===================================================================
: RCS file: /home/ncvs/src/sys/dev/usb/uhub.c,v
: retrieving revision 1.62
: diff -u -r1.62 uhub.c
: --- dev/usb/uhub.c	15 Aug 2004 23:39:18 -0000	1.62
: +++ dev/usb/uhub.c	8 Sep 2004 14:06:45 -0000
: @@ -707,7 +707,9 @@
:  			continue;
:  		for (i = 0; dev->subdevs[i]; i++) {
:  			if (dev->subdevs[i] == child) {
: +#ifdef ROAM_SKIP_USB_DEVICE_LEAK
:  				dev->subdevs[i] = NULL;
: +#endif
:  				return;
:  			}
:  		}

I agree with your assessment that the above really makes the routine:

Static void
uhub_child_detached(device_t self, device_t child)
{
	struct uhub_softc *sc = device_get_softc(self);
	usbd_device_handle devhub = sc->sc_hub;

	if (!devhub->hub)
		/* should never happen; children are only created after init */
		panic("hub not fully initialised, but child deleted?");
}

which looks right to me (although having the panic there seems overly
defensive).

: Index: dev/usb/usb_subr.c
: ===================================================================
: RCS file: /home/ncvs/src/sys/dev/usb/usb_subr.c,v
: retrieving revision 1.69
: diff -u -r1.69 usb_subr.c
: --- dev/usb/usb_subr.c	15 Aug 2004 23:39:18 -0000	1.69
: +++ dev/usb/usb_subr.c	8 Sep 2004 14:05:51 -0000
: @@ -1016,9 +1016,11 @@
:  	if (dv != NULL) {
:  		return (USBD_NORMAL_COMPLETION);
:  	}
: +#ifdef ROAM_SKIP_USB_DEVICE_LEAK
:  	tmpdv = dev->subdevs;
:  	dev->subdevs = 0;
:  	free(tmpdv, M_USB);
: +#endif
:  
:  	/*
:  	 * The generic attach failed, but leave the device as it is.

I'm not sure I understand why the above fixes anything.  Is it because
it makes the proper references go away?  If so, it can likely be
deleted.  I'll need to thread through the code to see if any of the
other instances of those 3 lines of code can go.

: @@ -1346,7 +1348,7 @@
:  	di->udi_speed = dev->speed;
:  
:  	if (dev->subdevs != NULL) {
: -		for (i = 0; dev->subdevs[i] &&
: +		for (i = 0; dev->subdevs[i] && device_is_attached(dev->subdevs[i]) &&
:  			     i < USB_MAX_DEVNAMES; i++) {
:  			strncpy(di->udi_devnames[i], USBDEVPTRNAME(dev->subdevs[i]),
:  				USB_MAX_DEVNAMELEN);

OK.  I understand this as well.  It likely is a good change.

Warner