From: Mark Kettenis Subject: Re: Missing unlock in uvmfault_promote() To: Martin Pieuchot Cc: tech@openbsd.org, stu@spacehopper.org Date: Fri, 03 Jan 2025 15:05:50 +0100 > Date: Fri, 3 Jan 2025 11:46:18 +0100 > From: Martin Pieuchot > > On 01/01/25(Wed) 13:10, Mark Kettenis wrote: > > > Date: Wed, 1 Jan 2025 11:47:51 +0100 > > > From: Martin Pieuchot > > > > > > Stuart reported a panic "locking against myself" in the lower fault > > > handler which is almost certainly related to a missing unlock in the > > > recently introduced uvmfault_promote(). > > > > > > Diff below fixes it, ok? > > > > Not sure this is correct. When uvmfault_promote() is called from > > uvm_fault_upper(), are we sure that the oanon->an_page doesn't have an > > object associated to it? Because if that's the case, you'll now try > > to unlock an object that isn't locked... > > Indeed, thanks for asking. > > Contrarily to NetBSD, OpenBSD's UVM doesn't include "loan" pages used to > implement zero-copy mechanisms. To my understanding this is the only case > where a physical page can be part of an anon and linked to an underlying > uobject. Do you share this view? Sorry, but my understanding of uvm (and especially the NetBSD code) isn't good enough to have a view on that. But you may very well be right. > > The NetBSD version of this has some additional setup stuff with > > additional assertions. That code does suggest that oanon->an_page > > should not have an object associated with it (the "anon COW" case). > > If you think that should be the case for us as well, I think it would > > be a good idea to also add... > > Indeed, updated diff below with that check added. I also included the > removal of non-trivial left-overs from loans. I'd commit them separately. > > ok? ok kettenis@ for the updated bits with the check added. But I'd hold off with cleaning up the loan left-overs and get those tested separately. Let's see if someone manages to hit the added check first... > Index: uvm/uvm_fault.c > =================================================================== > RCS file: /cvs/src/sys/uvm/uvm_fault.c,v > diff -u -p -r1.158 uvm_fault.c > --- uvm/uvm_fault.c 27 Dec 2024 12:04:40 -0000 1.158 > +++ uvm/uvm_fault.c 1 Jan 2025 15:31:38 -0000 > @@ -321,16 +321,9 @@ uvmfault_anonget(struct uvm_faultinfo *u > * The last unlock must be an atomic unlock and wait > * on the owner of page. > */ > - if (pg->uobject) { > - /* Owner of page is UVM object. */ > - uvmfault_unlockall(ufi, amap, NULL); > - uvm_pagewait(pg, pg->uobject->vmobjlock, > - "anonget1"); > - } else { > - /* Owner of page is anon. */ > - uvmfault_unlockall(ufi, NULL, NULL); > - uvm_pagewait(pg, anon->an_lock, "anonget2"); > - } > + KASSERT(pg->uobject == NULL); > + uvmfault_unlockall(ufi, NULL, NULL); > + uvm_pagewait(pg, anon->an_lock, "anonget"); > } else { > /* > * No page, therefore allocate one. > @@ -501,9 +494,15 @@ uvmfault_promote(struct uvm_faultinfo *u > struct vm_page **npg) > { > struct vm_amap *amap = ufi->entry->aref.ar_amap; > + struct uvm_object *uobj = NULL; > struct vm_anon *anon; > struct vm_page *pg = NULL; > > + if (uobjpage != PGO_DONTCARE) > + uobj = uobjpage->uobject; > + > + KASSERT(uobj == NULL || rw_lock_held(uobj->vmobjlock)); > + > anon = uvm_analloc(); > if (anon) { > anon->an_lock = amap->am_lock; > @@ -513,7 +512,7 @@ uvmfault_promote(struct uvm_faultinfo *u > > /* check for out of RAM */ > if (anon == NULL || pg == NULL) { > - uvmfault_unlockall(ufi, amap, NULL); > + uvmfault_unlockall(ufi, amap, uobj); > if (anon == NULL) > counters_inc(uvmexp_counters, flt_noanon); > else { > @@ -998,8 +997,6 @@ uvm_fault_upper(struct uvm_faultinfo *uf > * if it fails (!OK) it will unlock everything for us. > * if it succeeds, locks are still valid and locked. > * also, if it is OK, then the anon's page is on the queues. > - * if the page is on loan from a uvm_object, then anonget will > - * lock that object for us if it does not fail. > */ > error = uvmfault_anonget(ufi, amap, anon); > switch (error) { > > >