NetBSD Problem Report #57574

From www@netbsd.org  Wed Aug  9 07:03:21 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 524C51A9238
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  9 Aug 2023 07:03:21 +0000 (UTC)
Message-Id: <20230809070319.8ED861A923A@mollari.NetBSD.org>
Date: Wed,  9 Aug 2023 07:03:19 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: workqueue_wait use-after-free
X-Send-Pr-Version: www-1.0

>Number:         57574
>Category:       kern
>Synopsis:       workqueue_wait use-after-free
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Aug 09 07:05:00 +0000 2023
>Closed-Date:    
>Last-Modified:  Fri Apr 19 02:34:16 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, 8
>Organization:
The NetBSD Workqueue
>Environment:
>Description:
1. workqueue_enqueue puts a struct work on q_queue_pending.
2. workqueue_worker transfers a batch of work from q_queue_pending to q_queue_running, and then calls workqueue_runlist without the lock.
3. workqueue_runlist calls the workqueue function on each struct work in the queue -- and, although it doesn't _modify_ anything in q_queue_running, it must not touch the struct work after calling the function, because...
4. The workqueue function is allowed to free the structure containing the struct work.
5. workqueue_wait searches through both q_queue_pending and q_queue_running for the matching struct work.

However, the first struct work in q_queue_running may have been freed by the time workqueue_wait touches it.
>How-To-Repeat:
code inspection
>Fix:
Some options:

(a) Add lock/unlock cycles in workqueue_runlist to keep q_queue_running updated under the lock, and do something to keep a pointer to the current struct work -- not safe to dereference to find the next entry in the list, but safe to compare in workqueue_wait.  However, presumably avoiding these lock/unlock cycles on each struct work was an intentional part of the workqueue(9) implementation.

(b) Add a generation number to each queue, incremented each time workqueue_runlist finishes a batch of work, and a flag indicating whether workqueue_runlist is in progress.  workqueue_wait must 1. wait until the struct work is not in q_queue_pending, and then 2. if workqueue_runlist is in progress, wait until the generation number changes.  If workqueue_wait found the struct work in step (1), we can skip it on all other CPUs.  This might raise the cost of workqueue_wait (waiting for more work to finish than it strictly needs to), but workqueue_wait is a slow path on teardown; this avoids raising the cost in the fast path of running work.

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57574 CVS commit: src/tests/rump
Date: Wed, 9 Aug 2023 08:23:03 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Aug  9 08:23:03 UTC 2023

 Modified Files:
 	src/tests/rump/kernspace: kernspace.h workqueue.c
 	src/tests/rump/rumpkern: t_workqueue.c

 Log Message:
 workqueue(9) tests: Add test for PR kern/57574.

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.8 -r1.9 src/tests/rump/kernspace/kernspace.h \
     src/tests/rump/kernspace/workqueue.c
 cvs rdiff -u -r1.2 -r1.3 src/tests/rump/rumpkern/t_workqueue.c

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

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57574 CVS commit: src
Date: Wed, 9 Aug 2023 08:23:14 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Aug  9 08:23:13 UTC 2023

 Modified Files:
 	src/sys/kern: subr_workqueue.c
 	src/tests/rump/rumpkern: t_workqueue.c

 Log Message:
 workqueue(9): Avoid touching running work items in workqueue_wait.

 As soon as the workqueue function has called, it is forbidden to
 touch the struct work passed to it -- the function might free or
 reuse the data structure it is embedded in.

 So workqueue_wait is forbidden to search the queue for the batch of
 running work items.  Instead, use a generation number which is odd
 while the thread is processing a batch of work and even when not.

 There's still a small optimization available with the struct work
 pointer to wait for: if we find the work item in one of the per-CPU
 _pending_ queues, then after we wait for a batch of work to complete
 on that CPU, we don't need to wait for work on any other CPUs.

 PR kern/57574

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.41 -r1.42 src/sys/kern/subr_workqueue.c
 cvs rdiff -u -r1.3 -r1.4 src/tests/rump/rumpkern/t_workqueue.c

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

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 09 Aug 2023 08:26:25 +0000
State-Changed-Why:
test and fix committed


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57574 CVS commit: [netbsd-10] src
Date: Mon, 4 Sep 2023 16:57:57 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Sep  4 16:57:57 UTC 2023

 Modified Files:
 	src/sys/kern [netbsd-10]: subr_workqueue.c
 	src/tests/rump/kernspace [netbsd-10]: kernspace.h workqueue.c
 	src/tests/rump/rumpkern [netbsd-10]: Makefile t_workqueue.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #342):

 	sys/kern/subr_workqueue.c: revision 1.42
 	sys/kern/subr_workqueue.c: revision 1.43
 	sys/kern/subr_workqueue.c: revision 1.44
 	sys/kern/subr_workqueue.c: revision 1.45
 	sys/kern/subr_workqueue.c: revision 1.46
 	tests/rump/kernspace/workqueue.c: revision 1.7
 	sys/kern/subr_workqueue.c: revision 1.47
 	tests/rump/kernspace/workqueue.c: revision 1.8
 	tests/rump/kernspace/workqueue.c: revision 1.9
 	tests/rump/rumpkern/t_workqueue.c: revision 1.3
 	tests/rump/rumpkern/t_workqueue.c: revision 1.4
 	tests/rump/kernspace/kernspace.h: revision 1.9
 	tests/rump/rumpkern/Makefile: revision 1.20

 tests/rump/rumpkern: Use PROGDPLIBS, not explicit -L/-l.

 This way we relink the t_* test programs whenever changes under
 tests/rump/kernspace change libkernspace.a.

 workqueue(9) tests: Nix trailing whitespace.

 workqueue(9) tests: Destroy struct work immediately on entry.

 workqueue(9) tests: Add test for PR kern/57574.

 workqueue(9): Avoid touching running work items in workqueue_wait.

 As soon as the workqueue function has called, it is forbidden to
 touch the struct work passed to it -- the function might free or
 reuse the data structure it is embedded in.

 So workqueue_wait is forbidden to search the queue for the batch of
 running work items.  Instead, use a generation number which is odd
 while the thread is processing a batch of work and even when not.

 There's still a small optimization available with the struct work
 pointer to wait for: if we find the work item in one of the per-CPU
 _pending_ queues, then after we wait for a batch of work to complete
 on that CPU, we don't need to wait for work on any other CPUs.
 PR kern/57574

 workqueue(9): Sprinkle dtrace probes for workqueue_wait edge cases.

 Let's make it easy to find out whether these are hit.

 workqueue(9): Stop violating queue(3) internals.

 workqueue(9): Avoid unnecessary mutex_exit/enter cycle each loop.

 workqueue(9): Sort includes.
 No functional change intended.

 workqueue(9): Factor out wq->wq_flags & WQ_FPU in workqueue_worker.
 No functional change intended.  Makes it clearer that s is
 initialized when used.


 To generate a diff of this commit:
 cvs rdiff -u -r1.41 -r1.41.2.1 src/sys/kern/subr_workqueue.c
 cvs rdiff -u -r1.8 -r1.8.10.1 src/tests/rump/kernspace/kernspace.h
 cvs rdiff -u -r1.6 -r1.6.16.1 src/tests/rump/kernspace/workqueue.c
 cvs rdiff -u -r1.19 -r1.19.8.1 src/tests/rump/rumpkern/Makefile
 cvs rdiff -u -r1.2 -r1.2.16.1 src/tests/rump/rumpkern/t_workqueue.c

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

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 04 Apr 2024 20:04:20 +0000
State-Changed-Why:
already pulled up to 10
pullup-9 #1830
needs more work for 8, probably


Responsible-Changed-From-To: kern-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Thu, 04 Apr 2024 20:04:51 +0000
Responsible-Changed-Why:
mine


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57574 CVS commit: [netbsd-9] src
Date: Thu, 18 Apr 2024 15:51:36 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Apr 18 15:51:36 UTC 2024

 Modified Files:
 	src/share/man/man9 [netbsd-9]: workqueue.9
 	src/sys/kern [netbsd-9]: subr_workqueue.c
 	src/tests/rump/kernspace [netbsd-9]: kernspace.h workqueue.c
 	src/tests/rump/rumpkern [netbsd-9]: Makefile t_workqueue.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1830):

 	sys/kern/subr_workqueue.c: revision 1.40
 	sys/kern/subr_workqueue.c: revision 1.41
 	sys/kern/subr_workqueue.c: revision 1.42
 	sys/kern/subr_workqueue.c: revision 1.43
 	sys/kern/subr_workqueue.c: revision 1.44
 	sys/kern/subr_workqueue.c: revision 1.45
 	sys/kern/subr_workqueue.c: revision 1.46
 	tests/rump/kernspace/workqueue.c: revision 1.7
 	sys/kern/subr_workqueue.c: revision 1.47
 	tests/rump/kernspace/workqueue.c: revision 1.8
 	tests/rump/kernspace/workqueue.c: revision 1.9
 	tests/rump/rumpkern/t_workqueue.c: revision 1.3
 	tests/rump/rumpkern/t_workqueue.c: revision 1.4
 	tests/rump/kernspace/kernspace.h: revision 1.9
 	tests/rump/rumpkern/Makefile: revision 1.20
 	sys/kern/subr_workqueue.c: revision 1.39
 	share/man/man9/workqueue.9: revision 1.15
 	(all via patch)

 workqueue: Lift unnecessary restriction on workqueue_wait.

 Allow multiple concurrent waits at a time, and allow enqueueing work
 at the same time (as long as it's not the work we're waiting for).

 This way multiple users can use a shared global workqueue and safely
 wait for individual work items concurrently, while the workqueue is
 still in use for other items (e.g., wg(4) peers).

 This has the side effect of taking away a diagnostic measure, but I
 think allowing the diagnostic's false positives instead of rejecting
 them is worth it.  We could cheaply add it back with some false
 negatives if it's important.
 workqueue(9): workqueue_wait and workqueue_destroy may sleep.

 But might not, so assert sleepable up front.
 workqueue(9): Sprinkle dtrace probes.
 tests/rump/rumpkern: Use PROGDPLIBS, not explicit -L/-l.

 This way we relink the t_* test programs whenever changes under
 tests/rump/kernspace change libkernspace.a.

 workqueue(9) tests: Nix trailing whitespace.

 workqueue(9) tests: Destroy struct work immediately on entry.

 workqueue(9) tests: Add test for PR kern/57574.

 workqueue(9): Avoid touching running work items in workqueue_wait.

 As soon as the workqueue function has called, it is forbidden to
 touch the struct work passed to it -- the function might free or
 reuse the data structure it is embedded in.

 So workqueue_wait is forbidden to search the queue for the batch of
 running work items.  Instead, use a generation number which is odd
 while the thread is processing a batch of work and even when not.
 There's still a small optimization available with the struct work
 pointer to wait for: if we find the work item in one of the per-CPU
 _pending_ queues, then after we wait for a batch of work to complete
 on that CPU, we don't need to wait for work on any other CPUs.
 PR kern/57574

 workqueue(9): Sprinkle dtrace probes for workqueue_wait edge cases.

 Let's make it easy to find out whether these are hit.

 workqueue(9): Stop violating queue(3) internals.

 workqueue(9): Avoid unnecessary mutex_exit/enter cycle each loop.

 workqueue(9): Sort includes.
 No functional change intended.

 workqueue(9): Factor out wq->wq_flags & WQ_FPU in workqueue_worker.
 No functional change intended.  Makes it clearer that s is
 initialized when used.


 To generate a diff of this commit:
 cvs rdiff -u -r1.12 -r1.12.6.1 src/share/man/man9/workqueue.9
 cvs rdiff -u -r1.37 -r1.37.6.1 src/sys/kern/subr_workqueue.c
 cvs rdiff -u -r1.8 -r1.8.2.1 src/tests/rump/kernspace/kernspace.h
 cvs rdiff -u -r1.6 -r1.6.8.1 src/tests/rump/kernspace/workqueue.c
 cvs rdiff -u -r1.18 -r1.18.2.1 src/tests/rump/rumpkern/Makefile
 cvs rdiff -u -r1.2 -r1.2.8.1 src/tests/rump/rumpkern/t_workqueue.c

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

State-Changed-From-To: pending-pullups->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 19 Apr 2024 02:34:16 +0000
State-Changed-Why:
fixed and pulled up to 10 and 9
still needs pullup-8 until 8 is EOL


>Unformatted:

Home	PR Database Search