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@

diff --git a/sys/uvm/uvm_map.c b/sys/uvm/uvm_map.c
index d73835e..794095b 100644 (file)
--- 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 76e52a2..e1bf919 100644 (file)
--- 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. */