Re: [PATCH v3 01/40] mm/vma: fix mmap_prepare file handling, remove file_doesnt_need_get

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

(forgive my possibly semi-broken efforts to trim the mail)

On Thu, Sep 24, 2026 at 03:00:44PM -0400, Liam R. Howlett wrote:
> On 26/09/17 05:22PM, Lorenzo Stoakes (ARM) wrote:

> > diff --git a/mm/internal.h b/mm/internal.h
> > index 0dca33db068f..fe576d468af4 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __MM_INTERNAL_H
> >  #define __MM_INTERNAL_H
> >
> > +#include <linux/file.h>
> >  #include <linux/fs.h>
> >  #include <linux/khugepaged.h>
> >  #include <linux/mm.h>
> > diff --git a/mm/util.c b/mm/util.c
> > index bf0513d1d3d0..016932780925 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -1228,8 +1228,11 @@ int __compat_vma_mmap(struct vm_area_desc *desc,
> >
> >  	/* Perform any preparatory tasks for mmap action. */
> >  	err = mmap_action_prepare(desc);
> > -	if (err)
> > +	if (err) {
> > +		if (desc->vm_file != vma->vm_file)
> > +			fput(desc->vm_file);
> >  		return err;
> > +	}
> >  	/* Update the VMA from the descriptor. */
> >  	compat_set_vma_from_desc(vma, desc);
> >  	/* Complete any specified mmap actions. */
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 55917d097933..fa784f069da4 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -24,7 +24,8 @@ struct mmap_state {
> >  		vm_flags_t vm_flags;
> >  		vma_flags_t vma_flags;
> >  	};
> > -	struct file *file;
> > +	struct file *file;	/* mmap()-specified file. */
>
> I guess renaming file to mmaped_file would be a lot more changes.

Yeah :) and want to keep things sync'd with vm_area_desc.

Can always obviously follow up later with renames sync'd across both.

<snip>

> >  static int __mmap_new_file_vma(struct mmap_state *map,
> >  			       struct vm_area_struct *vma)
> > @@ -2593,20 +2597,23 @@ static int __mmap_new_file_vma(struct mmap_state *map,
> >  	struct vma_iterator *vmi = map->vmi;
> >  	int error;
> >
> > -	vma->vm_file = map->file;
> > -	if (!map->file_doesnt_need_get)
> > -		get_file(map->file);
> > +	vma->vm_file = map->vm_file;
> > +	if (map_same_file(map))
> > +		get_file(map->vm_file);
> >
> > -	if (!map->file->f_op->mmap)
> > +	if (!map->vm_file->f_op->mmap)
> >  		return 0;
> >
> >  	error = mmap_file(vma->vm_file, vma);
> > +	map->vm_file = vma->vm_file;
> > +
>
> You set vma->vm_file to map->vm_file unconditionally above, is this
> necessary?

Yeah, because the mmap hook can change vma->vm_file, and this is necessary
for the correct file refcount accounting.

The accounting is actually very tricky, because the file that was passed
via mmap() is fput() after the operation is done but if its swapped then
you have to make sure everything works out correctly on both error and
success paths.

Which this patch does (with a lot of AI review checking to make sure it's
not broken! FWIW)

<snip>

> > @@ -2688,7 +2694,7 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
> >  	}
> >
> >  	/* Invoke callbacks. */
> > -	if (map->file)
> > +	if (map->vm_file)
> >  		error = __mmap_new_file_vma(map, vma);
> >  	else if (!is_anon)
> >  		error = shmem_zero_setup(vma);
> > @@ -2797,11 +2803,15 @@ static int call_mmap_prepare(struct mmap_state *map,
> >  	int err;
> >
> >  	/* Invoke the hook. */
> > -	err = vfs_mmap_prepare(map->file, desc);
> > +	err = vfs_mmap_prepare(map->vm_file, desc);
> >  	if (err)
> >  		return err;
> >
> > -	/* It's invalid for mmap_preprare hooks to clear vm_ops. */
> > +	/* Update first so file refcount tracked correctly. */
> > +	if (desc->vm_file != map->vm_file)
> > +		map->vm_file = desc->vm_file;
> > +
> > +	/* It's invalid for mmap_prepare hooks to clear vm_ops. */
>
> The less rare of prepare ;)

Haha 'It's a rare prepare that would dare' is what I'd LIKE to put here as a
comment but probably can't :P

<snip>

> > +static void put_map(struct mmap_state *map)
>
>
> I like the put_map_file() instead, like Suren suggested.. but maybe
> put_map_vm_file(), especially since it could be read as put to the file
> pointer instead of vm_file.

Ack, and of course to bikeshed it a bit :P maybe map_put_vm_file() so the
'put vm_file' bit is clearer?

<snip>

> > diff --git a/mm/vma.h b/mm/vma.h
> > index e97bd2dfa786..f15faa83f3d6 100644
> > --- a/mm/vma.h
> > +++ b/mm/vma.h
> > @@ -394,8 +394,10 @@ static inline void compat_set_vma_from_desc(struct vm_area_struct *vma,
> >
> >  	/* Mutable fields. Populated with initial state. */
> >  	vma_set_pgoff(vma, desc->pgoff);
> > -	if (desc->vm_file != vma->vm_file)
> > -		vma_set_file(vma, desc->vm_file);
> > +	if (desc->vm_file != vma->vm_file) {
> > +		fput(vma->vm_file);
> > +		vma->vm_file = desc->vm_file;
>
> This isn't going to be racy somehow, right?

No, at this point the VMA write lock is held and everything should be
pinned correctly.

The vma_set_file() dance was the problematic bit here as it didn't
handle the file refcount properly.

>
> > +	}
> >  	vma->flags = desc->vma_flags;
> >  	vma->vm_page_prot = desc->page_prot;
> >
> >
> > --
> > 2.55.0
> >

--
Cheers, Lorenzo