Re: sys/uvm: unwind failed amap copies

> Date: Sat, 08 Aug 2026 19:08:56 +0200
> From: Kirill A. Korinsky <[email protected]>
> 
> On Sat, 08 Aug 2026 15:55:04 +0200,
> Mark Kettenis <[email protected]> wrote:
> > 
> > > Date: Mon, 03 Aug 2026 01:23:08 +0200
> > > From: Kirill A. Korinsky <[email protected]>
> > 
> > Hi Kirill,
> > 
> > > 
> > > tech@,
> > > 
> > > I continue my hunt on hangs and crashes at my very small octeon.
> > > 
> > > Here the second finding from that a few days long sprint.
> > > 
> > > A PR_NOWAIT chunk allocation in amap_copy() may fail after earlier
> > > chunks already contain copied anons; amap_free() requires an empty amap,
> > > so direct cleanup trips its diagnostic assertion or leaves copied anon
> > > references orphaned which may end who knows how.
> > > 
> > > Here, I use amap_wipeout() instead to reverse the partial copy, release
> > > the shared lock, and free the temporary amap.
> > > 
> > > Thought?
> > > 
> > > Index: sys/uvm/uvm_amap.c
> > > ===================================================================
> > > RCS file: /home/cvs/src/sys/uvm/uvm_amap.c,v
> > > diff -u -p -r1.99 uvm_amap.c
> > > --- sys/uvm/uvm_amap.c	18 Jun 2026 13:14:26 -0000	1.99
> > > +++ sys/uvm/uvm_amap.c	2 Aug 2026 23:08:35 -0000
> > > @@ -640,10 +640,8 @@ amap_copy(struct vm_map *map, struct vm_
> > >  
> > >  		chunk = amap_chunk_get(amap, lcv, 1, PR_NOWAIT);
> > >  		if (chunk == NULL) {
> > > -			amap_unlock(srcamap);
> > > -			/* Destroy the new amap. */
> > > -			amap->am_ref--;
> > > -			amap_free(amap);
> > > +			amap->am_ref = 0;
> > 
> > Why do you set am_ref to zero here instead of just decreasing it?
> > 
> 
> Well, we shouldn't be here with am_ref not 1 as far as I understand the
> code, and that am_ref-- was a bit missleading.
> 
> But after think more, I see that it is bad idea to replace it to 0.
> 
> We shouldn't be here with am_ref non 1 never means that we actually won't be
> here, and if we here with am_ref not 1, am_ref-- makes am_ref not 0 and it
> will crash inside amap_wipeout() at KASSERT(amap->am_ref == 0).
> 
> So, here cleaner diff.
> 
> Ok?

Right!  ok kettenis@


> Index: sys/uvm/uvm_amap.c
> ===================================================================
> RCS file: /home/cvs/src/sys/uvm/uvm_amap.c,v
> diff -u -p -r1.99 uvm_amap.c
> --- sys/uvm/uvm_amap.c	18 Jun 2026 13:14:26 -0000	1.99
> +++ sys/uvm/uvm_amap.c	8 Aug 2026 17:05:19 -0000
> @@ -640,10 +640,9 @@ amap_copy(struct vm_map *map, struct vm_
>  
>  		chunk = amap_chunk_get(amap, lcv, 1, PR_NOWAIT);
>  		if (chunk == NULL) {
> -			amap_unlock(srcamap);
> -			/* Destroy the new amap. */
> +			/* amap_wipeout() releases the shared lock. */
>  			amap->am_ref--;
> -			amap_free(amap);
> +			amap_wipeout(amap);
>  			return;
>  		}
>  
> 
> 
> -- 
> wbr, Kirill
>