NetBSD Problem Report #58596

From www@netbsd.org  Wed Aug 14 13:11:57 2024
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)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 58CE11A9242
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 14 Aug 2024 13:11:57 +0000 (UTC)
Message-Id: <20240814131155.C0E571A9243@mollari.NetBSD.org>
Date: Wed, 14 Aug 2024 13:11:55 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: bpf(4) MP-safety issues
X-Send-Pr-Version: www-1.0

>Number:         58596
>Category:       kern
>Synopsis:       bpf(4) MP-safety issues
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Aug 14 13:15:00 +0000 2024
>Last-Modified:  Fri Sep 13 14:15:02 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NeFoundatBPF tion
>Environment:
>Description:
Correctness issues:

1. access to bif_mbuf_head/tail is not serialized by a lock, and it's not clear the caller is always guaranteed to run on the same CPU
2. concurrent bpf_read on the same instance is not serialized, which might lead to uiomove on d->bd_hbuf in one thread while another thread is rotating the buffer
3. selnotify is done outside the lock
4. fownsignal is done in softint context (_bpf_mtap/_bpf_mptap2 -> bpf_deliver -> catchpacket [under pserialize] -> bpf_wakeup -> fownsignal) but takes the adaptive lock proc_lock which is probably forbidden in softint and certainly forbidden during pserialize (comment in bpf_deliver: /* Assume catchpacket doesn't sleep */)
5. updates to bd_pid are not consistently serialized by a lock (also bd_pid concept seems incoherent and appears to be only used for stat, why bother?)
6. bpf_detachd does pserialize_perform under a spin lock (bd_buf_mtx), which is forbidden
7. callout_halt in bpf_ioctl and bpf_read doesn't prevent concurrent select/poll from starting up timer again (also unclear why bpf_ioctl or bpf_read should cancel a concurrent select/poll)

Performance issues:

8. bpf_write shouldn't need to take bd_mtx; should do it with pserialize
>How-To-Repeat:
1. code inspection
2. write a program that opens /dev/bpf and reads from the same fd in two threads at once
>Fix:
Yes, please!

>Audit-Trail:
From: "Rin Okuyama" <rin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58596 CVS commit: src/sys/net
Date: Thu, 15 Aug 2024 02:22:46 +0000

 Module Name:	src
 Committed By:	rin
 Date:		Thu Aug 15 02:22:46 UTC 2024

 Modified Files:
 	src/sys/net: bpf.c

 Log Message:
 bpf: Mark bpfread_filtops FILTEROP_MPSAFE

 Fix deadlock for non-NET_MPSAFE kernel, reported as
 PR kern/58531 (thanks manu@ for test).

 I've confirmed that there is no new regression for ATF with
 any combination of -HEAD/netbsd-10 and default/NET_MPSAFE
 rump kernels (aarch64).

 Although, some problems have been reported on MP-safety for
 bpf(4), PR kern/58596. But, it should take some time to fix.
 At the moment, commit this part in advance.

 OK ozaki-r@


 To generate a diff of this commit:
 cvs rdiff -u -r1.252 -r1.253 src/sys/net/bpf.c

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

From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58596 CVS commit: src/sys/net
Date: Mon, 19 Aug 2024 07:45:31 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Mon Aug 19 07:45:31 UTC 2024

 Modified Files:
 	src/sys/net: bpf.c bpfdesc.h

 Log Message:
 bpf: restore wakeup softint

 This change fixes the issue that fownsignal which can take an
 adaptive mutex is called inside a pserialize read section in
 bpf_deliver.

 Fix issue #4 (only the latter of two) in PR#58596


 To generate a diff of this commit:
 cvs rdiff -u -r1.255 -r1.256 src/sys/net/bpf.c
 cvs rdiff -u -r1.48 -r1.49 src/sys/net/bpfdesc.h

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

From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58596 CVS commit: src/sys/net
Date: Mon, 19 Aug 2024 07:47:16 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Mon Aug 19 07:47:16 UTC 2024

 Modified Files:
 	src/sys/net: bpf.c bpfdesc.h

 Log Message:
 bpf: protect selnotify and selrecord with bd_buf_mtx

 We have to make updates and checks of buffers and calls of
 selnotify/selrecord atomic to satisfy constraints of sel* API.

 Also, bd_state and bd_cv are protected by bd_buf_mtx now.

 Fix issue #3 of PR#58596

 Part of the fix is inspired by riastradh's patch.


 To generate a diff of this commit:
 cvs rdiff -u -r1.256 -r1.257 src/sys/net/bpf.c
 cvs rdiff -u -r1.49 -r1.50 src/sys/net/bpfdesc.h

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

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58596 CVS commit: [netbsd-10] src/sys/net
Date: Thu, 22 Aug 2024 19:31:08 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Aug 22 19:31:08 UTC 2024

 Modified Files:
 	src/sys/net [netbsd-10]: bpf.c

 Log Message:
 Pull up following revision(s) (requested by rin in ticket #784):

 	sys/net/bpf.c: revision 1.253

 bpf: Mark bpfread_filtops FILTEROP_MPSAFE

 Fix deadlock for non-NET_MPSAFE kernel, reported as
 PR kern/58531 (thanks manu@ for test).

 I've confirmed that there is no new regression for ATF with
 any combination of -HEAD/netbsd-10 and default/NET_MPSAFE
 rump kernels (aarch64).

 Although, some problems have been reported on MP-safety for
 bpf(4), PR kern/58596. But, it should take some time to fix.

 At the moment, commit this part in advance.
 OK ozaki-r@


 To generate a diff of this commit:
 cvs rdiff -u -r1.249.2.1 -r1.249.2.2 src/sys/net/bpf.c

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

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58596 CVS commit: [netbsd-10] src/sys/net
Date: Fri, 13 Sep 2024 14:13:06 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Fri Sep 13 14:13:06 UTC 2024

 Modified Files:
 	src/sys/net [netbsd-10]: bpf.c bpfdesc.h

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #858):

 	sys/net/bpfdesc.h: revision 1.49
 	sys/net/bpf.c: revision 1.256
 	sys/net/bpf.c: revision 1.257
 	sys/net/bpfdesc.h: revision 1.50

 bpf: restore wakeup softint

 This change fixes the issue that fownsignal which can take an
 adaptive mutex is called inside a pserialize read section in
 bpf_deliver.

 Fix issue #4 (only the latter of two) in PR#58596
 bpf: protect selnotify and selrecord with bd_buf_mtx

 We have to make updates and checks of buffers and calls of
 selnotify/selrecord atomic to satisfy constraints of sel* API.

 Also, bd_state and bd_cv are protected by bd_buf_mtx now.

 Fix issue #3 of PR#58596

 Part of the fix is inspired by riastradh's patch.


 To generate a diff of this commit:
 cvs rdiff -u -r1.249.2.2 -r1.249.2.3 src/sys/net/bpf.c
 cvs rdiff -u -r1.48 -r1.48.10.1 src/sys/net/bpfdesc.h

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

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58596 CVS commit: [netbsd-9] src/sys/net
Date: Fri, 13 Sep 2024 14:14:41 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Fri Sep 13 14:14:41 UTC 2024

 Modified Files:
 	src/sys/net [netbsd-9]: bpf.c bpfdesc.h

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #1886):

 	sys/net/bpfdesc.h: revision 1.49
 	sys/net/bpf.c: revision 1.256
 	sys/net/bpf.c: revision 1.257
 	sys/net/bpfdesc.h: revision 1.50

 bpf: restore wakeup softint

 This change fixes the issue that fownsignal which can take an
 adaptive mutex is called inside a pserialize read section in
 bpf_deliver.

 Fix issue #4 (only the latter of two) in PR#58596
 bpf: protect selnotify and selrecord with bd_buf_mtx

 We have to make updates and checks of buffers and calls of
 selnotify/selrecord atomic to satisfy constraints of sel* API.

 Also, bd_state and bd_cv are protected by bd_buf_mtx now.

 Fix issue #3 of PR#58596

 Part of the fix is inspired by riastradh's patch.


 To generate a diff of this commit:
 cvs rdiff -u -r1.229.2.3 -r1.229.2.4 src/sys/net/bpf.c
 cvs rdiff -u -r1.46 -r1.46.6.1 src/sys/net/bpfdesc.h

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

Home	PR Database Search