Bug 238814
| Summary: | geom: topology lock being dropped in dumpconf of gate, raid, & raid3 | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Ryan Libby <rlibby> |
| Component: | kern | Assignee: | Mark Johnston <markj> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | admin, bugmeister, markj |
| Priority: | --- | Keywords: | crash |
| Version: | CURRENT | Flags: | linimon:
mfc-stable13?
|
| Hardware: | Any | ||
| OS: | Any | ||
| See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=238817 | ||
I fixed a similar problem in gmirror in r333278. I suspect a similar approach would work for graid, but have not yet looked at the code. (In reply to Mark Johnston from comment #1) Thanks for the pointer, I will take a look. I ran the tests and couldn't panic on FreeBSD 12.3-STABLE # uname -a FreeBSD 12.3-STABLE #0 r372167M: Wed Jun 22 15:14:34 EEST 2022 ^Triage: clear stale flags. To submitter: is this aging PR still relevant? (In reply to Mark Linimon from comment #4) Yes, the problem still exists. I think the gate GEOM is okay, but the patch from comment 1 needs to be adapted to the raid and raid3 GEOMs. (In reply to Mark Johnston from comment #5) I spent a bit of time looking at this, but for g_raid it's quite non-trivial and hard to test. My patch for gmirror also relaxes synchronization perhaps a bit too much, in that inconsistencies are possible (e.g., disk state transitions and syncid/genid bumps happen without locking), though it fixes the larger problem. (In reply to Mark Johnston from comment #5) My comment about GEOM gate is wrong, the topology lock simply can't be dropped. Fortunately, that one is easy to fix. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b37b2543a23b44c78f6d78823dcfcedba46570db commit b37b2543a23b44c78f6d78823dcfcedba46570db Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2024-10-04 14:53:57 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-10-04 15:56:34 +0000 ggate: Avoid dropping the GEOM topology lock in dumpconf In general it's not safe to drop the topology lock in these routines, as GEOM assumes that the mesh will be consistent during traversal. However, there's no reason we can't hold the topology lock across calls to g_gate_release(). (Note that g_gate_hold() can be called with the topology lock held.) PR: 238814 MFC after: 2 weeks sys/geom/gate/g_gate.c | 3 --- 1 file changed, 3 deletions(-) A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=23b44ad101ca2381186aa565b6c04a978c02a35e commit 23b44ad101ca2381186aa565b6c04a978c02a35e Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2024-10-04 14:53:57 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-10-18 12:27:13 +0000 ggate: Avoid dropping the GEOM topology lock in dumpconf In general it's not safe to drop the topology lock in these routines, as GEOM assumes that the mesh will be consistent during traversal. However, there's no reason we can't hold the topology lock across calls to g_gate_release(). (Note that g_gate_hold() can be called with the topology lock held.) PR: 238814 MFC after: 2 weeks (cherry picked from commit b37b2543a23b44c78f6d78823dcfcedba46570db) sys/geom/gate/g_gate.c | 3 --- 1 file changed, 3 deletions(-) |
These g_geom->dumpconf implementations are dropping and reacquiring the topology lock: - g_gate_dumpconf() - g_raid_dumpconf() - g_raid3_dumpconf() I think that is unsafe in two ways. First, as soon as the topology lock is dropped, the state may be invalidated. In particular the softc may be destroyed. Second, the dumpconf procedures are called during topology iteration (g_conf{txt,dot,xml}), and dropping the lock may also invalidate those iterations. I suspect that none of the above actually need to drop the topology lock, but I haven't read each carefully yet. For raid and raid3, they seem to be doing it for lock order with the softc lock, but I wonder if they don't need the softc lock at all. And for gate, the lock order is already compatible (topology before the g_gate_units_lock, although I think perhaps it doesn't even need the hold/release). Here's an example crash with debug logging turned up: sysctl kern.geom.raid3.debug=4 sysctl debug.fail_point.mnowait=1%return while true; do kyua test -k /usr/tests/sys/geom/class/raid3/Kyuafile; done db> show msgbuf msgbufp = 0xfffff8023fffffb8 magic = 63062, size = 98232, r= 1388339, w = 1390017, ptr = 0xfffff8023ffe8000, cksum= 7799430 GEOM_RAID3[2]: Access md9 r-1w-1e-1 = 0 GEOM_RAID3[2]: Metadata on md12 updated. GEOM_RAID3[2]: Consumer md12 destroyed. GEOM_RAID3[2]: Access md12 r-1w-1e-1 = 0 GEOM_RAID3[0]: Device graid3.Z4UWAd destroyed. GEOM_RAID3 [1]: Thread exiting. Fatal trap 9: general protection fault while in kernel mode cpuid = 7; apic id = 07 instruction pointer = 0x20:0xffffffff80ba7774 stack pointer = 0x28:0xfffffe00005708c0 frame pointer = 0x28:0xfffffe0000570960 code segment = base rx0, limit 0xfffff, type 0x1b = DPL 0, pres 1, long 1, def32 0, gran 1 processor eflags = interrupt enabled, resume, IOPL = 0 current process = 13 (g_event) trap number = 9 panic: general protection fault cpuid = 7 time = 1561493868 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00005705d0 vpanic() at vpanic+0x19d/frame 0xfffffe0000570620 panic() at panic+0x43/frame 0xfffffe0000570680 trap_fatal() at trap_fatal+0x39c/frame 0xfffffe00005706e0 trap() at trap+0x6c/frame 0xfffffe00005707f0 calltrap() at calltrap+0x8/frame 0xfffffe00005707f0 --- trap 0x9, rip = 0xffffffff80ba7774, rsp = 0xfffffe00005708c0, rbp = 0xfffffe0000570960 --- _sx_xlock_hard() at _sx_xlock_hard+0x274/frame 0xfffffe0000570960 _sx_xlock() at _sx_xlock+0xc1/frame 0xfffffe00005709a0 g_raid3_dumpconf() at g_raid3_dumpconf+0x11c/frame 0xfffffe00005709e0 g_conf_specific() at g_conf_specific+0x1aa/frame 0xfffffe0000570a30 g_run_events() at g_run_events+0x197/frame 0xfffffe0000570a70 fork_exit() at fork_exit+0x84/frame 0xfffffe0000570ab0 fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe0000570ab0 --- trap 0, rip = 0, rsp = 0, rbp = 0 --- KDB: enter: panic db> bt 100214 Tracing pid 2864 tid 100214 td 0xfffff80003c8e5a0 sched_switch() at sched_switch+0x713/frame 0xfffffe00512b7560 mi_switch() at mi_switch+0x174/frame 0xfffffe00512b7590 sleepq_switch() at sleepq_switch+0x110/frame 0xfffffe00512b75d0 sleepq_timedwait() at sleepq_timedwait+0x4f/frame 0xfffffe00512b7610 _sleep() at _sleep+0x279/frame 0xfffffe00512b76b0 g_waitfor_event() at g_waitfor_event+0xe3/frame 0xfffffe00512b7740 sysctl_kern_geom_confxml() at sysctl_kern_geom_confxml+0x39/frame 0xfffffe00512b7770 sysctl_root_handler_locked() at sysctl_root_handler_locked+0x7b/frame 0xfffffe00512b77b0 sysctl_root() at sysctl_root+0x20c/frame 0xfffffe00512b7830 userland_sysctl() at userland_sysctl+0x17b/frame 0xfffffe00512b78e0 sys___sysctl() at sys___sysctl+0x5f/frame 0xfffffe00512b7990 amd64_syscall() at amd64_syscall+0x276/frame 0xfffffe00512b7ab0 fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe00512b7ab0 --- syscall (202, FreeBSD ELF64, sys___sysctl), rip = 0x800442b7a, rsp = 0x7fffffffb088, rbp = 0x7fffffffb0c0 --- db> x/s version version: FreeBSD 13.0-CURRENT #40 r349025+3bdd0fc24f5b(mnowait-dbg)-dirty: Mon Jun 24 08:36:20 PDT 2019\012 root@vali.kishkinda.net:/usr/obj/usr/src/freebsd/amd64.amd64/sys/GENERIC\012