diff options
author | Martin Pieuchot <mpi@cvs.openbsd.org> | 2023-04-25 12:36:31 +0000 |
---|---|---|
committer | Martin Pieuchot <mpi@cvs.openbsd.org> | 2023-04-25 12:36:31 +0000 |
commit | eba21f3d8d841dbd7d6a710260d947da9c161550 (patch) | |
tree | a76d2e31f92885408d5d66c6ff1f5ba875d2b241 /sys/uvm/uvm_map.c | |
parent | 3b62579e949755a87ca795e4b45821d5b4975a74 (diff) |
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@
Diffstat (limited to 'sys/uvm/uvm_map.c')
-rw-r--r-- | sys/uvm/uvm_map.c | 49 |
1 files changed, 23 insertions, 26 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) |