From amistry@am-productions.biz Sun May 14 20:11:51 2006 Return-Path: Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 96F7A16A403 for ; Sun, 14 May 2006 20:11:51 +0000 (UTC) (envelope-from amistry@am-productions.biz) Received: from smtp3.fuse.net (mail-out3.fuse.net [216.68.8.176]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1CC6143D48 for ; Sun, 14 May 2006 20:11:50 +0000 (GMT) (envelope-from amistry@am-productions.biz) Received: from gx5.fuse.net ([69.61.164.22]) by smtp3.fuse.net (InterMail vM.6.01.04.04 201-2131-118-104-20050224) with ESMTP id <20060514201150.IMGN12345.smtp3.fuse.net@gx5.fuse.net> for ; Sun, 14 May 2006 16:11:50 -0400 Received: from bigguy.am-productions.biz ([69.61.164.22]) by gx5.fuse.net (InterMail vG.1.02.00.02 201-2136-104-102-20041210) with ESMTP id <20060514201149.GYCY4651.gx5.fuse.net@bigguy.am-productions.biz> for ; Sun, 14 May 2006 16:11:49 -0400 Message-Id: <1147637538.57596@bigguy.am-productions.biz> Date: Sun, 14 May 2006 16:12:18 -0400 From: "Anish Mistry" To: "FreeBSD gnats submit" Subject: Fix Multiple ugen panics X-Send-Pr-Version: gtk-send-pr 0.4.7 X-GNATS-Notify: >Number: 97271 >Category: usb >Synopsis: Fix Multiple ugen panics >Confidential: no >Severity: serious >Priority: low >Responsible: iedowse >State: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sun May 14 20:20:27 GMT 2006 >Closed-Date: Sun Sep 24 14:57:02 GMT 2006 >Last-Modified: Sun Sep 24 14:57:02 GMT 2006 >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@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: State-Changed-From-To: open->patched State-Changed-By: iedowse State-Changed-When: Mon Jun 5 14:47:37 UTC 2006 State-Changed-Why: Fixed in -current, thanks! Responsible-Changed-From-To: freebsd-usb->iedowse Responsible-Changed-By: iedowse Responsible-Changed-When: Mon Jun 5 14:47:37 UTC 2006 Responsible-Changed-Why: My MFC reminder. http://www.freebsd.org/cgi/query-pr.cgi?pr=97271 State-Changed-From-To: patched->closed State-Changed-By: iedowse State-Changed-When: Sun Sep 24 14:56:18 UTC 2006 State-Changed-Why: This has now been merged into RELENG_6 - sorry for the long delay. http://www.freebsd.org/cgi/query-pr.cgi?pr=97271 >Unformatted: