usb/97271: Fix Multiple ugen panics
Anish Mistry
amistry at am-productions.biz
Sun May 14 20:20:28 UTC 2006
>Number: 97271
>Category: usb
>Synopsis: Fix Multiple ugen panics
>Confidential: no
>Severity: serious
>Priority: low
>Responsible: freebsd-usb
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Sun May 14 20:20:27 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator: Anish Mistry
>Release: FreeBSD 6.1-RELEASE i386
>Organization:
AM Productions
>Environment:
System: FreeBSD 6.1-RELEASE #0: Wed May 10 01:44:17 EDT 2006
amistry at bigguy.am-productions.biz:/usr/obj/usr/src/sys/BIGGUY
>Description:
There are several race conditions is the ugen driver that make it easy to panic the system when a device is detached during an IO operation. There are also panics when select() and poll() are used on ugen device endpoints.
The attached patch fixes these issues. The patch also fixes the current PRs:
usb/94311
usb/68232
Some of these issues are relevant to NetBSD and probably OpenBSD and DragonflyBSD.
>How-To-Repeat:
>Fix:
--- ugen-multiple-panics.patch begins here ---
--- /sys/dev/usb/ugen.c.orig Thu Dec 15 16:57:32 2005
+++ /sys/dev/usb/ugen.c Tue May 9 12:16:28 2006
@@ -1,4 +1,4 @@
-/* $NetBSD: ugen.c,v 1.59 2002/07/11 21:14:28 augustss Exp $ */
+/* $NetBSD: ugen.c,v 1.79 2006/03/01 12:38:13 yamt Exp $ */
/* Also already merged from NetBSD:
* $NetBSD: ugen.c,v 1.61 2002/09/23 05:51:20 simonb Exp $
@@ -284,6 +284,9 @@
ugen_make_devnodes(sc);
#endif
+ usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, sc->sc_udev,
+ USBDEV(sc->sc_dev));
+
USB_ATTACH_SUCCESS_RETURN;
}
@@ -383,6 +386,7 @@
M_WAITOK);
niface_cache = niface;
+ memset(sc->sc_endpoints, 0, sizeof sc->sc_endpoints);
for (ifaceno = 0; ifaceno < niface; ifaceno++) {
DPRINTFN(1,("ugen_set_config: ifaceno %d\n", ifaceno));
err = usbd_device2interface_handle(dev, ifaceno, &iface);
@@ -511,7 +515,7 @@
for (dir = OUT; dir <= IN; dir++) {
if (flag & (dir == OUT ? FWRITE : FREAD)) {
sce = &sc->sc_endpoints[endpt][dir];
- if (sce->edesc == 0)
+ if (sce == 0 ||sce->edesc == 0)
return (ENXIO);
}
}
@@ -650,7 +654,7 @@
if (!(flag & (dir == OUT ? FWRITE : FREAD)))
continue;
sce = &sc->sc_endpoints[endpt][dir];
- if (sce->pipeh == NULL)
+ if (sce == NULL || sce->pipeh == NULL)
continue;
DPRINTFN(5, ("ugenclose: endpt=%d dir=%d sce=%p\n",
endpt, dir, sce));
@@ -835,6 +839,9 @@
USB_GET_SC(ugen, UGENUNIT(dev), sc);
+ if(sc->sc_dying)
+ return (EIO);
+
UGEN_DEV_REF(dev, sc);
error = ugen_do_read(sc, endpt, uio, flag);
UGEN_DEV_RELE(dev, sc);
@@ -933,6 +940,9 @@
USB_GET_SC(ugen, UGENUNIT(dev), sc);
+ if (sc->sc_dying)
+ return (EIO);
+
UGEN_DEV_REF(dev, sc);
error = ugen_do_write(sc, endpt, uio, flag);
UGEN_DEV_RELE(dev, sc);
@@ -969,8 +979,31 @@
return;
USB_GET_SC(ugen, UGENUNIT(dev), sc);
sce = &sc->sc_endpoints[endpt][IN];
- if (sce->pipeh)
- usbd_abort_pipe(sce->pipeh);
+ if (sce)
+ {
+ if(sce->pipeh)
+ usbd_abort_pipe(sce->pipeh);
+
+ if (sce->state & UGEN_ASLP) {
+ sce->state &= ~UGEN_ASLP;
+ DPRINTFN(5, ("ugenpurge: waking %p\n", sce));
+ wakeup(sce);
+ }
+ selwakeuppri(&sce->rsel, PZERO);
+ }
+ sce = &sc->sc_endpoints[endpt][OUT];
+ if (sce)
+ {
+ if(sce->pipeh)
+ usbd_abort_pipe(sce->pipeh);
+
+ if (sce->state & UGEN_ASLP) {
+ sce->state &= ~UGEN_ASLP;
+ DPRINTFN(5, ("ugenpurge: waking %p\n", sce));
+ wakeup(sce);
+ }
+ selwakeuppri(&sce->rsel, PZERO);
+ }
}
#endif
@@ -994,11 +1027,13 @@
for (i = 0; i < USB_MAX_ENDPOINTS; i++) {
for (dir = OUT; dir <= IN; dir++) {
sce = &sc->sc_endpoints[i][dir];
- if (sce->pipeh)
+ if (sce && sce->pipeh)
usbd_abort_pipe(sce->pipeh);
+ selwakeuppri(&sce->rsel, PZERO);
}
}
+
#if defined(__NetBSD__) || defined(__OpenBSD__)
s = splusb();
if (sc->sc_refcnt > 0) {
@@ -1035,6 +1070,9 @@
destroy_dev(sc->dev);
#endif
+ usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc->sc_udev,
+ USBDEV(sc->sc_dev));
+
return (0);
}
@@ -1292,7 +1330,7 @@
/* This flag only affects read */
sce = &sc->sc_endpoints[endpt][IN];
- if (sce->pipeh == NULL) {
+ if (sce == NULL || sce->pipeh == NULL) {
printf("ugenioctl: USB_SET_SHORT_XFER, no pipe\n");
return (EIO);
}
@@ -1304,6 +1342,12 @@
return (0);
case USB_SET_TIMEOUT:
sce = &sc->sc_endpoints[endpt][IN];
+ if (sce == NULL
+ /* XXX this shouldn't happen, but the distinction between
+ input and output pipes isn't clear enough.
+ || sce->pipeh == NULL */
+ )
+ return (EINVAL);
sce->timeout = *(int *)addr;
return (0);
default:
@@ -1331,9 +1375,6 @@
err = ugen_set_config(sc, *(int *)addr);
switch (err) {
case USBD_NORMAL_COMPLETION:
-#if defined(__FreeBSD__)
- ugen_make_devnodes(sc);
-#endif
break;
case USBD_IN_USE:
return (EBUSY);
@@ -1541,6 +1582,9 @@
USB_GET_SC(ugen, UGENUNIT(dev), sc);
+ if (sc->sc_dying)
+ return (EIO);
+
UGEN_DEV_REF(dev, sc);
error = ugen_do_ioctl(sc, endpt, cmd, addr, flag, p);
UGEN_DEV_RELE(dev, sc);
@@ -1555,14 +1599,32 @@
int revents = 0;
int s;
+ /* Do not allow to poll a control endpoint */
+ if ( UGENENDPOINT(dev) == USB_CONTROL_ENDPOINT )
+ return (EIO);
+
USB_GET_SC(ugen, UGENUNIT(dev), sc);
if (sc->sc_dying)
return (EIO);
- /* XXX always IN */
- sce = &sc->sc_endpoints[UGENENDPOINT(dev)][IN];
-#ifdef DIAGNOSTIC
+ if((events & POLLIN) && (events & POLLOUT)) {
+ printf("ugenpoll: POLLIN and POLLOUT? We're not handling it, so bail.\n");
+ return (EIO);
+ }
+
+ if(events & (POLLIN | POLLRDNORM))
+ sce = &sc->sc_endpoints[UGENENDPOINT(dev)][IN];
+ else if(events & (POLLOUT | POLLWRNORM))
+ sce = &sc->sc_endpoints[UGENENDPOINT(dev)][OUT];
+ else {
+ printf("ugenpoll: unhandled input event\n");
+ return (EIO);
+ }
+
+ if (sce == NULL)
+ return (EIO);
+
if (!sce->edesc) {
printf("ugenpoll: no edesc\n");
return (EIO);
@@ -1571,23 +1633,26 @@
printf("ugenpoll: no pipe\n");
return (EIO);
}
-#endif
s = splusb();
switch (sce->edesc->bmAttributes & UE_XFERTYPE) {
case UE_INTERRUPT:
- if (events & (POLLIN | POLLRDNORM)) {
- if (sce->q.c_cc > 0)
+ if (sce->q.c_cc > 0) {
+ if (events & (POLLIN | POLLRDNORM))
revents |= events & (POLLIN | POLLRDNORM);
- else
- selrecord(p, &sce->rsel);
+ else if (events & (POLLOUT | POLLWRNORM))
+ revents |= events & (POLLOUT | POLLWRNORM);
+ } else {
+ selrecord(p, &sce->rsel);
}
break;
case UE_ISOCHRONOUS:
- if (events & (POLLIN | POLLRDNORM)) {
- if (sce->cur != sce->fill)
+ if (sce->cur != sce->fill) {
+ if (events & (POLLIN | POLLRDNORM))
revents |= events & (POLLIN | POLLRDNORM);
- else
- selrecord(p, &sce->rsel);
+ else if (events & (POLLOUT | POLLWRNORM))
+ revents |= events & (POLLOUT | POLLWRNORM);
+ } else {
+ selrecord(p, &sce->rsel);
}
break;
case UE_BULK:
--- ugen-multiple-panics.patch ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-usb
mailing list