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); > + 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 >
