From: Theo Buehler <tb@theobuehler.org>
Subject: Re: uvm_fault_lower(): page leak in error path
To: tech@openbsd.org
Date: Tue, 3 Dec 2024 00:18:53 +0100

On Mon, Dec 02, 2024 at 05:32:06PM +0100, Martin Pieuchot wrote:
> On 02/12/24(Mon) 15:41, Theo Buehler wrote:
> > 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?
> 
> pg can only be uobjpage for the direct mapping path, the chunk below is
> in the promote path where pg is either 0 filled or a new copy of uobjpage.
> 
> So unless I missed something there's no possibility of double cleanup
> here.

I agree. Thanks for spelling it out.

ok tb

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