NetBSD Problem Report #44907

From www@NetBSD.org  Tue Apr 26 21:18:04 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 9E8B863C344
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 26 Apr 2011 21:18:04 +0000 (UTC)
Message-Id: <20110426211803.B80B263B9D1@www.NetBSD.org>
Date: Tue, 26 Apr 2011 21:18:03 +0000 (UTC)
From: gmcnutt@cradlepoint.com
Reply-To: gmcnutt@cradlepoint.com
To: gnats-bugs@NetBSD.org
Subject: crash due to race in ehci.c
X-Send-Pr-Version: www-1.0

>Number:         44907
>Category:       kern
>Synopsis:       crash due to race in ehci.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    skrll
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Apr 26 21:20:01 +0000 2011
>Closed-Date:    Thu Dec 05 20:43:45 +0000 2019
>Last-Modified:  Thu Dec 05 20:43:45 +0000 2019
>Originator:     Gordon McNutt
>Release:        5.1
>Organization:
CradlePoint, Inc
>Environment:
NetBSD  5.1 NetBSD 5.1 (SR) #8: Tue Apr 26 15:01:01 MDT 2011  gmcnutt@gmcnutt-desktop:/home/gmcnutt/svn/cpbsd/trunk/sys/arch/evbmips/compile/obj/SR evbmips
#
>Description:
There is a race condition in ehci.c between ehci_timeout and callers of
ehci_freex/ehci_allocx:

1. ehci_timeout() calls usb_add_task(), enqueueing xfer.abort_task
2. someone calls ehci_freex on the same xfer (its abort_task remains enqueued)
3. someone calls ehci_allocx and gets the same xfer, wiping it to zero
4. usb_task_thread subsequently runs and tries to use TAILQ_REMOVE(), which
   results in a 0 pointer deref

Yep, it really happens. The following suggested patch fixes it by always
initializing the abort_task in ehci_allocx and always removing it in
ehci_freex.
>How-To-Repeat:
I run lots of torrents concurrently over multiple usb modems. A couple of usb hubs are in the tree as well.
>Fix:
Index: sys/dev/usb/ehci.c
===================================================================
--- sys/dev/usb/ehci.c	(revision 4123)
+++ sys/dev/usb/ehci.c	(working copy)
@@ -1298,7 +1299,9 @@
 		xfer = malloc(sizeof(struct ehci_xfer), M_USB, M_NOWAIT);
 	}
 	if (xfer != NULL) {
+		struct ehci_xfer *exfer = EXFER(xfer);
 		memset(xfer, 0, sizeof(struct ehci_xfer));
+		usb_init_task(&exfer->abort_task, ehci_timeout_task, exfer);
 #ifdef DIAGNOSTIC
 		EXFER(xfer)->isdone = 1;
 		xfer->busy_free = XFER_BUSY;
@@ -1311,6 +1314,7 @@
 ehci_freex(struct usbd_bus *bus, usbd_xfer_handle xfer)
 {
 	struct ehci_softc *sc = bus->hci_private;
+	struct ehci_xfer *exfer = EXFER(xfer);

 #ifdef DIAGNOSTIC
 	if (xfer->busy_free != XFER_BUSY) {
@@ -1322,6 +1326,7 @@
 		printf("ehci_freex: !isdone\n");
 	}
 #endif
+	usb_rem_task(xfer->pipe->device, &exfer->abort_task);
 	SIMPLEQ_INSERT_HEAD(&sc->sc_free_xfers, xfer, next);
 }

@@ -3115,7 +3120,6 @@
 	}

 	/* Execute the abort in a process context. */
-	usb_init_task(&exfer->abort_task, ehci_timeout_task, addr);
 	usb_add_task(exfer->xfer.pipe->device, &exfer->abort_task,
 	    USB_TASKQ_HC);
 }

>Release-Note:

>Audit-Trail:
From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44907 CVS commit: src/sys/dev/usb
Date: Fri, 27 May 2011 19:04:25 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Fri May 27 19:04:24 UTC 2011

 Modified Files:
 	src/sys/dev/usb: ehci.c ohci.c ohcivar.h uhci.c

 Log Message:
 Apply patch in PR kern/44907 (crash due to race in ehci.c):
  - make sure to remove abort_task in ehci_freex
  - always initialize abort_task in ehci_allocx,
    not in ehci_timeout just before adding the task
 Also apply similar fixes to ohci and uhci.

 XXX: should we also call abort_task handler before removing it from queue
      if *hci_freex() is called for usbd_xfer_handle with queued abort_task?


 To generate a diff of this commit:
 cvs rdiff -u -r1.175 -r1.176 src/sys/dev/usb/ehci.c
 cvs rdiff -u -r1.212 -r1.213 src/sys/dev/usb/ohci.c
 cvs rdiff -u -r1.49 -r1.50 src/sys/dev/usb/ohcivar.h
 cvs rdiff -u -r1.236 -r1.237 src/sys/dev/usb/uhci.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44907 CVS commit: src/sys/dev/usb
Date: Sat, 28 May 2011 15:47:18 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Sat May 28 15:47:17 UTC 2011

 Modified Files:
 	src/sys/dev/usb: ehci.c ohci.c ohcivar.h uhci.c

 Log Message:
 Revert changes for PR kern/44907
 http://mail-index.NetBSD.org/source-changes/2011/05/27/msg022584.html
 for now.  It might cause a panic in ehci_freex() on device detach
 as reported by Paul Goyette on current-users@.


 To generate a diff of this commit:
 cvs rdiff -u -r1.176 -r1.177 src/sys/dev/usb/ehci.c
 cvs rdiff -u -r1.214 -r1.215 src/sys/dev/usb/ohci.c
 cvs rdiff -u -r1.50 -r1.51 src/sys/dev/usb/ohcivar.h
 cvs rdiff -u -r1.237 -r1.238 src/sys/dev/usb/uhci.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Gordon McNutt <gmcnutt@cradlepoint.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@NetBSD.org>
Cc: 
Subject: Re: PR/44907 CVS commit: src/sys/dev/usb
Date: Wed, 1 Jun 2011 17:19:18 -0600

 I haven't seen this on our systems, but I wonder if Paul's crash was due 
 to deref'ing a NULL pipe on this line:

  	usb_rem_task(xfer->pipe->device, &exfer->abort_task);

 Looking up his call stack at usbd_free_xfer it looks like xfer->device 
 should be a usable substitute. The arg is not even used in usb_rem_task() 
 in current code.

 On Fri, 27 May 2011, Izumi Tsutsui wrote:

 >
 > The following reply was made to PR kern/44907; it has been noted by GNATS.
 >
 > From: "Izumi Tsutsui" <tsutsui@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc:
 > Subject: PR/44907 CVS commit: src/sys/dev/usb
 > Date: Fri, 27 May 2011 19:04:25 +0000
 >
 > Module Name:	src
 > Committed By:	tsutsui
 > Date:		Fri May 27 19:04:24 UTC 2011
 >
 > Modified Files:
 > 	src/sys/dev/usb: ehci.c ohci.c ohcivar.h uhci.c
 >
 > Log Message:
 > Apply patch in PR kern/44907 (crash due to race in ehci.c):
 >  - make sure to remove abort_task in ehci_freex
 >  - always initialize abort_task in ehci_allocx,
 >    not in ehci_timeout just before adding the task
 > Also apply similar fixes to ohci and uhci.
 >
 > XXX: should we also call abort_task handler before removing it from queue
 >      if *hci_freex() is called for usbd_xfer_handle with queued abort_task?
 >
 >
 > To generate a diff of this commit:
 > cvs rdiff -u -r1.175 -r1.176 src/sys/dev/usb/ehci.c
 > cvs rdiff -u -r1.212 -r1.213 src/sys/dev/usb/ohci.c
 > cvs rdiff -u -r1.49 -r1.50 src/sys/dev/usb/ohcivar.h
 > cvs rdiff -u -r1.236 -r1.237 src/sys/dev/usb/uhci.c
 >
 > Please note that diffs are not public domain; they are subject to the
 > copyright notices on the relevant files.
 >
 >
 > !SIG:4ddff5e5127751999567911!
 >
 >

Responsible-Changed-From-To: kern-bug-people->skrll
Responsible-Changed-By: skrll@NetBSD.org
Responsible-Changed-When: Sun, 25 Aug 2013 07:10:22 +0000
Responsible-Changed-Why:
Take


State-Changed-From-To: open->feedback
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Thu, 05 Sep 2019 05:19:52 +0000
State-Changed-Why:
I think this is fixed now in supported branches


From: Gordon McNutt <gmcnutt@cradlepoint.com>
To: "gnats-bugs@gnats.netbsd.org" <gnats-bugs@gnats.netbsd.org>
Cc: 
Subject: Re: kern/44907
Date: Thu, 5 Dec 2019 19:08:27 +0000

 --_000_234D68E1BEFE44DEB4066FB6F3B61B93contosocom_
 Content-Type: text/plain; charset="utf-8"
 Content-Transfer-Encoding: base64

 V2Ugd29ya2VkIGFyb3VuZCB0aGlzIHByb2JsZW0geWVhcnMgYWdvIGFuZCBkb27igJl0IGNhcmUg
 YWJvdXQgaXQgYW55bW9yZSwgZmVlbCBmcmVlIHRvIGNsb3NlLg0K

 --_000_234D68E1BEFE44DEB4066FB6F3B61B93contosocom_
 Content-Type: text/html; charset="utf-8"
 Content-ID: <99925136EA9EFC4EB6BE8042E2861BF4@namprd12.prod.outlook.com>
 Content-Transfer-Encoding: base64

 PGh0bWwgeG1sbnM6bz0idXJuOnNjaGVtYXMtbWljcm9zb2Z0LWNvbTpvZmZpY2U6b2ZmaWNlIiB4
 bWxuczp3PSJ1cm46c2NoZW1hcy1taWNyb3NvZnQtY29tOm9mZmljZTp3b3JkIiB4bWxuczptPSJo
 dHRwOi8vc2NoZW1hcy5taWNyb3NvZnQuY29tL29mZmljZS8yMDA0LzEyL29tbWwiIHhtbG5zPSJo
 dHRwOi8vd3d3LnczLm9yZy9UUi9SRUMtaHRtbDQwIj4NCjxoZWFkPg0KPG1ldGEgaHR0cC1lcXVp
 dj0iQ29udGVudC1UeXBlIiBjb250ZW50PSJ0ZXh0L2h0bWw7IGNoYXJzZXQ9dXRmLTgiPg0KPG1l
 dGEgbmFtZT0iR2VuZXJhdG9yIiBjb250ZW50PSJNaWNyb3NvZnQgV29yZCAxNSAoZmlsdGVyZWQg
 bWVkaXVtKSI+DQo8c3R5bGU+PCEtLQ0KLyogRm9udCBEZWZpbml0aW9ucyAqLw0KQGZvbnQtZmFj
 ZQ0KCXtmb250LWZhbWlseToiQ2FtYnJpYSBNYXRoIjsNCglwYW5vc2UtMToyIDQgNSAzIDUgNCA2
 IDMgMiA0O30NCkBmb250LWZhY2UNCgl7Zm9udC1mYW1pbHk6Q2FsaWJyaTsNCglwYW5vc2UtMToy
 IDE1IDUgMiAyIDIgNCAzIDIgNDt9DQovKiBTdHlsZSBEZWZpbml0aW9ucyAqLw0KcC5Nc29Ob3Jt
 YWwsIGxpLk1zb05vcm1hbCwgZGl2Lk1zb05vcm1hbA0KCXttYXJnaW46MGluOw0KCW1hcmdpbi1i
 b3R0b206LjAwMDFwdDsNCglmb250LXNpemU6MTIuMHB0Ow0KCWZvbnQtZmFtaWx5OiJDYWxpYnJp
 IixzYW5zLXNlcmlmO30NCmE6bGluaywgc3Bhbi5Nc29IeXBlcmxpbmsNCgl7bXNvLXN0eWxlLXBy
 aW9yaXR5Ojk5Ow0KCWNvbG9yOiMwNTYzQzE7DQoJdGV4dC1kZWNvcmF0aW9uOnVuZGVybGluZTt9
 DQphOnZpc2l0ZWQsIHNwYW4uTXNvSHlwZXJsaW5rRm9sbG93ZWQNCgl7bXNvLXN0eWxlLXByaW9y
 aXR5Ojk5Ow0KCWNvbG9yOiM5NTRGNzI7DQoJdGV4dC1kZWNvcmF0aW9uOnVuZGVybGluZTt9DQpz
 cGFuLkVtYWlsU3R5bGUxNw0KCXttc28tc3R5bGUtdHlwZTpwZXJzb25hbC1jb21wb3NlOw0KCWZv
 bnQtZmFtaWx5OiJDYWxpYnJpIixzYW5zLXNlcmlmOw0KCWNvbG9yOndpbmRvd3RleHQ7fQ0KLk1z
 b0NocERlZmF1bHQNCgl7bXNvLXN0eWxlLXR5cGU6ZXhwb3J0LW9ubHk7DQoJZm9udC1mYW1pbHk6
 IkNhbGlicmkiLHNhbnMtc2VyaWY7fQ0KQHBhZ2UgV29yZFNlY3Rpb24xDQoJe3NpemU6OC41aW4g
 MTEuMGluOw0KCW1hcmdpbjoxLjBpbiAxLjBpbiAxLjBpbiAxLjBpbjt9DQpkaXYuV29yZFNlY3Rp
 b24xDQoJe3BhZ2U6V29yZFNlY3Rpb24xO30NCi0tPjwvc3R5bGU+DQo8L2hlYWQ+DQo8Ym9keSBs
 YW5nPSJFTi1VUyIgbGluaz0iIzA1NjNDMSIgdmxpbms9IiM5NTRGNzIiPg0KPGRpdiBjbGFzcz0i
 V29yZFNlY3Rpb24xIj4NCjxwIGNsYXNzPSJNc29Ob3JtYWwiPjxzcGFuIHN0eWxlPSJmb250LXNp
 emU6MTEuMHB0Ij5XZSB3b3JrZWQgYXJvdW5kIHRoaXMgcHJvYmxlbSB5ZWFycyBhZ28gYW5kIGRv
 buKAmXQgY2FyZSBhYm91dCBpdCBhbnltb3JlLCBmZWVsIGZyZWUgdG8gY2xvc2UuPG86cD48L286
 cD48L3NwYW4+PC9wPg0KPC9kaXY+DQo8L2JvZHk+DQo8L2h0bWw+DQo=

 --_000_234D68E1BEFE44DEB4066FB6F3B61B93contosocom_--

State-Changed-From-To: feedback->closed
State-Changed-By: maya@NetBSD.org
State-Changed-When: Thu, 05 Dec 2019 20:43:45 +0000
State-Changed-Why:
submission said: "We worked around this problem years ago and don’t care about it anymore, feel free to close.
"


>Unformatted:

Home	PR Database Search