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.

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