On Fri, Feb 04, 2022 at 01:37:41PM -0300, Martin Pieuchot wrote: > On 04/02/22(Fri) 03:39, Klemens Nanni wrote: > > [...] > > ... 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. > > This should be fine since this function should only be called when the > last reference of a map is dropped. In other words the locking here is > necessary to satisfy the assertions.
Correct. OK for this diff? This survived regress and package bulk builds on amd64 together with the assertions diff as well as the syscalls unlocking diff. > I wonder if lock shouldn't be taken & released around uvm_map_teardown() > which makes it easier to see that this is called after the last refcount > decrement. You mean keeping the caller's kernel lock by no longer unlocking inside of uvm_map_teardown()? You can do that, but I don't think it makes much of a difference. > > > 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); > > >
