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

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.

>
> --
>
> v1 -> v2:
> - handle error when vmi_shrink_vma() failed (reported by Sashiko)
> - prevents mremap() with being shrunk for file-backed one like munmap()
> - consider i_mmap updates on shrink/expand by calling newly decoupled
>   functions, add_vma_to_mapping()/remove_vma_from_mapping()
>
> v1: https://lore.kernel.org/linux-mm/[email protected]/
> ---
>  mm/nommu.c | 147 ++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 124 insertions(+), 23 deletions(-)
>
> 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

> +	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).

>  }
>
>  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);
>  }
>
>  /*
> @@ -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.

> +
> +	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.

>  	}
>
> @@ -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.

> +	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.

> +
> +		/* 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 :)

> +			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?

>  }
>
> --
> 2.43.0
>

--
Cheers, Lorenzo