On Thu, Feb 03, 2022 at 04:49:26PM +0000, Klemens Nanni wrote:
> On Mon, Jan 31, 2022 at 11:11:25AM -0300, Martin Pieuchot wrote:
> > On 31/01/22(Mon) 10:24, Klemens Nanni wrote:
> > > Running with my uvm assert diff showed that uvm_fault_unwire_locked()
> > > was called without any locks held.
> > >
> > > This happened when I rebooted my machine:
> > >
> > > uvm_fault_unwire_locked()
> > > uvm_unmap_kill_entry_withlock()
> > > uvm_unmap_kill_entry()
> > > uvm_map_teardown()
> > > uvmspace_free()
> > >
> > > This code does not corrupt anything because
> > > uvm_unmap_kill_entry_withlock() is grabbing the kernel lock aorund its
> > > uvm_fault_unwire_locked() call.
> > >
> > > But regardless of the kernel lock dances in this code path, the uvm map
> > > ought to be protected by its own lock. uvm_fault_unwire() does that.
> > >
> > > uvm_fault_unwire_locked()'s comment says the map must at least be read
> > > locked, which is what all other code paths to that function do.
> > >
> > > This makes my latest assert diff happy in the reboot case (it did not
> > > always hit that assert).
> >
> > I'm happy your asserts found a first bug.
> >
> > I"m afraid calling this function below could result in a grabbing the
> > lock twice. Can we be sure this doesn't happen?
>
> I see two call paths leading to uvm_unmap_kill_entry():
>
> 1. uvm_unmap_remove(): map must already be locked by the caller
> 2. uvmspace_free() -> uvm_map_teardown(): the vmspace is refcounted and
> the kernel lock must be held.
>
> I don't see how we can reach uvm_unmap_kill_entry() with another thread
> holding a lock to the vm_map backing the address space, for example.
> > uvm_unmap_kill_entry() is called in many different contexts and this is
> > currently a mess. I don't know what NetBSD did in this area but it is
> > worth looking at and see if there isn't a good idea to untangle this.
>
> Yes, reasoning about this with confidence is hard and I'm still
> hestitant to do so, but all my use/test cases so far failed to hit
> further lock assertions with uvm_unmap_kill_entry_withlock() locking the
> map, while running it unlocked would reliably panic.
... with the lock grabbed in uvm_map_teardown() that is, otherwise
the first call path can lock against itself (regress/misc/posixtestsuite
is a reproduce for this):
vm_map_lock_read_ln+0x38
uvm_fault_unwire+0x58
uvm_unmap_kill_entry_withlock+0x68
uvm_unmap_remove+0x2d4
sys_munmap+0x11c
which is obvious in hindsight.
So grabbing the lock in uvm_map_teardown() instead avoids that while
still ensuring a locked map in the path missing a lock.
> I'm still going through NetBSD's changes in this regard...
Index: uvm_map.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c 21 Dec 2021 22:21:32 -0000 1.282
+++ uvm_map.c 4 Feb 2022 02:51:00 -0000
@@ -2734,6 +2751,7 @@ uvm_map_teardown(struct vm_map *map)
KERNEL_ASSERT_UNLOCKED();
KASSERT((map->flags & VM_MAP_INTRSAFE) == 0);
+ vm_map_lock(map);
/* Remove address selectors. */
uvm_addr_destroy(map->uaddr_exe);
