From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: Missing unlock in uvmfault_promote()
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: tech@openbsd.org, stu@spacehopper.org
Date: Fri, 3 Jan 2025 11:46:18 +0100

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?

> 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?

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) {