bpf: avoid panic on multiple readers
Needs ReviewPublic
Actions

Authored by kp on Wed, Feb 26, 2:05 PM.

Details

Reviewers

None

Group Reviewers

network
pfsense

Summary

If we have multiple simultaneous readers on a single /dev/bpf fd it's possible
for the assertion after the bpf_uiomove() in bpfread() to fail.

Note that the bpf_uiomove() is done outside of the BPFD_LOCK, because uiomove
may sleep. As a result it's possible for another thread to have already
reclaimed toe bd_hbuf, thus causing us to fail the assertion.
Even without INVARIANTS this may provoke panics.

That results (with INVARIANTS) in a panic such as:

login: panic: bpfread: lost bd_hbuf
cpuid = 13
time = 1740567635
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe003972db10
vpanic() at vpanic+0x136/frame 0xfffffe003972dc40
panic() at panic+0x43/frame 0xfffffe003972dca0
bpfread() at bpfread+0x2e8/frame 0xfffffe003972dce0
devfs_read_f() at devfs_read_f+0xe4/frame 0xfffffe003972dd40
dofileread() at dofileread+0x80/frame 0xfffffe003972dd90
sys_read() at sys_read+0xb7/frame 0xfffffe003972de00
amd64_syscall() at amd64_syscall+0x15a/frame 0xfffffe003972df30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe003972df30

syscall (3, FreeBSD ELF64, read), rip = 0x302787166afa, rsp = 0x302782638a78, rbp = 0x302782638aa0 ---

Also add a test case replicating the known trigger for this panic.

Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 62633
Build 59517: arc lint + arc unit

Event Timeline

kp created this revision.Wed, Feb 26, 2:05 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptWed, Feb 26, 2:05 PM

kp requested review of this revision.Wed, Feb 26, 2:05 PM

Harbormaster completed remote builds in B62633: Diff 151495.Wed, Feb 26, 2:05 PM

panic: bpfread: lost bd_hbuf

Maybe another thread has done ROTATE_BUFFERS but ignores bd_hbuf_in_use ?

sys/net/bpf.c
1113	The only functions set `bd_hbuf_in_use` is `bpfopen()` and `bpfread()`. The former is for allocation new description so just ignore it. Given another concurrent thread can not proceed with `bpfread()` if `bd_hbuf_in_use` is set to 1, while (d->bd_hbuf_in_use) { error = mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, PRINET \| PCATCH, "bd_hbuf", 0); if (error != 0) { BPFD_UNLOCK(d); return (error); } } It is not possible for it ( the another concurrent thread ) to set `bd_hbuf_in_use` and the current thread see `bd_hbuf_in_use == 0`. So I believe this will not fix the underlaying issue.