From a4ddbf29eacf742b5ff96de79b141ca4b19db49b Mon Sep 17 00:00:00 2001 From: mpi Date: Tue, 25 Apr 2023 12:36:30 +0000 Subject: [PATCH] Do not grab the `vmmaplk' recursively, prevent a self-deadlock. Change the semantic of vm_map_busy() to be able to completely unlock the `vmmaplk' instead of downgrading it to a read lock in mlock(2). This is necessary because uvm_fault_wire() tries to re-grab the same lock. We now keep track of the thread currently holding the vmmap busy to ensure it can relock & unbusy the vmmap. The new pattern becomes: ....vm_map_lock(map); ....vm_map_busy(map); /* prevent other threads to grab an exclusive lock */ ....vm_map_unlock(map); .... ..../* .... * Do some stuff generally requiring a tsleep(9). .... */ .... ....vm_map_lock(map); ....vm_map_unbusy(map); /* allow other threads to make progress after unlock */ ....vm_map_unlock(map); Fix adapted from NetBSD's r1.249 of uvm/uvm_map.c. Issue reported by Jacqueline Jolicoeur exposed by a "wallet refresh" of the Monero App. Panic hand-copied below: sleep_finish() rw_enter() uvmfault_lookup() uvm_fault_check() uvm_fault() uvm_fault_wire() uvm_map_pageable_wire() sys_mlock() ok kettenis@ --- sys/uvm/uvm_map.c | 49 ++++++++++++++++++++++------------------------- sys/uvm/uvm_map.h | 3 ++- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/sys/uvm/uvm_map.c b/sys/uvm/uvm_map.c index d73835e0067..794095b96e3 100644 --- a/sys/uvm/uvm_map.c +++ b/sys/uvm/uvm_map.c @@ -1,4 +1,4 @@ -/* $OpenBSD: uvm_map.c,v 1.315 2023/04/13 15:23:23 miod Exp $ */ +/* $OpenBSD: uvm_map.c,v 1.316 2023/04/25 12:36:30 mpi Exp $ */ /* $NetBSD: uvm_map.c,v 1.86 2000/11/27 08:40:03 chs Exp $ */ /* @@ -2144,15 +2144,8 @@ uvm_map_pageable_wire(struct vm_map *map, struct vm_map_entry *first, * to be created. then we clip each map entry to the region to * be wired and increment its wiring count. * - * 2: we downgrade to a read lock, and call uvm_fault_wire to fault + * 2: we mark the map busy, unlock it and call uvm_fault_wire to fault * in the pages for any newly wired area (wired_count == 1). - * - * downgrading to a read lock for uvm_fault_wire avoids a possible - * deadlock with another thread that may have faulted on one of - * the pages to be wired (it would mark the page busy, blocking - * us, then in turn block on the map lock that we hold). - * because we keep the read lock on the map, the copy-on-write - * status of the entries we modify here cannot change. */ for (iter = first; iter != end; iter = RBT_NEXT(uvm_map_addr, iter)) { @@ -2185,7 +2178,7 @@ uvm_map_pageable_wire(struct vm_map *map, struct vm_map_entry *first, timestamp_save = map->timestamp; #endif vm_map_busy(map); - vm_map_downgrade(map); + vm_map_unlock(map); error = 0; for (iter = first; error == 0 && iter != end; @@ -2198,14 +2191,10 @@ uvm_map_pageable_wire(struct vm_map *map, struct vm_map_entry *first, iter->protection); } + vm_map_lock(map); + vm_map_unbusy(map); + if (error) { - /* - * uvm_fault_wire failure - * - * Reacquire lock and undo our work. - */ - vm_map_upgrade(map); - vm_map_unbusy(map); #ifdef DIAGNOSTIC if (timestamp_save != map->timestamp) panic("uvm_map_pageable_wire: stale map"); @@ -2244,13 +2233,10 @@ uvm_map_pageable_wire(struct vm_map *map, struct vm_map_entry *first, return error; } - /* We are currently holding a read lock. */ + if ((lockflags & UVM_LK_EXIT) == 0) { - vm_map_unbusy(map); - vm_map_unlock_read(map); + vm_map_unlock(map); } else { - vm_map_upgrade(map); - vm_map_unbusy(map); #ifdef DIAGNOSTIC if (timestamp_save != map->timestamp) panic("uvm_map_pageable_wire: stale map"); @@ -2495,6 +2481,7 @@ uvm_map_setup(struct vm_map *map, pmap_t pmap, vaddr_t min, vaddr_t max, map->s_start = map->s_end = 0; /* Empty stack area by default. */ map->flags = flags; map->timestamp = 0; + map->busy = NULL; if (flags & VM_MAP_ISVMSPACE) rw_init_flags(&map->lock, "vmmaplk", RWL_DUPOK); else @@ -5313,7 +5300,7 @@ vm_map_lock_try_ln(struct vm_map *map, char *file, int line) rv = mtx_enter_try(&map->mtx); } else { mtx_enter(&map->flags_lock); - if (map->flags & VM_MAP_BUSY) { + if ((map->flags & VM_MAP_BUSY) && (map->busy != curproc)) { mtx_leave(&map->flags_lock); return (FALSE); } @@ -5322,7 +5309,8 @@ vm_map_lock_try_ln(struct vm_map *map, char *file, int line) /* check if the lock is busy and back out if we won the race */ if (rv) { mtx_enter(&map->flags_lock); - if (map->flags & VM_MAP_BUSY) { + if ((map->flags & VM_MAP_BUSY) && + (map->busy != curproc)) { rw_exit(&map->lock); rv = FALSE; } @@ -5347,7 +5335,8 @@ vm_map_lock_ln(struct vm_map *map, char *file, int line) do { mtx_enter(&map->flags_lock); tryagain: - while (map->flags & VM_MAP_BUSY) { + while ((map->flags & VM_MAP_BUSY) && + (map->busy != curproc)) { map->flags |= VM_MAP_WANTLOCK; msleep_nsec(&map->flags, &map->flags_lock, PVM, vmmapbsy, INFSLP); @@ -5356,7 +5345,7 @@ tryagain: } while (rw_enter(&map->lock, RW_WRITE|RW_SLEEPFAIL) != 0); /* check if the lock is busy and back out if we won the race */ mtx_enter(&map->flags_lock); - if (map->flags & VM_MAP_BUSY) { + if ((map->flags & VM_MAP_BUSY) && (map->busy != curproc)) { rw_exit(&map->lock); goto tryagain; } @@ -5386,6 +5375,7 @@ vm_map_lock_read_ln(struct vm_map *map, char *file, int line) void vm_map_unlock_ln(struct vm_map *map, char *file, int line) { + KASSERT(map->busy == NULL || map->busy == curproc); uvm_tree_sanity(map, file, line); uvm_tree_size_chk(map, file, line); LPRINTF(("map unlock: %p (at %s %d)\n", map, file, line)); @@ -5438,7 +5428,11 @@ void vm_map_busy_ln(struct vm_map *map, char *file, int line) { KASSERT((map->flags & VM_MAP_INTRSAFE) == 0); + KASSERT(rw_write_held(&map->lock)); + KASSERT(map->busy == NULL); + mtx_enter(&map->flags_lock); + map->busy = curproc; map->flags |= VM_MAP_BUSY; mtx_leave(&map->flags_lock); } @@ -5449,8 +5443,11 @@ vm_map_unbusy_ln(struct vm_map *map, char *file, int line) int oflags; KASSERT((map->flags & VM_MAP_INTRSAFE) == 0); + KASSERT(map->busy == curproc); + mtx_enter(&map->flags_lock); oflags = map->flags; + map->busy = NULL; map->flags &= ~(VM_MAP_BUSY|VM_MAP_WANTLOCK); mtx_leave(&map->flags_lock); if (oflags & VM_MAP_WANTLOCK) diff --git a/sys/uvm/uvm_map.h b/sys/uvm/uvm_map.h index 76e52a2e7a0..e1bf919ae62 100644 --- a/sys/uvm/uvm_map.h +++ b/sys/uvm/uvm_map.h @@ -1,4 +1,4 @@ -/* $OpenBSD: uvm_map.h,v 1.83 2023/01/31 15:18:55 deraadt Exp $ */ +/* $OpenBSD: uvm_map.h,v 1.84 2023/04/25 12:36:30 mpi Exp $ */ /* $NetBSD: uvm_map.h,v 1.24 2001/02/18 21:19:08 chs Exp $ */ /* @@ -274,6 +274,7 @@ struct vm_map { int ref_count; /* [a] Reference count */ int flags; /* flags */ unsigned int timestamp; /* Version number */ + struct proc *busy; /* [v] thread holding map busy*/ vaddr_t min_offset; /* [I] First address in map. */ vaddr_t max_offset; /* [I] Last address in map. */ -- 2.20.1