usb/80862: USB locking issues

[Hans Petter Selasky]

>Number:         80862
>Category:       usb
>Synopsis:       USB locking issues
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    freebsd-usb
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Tue May 10 18:00:24 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     HPS
>Release:        FreeBSD 6.0-CURRENT i386
>Organization:
>Environment:
System: FreeBSD 6.0-CURRENT FreeBSD 6.0-CURRENT #45: Mon Mar 21 15:40:17 CET 
2005 root@:/usr/obj/usr/src/sys/custom i386

>Description:

I'm not sure if "sysctl" calls are under Giant, but "uhub_child_xxxx()" are 
called from sysctl context, and should lock Giant or assert Giant. Secondly 
these calls should not sleep, because then Giant will be exited, and the 
"usb_event_thread()" can detach the USB-device meanwhile.

>How-To-Repeat:

>Fix:

In the file "/sys/dev/usb/uhub.c":

uhub_child_location_str()
{
...

mtx_lock(&Giant); /* missing */

...

mtx_unlock(&Giant);

}


uhub_child_pnpinfo_str()
{
...
mtx_lock(&Giant); /* missing */

This must be moved into "usbd_new_device()", because it can sleep:

        serial[0] = '\0';
        (void)usbd_get_string(dev, dev->ddesc.iSerialNumber, &serial[0]);

Instead something like:

  strcpy(&serial[0], &dev->serial[0]);

must be used. Or just replace "serial" with "dev->serial".

...

mtx_unlock(&Giant);

}

usbd_new_device()
{
...

 udev->serial[0] = '\0';
 (void)usbd_get_string(dev, udev->ddesc.iSerialNumber, &udev->serial[0] 
 /* nice with a length parameter here to limit the 
  length of the serial number */);

 /* format checking */

 for(i = 0;;i++)
 {
  if(udev->serial[i] == '\0') break;
  if(udev->serial[i] == '\"') udev->serial[i] = ' ';
  if(udev->serial[i] == '\n') udev->serial[i] = ' ';
 }

...
}

Something else: 
Should allow root hub to detach:

static driver_t uhubroot_driver =
{
...

   DEVMETHOD(device_detach, uhub_detach),
...
}
>Release-Note:
>Audit-Trail:
>Unformatted: