Re: [PATCH 1/3] mm: introduce new .mmap_prepare() file callback
From: David Hildenbrand
Date: Fri May 09 2025 - 06:00:57 EST
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@xxxxxxxxxx>
---
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