> Date: Fri, 3 Jan 2025 11:46:18 +0100
> From: Martin Pieuchot <mpi@grenadille.net>
> 
> On 01/01/25(Wed) 13:10, Mark Kettenis wrote:
> > > Date: Wed, 1 Jan 2025 11:47:51 +0100
> > > From: Martin Pieuchot <mpi@grenadille.net>
> > > 
> > > 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) {
> 
> 
>