Provide a means by which drivers can specify which fields of those
permitted to be changed should be altered to prior to mmap()'ing a
range (which may either result from a merge or from mapping an entirely new
VMA).

Doing so is substantially safer than the existing .mmap() calback which
provides unrestricted access to the part-constructed VMA and permits
drivers and file systems to do 'creative' things which makes it hard to
reason about the state of the VMA after the function returns.

The existing .mmap() callback's freedom has caused a great deal of issues,
especially in error handling, as unwinding the mmap() state has proven to
be non-trivial and caused significant issues in the past, for instance
those addressed in commit 5de195060b2e ("mm: resolve faulty mmap_region()
error path behaviour").

It also necessitates a second attempt at merge once the .mmap() callback
has completed, which has caused issues in the past, is awkward, adds
overhead and is difficult to reason about.

The .mmap_prepare() callback eliminates this requirement, as we can update
fields prior to even attempting the first merge. It is safer, as we heavily
restrict what can actually be modified, and being invoked very early in the
mmap() process, error handling can be performed safely with very little
unwinding of state required.

The .mmap_prepare() and deprecated .mmap() callbacks are mutually
exclusive, so we permit only one to be invoked at a time.

Update vma userland test stubs to account for changes.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 include/linux/fs.h               | 38 +++++++++++++++
 include/linux/mm_types.h         | 24 ++++++++++
 mm/memory.c                      |  3 +-
 mm/mmap.c                        |  2 +-
 mm/vma.c                         | 70 +++++++++++++++++++++++++++-
 tools/testing/vma/vma_internal.h | 79 ++++++++++++++++++++++++++++++--
 6 files changed, 208 insertions(+), 8 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 016b0fe1536e..d6c5a703a215 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2169,6 +2169,7 @@ struct file_operations {
 	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
 	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
 				unsigned int poll_flags);
+	int (*mmap_prepare)(struct vm_area_desc *);
 } __randomize_layout;
 
 /* Supports async buffered reads */
@@ -2238,11 +2239,48 @@ struct inode_operations {
 	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
 } ____cacheline_aligned;
 
+static inline bool file_has_deprecated_mmap_hook(struct file *file)
+{
+	return file->f_op->mmap;
+}
+
+static inline bool file_has_mmap_prepare_hook(struct file *file)
+{
+	return file->f_op->mmap_prepare;
+}
+
+/* Did the driver provide valid mmap hook configuration? */
+static inline bool file_has_valid_mmap_hooks(struct file *file)
+{
+	bool has_mmap = file_has_deprecated_mmap_hook(file);
+	bool has_mmap_prepare = file_has_mmap_prepare_hook(file);
+
+	/* Hooks are mutually exclusive. */
+	if (has_mmap && has_mmap_prepare)
+		return false;
+
+	/* But at least one must be specified. */
+	if (!has_mmap && !has_mmap_prepare)
+		return false;
+
+	return true;
+}
+
 static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
 {
+	/* If the driver specifies .mmap_prepare() this call is invalid. */
+	if (file_has_mmap_prepare_hook(file))
+		return -EINVAL;
+
 	return file->f_op->mmap(file, vma);
 }
 
+static inline int __call_mmap_prepare(struct file *file,
+		struct vm_area_desc *desc)
+{
+	return file->f_op->mmap_prepare(desc);
+}
+
 extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
 extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index e76bade9ebb1..15808cad2bc1 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -763,6 +763,30 @@ struct vma_numab_state {
 	int prev_scan_seq;
 };
 
+/*
+ * Describes a VMA that is about to be mmap()'ed. Drivers may choose to
+ * manipulate mutable fields which will cause those fields to be updated in the
+ * resultant VMA.
+ *
+ * Helper functions are not required for manipulating any field.
+ */
+struct vm_area_desc {
+	/* Immutable state. */
+	struct mm_struct *mm;
+	unsigned long start;
+	unsigned long end;
+
+	/* Mutable fields. Populated with initial state. */
+	pgoff_t pgoff;
+	struct file *file;
+	vm_flags_t vm_flags;
+	pgprot_t page_prot;
+
+	/* Write-only fields. */
+	const struct vm_operations_struct *vm_ops;
+	void *private_data;
+};
+
 /*
  * This struct describes a virtual memory area. There is one of these
  * per VM-area/task. A VM area is any part of the process virtual memory
diff --git a/mm/memory.c b/mm/memory.c
index 68c1d962d0ad..99af83434e7c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -527,10 +527,11 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr,
 		dump_page(page, "bad pte");
 	pr_alert("addr:%px vm_flags:%08lx anon_vma:%px mapping:%px index:%lx\n",
 		 (void *)addr, vma->vm_flags, vma->anon_vma, mapping, index);
-	pr_alert("file:%pD fault:%ps mmap:%ps read_folio:%ps\n",
+	pr_alert("file:%pD fault:%ps mmap:%ps mmap_prepare: %ps read_folio:%ps\n",
 		 vma->vm_file,
 		 vma->vm_ops ? vma->vm_ops->fault : NULL,
 		 vma->vm_file ? vma->vm_file->f_op->mmap : NULL,
+		 vma->vm_file ? vma->vm_file->f_op->mmap_prepare : NULL,
 		 mapping ? mapping->a_ops->read_folio : NULL);
 	dump_stack();
 	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
diff --git a/mm/mmap.c b/mm/mmap.c
index 81dd962a1cfc..50f902c08341 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -475,7 +475,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
 				vm_flags &= ~VM_MAYEXEC;
 			}
 
-			if (!file->f_op->mmap)
+			if (!file_has_valid_mmap_hooks(file))
 				return -ENODEV;
 			if (vm_flags & (VM_GROWSDOWN|VM_GROWSUP))
 				return -EINVAL;
diff --git a/mm/vma.c b/mm/vma.c
index 1f2634b29568..acd5b98fe087 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -17,6 +17,11 @@ struct mmap_state {
 	unsigned long pglen;
 	unsigned long flags;
 	struct file *file;
+	pgprot_t page_prot;
+
+	/* User-defined fields, perhaps updated by .mmap_prepare(). */
+	const struct vm_operations_struct *vm_ops;
+	void *vm_private_data;
 
 	unsigned long charged;
 	bool retry_merge;
@@ -40,6 +45,7 @@ struct mmap_state {
 		.pglen = PHYS_PFN(len_),				\
 		.flags = flags_,					\
 		.file = file_,						\
+		.page_prot = vm_get_page_prot(flags_),			\
 	}
 
 #define VMG_MMAP_STATE(name, map_, vma_)				\
@@ -2385,6 +2391,10 @@ static int __mmap_new_file_vma(struct mmap_state *map,
 	int error;
 
 	vma->vm_file = get_file(map->file);
+
+	if (!file_has_deprecated_mmap_hook(map->file))
+		return 0;
+
 	error = mmap_file(vma->vm_file, vma);
 	if (error) {
 		fput(vma->vm_file);
@@ -2441,7 +2451,7 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap)
 	vma_iter_config(vmi, map->addr, map->end);
 	vma_set_range(vma, map->addr, map->end, map->pgoff);
 	vm_flags_init(vma, map->flags);
-	vma->vm_page_prot = vm_get_page_prot(map->flags);
+	vma->vm_page_prot = map->page_prot;
 
 	if (vma_iter_prealloc(vmi, vma)) {
 		error = -ENOMEM;
@@ -2528,6 +2538,58 @@ static void __mmap_complete(struct mmap_state *map, struct vm_area_struct *vma)
 	vma_set_page_prot(vma);
 }
 
+/*
+ * Invoke the f_op->mmap_prepare() callback for a file-backed mapping that
+ * specifies it.
+ *
+ * This is called prior to any merge attempt, and updates whitelisted fields
+ * that are permitted to be updated by the caller.
+ *
+ * All but user-defined fields will be pre-populated with original values.
+ *
+ * Returns 0 on success, or an error code otherwise.
+ */
+static int call_mmap_prepare(struct mmap_state *map)
+{
+	int err;
+	struct vm_area_desc desc = {
+		.mm = map->mm,
+		.start = map->addr,
+		.end = map->end,
+
+		.pgoff = map->pgoff,
+		.file = map->file,
+		.vm_flags = map->flags,
+		.page_prot = map->page_prot,
+	};
+
+	VM_WARN_ON(!file_has_valid_mmap_hooks(map->file));
+
+	/* Invoke the hook. */
+	err = __call_mmap_prepare(map->file, &desc);
+	if (err)
+		return err;
+
+	/* Update fields permitted to be changed. */
+	map->pgoff = desc.pgoff;
+	map->file = desc.file;
+	map->flags = desc.vm_flags;
+	map->page_prot = desc.page_prot;
+	/* User-defined fields. */
+	map->vm_ops = desc.vm_ops;
+	map->vm_private_data = desc.private_data;
+
+	return 0;
+}
+
+static void set_vma_user_defined_fields(struct vm_area_struct *vma,
+		struct mmap_state *map)
+{
+	if (map->vm_ops)
+		vma->vm_ops = map->vm_ops;
+	vma->vm_private_data = map->vm_private_data;
+}
+
 static unsigned long __mmap_region(struct file *file, unsigned long addr,
 		unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
 		struct list_head *uf)
@@ -2535,10 +2597,13 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma = NULL;
 	int error;
+	bool have_mmap_prepare = file && file_has_mmap_prepare_hook(file);
 	VMA_ITERATOR(vmi, mm, addr);
 	MMAP_STATE(map, mm, &vmi, addr, len, pgoff, vm_flags, file);
 
 	error = __mmap_prepare(&map, uf);
+	if (!error && have_mmap_prepare)
+		error = call_mmap_prepare(&map);
 	if (error)
 		goto abort_munmap;
 
@@ -2556,6 +2621,9 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 			goto unacct_error;
 	}
 
+	if (have_mmap_prepare)
+		set_vma_user_defined_fields(vma, &map);
+
 	/* If flags changed, we might be able to merge, so try again. */
 	if (map.retry_merge) {
 		struct vm_area_struct *merged;
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 198abe66de5a..a2cc54e9ed36 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -253,8 +253,40 @@ struct mm_struct {
 	unsigned long flags; /* Must use atomic bitops to access */
 };
 
+struct vm_area_struct;
+
+/*
+ * Describes a VMA that is about to be mmap()'ed. Drivers may choose to
+ * manipulate mutable fields which will cause those fields to be updated in the
+ * resultant VMA.
+ *
+ * Helper functions are not required for manipulating any field.
+ */
+struct vm_area_desc {
+	/* Immutable state. */
+	struct mm_struct *mm;
+	unsigned long start;
+	unsigned long end;
+
+	/* Mutable fields. Populated with initial state. */
+	pgoff_t pgoff;
+	struct file *file;
+	vm_flags_t vm_flags;
+	pgprot_t page_prot;
+
+	/* Write-only fields. */
+	const struct vm_operations_struct *vm_ops;
+	void *private_data;
+};
+
+struct file_operations {
+	int (*mmap)(struct file *, struct vm_area_struct *);
+	int (*mmap_prepare)(struct vm_area_desc *);
+};
+
 struct file {
 	struct address_space	*f_mapping;
+	const struct file_operations	*f_op;
 };
 
 #define VMA_LOCK_OFFSET	0x40000000
@@ -1125,11 +1157,6 @@ static inline void vm_flags_clear(struct vm_area_struct *vma,
 	vma->__vm_flags &= ~flags;
 }
 
-static inline int call_mmap(struct file *, struct vm_area_struct *)
-{
-	return 0;
-}
-
 static inline int shmem_zero_setup(struct vm_area_struct *)
 {
 	return 0;
@@ -1405,4 +1432,46 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma)
 	(void)vma;
 }
 
+static inline bool file_has_deprecated_mmap_hook(struct file *file)
+{
+	return file->f_op->mmap;
+}
+
+static inline bool file_has_mmap_prepare_hook(struct file *file)
+{
+	return file->f_op->mmap_prepare;
+}
+
+/* Did the driver provide valid mmap hook configuration? */
+static inline bool file_has_valid_mmap_hooks(struct file *file)
+{
+	bool has_mmap = file_has_deprecated_mmap_hook(file);
+	bool has_mmap_prepare = file_has_mmap_prepare_hook(file);
+
+	/* Hooks are mutually exclusive. */
+	if (has_mmap && has_mmap_prepare)
+		return false;
+
+	/* But at least one must be specified. */
+	if (!has_mmap && !has_mmap_prepare)
+		return false;
+
+	return true;
+}
+
+static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	/* If the driver specifies .mmap_prepare() this call is invalid. */
+	if (file_has_mmap_prepare_hook(file))
+		return -EINVAL;
+
+	return file->f_op->mmap(file, vma);
+}
+
+static inline int __call_mmap_prepare(struct file *file,
+		struct vm_area_desc *desc)
+{
+	return file->f_op->mmap_prepare(desc);
+}
+
 #endif	/* __MM_VMA_INTERNAL_H */
-- 
2.49.0

On 07.05.25 13:03, Lorenzo Stoakes wrote:
> Provide a means by which drivers can specify which fields of those
> permitted to be changed should be altered to prior to mmap()'ing a
> range (which may either result from a merge or from mapping an entirely new
> VMA).
> 
> Doing so is substantially safer than the existing .mmap() calback which
> provides unrestricted access to the part-constructed VMA and permits
> drivers and file systems to do 'creative' things which makes it hard to
> reason about the state of the VMA after the function returns.
> 
> The existing .mmap() callback's freedom has caused a great deal of issues,
> especially in error handling, as unwinding the mmap() state has proven to
> be non-trivial and caused significant issues in the past, for instance
> those addressed in commit 5de195060b2e ("mm: resolve faulty mmap_region()
> error path behaviour").
> 
> It also necessitates a second attempt at merge once the .mmap() callback
> has completed, which has caused issues in the past, is awkward, adds
> overhead and is difficult to reason about.
> 
> The .mmap_prepare() callback eliminates this requirement, as we can update
> fields prior to even attempting the first merge. It is safer, as we heavily
> restrict what can actually be modified, and being invoked very early in the
> mmap() process, error handling can be performed safely with very little
> unwinding of state required.
> 
> The .mmap_prepare() and deprecated .mmap() callbacks are mutually
> exclusive, so we permit only one to be invoked at a time.
> 
> Update vma userland test stubs to account for changes.
> 

In general, looks very good to me.

Some comments, especially regarding suboptimal code duplciation with the 
stubs. (unless I am missing fine details :) )

> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>   include/linux/fs.h               | 38 +++++++++++++++
>   include/linux/mm_types.h         | 24 ++++++++++
>   mm/memory.c                      |  3 +-
>   mm/mmap.c                        |  2 +-
>   mm/vma.c                         | 70 +++++++++++++++++++++++++++-
>   tools/testing/vma/vma_internal.h | 79 ++++++++++++++++++++++++++++++--
>   6 files changed, 208 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 016b0fe1536e..d6c5a703a215 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2169,6 +2169,7 @@ struct file_operations {
>   	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
>   	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
>   				unsigned int poll_flags);
> +	int (*mmap_prepare)(struct vm_area_desc *);
>   } __randomize_layout;
>   
>   /* Supports async buffered reads */
> @@ -2238,11 +2239,48 @@ struct inode_operations {
>   	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
>   } ____cacheline_aligned;
>   
> +static inline bool file_has_deprecated_mmap_hook(struct file *file)
> +{
> +	return file->f_op->mmap;
> +}
> +
> +static inline bool file_has_mmap_prepare_hook(struct file *file)
> +{
> +	return file->f_op->mmap_prepare;
> +}

I am usually not a fan of such dummy helper functions .. I mean, how far 
do we go?

file_has_f_op()

file_is_non_null()

...

Or is this required for some stubbing regarding vma tests? But even the 
stubs below confuse me a bit, because they do exactly the same thing :(

:)

> +
> +/* Did the driver provide valid mmap hook configuration? */
> +static inline bool file_has_valid_mmap_hooks(struct file *file)
> +{
> +	bool has_mmap = file_has_deprecated_mmap_hook(file);
> +	bool has_mmap_prepare = file_has_mmap_prepare_hook(file);
> +
> +	/* Hooks are mutually exclusive. */
> +	if (has_mmap && has_mmap_prepare)

Should this be WARN_ON_ONCE() ?

> +		return false;
> +
> +	/* But at least one must be specified. */
> +	if (!has_mmap && !has_mmap_prepare)
> +		return false;
> +
 > +	return true;

return has_mmap || has_mmap_prepare;

And I think you can drop the comment about "at least one" with that, 
should be quite clear from that simplified version.

> +}
> +
>   static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
>   {
> +	/* If the driver specifies .mmap_prepare() this call is invalid. */
> +	if (file_has_mmap_prepare_hook(file))

Should this be WARN_ON_ONCE() ?

> +		return -EINVAL;
> +
>   	return file->f_op->mmap(file, vma);
>   }
>   
> +static inline int __call_mmap_prepare(struct file *file,
> +		struct vm_area_desc *desc)
> +{
> +	return file->f_op->mmap_prepare(desc);
> +}
> +

[...]

>   struct file {
>   	struct address_space	*f_mapping;
> +	const struct file_operations	*f_op;
>   };
>   
>   #define VMA_LOCK_OFFSET	0x40000000
> @@ -1125,11 +1157,6 @@ static inline void vm_flags_clear(struct vm_area_struct *vma,
>   	vma->__vm_flags &= ~flags;
>   }
>   
> -static inline int call_mmap(struct file *, struct vm_area_struct *)
> -{
> -	return 0;
> -}
> -
>   static inline int shmem_zero_setup(struct vm_area_struct *)
>   {
>   	return 0;
> @@ -1405,4 +1432,46 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma)
>   	(void)vma;
>   }
>   
> +static inline bool file_has_deprecated_mmap_hook(struct file *file)
> +{
> +	return file->f_op->mmap;
> +}
> +
> +static inline bool file_has_mmap_prepare_hook(struct file *file)
> +{
> +	return file->f_op->mmap_prepare;
> +}
 > +> +/* Did the driver provide valid mmap hook configuration? */
> +static inline bool file_has_valid_mmap_hooks(struct file *file)
> +{
> +	bool has_mmap = file_has_deprecated_mmap_hook(file);
> +	bool has_mmap_prepare = file_has_mmap_prepare_hook(file);
> +
> +	/* Hooks are mutually exclusive. */
> +	if (has_mmap && has_mmap_prepare)
> +		return false;
 > +> +	/* But at least one must be specified. */
> +	if (!has_mmap && !has_mmap_prepare)
> +		return false;
> +
 > +	return true;> +}
> +
> +static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	/* If the driver specifies .mmap_prepare() this call is invalid. */
> +	if (file_has_mmap_prepare_hook(file))
 > +		return -EINVAL;> +
> +	return file->f_op->mmap(file, vma);
> +}
> +
> +static inline int __call_mmap_prepare(struct file *file,
> +		struct vm_area_desc *desc)
> +{
> +	return file->f_op->mmap_prepare(desc);
> +}

Hm, is there a way avoid a copy of the exact same code from fs.h, and 
essentially test the implementation in fs.h (-> more coverage by using 
less duplciated stubs?).

-- 
Cheers,

David / dhildenb

On Fri, May 09, 2025 at 12:00:38PM +0200, David Hildenbrand wrote:
> On 07.05.25 13:03, Lorenzo Stoakes wrote:
> > Provide a means by which drivers can specify which fields of those
> > permitted to be changed should be altered to prior to mmap()'ing a
> > range (which may either result from a merge or from mapping an entirely new
> > VMA).
> >
> > Doing so is substantially safer than the existing .mmap() calback which
> > provides unrestricted access to the part-constructed VMA and permits
> > drivers and file systems to do 'creative' things which makes it hard to
> > reason about the state of the VMA after the function returns.
> >
> > The existing .mmap() callback's freedom has caused a great deal of issues,
> > especially in error handling, as unwinding the mmap() state has proven to
> > be non-trivial and caused significant issues in the past, for instance
> > those addressed in commit 5de195060b2e ("mm: resolve faulty mmap_region()
> > error path behaviour").
> >
> > It also necessitates a second attempt at merge once the .mmap() callback
> > has completed, which has caused issues in the past, is awkward, adds
> > overhead and is difficult to reason about.
> >
> > The .mmap_prepare() callback eliminates this requirement, as we can update
> > fields prior to even attempting the first merge. It is safer, as we heavily
> > restrict what can actually be modified, and being invoked very early in the
> > mmap() process, error handling can be performed safely with very little
> > unwinding of state required.
> >
> > The .mmap_prepare() and deprecated .mmap() callbacks are mutually
> > exclusive, so we permit only one to be invoked at a time.
> >
> > Update vma userland test stubs to account for changes.
> >
>
> In general, looks very good to me.

Thanks!

>
> Some comments, especially regarding suboptimal code duplciation with the
> stubs. (unless I am missing fine details :) )

Responding inline... :)

>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >   include/linux/fs.h               | 38 +++++++++++++++
> >   include/linux/mm_types.h         | 24 ++++++++++
> >   mm/memory.c                      |  3 +-
> >   mm/mmap.c                        |  2 +-
> >   mm/vma.c                         | 70 +++++++++++++++++++++++++++-
> >   tools/testing/vma/vma_internal.h | 79 ++++++++++++++++++++++++++++++--
> >   6 files changed, 208 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 016b0fe1536e..d6c5a703a215 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2169,6 +2169,7 @@ struct file_operations {
> >   	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
> >   	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
> >   				unsigned int poll_flags);
> > +	int (*mmap_prepare)(struct vm_area_desc *);
> >   } __randomize_layout;
> >   /* Supports async buffered reads */
> > @@ -2238,11 +2239,48 @@ struct inode_operations {
> >   	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> >   } ____cacheline_aligned;
> > +static inline bool file_has_deprecated_mmap_hook(struct file *file)
> > +{
> > +	return file->f_op->mmap;
> > +}
> > +
> > +static inline bool file_has_mmap_prepare_hook(struct file *file)
> > +{
> > +	return file->f_op->mmap_prepare;
> > +}
>
> I am usually not a fan of such dummy helper functions .. I mean, how far do
> we go?
>
> file_has_f_op()
>
> file_is_non_null()
>
> ...
>
> Or is this required for some stubbing regarding vma tests? But even the
> stubs below confuse me a bit, because they do exactly the same thing :(
>
> :)

Yeah I know... it was more for clarity, but I take your point, this is possibly
not really adding much.

In the first version this had a file && file->... which made a lot more sense
for this. But then I fixed things up such that e.g. file_has_valid_mmap_hooks()
assumed file != NULL.

So, will drop these on respin.

>
> > +
> > +/* Did the driver provide valid mmap hook configuration? */
> > +static inline bool file_has_valid_mmap_hooks(struct file *file)
> > +{
> > +	bool has_mmap = file_has_deprecated_mmap_hook(file);
> > +	bool has_mmap_prepare = file_has_mmap_prepare_hook(file);
> > +
> > +	/* Hooks are mutually exclusive. */
> > +	if (has_mmap && has_mmap_prepare)
>
> Should this be WARN_ON_ONCE() ?

Ack you're right, will update!

>
> > +		return false;
> > +
> > +	/* But at least one must be specified. */
> > +	if (!has_mmap && !has_mmap_prepare)
> > +		return false;
> > +
> > +	return true;
>
> return has_mmap || has_mmap_prepare;
>
> And I think you can drop the comment about "at least one" with that, should
> be quite clear from that simplified version.

Ack, will change.

>
> > +}
> > +
> >   static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> >   {
> > +	/* If the driver specifies .mmap_prepare() this call is invalid. */
> > +	if (file_has_mmap_prepare_hook(file))
>
> Should this be WARN_ON_ONCE() ?

Ack, will fix!

>
> > +		return -EINVAL;
> > +
> >   	return file->f_op->mmap(file, vma);
> >   }
> > +static inline int __call_mmap_prepare(struct file *file,
> > +		struct vm_area_desc *desc)
> > +{
> > +	return file->f_op->mmap_prepare(desc);
> > +}
> > +
>
> [...]
>
> >   struct file {
> >   	struct address_space	*f_mapping;
> > +	const struct file_operations	*f_op;
> >   };
> >   #define VMA_LOCK_OFFSET	0x40000000
> > @@ -1125,11 +1157,6 @@ static inline void vm_flags_clear(struct vm_area_struct *vma,
> >   	vma->__vm_flags &= ~flags;
> >   }
> > -static inline int call_mmap(struct file *, struct vm_area_struct *)
> > -{
> > -	return 0;
> > -}
> > -
> >   static inline int shmem_zero_setup(struct vm_area_struct *)
> >   {
> >   	return 0;
> > @@ -1405,4 +1432,46 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma)
> >   	(void)vma;
> >   }
> > +static inline bool file_has_deprecated_mmap_hook(struct file *file)
> > +{
> > +	return file->f_op->mmap;
> > +}
> > +
> > +static inline bool file_has_mmap_prepare_hook(struct file *file)
> > +{
> > +	return file->f_op->mmap_prepare;
> > +}
> > +> +/* Did the driver provide valid mmap hook configuration? */
> > +static inline bool file_has_valid_mmap_hooks(struct file *file)
> > +{
> > +	bool has_mmap = file_has_deprecated_mmap_hook(file);
> > +	bool has_mmap_prepare = file_has_mmap_prepare_hook(file);
> > +
> > +	/* Hooks are mutually exclusive. */
> > +	if (has_mmap && has_mmap_prepare)
> > +		return false;
> > +> +	/* But at least one must be specified. */
> > +	if (!has_mmap && !has_mmap_prepare)
> > +		return false;
> > +
> > +	return true;> +}
> > +
> > +static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > +	/* If the driver specifies .mmap_prepare() this call is invalid. */
> > +	if (file_has_mmap_prepare_hook(file))
> > +		return -EINVAL;> +
> > +	return file->f_op->mmap(file, vma);
> > +}
> > +
> > +static inline int __call_mmap_prepare(struct file *file,
> > +		struct vm_area_desc *desc)
> > +{
> > +	return file->f_op->mmap_prepare(desc);
> > +}
>
> Hm, is there a way avoid a copy of the exact same code from fs.h, and
> essentially test the implementation in fs.h (-> more coverage by using less
> duplciated stubs?).

Not really, this kind of copying is sadly part of it because we're
intentionally isolating vma.c from everything else, and if we try to bring
in other headers they import yet others and etc. etc. it becomes a
combinatorial explosion potentially.

We might be able to address with the tools/include stuff, but I think this
is one to be addressed at a later time in some cleanup code there.

I am keen to avoid this kind of thing as obviously things can get out of
sync (the VMA stuff tries _very hard_ to minimise this and only provide
stubs that truly are stubbed out.

I will add to todo to improve this situation!

>
> --
> Cheers,
>
> David / dhildenb
>

Thanks for review! :) will respin a v2 shortly.

>>> +
>>> +static inline int __call_mmap_prepare(struct file *file,
>>> +		struct vm_area_desc *desc)
>>> +{
>>> +	return file->f_op->mmap_prepare(desc);
>>> +}
>>
>> Hm, is there a way avoid a copy of the exact same code from fs.h, and
>> essentially test the implementation in fs.h (-> more coverage by using less
>> duplciated stubs?).
> 
> Not really, this kind of copying is sadly part of it because we're
> intentionally isolating vma.c from everything else, and if we try to bring
> in other headers they import yet others and etc. etc. it becomes a
> combinatorial explosion potentially.

I guess what would work is inlining __call_mmap_prepare() -- again, 
rather simple wrapper ... and having file_has_valid_mmap_hooks() + 
call_mmap() reside in vma.c. Hm.

As an alternative, we'd really need some separate header that does not 
allow for any other includes, and is essentially only included in the 
other header files.

Duplicating functions in such a way that they can easily go out of sync 
and are not getting tested is really suboptimal. :(

-- 
Cheers,

David / dhildenb

On Fri, May 09, 2025 at 12:51:14PM +0200, David Hildenbrand wrote:
>
> > > > +
> > > > +static inline int __call_mmap_prepare(struct file *file,
> > > > +		struct vm_area_desc *desc)
> > > > +{
> > > > +	return file->f_op->mmap_prepare(desc);
> > > > +}
> > >
> > > Hm, is there a way avoid a copy of the exact same code from fs.h, and
> > > essentially test the implementation in fs.h (-> more coverage by using less
> > > duplciated stubs?).
> >
> > Not really, this kind of copying is sadly part of it because we're
> > intentionally isolating vma.c from everything else, and if we try to bring
> > in other headers they import yet others and etc. etc. it becomes a
> > combinatorial explosion potentially.
>
> I guess what would work is inlining __call_mmap_prepare() -- again, rather
> simple wrapper ... and having file_has_valid_mmap_hooks() + call_mmap()
> reside in vma.c. Hm.
>
> As an alternative, we'd really need some separate header that does not allow
> for any other includes, and is essentially only included in the other header
> files.
>
> Duplicating functions in such a way that they can easily go out of sync and
> are not getting tested is really suboptimal. :(

This is a problem that already exists, if minimised. Perfect is the enemy of
good - if we had make these tests existence depend on being able to isolate
_everything_ they'd never happen :)

But I will definitely try to improve the situation, as I couldn't agree more
about de-syncing and it's a concern I share with you.

I think we have a bit of a mess of header files anyway like this, random helpers
put in random places etc.

It doesn't help that a random driver/shm reference call_mmap()...

Anyway, this is somehwat out of scope for this series, as we already have a
number of instances like this and this is just symptomatic of an existing
problem rather than introducing it.

I think one thing to do might be to have a separate header which is explicitly
for functions like these to at least absolutely highlight this case.

The VMA tests need restructuring anyway, so it can be part of a bigger project
to do some work cleaning up there.

todo++; :>)

>
> --
> Cheers,
>
> David / dhildenb
>

On 09.05.25 12:57, Lorenzo Stoakes wrote:
> On Fri, May 09, 2025 at 12:51:14PM +0200, David Hildenbrand wrote:
>>
>>>>> +
>>>>> +static inline int __call_mmap_prepare(struct file *file,
>>>>> +		struct vm_area_desc *desc)
>>>>> +{
>>>>> +	return file->f_op->mmap_prepare(desc);
>>>>> +}
>>>>
>>>> Hm, is there a way avoid a copy of the exact same code from fs.h, and
>>>> essentially test the implementation in fs.h (-> more coverage by using less
>>>> duplciated stubs?).
>>>
>>> Not really, this kind of copying is sadly part of it because we're
>>> intentionally isolating vma.c from everything else, and if we try to bring
>>> in other headers they import yet others and etc. etc. it becomes a
>>> combinatorial explosion potentially.
>>
>> I guess what would work is inlining __call_mmap_prepare() -- again, rather
>> simple wrapper ... and having file_has_valid_mmap_hooks() + call_mmap()
>> reside in vma.c. Hm.
>>
>> As an alternative, we'd really need some separate header that does not allow
>> for any other includes, and is essentially only included in the other header
>> files.
>>
>> Duplicating functions in such a way that they can easily go out of sync and
>> are not getting tested is really suboptimal. :(
> 
> This is a problem that already exists, if minimised. Perfect is the enemy of
> good - if we had make these tests existence depend on being able to isolate
> _everything_ they'd never happen :)
> 
> But I will definitely try to improve the situation, as I couldn't agree more
> about de-syncing and it's a concern I share with you.
> 
> I think we have a bit of a mess of header files anyway like this, random helpers
> put in random places etc.
> 
> It doesn't help that a random driver/shm reference call_mmap()...

Yes ...

> 
> Anyway, this is somehwat out of scope for this series, as we already have a
> number of instances like this and this is just symptomatic of an existing
> problem rather than introducing it.
> 
> I think one thing to do might be to have a separate header which is explicitly
> for functions like these to at least absolutely highlight this case.

Yes, and then just include it in the relevant header files.

> 
> The VMA tests need restructuring anyway, so it can be part of a bigger project
> to do some work cleaning up there.

Cool!

-- 
Cheers,

David / dhildenb