Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: Missing unlock in uvmfault_promote()
To:
Martin Pieuchot <mpi@grenadille.net>
Cc:
tech@openbsd.org, stu@spacehopper.org
Date:
Wed, 01 Jan 2025 13:10:31 +0100

Download raw body.

Thread
> 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...

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

> 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 10:43:40 -0000
> @@ -501,9 +501,13 @@ 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;

... a KASSERT(uobj == NULL || rwLock_held(uobj->lock)) here.

> +
>  	anon = uvm_analloc();
>  	if (anon) {
>  		anon->an_lock = amap->am_lock;
> @@ -513,7 +517,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 {
> 
> 
>