It only makes sense to manipulate VMA fields if a new VMA was allocated,
rather than merged.

VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
keeps its own, which is also what the legacy f_op->mmap path does since it
never touches an existing VMA.

Currently, these fields will get overwritten by whatever state is
established in the mmap_prepare hook, and if the VMA was merged,
vm_ops->mapped will not have been called, so this could destructively clear
existing state without replacing it with anything valid.

There is an implicit requirement that vm_private_data and vm_ops are
fungible across VMAs which means that losing the 'new' state is
fine.

However in this case the 'old' state is being overwritten by potentially
invalid 'new' state, so this must be rectified.

Additionally constify have_mmap_prepare while here.

All existing in-tree users either derive state for the tree or are
unmergeable due to VMA flags, so this has no direct impact.

Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
Cc: stable@vger.kernel.org
Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
---
Note that this is cc: stable to account for any possible back-ports that could
break it (unlikely) or out-of-tree modules which might be affected.
---
 mm/vma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vma.c b/mm/vma.c
index 9f0a0acf694a..6cde67883fb0 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -2849,7 +2849,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma = NULL;
-	bool have_mmap_prepare = file && file->f_op->mmap_prepare;
+	const bool have_mmap_prepare = file && file->f_op->mmap_prepare;
 	VMA_ITERATOR(vmi, mm, addr);
 	const pgoff_t anon_pgoff = addr >> PAGE_SHIFT;
 	MMAP_STATE(map, mm, &vmi, addr, len, pgoff, anon_pgoff, vma_flags, file);
@@ -2892,7 +2892,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 		allocated_new = true;
 	}
 
-	if (have_mmap_prepare)
+	if (have_mmap_prepare && allocated_new)
 		set_vma_user_defined_fields(vma, &map);
 
 	__mmap_complete(&map, vma);

---
base-commit: fe2ec83746e501645709761605c2464a44fd2929
change-id: 20260923-fix-mmap-prepare-overwrite-6304d112a4c7

Best regards,
-- 
Lorenzo Stoakes (ARM) <ljs@kernel.org>

On Wed, Sep 23, 2026 at 06:45:41PM +0100, Lorenzo Stoakes (ARM) wrote:
> It only makes sense to manipulate VMA fields if a new VMA was allocated,
> rather than merged.
> 
> VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
> keeps its own, which is also what the legacy f_op->mmap path does since it
> never touches an existing VMA.
> 
> Currently, these fields will get overwritten by whatever state is
> established in the mmap_prepare hook, and if the VMA was merged,
> vm_ops->mapped will not have been called, so this could destructively clear
> existing state without replacing it with anything valid.
> 
> There is an implicit requirement that vm_private_data and vm_ops are
> fungible across VMAs which means that losing the 'new' state is
> fine.

Hmm, can you explain how this is safe? I was wondering if _any_ kind of
mismatch should be a WARN_ON (sounds like something odd is happening if
they don't match). But if the new state is practically discardable, that
doesn't make sense.

-- 
Pedro

On Thu, Sep 24, 2026 at 09:57:03AM +0100, Pedro Falcato wrote:
> On Wed, Sep 23, 2026 at 06:45:41PM +0100, Lorenzo Stoakes (ARM) wrote:
> > It only makes sense to manipulate VMA fields if a new VMA was allocated,
> > rather than merged.
> >
> > VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
> > keeps its own, which is also what the legacy f_op->mmap path does since it
> > never touches an existing VMA.
> >
> > Currently, these fields will get overwritten by whatever state is
> > established in the mmap_prepare hook, and if the VMA was merged,
> > vm_ops->mapped will not have been called, so this could destructively clear
> > existing state without replacing it with anything valid.
> >
> > There is an implicit requirement that vm_private_data and vm_ops are
> > fungible across VMAs which means that losing the 'new' state is
> > fine.
>
> Hmm, can you explain how this is safe? I was wondering if _any_ kind of
> mismatch should be a WARN_ON (sounds like something odd is happening if
> they don't match). But if the new state is practically discardable, that
> doesn't make sense.

I mean this is asking me to defend how the kernel has worked forever :) don't
make me responsible for that...

For the VAST majority of cases, vm_ops is static. So they WILL be identical.

shmem does something really stupid with that which it shouldn't do (vary the
vm_ops on whether the file is unlinked or not), and maybe that needs a follow
up.

But anyway, one thing that saves us from a lot of this crap is that the majority
of things that store meaningful state there are non-mergeable,
i.e. VMA_PFNMAP_BIT, et al.

Of those that remain, the state is very often simply forwarding file state,
like:

	vma->vm_private_data = file->private_data;

At which point you already have equality.

Even if shared file state things might be tied to VMAs, but that should always
come out in the wash.

If you, for instance, stored the number of mappings there as a reference count (I
think fairly typical) then you automatically get the correct number because
vm_ops->close() will be called on a fully merged VMA (but not a partially merged
one which is correct).

Typical usage for legacy hook is mmap -> set refcount 1, open refcount++
(e.g. on split), close refcount--.

The mmap_prepare equivalent for that is vm_ops->mapped set refcount 1 (you
mustn't set state like that in mmap_prepare), and open/close as usual.

Another thing here is the idea that vm_private_data is ephemeral state _tied to
the VMA_.

It is already the case that that state will not be updated on expand/shrink of a
VMA, and so it is the case that if the VMA is unmapped for any reason the state
disappears.

So this ephemeral quality extends, by convention, to not impacting merge.

Anyway, I sent that 40 patch series (sorry for the size) in large part to
_strengthen_ assumptions about mmap_prepare (and, via legacy, mmap) hook
behaviour.

In general part of the mmap_prepare project is to allow us to get rid of
vagueries of the past and be able to make strong assumptions about behaviour.

So in the same vein, I'll do an audit of the codebase to make sure nothing
relies on this.

It might be interesting as well to audit what this data is actually set
to.

Maybe it makes sense to enforce that vm_private_data is always set to file
private dataa?

But that then implies that the VMA private data is unnecessary.

I do think requiring equivalent vm_ops might be a good thing to change, and
then to at least document the fact that vm_private_data is ephemeral like
this on merge.

Let me investigate how they're used and I can figure out how to improve
things.

>
> --
> Pedro

--
Cheers, Lorenzo

On 9/23/26 19:45, Lorenzo Stoakes (ARM) wrote:
> It only makes sense to manipulate VMA fields if a new VMA was allocated,
> rather than merged.
> 
> VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
> keeps its own, which is also what the legacy f_op->mmap path does since it
> never touches an existing VMA.
> 
> Currently, these fields will get overwritten by whatever state is
> established in the mmap_prepare hook, and if the VMA was merged,
> vm_ops->mapped will not have been called, so this could destructively clear
> existing state without replacing it with anything valid.
> 
> There is an implicit requirement that vm_private_data and vm_ops are
> fungible across VMAs which means that losing the 'new' state is
> fine.
> 
> However in this case the 'old' state is being overwritten by potentially
> invalid 'new' state, so this must be rectified.
> 
> Additionally constify have_mmap_prepare while here.
> 
> All existing in-tree users either derive state for the tree or are
> unmergeable due to VMA flags, so this has no direct impact.
> 
> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
> Note that this is cc: stable to account for any possible back-ports that could
> break it (unlikely)

Does it mean that patches are on the way to mainline that will break it, but
it's unlikely they will be backported? Or there are no such patches yet?
Just curious... if it's the first case then with the amount of random stuff
that goes to stable these days, I'd rather assume they could be backported
at some point :)

or out-of-tree modules which might be affected.

That is never a concern, and even suggesting it can bring hch's wrath ;)

Anyway,

Acked-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>


> ---
>  mm/vma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/vma.c b/mm/vma.c
> index 9f0a0acf694a..6cde67883fb0 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -2849,7 +2849,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
>  {
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma = NULL;
> -	bool have_mmap_prepare = file && file->f_op->mmap_prepare;
> +	const bool have_mmap_prepare = file && file->f_op->mmap_prepare;
>  	VMA_ITERATOR(vmi, mm, addr);
>  	const pgoff_t anon_pgoff = addr >> PAGE_SHIFT;
>  	MMAP_STATE(map, mm, &vmi, addr, len, pgoff, anon_pgoff, vma_flags, file);
> @@ -2892,7 +2892,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
>  		allocated_new = true;
>  	}
>  
> -	if (have_mmap_prepare)
> +	if (have_mmap_prepare && allocated_new)
>  		set_vma_user_defined_fields(vma, &map);
>  
>  	__mmap_complete(&map, vma);
> 
> ---
> base-commit: fe2ec83746e501645709761605c2464a44fd2929
> change-id: 20260923-fix-mmap-prepare-overwrite-6304d112a4c7
> 
> Best regards,

On Thu, Sep 24, 2026 at 09:19:26AM +0200, Vlastimil Babka (SUSE) wrote:
> On 9/23/26 19:45, Lorenzo Stoakes (ARM) wrote:
> > It only makes sense to manipulate VMA fields if a new VMA was allocated,
> > rather than merged.
> >
> > VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
> > keeps its own, which is also what the legacy f_op->mmap path does since it
> > never touches an existing VMA.
> >
> > Currently, these fields will get overwritten by whatever state is
> > established in the mmap_prepare hook, and if the VMA was merged,
> > vm_ops->mapped will not have been called, so this could destructively clear
> > existing state without replacing it with anything valid.
> >
> > There is an implicit requirement that vm_private_data and vm_ops are
> > fungible across VMAs which means that losing the 'new' state is
> > fine.
> >
> > However in this case the 'old' state is being overwritten by potentially
> > invalid 'new' state, so this must be rectified.
> >
> > Additionally constify have_mmap_prepare while here.
> >
> > All existing in-tree users either derive state for the tree or are
> > unmergeable due to VMA flags, so this has no direct impact.
> >
> > Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> > ---
> > Note that this is cc: stable to account for any possible back-ports that could
> > break it (unlikely)
>
> Does it mean that patches are on the way to mainline that will break it, but
> it's unlikely they will be backported? Or there are no such patches yet?

Nope it's highly unlikely. You'd have to introduce a brand new mmap_prepare
etc. etc.

> Just curious... if it's the first case then with the amount of random stuff
> that goes to stable these days, I'd rather assume they could be backported
> at some point :)

Suren insisted on it being a fix and I didn't really want to argue.

I thought perhaps it hit something real but when writing the patch I asked
the LLM to actually check and it seems not, which is exactly what I thought
initially and why this wasn't a fix.

Anyway it's a small change so I think it's fine for stable.

>
> or out-of-tree modules which might be affected.
>
> That is never a concern, and even suggesting it can bring hch's wrath ;)

Yeah that's what I assumed.

>
> Anyway,
>
> Acked-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>

Thanks

>
>
> > ---
> >  mm/vma.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 9f0a0acf694a..6cde67883fb0 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -2849,7 +2849,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
> >  {
> >  	struct mm_struct *mm = current->mm;
> >  	struct vm_area_struct *vma = NULL;
> > -	bool have_mmap_prepare = file && file->f_op->mmap_prepare;
> > +	const bool have_mmap_prepare = file && file->f_op->mmap_prepare;
> >  	VMA_ITERATOR(vmi, mm, addr);
> >  	const pgoff_t anon_pgoff = addr >> PAGE_SHIFT;
> >  	MMAP_STATE(map, mm, &vmi, addr, len, pgoff, anon_pgoff, vma_flags, file);
> > @@ -2892,7 +2892,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
> >  		allocated_new = true;
> >  	}
> >
> > -	if (have_mmap_prepare)
> > +	if (have_mmap_prepare && allocated_new)
> >  		set_vma_user_defined_fields(vma, &map);
> >
> >  	__mmap_complete(&map, vma);
> >
> > ---
> > base-commit: fe2ec83746e501645709761605c2464a44fd2929
> > change-id: 20260923-fix-mmap-prepare-overwrite-6304d112a4c7
> >
> > Best regards,
>

--
Cheers, Lorenzo

On Wed, Sep 23, 2026 at 06:45:41PM +0100, Lorenzo Stoakes (ARM) wrote:
> It only makes sense to manipulate VMA fields if a new VMA was allocated,
> rather than merged.
> 
> VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
> keeps its own, which is also what the legacy f_op->mmap path does since it
> never touches an existing VMA.
> 
> Currently, these fields will get overwritten by whatever state is
> established in the mmap_prepare hook, and if the VMA was merged,
> vm_ops->mapped will not have been called, so this could destructively clear
> existing state without replacing it with anything valid.
> 
> There is an implicit requirement that vm_private_data and vm_ops are
> fungible across VMAs which means that losing the 'new' state is
> fine.
> 
> However in this case the 'old' state is being overwritten by potentially
> invalid 'new' state, so this must be rectified.
> 
> Additionally constify have_mmap_prepare while here.
> 
> All existing in-tree users either derive state for the tree or are
> unmergeable due to VMA flags, so this has no direct impact.
> 
> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

Reviewed-by: Gregory Price (Meta) <gourry@gourry.net>

On Wed, Sep 23, 2026 at 06:45:41PM +0100, Lorenzo Stoakes (ARM) wrote:
> It only makes sense to manipulate VMA fields if a new VMA was allocated,
> rather than merged.
>
> VMA merging does not compare vm_ops or vm_private_data, so a merged VMA
> keeps its own, which is also what the legacy f_op->mmap path does since it
> never touches an existing VMA.
>
> Currently, these fields will get overwritten by whatever state is
> established in the mmap_prepare hook, and if the VMA was merged,
> vm_ops->mapped will not have been called, so this could destructively clear
> existing state without replacing it with anything valid.
>
> There is an implicit requirement that vm_private_data and vm_ops are
> fungible across VMAs which means that losing the 'new' state is
> fine.
>
> However in this case the 'old' state is being overwritten by potentially
> invalid 'new' state, so this must be rectified.
>
> Additionally constify have_mmap_prepare while here.
>
> All existing in-tree users either derive state for the tree or are
> unmergeable due to VMA flags, so this has no direct impact.

Instantly noticed a typo when I hit send (ugh).

'derive state for the tree' should be 'derive state from the file'. It's late :)

>
> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
> Note that this is cc: stable to account for any possible back-ports that could
> break it (unlikely) or out-of-tree modules which might be affected.
> ---
>  mm/vma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 9f0a0acf694a..6cde67883fb0 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -2849,7 +2849,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
>  {
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma = NULL;
> -	bool have_mmap_prepare = file && file->f_op->mmap_prepare;
> +	const bool have_mmap_prepare = file && file->f_op->mmap_prepare;
>  	VMA_ITERATOR(vmi, mm, addr);
>  	const pgoff_t anon_pgoff = addr >> PAGE_SHIFT;
>  	MMAP_STATE(map, mm, &vmi, addr, len, pgoff, anon_pgoff, vma_flags, file);
> @@ -2892,7 +2892,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
>  		allocated_new = true;
>  	}
>
> -	if (have_mmap_prepare)
> +	if (have_mmap_prepare && allocated_new)
>  		set_vma_user_defined_fields(vma, &map);
>
>  	__mmap_complete(&map, vma);
>
> ---
> base-commit: fe2ec83746e501645709761605c2464a44fd2929
> change-id: 20260923-fix-mmap-prepare-overwrite-6304d112a4c7
>
> Best regards,
> --
> Lorenzo Stoakes (ARM) <ljs@kernel.org>
>

--
Cheers, Lorenzo