Message-ID: <9f479d46-cf06-4dfe-ac26-21fce0aafa06@lucifer.local>
Date: Fri, 9 May 2025 11:45:31 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
        Jann Horn <jannh@...gle.com>, Pedro Falcato <pfalcato@...e.de>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH 1/3] mm: introduce new .mmap_prepare() file callback

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@...cle.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.

Powered by blists - more mailing lists