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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Sep 07, 2026 at 06:56:51PM +0200, David Hildenbrand (Arm) wrote:
> > 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.

A 'shared anon' mapping is not anon at all, and that's handled early in
do_mmap().

I wish that we didn't confuse people by allowing MAP_SHARED | MAP_ANON as a
shorthand but there we are.

So I don't like to make that distinction on the basis that a 'shared
anon' mapping isn't something that exists :)

And then imagine vma_is_private_anonymous() vs. vma_is_anonymous() etc. It would
get silly, quick...

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

Not really, it's there to catch drivers doing something silly/broken (likely by
mistake).

I want to catch that early. I have a 36 patch series that extends this kind of
idea... a lot :)

>
> Should it be
>
> "vm_ops are strictly required with mmap_prepare"
>
> or sth like that?

Well that's confusing though, because desc->vm_ops defaults to &dummy_vma_ops,
and we absolutely do not require drivers to set vm_ops at all.

And as far as the driver is concerned maybe it's NULL? They maybe don't realise
:)

So the idea is to say don't allow them to try to do something they can't do.

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

I think I'd rather that be a follow up :) this series is about eliminiating the
one last (I hope?) corner case for anon VMAs.

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo