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 -0000 1.282
> > +++ uvm_map.c 31 Jan 2022 09:28:04 -0000
> > @@ -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();
> > }
> >
> >
>
