Re: [PATCH v1 0/2] mremap refactor: check src address for vma boundaries first.

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Jeff Xu <jeffxu@chromium.org> [240814 12:57]:
> On Wed, Aug 14, 2024 at 7:40 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * jeffxu@chromium.org <jeffxu@chromium.org> [240814 03:14]:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > mremap doesn't allow relocate, expand, shrink across VMA boundaries,
> > > refactor the code to check src address range before doing anything on
> > > the destination, i.e. destination won't be unmapped, if src address
> > > failed the boundaries check.
> > >
> > > This also allows us to remove can_modify_mm from mremap.c, since
> > > the src address must be single VMA, can_modify_vma is used.
> >
> > I don't think sending out a separate patch to address the same thing as
> > the patch you said you were testing [1] is the correct approach.  You
> > had already sent suggestions on mremap changes - why send this patch set
> > instead of making another suggestion?
> >
> As indicated in the cover letter, this patch aims to improve mremap
> performance while preserving existing mseal's semantics.

They are not worth preserving.

> And this
> patch can go in-dependantly regardless of in-loop out-loop discussion.

No, it conflicts with the other mremap patch as it changes the same
code - in a very similar way.

> 
> [1] link in your email is broken, but I assume you meant Pedro's V1/V2
> of in-loop change.

Yes, the email where you delayed discussing the fix so that you could
test it.  Which brings up the question you didn't answer and deleted:
Does your testing pass on those patches?

> In-loop change has a semantic/regression risk to
> mseal, and will take longer time to review/test/prove and bake.

There are no uses, so the risk is minimal.

> We can leave in-loop discussion in Pedro's thread,

No, it is directly linked to these patches as this should have just been
a comment on a patch in that series.

> I hope the V3 of
> Pedro's patch adds more testing coverage and addresses existing
> comments in V2.

The majority of the comments to V2 are mine, you only told us that
splitting a sealed vma is wrong (after I asked you directly to answer)
and then you made a comment about testing of the patch set. Besides the
direct responses to me, your comment was "wait for me to test".

You are holding us hostage by asking for more testing but not sharing
what is and is not valid for mseal() - or even answering questions on
tests you run.  Splitting a vma doesn't change the memory, but that's
not allowed for some reason.

These patches should be rejected in favour of fixing the feature like it
should have been written in the first place.  Anything less is just to
simplify backports and avoiding testing - "avoiding the business logic".

Liam

[1] https://lore.kernel.org/all/CALmYWFvURJBgyFw7x5qrL4CqoZjy92NeFAS750XaLxO7o7Cv9A@mail.gmail.com/