On Wed, Sep 23, 2026 at 8:47 AM Lorenzo Stoakes (ARM) <[email protected]> wrote:
>
> On Wed, Sep 23, 2026 at 08:21:58AM -0700, Suren Baghdasaryan wrote:
> > On Thu, Sep 17, 2026 at 9:23 AM Lorenzo Stoakes (ARM) <[email protected]>
> > wrote:
> > >
> > > The map->file_doesnt_need_get flag is confusing and the existing
> > > implementation has holes.
> > >
> > > Drivers are permitted to change the owning file of a mapping. If they do
> > > so, they are required to take a reference on that file.
> > >
> > > The mmap() operation which ultimately invokes __mmap_region() is
> > > guaranteed
> > > to drop the refcount for the original file the mapping was made under, but
> > > this is not true for the replaced file.
> > >
> > > This has been addressed so far by tracking map->file_doesnt_need_get,
> > > which
> > > is rather poorly named and unfortunately fails to correctly track whether
> > > or not an additional put were needed in a number of cases.
> > >
> > > Make life easier by removing this flag, and instead drop the reference for
> > > both mmap_prepare and the deprecated mmap callback in a new function
> > > put_map().
> > >
> > > Track whether this needs to be done by aligning mmap_state with
> > > vm_area_desc and store the original file in the map->file field, keeping
> > > the updated file in map->vm_file.
> > >
> > > In order to have the same behaviour for both types of hooks, only drop the
> > > reference __mmap_new_file_vma() itself took in its error path, deferring
> > > the replaced file's reference to put_map().
> > >
> > > To make this work correctly, map->vm_file has to be updated before any
> > > error handling, so update __mmap_new_file_vma() and call_mmap_prepare() to
> > > set this field first.
> > >
> > > Also when mmap_prepare() changes the file and is then merged, the
> > > reference
> > > count also must be decremented, so update the logic to call put_map() in
> > > this case too.
> > >
> > > Also update __compat_vma_mmap() to manually perform this step for stacked
> > > file systems using the compatibility layer, and update
> > > compat_set_vma_from_desc() to replace vma_set_file() with a correct
> > > refcount/file update.
> > >
> > > No in-tree driver is impacted by the incorrect implementation of this
> > > currently (no driver that does this is mergeable for one), so this does
> > > not
> > > need to be a fix.
> > >
> > > Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
> > > ---
> > > mm/internal.h | 1 +
> > > mm/util.c | 5 +++-
> > > mm/vma.c | 83
> > > +++++++++++++++++++++++++++++++++++------------------------
> > > mm/vma.h | 6 +++--
> > > 4 files changed, 59 insertions(+), 36 deletions(-)
> > >
> > > 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. */
> > > + struct file *vm_file; /* May be updated by mmap_prepare. */
> >
> > Overall I like the change but I think you could avoid extra churn by
> > keeping the name of `file` as is and add `struct file *orig_file`
> > instead as:
> >
> > + struct file *orig_file; /* mmap()-specified original file. */
> > - struct file *file;
> > + struct file *file; /* May be updated by mmap_prepare. */
> >
> > Then many uses of map->file would stay unchanged.
>
> I didn't actually want to do the churny version of this but the problem
> with doing something else is that would then contradicts what's in
> vm_area_desc:
>
> struct vm_area_desc {
> /* Immutable state. */
> ...
> struct file *file; /* May vary from vm_file in stacked callers. */
> ...
> /* Mutable fields. Populated with initial state. */
> ...
> struct file *vm_file;
> ...
> };
>
> And suddenly what is 'file' there (inherited file) is the modifiable one
> here and it's more confusing.
>
> As per the commit msg:
>
> Track whether this needs to be done by aligning mmap_state with
> vm_area_desc and store the original file in the map->file field,
> keeping
> the updated file in map->vm_file.
>
> But maybe I need to make that decision clearer there?
Ah, I see. The description is clear, I was just too focused on the
churn, I guess.
The reasoning to rename makes sense to me.
>
> >
> > > pgprot_t page_prot;
> > >
> > > /* User-defined fields, perhaps updated by .mmap_prepare(). */
> > > @@ -43,8 +44,6 @@ struct mmap_state {
> > >
> > > /* Determine if we can check KSM flags early in mmap() logic. */
> > > bool check_ksm_early :1;
> > > - /* If .mmap_prepare changed the file, we don't need to pin. */
> > > - bool file_doesnt_need_get :1;
> > > };
> > >
> > > #define MMAP_STATE(name, mm_, vmi_, addr_, len_, pgoff_, anon_pgoff_,
> > > vma_flags_, file_) \
> > > @@ -58,6 +57,7 @@ struct mmap_state {
> > > .pglen = PHYS_PFN(len_), \
> > > .vma_flags = vma_flags_, \
> > > .file = file_, \
> > > + .vm_file = file_, \
> > > .page_prot = vma_flags_to_page_prot(vma_flags_), \
> > > }
> > >
> > > @@ -70,7 +70,7 @@ struct mmap_state {
> > > .vma_flags = (map_)->vma_flags, \
> > > .pgoff = (map_)->pgoff, \
> > > .anon_pgoff = (map_)->anon_pgoff, \
> > > - .file = (map_)->file, \
> > > + .file = (map_)->vm_file, \
> > > .prev = (map_)->prev, \
> > > .middle = vma_, \
> > > .next = (vma_) ? NULL : (map_)->next, \
> > > @@ -2447,7 +2447,7 @@ void mm_drop_all_locks(struct mm_struct *mm)
> > > */
> > > static bool accountable_mapping(struct mmap_state *map)
> > > {
> > > - const struct file *file = map->file;
> > > + const struct file *file = map->vm_file;
> > >
> > > /*
> > > * hugetlb has its own accounting separate from the core VM
> > > @@ -2496,7 +2496,7 @@ static void vms_abort_munmap_vmas(struct
> > > vma_munmap_struct *vms,
> > >
> > > static void update_ksm_flags(struct mmap_state *map)
> > > {
> > > - map->vma_flags = ksm_vma_flags(map->mm, map->file,
> > > map->vma_flags);
> > > + map->vma_flags = ksm_vma_flags(map->mm, map->vm_file,
> > > map->vma_flags);
> > > }
> > >
> > > static void set_desc_from_map(struct vm_area_desc *desc,
> > > @@ -2506,7 +2506,7 @@ static void set_desc_from_map(struct vm_area_desc
> > > *desc,
> > > desc->end = map->end;
> > >
> > > desc->pgoff = map->pgoff;
> > > - desc->vm_file = map->file;
> > > + desc->vm_file = map->vm_file;
> > > desc->vma_flags = map->vma_flags;
> > > desc->page_prot = map->page_prot;
> > > }
> > > @@ -2586,6 +2586,10 @@ static int __mmap_setup(struct mmap_state *map,
> > > struct vm_area_desc *desc,
> > > return 0;
> > > }
> > >
> > > +static bool map_same_file(struct mmap_state *map)
> > > +{
> > > + return map->vm_file == map->file;
> > > +}
> > >
> > > 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;
> > > +
> > > if (error) {
> > > UNMAP_STATE(unmap, vmi, vma, vma->vm_start, vma->vm_end,
> > > map->prev, map->next);
> > > - fput(vma->vm_file);
> > > - vma->vm_file = NULL;
> > > + if (map_same_file(map))
> > > + fput(map->vm_file);
> > >
> > > + vma->vm_file = NULL;
> > > vma_iter_set(vmi, vma->vm_end);
> > > /* Undo any partial mapping done by a device driver. */
> > > unmap_region(&unmap);
> > > @@ -2623,7 +2630,6 @@ static int __mmap_new_file_vma(struct mmap_state
> > > *map,
> > > !vma_flags_test(&map->vma_flags,
> > > VMA_MAYWRITE_BIT) &&
> > > vma_test(vma, VMA_MAYWRITE_BIT));
> > >
> > > - map->file = vma->vm_file;
> > > map->vma_flags = vma->flags;
> > >
> > > return 0;
> > > @@ -2631,7 +2637,7 @@ static int __mmap_new_file_vma(struct mmap_state
> > > *map,
> > >
> > > static void map_set_anon(struct mmap_state *map)
> > > {
> > > - map->file = NULL;
> > > + map->vm_file = NULL;
> > > map->vm_ops = NULL;
> > > map->pgoff = map->addr >> PAGE_SHIFT;
> > > }
> > > @@ -2643,7 +2649,7 @@ static bool map_is_private(const struct mmap_state
> > > *map)
> > >
> > > static bool map_is_anon(const struct mmap_state *map)
> > > {
> > > - return map_is_private(map) && !map->file;
> > > + return map_is_private(map) && !map->vm_file;
> > > }
> > >
> > > /*
> > > @@ -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. */
> > > if (!desc->vm_ops)
> > > return -EINVAL;
> > >
> > > @@ -2811,10 +2821,6 @@ static int call_mmap_prepare(struct mmap_state
> > > *map,
> > >
> > > /* Update fields permitted to be changed. */
> > > map->pgoff = desc->pgoff;
> > > - if (desc->vm_file != map->file) {
> > > - map->file_doesnt_need_get = true;
> > > - map->file = desc->vm_file;
> > > - }
> > > map->vma_flags = desc->vma_flags;
> > > map->page_prot = desc->page_prot;
> > > /* User-defined fields. */
> > > @@ -2826,7 +2832,7 @@ static int call_mmap_prepare(struct mmap_state *map,
> > > * anonymous mappings. Rather than allowing these mappings to be
> > > odd
> > > * outliers, simply make them truly anonymous.
> > > */
> > > - if (map_is_private(map) && file_is_dev_zero(map->file))
> > > + if (map_is_private(map) && file_is_dev_zero(map->vm_file))
> > > map_set_anon(map);
> > >
> > > return 0;
> > > @@ -2845,7 +2851,7 @@ static void set_vma_user_defined_fields(struct
> > > vm_area_struct *vma,
> > > */
> > > static bool can_set_ksm_flags_early(struct mmap_state *map)
> > > {
> > > - struct file *file = map->file;
> > > + struct file *file = map->vm_file;
> > >
> > > /* Anonymous mappings have no driver which can change them. */
> > > if (!file)
> > > @@ -2868,6 +2874,20 @@ static bool can_set_ksm_flags_early(struct
> > > mmap_state *map)
> > > return false;
> > > }
> > >
> > > +static void put_map(struct mmap_state *map)
> >
> > nit: maybe put_map_file() to be more specific?
>
> Sure will change.
>
> >
> > > +{
> > > + /*
> > > + * An error occurred or the VMA was merged.
> >
> > It's a bit weird that the function explains when it is being used.
> > Having an appropriate comment at the call site seems better to me.
>
> Ack will change.
>
> >
> > > + *
> > > + * If the file was changed by the driver (which is required to
> > > increment
> > > + * the replacement file's reference count), drop its reference
> > > count.
> > > + *
> > > + * On error, the caller always drops the original file regardless.
> > > + */
> > > + if (map->vm_file && !map_same_file(map))
> > > + fput(map->vm_file);
> > > +}
> > > +
> > > static unsigned long __mmap_region(struct file *file, unsigned long addr,
> > > unsigned long len, vma_flags_t vma_flags,
> > > unsigned long pgoff, struct list_head *uf)
> > > @@ -2922,7 +2942,10 @@ static unsigned long __mmap_region(struct file
> > > *file, unsigned long addr,
> > >
> > > __mmap_complete(&map, vma);
> > >
> > > - if (have_mmap_prepare && allocated_new) {
> > > + if (!allocated_new) {
> > > + /* Merged, so need to drop refcount. */
> > > + put_map(&map);
> > > + } else if (have_mmap_prepare) {
> > > error = mmap_action_complete(vma, &desc.action,
> > > /*is_compat=*/false);
> > > if (error)
> > > @@ -2936,13 +2959,7 @@ static unsigned long __mmap_region(struct file
> > > *file, unsigned long addr,
> > > if (map.charged)
> > > vm_unacct_memory(map.charged);
> > > abort_munmap:
> > > - /*
> > > - * This indicates that .mmap_prepare has set a new file,
> > > differing from
> > > - * desc->vm_file. But since we're aborting the operation, only the
> > > - * original file will be cleaned up. Ensure we clean up both.
> > > - */
> > > - if (map.file_doesnt_need_get)
> > > - fput(map.file);
> > > + put_map(&map);
> > > vms_abort_munmap_vmas(&map.vms, &map.mas_detach);
> > > return error;
> > > }
> > > 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;
> > > + }
> > > vma->flags = desc->vma_flags;
> > > vma->vm_page_prot = desc->page_prot;
> > >
> > >
> > > --
> > > 2.55.0
> > >
>
> --
> Cheers, Lorenzo
