On Fri, Oct 28, 2022 at 11:08:55AM +0200, Martin Pieuchot wrote: > On 20/10/22(Thu) 16:17, Martin Pieuchot wrote: > > On 11/09/22(Sun) 12:26, Martin Pieuchot wrote: > > > Diff below adds a minimalist set of assertions to ensure proper locks > > > are held in uvm_mapanon() and uvm_unmap_remove() which are the guts of > > > mmap(2) for anons and munmap(2). > > > > > > Please test it with WITNESS enabled and report back. > > > > New version of the diff that includes a lock/unlock dance in > > uvm_map_teardown(). While grabbing this lock should not be strictly > > necessary because no other reference to the map should exist when the > > reaper is holding it, it helps make progress with asserts. Grabbing > > the lock is easy and it can also save us a lot of time if there is any > > reference counting bugs (like we've discovered w/ vnode and swapping). > > Here's an updated version that adds a lock/unlock dance in > uvm_map_deallocate() to satisfy the assert in uvm_unmap_remove(). > Thanks to tb@ from pointing this out. > > I received many positive feedback and test reports, I'm now asking for > oks.
In principal, this is what I did a few months back, with less asserts and less "Must be called with map [un]locked" comment additions for functions that gained assertions. I went through the tree to manually verify that each function you added an assert to is called with[out] the map lock held: this looks good. GENERIC.MP+uvm (your diff) with mmap(2) as and munmap(2) unlocked has been working fine on my daily x230 driver with browsing, compiling, etc. i386 kernels build fine using GENERIC.MP+uvm and i386 regress runs fine using GENERIC.MP+uvm+WITNESS. sparc64 kernels build fine using GENERIC.MP+uvm and sparc64 regress runs fine using GENERIC.MP+uvm (WITNESS doesn't boot). Please keep an eye on syzkaller after this gets committed. Last time I committed something like this syzcaller was broken. This was reported off-list. OK kn
