Re: [RFC PATCH 1/6] mm: nommu: fix do_mremap() to correctly update internal states

On Fri, 14 Aug 2026 20:52:05 +0900,
Lorenzo Stoakes (ARM) wrote:
> 
> On Thu, Aug 13, 2026 at 03:33:56PM +0900, Hajime Tazaki wrote:
> > When shrinking a VMA via mremap, the bounds are modified directly:
> > mm/nommu.c:do_mremap() {
> >     ...
> >     vma->vm_end = vma->vm_start + new_len;
> >     ...
> > }
> > This shrinks the VMA without updating its bounds in the maple tree.
> > If the maple tree (mm->mm_mt) still contains the old bounds, a user
> > process could access the freed portion. The stale maple tree would
> > incorrectly return the shrunk VMA for an address past its new vm_end.
> >
> > This commit fixes this issue by calling vmi_shrink_vma() when shrink
> > happens.  Additionally, if a file-backed, non-anonymous map is to be
> > shrunk, it reports -EINVAL like do_munmap() does.
> >
> > Moreover, to maintain i_mmap interval tree, two functions,
> > add_vma_to_mapping() and remove_vma_from_mapping(), are decoupled from
> > setup_vma_to_mm() and cleanup_vma_from_mm() respectively.
> >
> > Cc: Andrew Morton <[email protected]>
> > Cc: "Liam R. Howlett" <[email protected]>
> > Cc: Lorenzo Stoakes <[email protected]>
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Jann Horn <[email protected]>
> > Cc: Pedro Falcato <[email protected]>
> > Cc: [email protected]
> > Closes: https://sashiko.dev/#/patchset/[email protected]
> > Closes: https://sashiko.dev/#/patchset/20260710021028.892645-1-thehajime%40gmail.com
> > Signed-off-by: Hajime Tazaki <[email protected]>
> 
> Fies: 8220543df148 ("nommu: remove uses of VMA linked list")?
> Cc: stable?
> 
> But if you're going to do that, you really need to make this as small as
> possible and maybe separate out everything but what is required to fix the bug.

ah, I missed this point when preparing the patches.
Yes, I would look more carefully to past commits, clean up things, and
prepare a meaningful set of patches which can be backported without hustle.

> > diff --git a/mm/nommu.c b/mm/nommu.c
> > index ed3934bc2de4..89444ee2aca6 100644
> > --- a/mm/nommu.c
> > +++ b/mm/nommu.c
> > @@ -559,36 +559,51 @@ static void put_nommu_region(struct vm_region *region)
> >  	__put_nommu_region(region);
> >  }
> >
> > +static void add_vma_to_mapping(struct vm_area_struct *vma)
> > +{
> > +	struct address_space *mapping;
> > +
> > +	if (!vma->vm_file)
> > +		return;
> > +
> > +	mapping = vma->vm_file->f_mapping;
> > +	i_mmap_lock_write(mapping);
> > +	flush_dcache_mmap_lock(mapping);
> > +	vma_interval_tree_insert(vma, &mapping->i_mmap);
> > +	flush_dcache_mmap_unlock(mapping);
> > +	i_mmap_unlock_write(mapping);
> > +}
> > +
> > +static void remove_vma_from_mapping(struct vm_area_struct *vma)
> > +{
> > +	struct address_space *mapping;
> > +
> > +	if (!vma->vm_file)
> > +		return;
> > +
> > +	mapping = vma->vm_file->f_mapping;
> > +	i_mmap_lock_write(mapping);
> > +	flush_dcache_mmap_lock(mapping);
> > +	vma_interval_tree_remove(vma, &mapping->i_mmap);
> 
> This function doesn't exist in mm-unstable, it's now mapping_rmap_tree_remove().
> 
> Please always base mm changes on
> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/?h=mm-unstable

I understand, will base this branch from next time.

> > +	flush_dcache_mmap_unlock(mapping);
> > +	i_mmap_unlock_write(mapping);
> > +}
> > +
> >  static void setup_vma_to_mm(struct vm_area_struct *vma, struct mm_struct *mm)
> >  {
> >  	vma->vm_mm = mm;
> >
> >  	/* add the VMA to the mapping */
> > -	if (vma->vm_file) {
> > -		struct address_space *mapping = vma->vm_file->f_mapping;
> > -
> > -		i_mmap_lock_write(mapping);
> > -		flush_dcache_mmap_lock(mapping);
> > -		vma_interval_tree_insert(vma, &mapping->i_mmap);
> > -		flush_dcache_mmap_unlock(mapping);
> > -		i_mmap_unlock_write(mapping);
> > -	}
> > +	if (vma->vm_file)
> > +		add_vma_to_mapping(vma);
> 
> add_vma_to_mapping() also has a guard against vma->vm_file, either remove this
> one or that one (this one seems better to remove).

yes, I agree.

> >  }
> >
> >  static void cleanup_vma_from_mm(struct vm_area_struct *vma)
> >  {
> >  	vma->vm_mm->map_count--;
> >  	/* remove the VMA from the mapping */
> > -	if (vma->vm_file) {
> > -		struct address_space *mapping;
> > -		mapping = vma->vm_file->f_mapping;
> > -
> > -		i_mmap_lock_write(mapping);
> > -		flush_dcache_mmap_lock(mapping);
> > -		vma_interval_tree_remove(vma, &mapping->i_mmap);
> > -		flush_dcache_mmap_unlock(mapping);
> > -		i_mmap_unlock_write(mapping);
> > -	}
> > +	if (vma->vm_file)
> > +		remove_vma_from_mapping(vma);
> >  }

ditto; this might be too.

> >  /*
> > @@ -1351,6 +1366,8 @@ static int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  	if (new->vm_ops && new->vm_ops->open)
> >  		new->vm_ops->open(new);
> >
> > +	remove_vma_from_mapping(vma);
> > +
> >  	down_write(&nommu_region_sem);
> >  	delete_nommu_region(vma->vm_region);
> >  	if (new_below) {
> > @@ -1364,6 +1381,11 @@ static int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  	add_nommu_region(new->vm_region);
> >  	up_write(&nommu_region_sem);
> >
> > +	if (new->vm_file) {
> > +		vma->vm_file = get_file(vma->vm_file);
> > +		new->vm_file = get_file(new->vm_file);
> > +	}
> > +
> >  	setup_vma_to_mm(vma, mm);
> >  	setup_vma_to_mm(new, mm);
> >  	vma_iter_store_new(vmi, new);
> > @@ -1386,16 +1408,20 @@ static int vmi_shrink_vma(struct vma_iterator *vmi,
> >  		      unsigned long from, unsigned long to)
> >  {
> >  	struct vm_region *region;
> > +	bool has_mapping = !!vma->vm_file;
> 
> Better to use const for this kind of thing.

thanks, I'll fix this.

> > +
> > +	if (has_mapping)
> > +		remove_vma_from_mapping(vma);
> >
> >  	/* adjust the VMA's pointers, which may reposition it in the MM's tree
> >  	 * and list */
> >  	if (from > vma->vm_start) {
> >  		if (vma_iter_clear_gfp(vmi, from, vma->vm_end, GFP_KERNEL))
> > -			return -ENOMEM;
> > +			goto restore_mapping;
> >  		vma->vm_end = from;
> >  	} else {
> >  		if (vma_iter_clear_gfp(vmi, vma->vm_start, to, GFP_KERNEL))
> > -			return -ENOMEM;
> > +			goto restore_mapping;
> >  		vma->vm_start = to;
> 
> you're not updating vma/region->vm_pgoff here, that's incorrect.

if this (vmi_shrink_vma()) is called with split_vma(), it looks like
the offset was updated before coming here, but it it's not, yes, looks
like the value remains same.

I'll look into detail.

> >  	}
> >
> > @@ -1415,7 +1441,15 @@ static int vmi_shrink_vma(struct vma_iterator *vmi,
> >  	up_write(&nommu_region_sem);
> >
> >  	free_page_series(from, to);
> > +	if (has_mapping)
> > +		add_vma_to_mapping(vma);
> > +
> >  	return 0;
> > +
> > +restore_mapping:
> > +	if (has_mapping)
> > +		add_vma_to_mapping(vma);
> > +	return -ENOMEM;
> >  }
> >
> >  /*
> > @@ -1544,6 +1578,9 @@ static unsigned long do_mremap(unsigned long addr,
> >  			unsigned long flags, unsigned long new_addr)
> >  {
> >  	struct vm_area_struct *vma;
> > +	int ret;
> > +
> > +	VMA_ITERATOR(vmi, current->mm, addr);
> >
> >  	/* insanity checks first */
> >  	old_len = PAGE_ALIGN(old_len);
> > @@ -1567,11 +1604,75 @@ static unsigned long do_mremap(unsigned long addr,
> >  	if (is_nommu_shared_mapping(vma->vm_flags))
> >  		return (unsigned long) -EPERM;
> >
> > -	if (new_len > vma->vm_region->vm_end - vma->vm_region->vm_start)
> > +	/* vm_region->vm_top != vm_region->vm_end when sysctl_nr_trim_pages is 0 (default: 1) */
> > +	if (new_len > vma->vm_region->vm_top - vma->vm_region->vm_start)
> >  		return (unsigned long) -ENOMEM;
> >
> >  	/* all checks complete - do it */
> > -	vma->vm_end = vma->vm_start + new_len;
> > +	if (new_len == old_len)
> > +		return vma->vm_start;
> > +
> > +	/* shrink only happens addr + new_len and old_len are in different pages */
> 
> Unclear really I don't think you really need an explanation like that I'd drop
> the comment altogether.

I agree.

> > +	if (new_len < old_len) {
> > +		/* like do_munmap(), we're allowed to shrink an anonymous VMA but not
> > +		 * a file-backed one
> > +		 */
> > +		if (vma->vm_file)
> > +			return (unsigned long) -EINVAL;
> 
> You break MAP_PRIVATE-/dev/zero here but then unbreak it in the next commit,
> this is a bisection hazard.
> 
> I'd just leave this check out until you bring in the /dev/zero stuff.

yes, the order, and the combination of chunks to patches are both
broken at this series.  I will reconsider the series to avoid such
issues.

> > +
> > +		/* vmi_shrink_vma() needs from/to pointers to be removed,
> > +		 * (mainly used in munmap) so, specify them.
> > +		 */
> > +		ret = vmi_shrink_vma(&vmi, vma, addr + new_len, addr + old_len);
> > +		if (ret < 0)
> > +			return (unsigned long) ret;
> > +	} else {
> > +		/* growth path: grow up to vm_top should be handled here. */
> > +		unsigned long old_end = vma->vm_end;
> > +		unsigned long end = vma->vm_start + new_len;
> > +		unsigned long grow_len = end - old_end;
> > +
> > +		/*
> > +		 * Initialize the newly exposed portion before making it visible
> > +		 * through the VMA or i_mmap.
> > +		 */
> > +
> > +		/* read contents of extended map from file, or zero-filled if !vm_file */
> > +		if (vma->vm_file) {
> > +			loff_t fpos;
> > +
> > +			fpos = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> > +			fpos += old_end - vma->vm_start;
> > +
> > +			ret = nommu_read_iter(vma->vm_file, (void *)old_end,
> > +					      grow_len, &fpos);
> 
> Umm, this function doesn't exist at the point of this patch so this breaks the
> compile :)

this is also same issue as mentioned previous one.  will also address
this part.

> > +			if (ret < 0)
> > +				return (unsigned long)ret;
> > +
> > +			if (ret < grow_len)
> > +				memset((char *)old_end + ret, 0, grow_len - ret);
> > +		} else {
> > +			memset((void *)old_end, 0, grow_len);
> > +		}
> > +
> > +		/* The backing contents are ready. Now update the VMA bookkeeping. */
> > +		remove_vma_from_mapping(vma);
> > +
> > +		vma->vm_end = end;
> > +		ret = vma_iter_store_gfp(&vmi, vma, GFP_KERNEL);
> > +		if (ret) {
> > +			vma->vm_end = old_end;
> > +			add_vma_to_mapping(vma);
> > +			return (unsigned long)ret;
> > +		}
> > +
> > +		/* vm_top remains unchanged; only the logical end grows. */
> > +		down_write(&nommu_region_sem);
> > +		vma->vm_region->vm_end = end;
> > +		up_write(&nommu_region_sem);
> > +
> > +		add_vma_to_mapping(vma);
> > +	}
> >  	return vma->vm_start;
> 
> In general you're making this function very long. Can you split it out please?

I understand.
I would split do_mremap() into 1) params tests, 2) shrink path, and 3)
growth path.

-- Hajime