Re: [PATCH 3/6] mm/vma: only permit MAP_PRIVATE /dev/zero to be mapped anonymous

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/2/26 20:00, Lorenzo Stoakes (ARM) wrote:
> In order to use mmap_prepare() with MAP_PRIVATE mappings of /dev/zero
> without the success_hook hack we explicitly permitted mmap_prepare handlers
> to set NULL vm_ops.
> 
> However this is dangerous and we really only want to allow this for
> MAP_PRIVATE-mapped /dev/zero.
> 
> Therefore use the newly introduced file_is_dev_zero() to uniquely identify
> MAP_PRIVATE-/dev/zero mappings and only permit this behaviour for them.
> 
> Then, remove all ability for mmap_prepare or mmap hooks to set a VMA
> anonymous and update mmap_zero_prepare() to leave it to the core mmap code
> to do so.
> 
> Note that this disallows nested MAP_PRIVATE-mappings of /dev/zero
> regions. Doing this would be broken in any case.
> 
> We therefore do not need to update the mmap_prepare() compatibility layer
> to reflect these changes, as the mmap hook check suffices to disallow this
> behaviour.
> 
> Now we're setting vma->vm_ops to NULL for an mmap_prepare-initialised
> MAP_PRIVATE-/dev/zero mapping, we have to avoid a subtle issue when
> updating user-defined fields via set_vma_user_defined_fields().
> 
> The default for vma->vm_ops for all mmap_prepare-initialised mappings is
> vma_dummy_vm_ops, so map->vm_ops will be set to this and setting
> vma->vm_ops to this will render the VMA mistakenly non-anon.
> 
> In general, we should never be setting user-defined fields for an anonymous
> VMA, so explicitly check for this to avoid doing so for the one case where
> a mapping can be both mmap_prepare and anonymous.
> 
> In the case of legacy ->mmap hooks some drivers may set vma->vm_ops NULL
> believing this is the equivalent of setting no VMA operations. Therefore
> update mmap_file() to correct this by setting dummy VMA operations if this
> occurs.
> 
> An example of this is drm_gem_shmem_mmap() which deliberately clears
> vma->vm_ops before handing the VMA to dma-buf. Cases such as this will be
> updated when they are converted to mmap_prepare.
> 
> Also, in order to avoid a single commit bisection hazard, add a temporary
> workaround to set the VMA anonymous only after vma->vm_file is assigned in
> __mmap_new_file_vma().
> 
> This is because vma_set_range() calls vma_set_pgoff() and
> assert_sane_pgoff() in turn, prior to the vma->vm_file being assigned. If
> we set the VMA anonymous early then this assert will fail.
> 
> This is removed in the subsequent commit.
> 
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
>  mm/char-mem.c |  6 +-----
>  mm/internal.h | 17 ++++++++++-------
>  mm/vma.c      | 33 +++++++++++++++++++++++++--------
>  3 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/char-mem.c b/mm/char-mem.c
> index e53e89e6ddd8..c0b5fb019223 100644
> --- a/mm/char-mem.c
> +++ b/mm/char-mem.c
> @@ -508,11 +508,7 @@ static int mmap_zero_prepare(struct vm_area_desc *desc)
>  	if (vma_desc_test(desc, VMA_SHARED_BIT))
>  		return shmem_zero_setup_desc(desc);
>  
> -	/*
> -	 * This is a highly unique situation where we mark a MAP_PRIVATE mapping
> -	 * of /dev/zero anonymous, despite it not being.
> -	 */
> -	vma_desc_set_anonymous(desc);
> +	/* MAP_PRIVATE semantics are taken care of for us by core mm. */
>  	return 0;
>  }
>  
> diff --git a/mm/internal.h b/mm/internal.h
> index 5d474e5f7709..da14c56fb24e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -226,15 +226,18 @@ static inline int mmap_file(struct file *file, struct vm_area_struct *vma)
>  {
>  	int err = vfs_mmap(file, vma);
>  
> -	if (likely(!err))
> -		return 0;
> -
>  	/*
> -	 * OK, we tried to call the file hook for mmap(), but an error
> -	 * arose. The mapping is in an inconsistent state and we must not invoke
> -	 * any further hooks on it.
> +	 * Either we tried to call the file hook for mmap() and an error arose
> +	 * or a driver set vma->vm_ops = NULL intending there to be no VMA
> +	 * operations.
> +	 *
> +	 * In the former case the VMA is in an inconsistent state and we mustn't
> +	 * invoke any further hooks on it, in the latter case the hook actually
> +	 * wanted no further hooks to be invoked, so fix both by setting dummy
> +	 * VMA ops.
>  	 */
> -	vma->vm_ops = &vma_dummy_vm_ops;
> +	if (unlikely(err || !vma->vm_ops))
> +		vma->vm_ops = &vma_dummy_vm_ops;
>  
>  	return err;
>  }
> diff --git a/mm/vma.c b/mm/vma.c
> index 35e7a64855fa..4b8d430d9619 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -2621,6 +2621,19 @@ static int __mmap_new_file_vma(struct mmap_state *map,
>  	return 0;
>  }
>  
> +static bool map_is_private(const struct mmap_state *map)
> +{
> +	return !vma_flags_test(&map->vma_flags, VMA_SHARED_BIT);
> +}
> +
> +static bool map_is_anon(const struct mmap_state *map)

I was wondering whether we should call this "map_is_private_anon", due to
MAP_ANON|MAP_SHARED. But looking at __mmap_new_vma(), the existing "is_anon" is
also limited to MAP_ANON|MAP_PRIVATE.

> +{
> +	if (!map_is_private(map))
> +		return false;
> +
> +	return !map->file || file_is_dev_zero(map->file);
> +}
> +
>  /*
>   * __mmap_new_vma() - Allocate a new VMA for the region, as merging was not
>   * possible.
> @@ -2634,8 +2647,7 @@ static int __mmap_new_file_vma(struct mmap_state *map,
>  static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
>  	struct mmap_action *action)
>  {
> -	const bool is_anon = !map->file &&
> -		!vma_flags_test(&map->vma_flags, VMA_SHARED_BIT);
> +	const bool is_anon = map_is_anon(map);
>  	struct vma_iterator *vmi = map->vmi;
>  	int error = 0;
>  	struct vm_area_struct *vma;
> @@ -2651,7 +2663,7 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
>  
>  	vma_iter_config(vmi, map->addr, map->end);
>  
> -	if (is_anon)
> +	if (is_anon && !map->file)
>  		vma_set_anonymous(vma);
>  
>  	vma_set_range(vma, map->addr, map->end, map->pgoff, map->anon_pgoff);
> @@ -2669,6 +2681,10 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
>  	else if (!is_anon)
>  		error = shmem_zero_setup(vma);
>  
> +	/* Temporary MAP_PRIVATE-/dev/zero workaround. */
> +	if (is_anon && map->file)
> +		vma_set_anonymous(vma);
> +
>  	if (error)
>  		goto free_iter_vma;
>  
> @@ -2777,6 +2793,10 @@ static int call_mmap_prepare(struct mmap_state *map,
>  	if (err)
>  		return err;
>  
> +	/* Hooks cannot mark themselves anonymous. */

I guess this comment will be stale soon (after #4 where you drop the
set_anonymous part).

Should it be

"vm_ops are strictly required with mmap_prepare"

or sth like that?

> +	if (!desc->vm_ops)
> +		return -EINVAL;
> +
>  	err = call_action_prepare(map, desc);
>  	if (err)
>  		return err;
> @@ -2799,10 +2819,7 @@ static int call_mmap_prepare(struct mmap_state *map,
>  static void set_vma_user_defined_fields(struct vm_area_struct *vma,
>  		struct mmap_state *map)
>  {
> -	if (map->vm_ops)
> -		vma->vm_ops = map->vm_ops;
> -	else	/* Only /dev/zero should do this. */
> -		vma_set_anonymous(vma);
> +	vma->vm_ops = map->vm_ops;
>  	vma->vm_private_data = map->vm_private_data;
>  }
>  
> @@ -2882,7 +2899,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
>  		allocated_new = true;
>  	}
>  
> -	if (have_mmap_prepare)
> +	if (have_mmap_prepare && !map_is_anon(&map))
>  		set_vma_user_defined_fields(vma, &map);

Ah, we have mmap_zero_prepare() for handling the shmem_zero_setup_desc(). I was
just about to ask whether we can just get rid of this here.


But, hold on, do we now even need that? Could core-mm now take care of that as
well, and we could just remove mmap_zero_prepare() entirely?

That is, we'd make shmem_zero_setup() in __mmap_new_vma() take care of this?
Then we might not even need shmem_zero_setup_desc() anymore.

Maybe harder than it sounds at first.

-- 
Cheers,

David