Re: [PATCH v4 4/5] mm/mseal: simplify and rename VMA gap check

[Pedro Falcato <pfalcato@suse.de>]

On Fri, Jul 25, 2025 at 11:09:13AM -0700, Jeff Xu wrote:
> Hi Lorenzo
> 
> On Fri, Jul 25, 2025 at 10:43 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Fri, Jul 25, 2025 at 10:30:08AM -0700, Jeff Xu wrote:
> > > Hi Lorenzo,
> > >
> > > On Fri, Jul 25, 2025 at 1:30 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > The check_mm_seal() function is doing something general - checking whether
> > > > a range contains only VMAs (or rather that it does NOT contain any
> > > > unmapped regions).
> > > >
> > > > So rename this function to range_contains_unmapped().
> > > >
> > > Thanks for keeping the comments.
> >
> > You're welcome.
> >
> > >
> > > In the prior version of this patch, I requested that we keep the
> > > check_mm_seal()  and its comments. And this version keeps the comments
> > > but removes the check_mm_seal() name.
> >
> > I didn't catch that being your request.
> >
> > >
> > > As I said, check_mm_seal() with its comments is a contract for
> > > entry-check for mseal().  My understanding is that you are going to
> > > move range_contains_unmapped() to vma.c. When that happens, mseal()
> > > will lose this entry-check contract.
> >
> > This is just bizarre.
> >
> > Code doesn't stop working if you put it in another function.
> >
> > And you're now reviewing me for stuff I haven't done? :P
> >
> > >
> > > Contact is a great way to hide implementation details. Could you
> > > please keep check_mm_seal() in mseal.c and create
> > > range_contains_unmapped() in vma.c. Then you can refactor as needed.
> >
> > Wait what?
> >
> > OK maybe now I see what you mean, you want a function that just wraps
> > range_contains_unmapped() with a comment explaining the 'contract'.
> >
> Yes. You can view it that way from an implementation point of view.
> 
> Contract mainly serves as a way to help design and abstract the code.
>

What code? This is an extremely simple file. We don't need deep design
and abstractions here.

> > range_contains_unmapped() enforces your required contract and the comments
> > make it extremely explicit, so this is not a reasonable request, sorry.
> 
> Technically, this contract belongs to mseal, but if you have strong
> opinions on this, that's fine, as long as range_contains_unmapped()
> doesn't accidentally remove those comments in the future, which I'm
> sure you won't.
> 

As far as I'm concerned, mseal() has little to no contract - we still don't have
a solid definition of what mseal() is supposed to do, things are still fluctuating,
and there's no man page (and no one is going to look into random kernel comments
for this).

FTR: I entirely plan on axing this function in the near future (or will try to).
There's no valid reason for this to exist, and it's causing extra burden on the
implementation - besides serving as a poor example for future mmap-ish syscalls.

-- 
Pedro