From: Theo Buehler <tb@theobuehler.org>
Subject: Re: uvm_fault_lower(): page leak in error path
To: tech@openbsd.org
Date: Mon, 2 Dec 2024 15:41:11 +0100

On Mon, Dec 02, 2024 at 01:49:06PM +0100, Martin Pieuchot wrote:
> When amap_add() has been changed to be able to fail in r1.90.  Since
> then a cleanup & wakeup are missing from the error path.
> 
> `pg' is allocated line 1326 and still has the PG_BUSY|PG_FAKE flags when
> amap_add() fails.  Then the anon is release in uvm_anfree() below and the
> page is marked as PG_RELEASED and leaked.
> 
> Since the page is allocated with the amap lock held I'm not sure any
> other thread can set the PG_WANTED flag.  So this might be revisited
> later, for the moment I keep the idiom used in the function.
> 
> ok?

One question: pg can be uobjpage and in this case this was already done
for it.  Should we not avoid doing it again?

> 
> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> diff -u -p -r1.148 uvm_fault.c
> --- uvm/uvm_fault.c	29 Nov 2024 06:44:57 -0000	1.148
> +++ uvm/uvm_fault.c	2 Dec 2024 12:16:01 -0000
> @@ -1420,6 +1416,12 @@ uvm_fault_lower(struct uvm_faultinfo *uf
>  
>  		if (amap_add(&ufi->entry->aref,
>  		    ufi->orig_rvaddr - ufi->entry->start, anon, 0)) {
> +			if (pg->pg_flags & PG_WANTED)
> +				wakeup(pg);
> +
> +			atomic_clearbits_int(&pg->pg_flags,
> +			    PG_BUSY|PG_FAKE|PG_WANTED);
> +			UVM_PAGE_OWN(pg, NULL);
>  			uvmfault_unlockall(ufi, amap, uobj);
>  			uvm_anfree(anon);
>  			counters_inc(uvmexp_counters, flt_noamap);
> 
>