Re: [RFC PATCH 1/3] mm: introduce new .mmap_proto() f_op callback

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, May 05, 2025 at 03:29:08PM +0200, Christian Brauner wrote:
> On Wed, Apr 30, 2025 at 11:58:14PM +0200, David Hildenbrand wrote:
> > On 30.04.25 21:54, 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_proto() 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.
> > >
> > > Update vma userland test stubs to account for changes.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> >
> > I really don't like the "proto" terminology. :)
> >
> > [yes, David and his naming :P ]
> >
> > No, the problem is that it is fairly unintuitive what is happening here.
> >
> > Coming from a different direction, the callback is trigger after
> > __mmap_prepare() ... could we call it "->mmap_prepare" or something like
> > that? (mmap_setup, whatever)
> >
> > Maybe mmap_setup and vma_setup_param? Just a thought ...
> >
> >
> > In general (although it's late in Germany), it does sound like an
> > interesting approach.
> >
> > How feasiable is it to remove ->mmap in the long run, and would we maybe
> > need other callbacks to make that possible?
>
> If mm needs new file operations that aim to replace the old ->mmap() I
> want the old method to be ripped out within a reasonable time frame. I
> don't want to have ->mmap() and ->mmap_$new() hanging around for the
> next 5 years. We have enough of that already. And it would be great to
> be clear whether that replacement can actually happen.

Ack, I am committed to making these changes myself and will be putting effort
into doing this (and obviously liaising with others along these lines). And
absolutely it can, and will happen.

This has been an ongoing bugbear for me for some time, and recent issues
relating to a direct consequence of the overly permissive callback (that is,
relating to the merge retry we have to do afterwards) brought it into sharp
relief and precipitated this action.

So you can take this as a personal commitment, and you know who to shout
at if needed ;)

Cheers, Lorenzo