Re: uvm_unmap_kill_entry(): unwire with map lock held
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 - 1.282 > > +++ uvm_map.c 4 Feb 2022 02:51:00 - > > @@ -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); > > >
Re: uvm_unmap_kill_entry(): unwire with map lock held
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. 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. > 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 - 1.282 > +++ uvm_map.c 4 Feb 2022 02:51:00 - > @@ -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); >
Re: uvm_unmap_kill_entry(): unwire with map lock held
On Thu, Feb 03, 2022 at 04:49:26PM +, 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 - 1.282 +++ uvm_map.c 4 Feb 2022 02:51:00 - @@ -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);
Re: uvm_unmap_kill_entry(): unwire with map lock held
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.
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 - 1.282
> > +++ uvm_map.c 31 Jan 2022 09:28:04 -
> > @@ -2132,7 +2143,7 @@ uvm_unmap_kill_entry_withlock(struct vm_
> > if (VM_MAPENT_ISWIRED(entry)) {
> > KERNEL_LOCK();
> > entry->wired_count = 0;
> > - uvm_fault_unwire_locked(map, entry->start, entry->end);
> > + uvm_fault_unwire(map, entry->start, entry->end);
> > KERNEL_UNLOCK();
> > }
> >
> >
>
Re: uvm_unmap_kill_entry(): unwire with map lock held
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?
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.
> 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 - 1.282
> +++ uvm_map.c 31 Jan 2022 09:28:04 -
> @@ -2132,7 +2143,7 @@ uvm_unmap_kill_entry_withlock(struct vm_
> if (VM_MAPENT_ISWIRED(entry)) {
> KERNEL_LOCK();
> entry->wired_count = 0;
> - uvm_fault_unwire_locked(map, entry->start, entry->end);
> + uvm_fault_unwire(map, entry->start, entry->end);
> KERNEL_UNLOCK();
> }
>
>
uvm_unmap_kill_entry(): unwire with map lock held
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).
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 - 1.282
+++ uvm_map.c 31 Jan 2022 09:28:04 -
@@ -2132,7 +2143,7 @@ uvm_unmap_kill_entry_withlock(struct vm_
if (VM_MAPENT_ISWIRED(entry)) {
KERNEL_LOCK();
entry->wired_count = 0;
- uvm_fault_unwire_locked(map, entry->start, entry->end);
+ uvm_fault_unwire(map, entry->start, entry->end);
KERNEL_UNLOCK();
}
