On Sun, Mar 15, 2026 at 3:56 PM Suren Baghdasaryan <[email protected]> wrote:
>
> On Thu, Mar 12, 2026 at 1:27 PM Lorenzo Stoakes (Oracle) <[email protected]>
> wrote:
> >
> > Rather than passing arbitrary fields, pass an mmap_action field directly to
> > mmap prepare and complete helpers to put all the action-specific logic in
> > the function actually doing the work.
> >
> > Additionally, allow mmap prepare functions to return an error so we can
> > error out as soon as possible if there is something logically incorrect in
> > the input.
> >
> > Update remap_pfn_range_prepare() to properly check the input range for the
> > CoW case.
>
> By "properly check" do you mean the replacement of desc->start and
> desc->end with action->remap.start and action->remap.start +
> action->remap.size when calling get_remap_pgoff() from
> remap_pfn_range_prepare()?
>
> >
> > While we're here, make remap_pfn_range_prepare_vma() a little neater, and
> > pass mmap_action directly to call_action_complete().
> >
> > Then, update compat_vma_mmap() to perform its logic directly, as
> > __compat_vma_map() is not used by anything so we don't need to export it.
>
> Not directly related to this patch but while reviewing, I was also
> checking vma locking rules in this mmap_prepare() + mmap() sequence
> and I noticed that the new VMA flag modification functions like
> vma_set_flags_mask() do assert vma_assert_locked(vma). It would be
> useful to add these but as a separate change. I will add it to my todo
> list.
>
> >
> > Also update compat_vma_mmap() to use vfs_mmap_prepare() rather than calling
> > the mmap_prepare op directly.
> >
> > Finally, update the VMA userland tests to reflect the changes.
> >
> > Signed-off-by: Lorenzo Stoakes (Oracle) <[email protected]>
> > ---
> > include/linux/fs.h | 2 -
> > include/linux/mm.h | 8 +--
> > mm/internal.h | 28 +++++---
> > mm/memory.c | 45 +++++++-----
> > mm/util.c | 112 +++++++++++++-----------------
> > mm/vma.c | 21 +++---
> > tools/testing/vma/include/dup.h | 9 ++-
> > tools/testing/vma/include/stubs.h | 9 +--
> > 8 files changed, 123 insertions(+), 111 deletions(-)
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 8b3dd145b25e..a2628a12bd2b 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2058,8 +2058,6 @@ static inline bool can_mmap_file(struct file *file)
> > return true;
> > }
> >
> > -int __compat_vma_mmap(const struct file_operations *f_op,
> > - struct file *file, struct vm_area_struct *vma);
> > int compat_vma_mmap(struct file *file, struct vm_area_struct *vma);
> >
> > static inline int vfs_mmap(struct file *file, struct vm_area_struct *vma)
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 4c4fd55fc823..cc5960a84382 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -4116,10 +4116,10 @@ static inline void mmap_action_ioremap_full(struct
> > vm_area_desc *desc,
> > mmap_action_ioremap(desc, desc->start, start_pfn,
> > vma_desc_size(desc));
> > }
> >
> > -void mmap_action_prepare(struct mmap_action *action,
> > - struct vm_area_desc *desc);
> > -int mmap_action_complete(struct mmap_action *action,
> > - struct vm_area_struct *vma);
> > +int mmap_action_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action);
> > +int mmap_action_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action);
> >
> > /* Look up the first VMA which exactly match the interval vm_start ...
> > vm_end */
> > static inline struct vm_area_struct *find_exact_vma(struct mm_struct *mm,
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 95b583e7e4f7..7bfa85b5e78b 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -1775,26 +1775,32 @@ int walk_page_range_debug(struct mm_struct *mm,
> > unsigned long start,
> > void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm);
> > int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm);
> >
> > -void remap_pfn_range_prepare(struct vm_area_desc *desc, unsigned long pfn);
> > -int remap_pfn_range_complete(struct vm_area_struct *vma, unsigned long
> > addr,
> > - unsigned long pfn, unsigned long size, pgprot_t pgprot);
> > +int remap_pfn_range_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action);
> > +int remap_pfn_range_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action);
> >
> > -static inline void io_remap_pfn_range_prepare(struct vm_area_desc *desc,
> > - unsigned long orig_pfn, unsigned long size)
> > +static inline int io_remap_pfn_range_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action)
> > {
> > + const unsigned long orig_pfn = action->remap.start_pfn;
> > + const unsigned long size = action->remap.size;
> > const unsigned long pfn = io_remap_pfn_range_pfn(orig_pfn, size);
> >
> > - return remap_pfn_range_prepare(desc, pfn);
> > + action->remap.start_pfn = pfn;
> > + return remap_pfn_range_prepare(desc, action);
> > }
> >
> > static inline int io_remap_pfn_range_complete(struct vm_area_struct *vma,
> > - unsigned long addr, unsigned long orig_pfn, unsigned long
> > size,
> > - pgprot_t orig_prot)
> > + struct mmap_action *action)
> > {
> > - const unsigned long pfn = io_remap_pfn_range_pfn(orig_pfn, size);
> > - const pgprot_t prot = pgprot_decrypted(orig_prot);
> > + const unsigned long size = action->remap.size;
> > + const unsigned long orig_pfn = action->remap.start_pfn;
> > + const pgprot_t orig_prot = vma->vm_page_prot;
> >
> > - return remap_pfn_range_complete(vma, addr, pfn, size, prot);
> > + action->remap.pgprot = pgprot_decrypted(orig_prot);
I'm guessing it doesn't really matter but after this change
action->remap.pgprot will store the decrypted value while before this
change it was kept the way mmap_prepare() originally set it. We pass
the action structure later to mmap_action_finish() but it does not use
action->remap.pgprot, so this probably doesn't matter.
> > + action->remap.start_pfn = io_remap_pfn_range_pfn(orig_pfn, size);
> > + return remap_pfn_range_complete(vma, action);
> > }
> >
> > #ifdef CONFIG_MMU_NOTIFIER
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 6aa0ea4af1fc..364fa8a45360 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3099,26 +3099,34 @@ static int do_remap_pfn_range(struct vm_area_struct
> > *vma, unsigned long addr,
> > }
> > #endif
> >
> > -void remap_pfn_range_prepare(struct vm_area_desc *desc, unsigned long pfn)
> > +int remap_pfn_range_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action)
> > {
> > - /*
> > - * We set addr=VMA start, end=VMA end here, so this won't fail, but
> > we
> > - * check it again on complete and will fail there if specified addr
> > is
> > - * invalid.
> > - */
> > - get_remap_pgoff(vma_desc_is_cow_mapping(desc), desc->start,
> > desc->end,
> > - desc->start, desc->end, pfn, &desc->pgoff);
> > + const unsigned long start = action->remap.start;
> > + const unsigned long end = start + action->remap.size;
> > + const unsigned long pfn = action->remap.start_pfn;
> > + const bool is_cow = vma_desc_is_cow_mapping(desc);
>
> I was trying to figure out who sets action->remap.start and
> action->remap.size and if they somehow guaranteed to be always equal
> to desc->start and (desc->end - desc->start). My understanding is that
> action->remap.start and action->remap.size are set by
> f_op->mmap_prepare() but I'm not sure if they are always the same as
> desc->start and (desc->end - desc->start) and if so, how do we enforce
> that.
>
> > + int err;
> > +
> > + err = get_remap_pgoff(is_cow, start, end, desc->start, desc->end,
> > pfn,
> > + &desc->pgoff);
> > + if (err)
> > + return err;
> > +
> > vma_desc_set_flags_mask(desc, VMA_REMAP_FLAGS);
> > + return 0;
> > }
> >
> > -static int remap_pfn_range_prepare_vma(struct vm_area_struct *vma,
> > unsigned long addr,
> > - unsigned long pfn, unsigned long size)
> > +static int remap_pfn_range_prepare_vma(struct vm_area_struct *vma,
> > + unsigned long addr, unsigned long
> > pfn,
> > + unsigned long size)
> > {
> > - unsigned long end = addr + PAGE_ALIGN(size);
> > + const unsigned long end = addr + PAGE_ALIGN(size);
> > + const bool is_cow = is_cow_mapping(vma->vm_flags);
> > int err;
> >
> > - err = get_remap_pgoff(is_cow_mapping(vma->vm_flags), addr, end,
> > - vma->vm_start, vma->vm_end, pfn,
> > &vma->vm_pgoff);
> > + err = get_remap_pgoff(is_cow, addr, end, vma->vm_start, vma->vm_end,
> > + pfn, &vma->vm_pgoff);
> > if (err)
> > return err;
> >
> > @@ -3151,10 +3159,15 @@ int remap_pfn_range(struct vm_area_struct *vma,
> > unsigned long addr,
> > }
> > EXPORT_SYMBOL(remap_pfn_range);
> >
> > -int remap_pfn_range_complete(struct vm_area_struct *vma, unsigned long
> > addr,
> > - unsigned long pfn, unsigned long size, pgprot_t prot)
> > +int remap_pfn_range_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action)
> > {
> > - return do_remap_pfn_range(vma, addr, pfn, size, prot);
> > + const unsigned long start = action->remap.start;
> > + const unsigned long pfn = action->remap.start_pfn;
> > + const unsigned long size = action->remap.size;
> > + const pgprot_t prot = action->remap.pgprot;
> > +
> > + return do_remap_pfn_range(vma, start, pfn, size, prot);
> > }
> >
> > /**
> > diff --git a/mm/util.c b/mm/util.c
> > index ce7ae80047cf..dba1191725b6 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -1163,43 +1163,6 @@ void flush_dcache_folio(struct folio *folio)
> > EXPORT_SYMBOL(flush_dcache_folio);
> > #endif
> >
> > -/**
> > - * __compat_vma_mmap() - See description for compat_vma_mmap()
> > - * for details. This is the same operation, only with a specific file
> > operations
> > - * struct which may or may not be the same as vma->vm_file->f_op.
> > - * @f_op: The file operations whose .mmap_prepare() hook is specified.
> > - * @file: The file which backs or will back the mapping.
> > - * @vma: The VMA to apply the .mmap_prepare() hook to.
> > - * Returns: 0 on success or error.
> > - */
> > -int __compat_vma_mmap(const struct file_operations *f_op,
> > - struct file *file, struct vm_area_struct *vma)
> > -{
> > - struct vm_area_desc desc = {
> > - .mm = vma->vm_mm,
> > - .file = file,
> > - .start = vma->vm_start,
> > - .end = vma->vm_end,
> > -
> > - .pgoff = vma->vm_pgoff,
> > - .vm_file = vma->vm_file,
> > - .vma_flags = vma->flags,
> > - .page_prot = vma->vm_page_prot,
> > -
> > - .action.type = MMAP_NOTHING, /* Default */
> > - };
> > - int err;
> > -
> > - err = f_op->mmap_prepare(&desc);
> > - if (err)
> > - return err;
> > -
> > - mmap_action_prepare(&desc.action, &desc);
> > - set_vma_from_desc(vma, &desc);
> > - return mmap_action_complete(&desc.action, vma);
> > -}
> > -EXPORT_SYMBOL(__compat_vma_mmap);
> > -
> > /**
> > * compat_vma_mmap() - Apply the file's .mmap_prepare() hook to an
> > * existing VMA and execute any requested actions.
> > @@ -1228,7 +1191,31 @@ EXPORT_SYMBOL(__compat_vma_mmap);
> > */
> > int compat_vma_mmap(struct file *file, struct vm_area_struct *vma)
> > {
> > - return __compat_vma_mmap(file->f_op, file, vma);
> > + struct vm_area_desc desc = {
> > + .mm = vma->vm_mm,
> > + .file = file,
> > + .start = vma->vm_start,
> > + .end = vma->vm_end,
> > +
> > + .pgoff = vma->vm_pgoff,
> > + .vm_file = vma->vm_file,
> > + .vma_flags = vma->flags,
> > + .page_prot = vma->vm_page_prot,
> > +
> > + .action.type = MMAP_NOTHING, /* Default */
> > + };
> > + int err;
> > +
> > + err = vfs_mmap_prepare(file, &desc);
> > + if (err)
> > + return err;
> > +
> > + err = mmap_action_prepare(&desc, &desc.action);
> > + if (err)
> > + return err;
> > +
> > + set_vma_from_desc(vma, &desc);
> > + return mmap_action_complete(vma, &desc.action);
> > }
> > EXPORT_SYMBOL(compat_vma_mmap);
> >
> > @@ -1320,8 +1307,8 @@ void snapshot_page(struct page_snapshot *ps, const
> > struct page *page)
> > }
> > }
> >
> > -static int mmap_action_finish(struct mmap_action *action,
> > - const struct vm_area_struct *vma, int err)
> > +static int mmap_action_finish(struct vm_area_struct *vma,
> > + struct mmap_action *action, int err)
> > {
> > /*
> > * If an error occurs, unmap the VMA altogether and return an
> > error. We
> > @@ -1355,35 +1342,36 @@ static int mmap_action_finish(struct mmap_action
> > *action,
> > * action which need to be performed.
> > * @desc: The VMA descriptor to prepare for @action.
> > * @action: The action to perform.
> > + *
> > + * Returns: 0 on success, otherwise error.
> > */
> > -void mmap_action_prepare(struct mmap_action *action,
> > - struct vm_area_desc *desc)
> > +int mmap_action_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action)
>
> Any reason you are swapping the arguments?
> It also looks like we always call mmap_action_prepare() with action ==
> desc->action, like this: mmap_action_prepare(&desc.action, &desc). Why
> don't we eliminate the action parameter altogether and use desc.action
> from inside the function?
>
> > +
>
> extra new line.
>
> > {
> > switch (action->type) {
> > case MMAP_NOTHING:
> > - break;
> > + return 0;
> > case MMAP_REMAP_PFN:
> > - remap_pfn_range_prepare(desc, action->remap.start_pfn);
> > - break;
> > + return remap_pfn_range_prepare(desc, action);
> > case MMAP_IO_REMAP_PFN:
> > - io_remap_pfn_range_prepare(desc, action->remap.start_pfn,
> > - action->remap.size);
> > - break;
> > + return io_remap_pfn_range_prepare(desc, action);
> > }
> > }
> > EXPORT_SYMBOL(mmap_action_prepare);
> >
> > /**
> > * mmap_action_complete - Execute VMA descriptor action.
> > - * @action: The action to perform.
> > * @vma: The VMA to perform the action upon.
> > + * @action: The action to perform.
> > *
> > * Similar to mmap_action_prepare().
> > *
> > * Return: 0 on success, or error, at which point the VMA will be unmapped.
> > */
> > -int mmap_action_complete(struct mmap_action *action,
> > - struct vm_area_struct *vma)
> > +int mmap_action_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action)
> > +
> > {
> > int err = 0;
> >
> > @@ -1391,23 +1379,19 @@ int mmap_action_complete(struct mmap_action *action,
> > case MMAP_NOTHING:
> > break;
> > case MMAP_REMAP_PFN:
> > - err = remap_pfn_range_complete(vma, action->remap.start,
> > - action->remap.start_pfn, action->remap.size,
> > - action->remap.pgprot);
> > + err = remap_pfn_range_complete(vma, action);
> > break;
> > case MMAP_IO_REMAP_PFN:
> > - err = io_remap_pfn_range_complete(vma, action->remap.start,
> > - action->remap.start_pfn, action->remap.size,
> > - action->remap.pgprot);
> > + err = io_remap_pfn_range_complete(vma, action);
> > break;
> > }
> >
> > - return mmap_action_finish(action, vma, err);
> > + return mmap_action_finish(vma, action, err);
> > }
> > EXPORT_SYMBOL(mmap_action_complete);
> > #else
> > -void mmap_action_prepare(struct mmap_action *action,
> > - struct vm_area_desc *desc)
> > +int mmap_action_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action)
> > {
> > switch (action->type) {
> > case MMAP_NOTHING:
> > @@ -1417,11 +1401,13 @@ void mmap_action_prepare(struct mmap_action *action,
> > WARN_ON_ONCE(1); /* nommu cannot handle these. */
> > break;
> > }
> > +
> > + return 0;
> > }
> > EXPORT_SYMBOL(mmap_action_prepare);
> >
> > -int mmap_action_complete(struct mmap_action *action,
> > - struct vm_area_struct *vma)
> > +int mmap_action_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action)
> > {
> > int err = 0;
> >
> > @@ -1436,7 +1422,7 @@ int mmap_action_complete(struct mmap_action *action,
> > break;
> > }
> >
> > - return mmap_action_finish(action, vma, err);
> > + return mmap_action_finish(vma, action, err);
> > }
> > EXPORT_SYMBOL(mmap_action_complete);
> > #endif
> > diff --git a/mm/vma.c b/mm/vma.c
> > index be64f781a3aa..054cf1d262fb 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -2613,15 +2613,19 @@ static void __mmap_complete(struct mmap_state *map,
> > struct vm_area_struct *vma)
> > vma_set_page_prot(vma);
> > }
> >
> > -static void call_action_prepare(struct mmap_state *map,
> > - struct vm_area_desc *desc)
> > +static int call_action_prepare(struct mmap_state *map,
> > + struct vm_area_desc *desc)
> > {
> > struct mmap_action *action = &desc->action;
> > + int err;
> >
> > - mmap_action_prepare(action, desc);
> > + err = mmap_action_prepare(desc, action);
> > + if (err)
> > + return err;
> >
> > if (action->hide_from_rmap_until_complete)
> > map->hold_file_rmap_lock = true;
> > + return 0;
> > }
> >
> > /*
> > @@ -2645,7 +2649,9 @@ static int call_mmap_prepare(struct mmap_state *map,
> > if (err)
> > return err;
> >
> > - call_action_prepare(map, desc);
> > + err = call_action_prepare(map, desc);
> > + if (err)
> > + return err;
> >
> > /* Update fields permitted to be changed. */
> > map->pgoff = desc->pgoff;
> > @@ -2700,13 +2706,12 @@ static bool can_set_ksm_flags_early(struct
> > mmap_state *map)
> > }
> >
> > static int call_action_complete(struct mmap_state *map,
> > - struct vm_area_desc *desc,
> > + struct mmap_action *action,
> > struct vm_area_struct *vma)
> > {
> > - struct mmap_action *action = &desc->action;
> > int ret;
> >
> > - ret = mmap_action_complete(action, vma);
> > + ret = mmap_action_complete(vma, action);
> >
> > /* If we held the file rmap we need to release it. */
> > if (map->hold_file_rmap_lock) {
> > @@ -2768,7 +2773,7 @@ static unsigned long __mmap_region(struct file *file,
> > unsigned long addr,
> > __mmap_complete(&map, vma);
> >
> > if (have_mmap_prepare && allocated_new) {
> > - error = call_action_complete(&map, &desc, vma);
> > + error = call_action_complete(&map, &desc.action, vma);
> >
> > if (error)
> > return error;
> > diff --git a/tools/testing/vma/include/dup.h
> > b/tools/testing/vma/include/dup.h
> > index 5eb313beb43d..908beb263307 100644
> > --- a/tools/testing/vma/include/dup.h
> > +++ b/tools/testing/vma/include/dup.h
> > @@ -1106,7 +1106,7 @@ static inline int __compat_vma_mmap(const struct
> > file_operations *f_op,
> >
> > .pgoff = vma->vm_pgoff,
> > .vm_file = vma->vm_file,
> > - .vm_flags = vma->vm_flags,
> > + .vma_flags = vma->flags,
> > .page_prot = vma->vm_page_prot,
> >
> > .action.type = MMAP_NOTHING, /* Default */
> > @@ -1117,9 +1117,12 @@ static inline int __compat_vma_mmap(const struct
> > file_operations *f_op,
> > if (err)
> > return err;
> >
> > - mmap_action_prepare(&desc.action, &desc);
> > + err = mmap_action_prepare(&desc, &desc.action);
> > + if (err)
> > + return err;
> > +
> > set_vma_from_desc(vma, &desc);
> > - return mmap_action_complete(&desc.action, vma);
> > + return mmap_action_complete(vma, &desc.action);
> > }
> >
> > static inline int compat_vma_mmap(struct file *file,
> > diff --git a/tools/testing/vma/include/stubs.h
> > b/tools/testing/vma/include/stubs.h
> > index 947a3a0c2566..76c4b668bc62 100644
> > --- a/tools/testing/vma/include/stubs.h
> > +++ b/tools/testing/vma/include/stubs.h
> > @@ -81,13 +81,14 @@ static inline void free_anon_vma_name(struct
> > vm_area_struct *vma)
> > {
> > }
> >
> > -static inline void mmap_action_prepare(struct mmap_action *action,
> > - struct vm_area_desc *desc)
> > +static inline int mmap_action_prepare(struct vm_area_desc *desc,
> > + struct mmap_action *action)
> > {
> > + return 0;
> > }
> >
> > -static inline int mmap_action_complete(struct mmap_action *action,
> > - struct vm_area_struct *vma)
> > +static inline int mmap_action_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action)
> > {
> > return 0;
> > }
> > --
> > 2.53.0
> >
