Re: [PATCH v2 1/4] mm/mremap: Optimize the start addresses in move_page_tables()

[Joel Fernandes <joel@joelfernandes.org>]

Hi Linus,

On Fri, May 19, 2023 at 3:21 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, May 19, 2023 at 12:09 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > +static bool check_addr_in_prev(struct vm_area_struct *vma, unsigned long addr,
> > +                              unsigned long mask)
> > +{
> > +       int addr_masked = addr & mask;
> > +       struct vm_area_struct *prev = NULL, *cur = NULL;
> > +
> > +       /* If the masked address is within vma, there is no prev mapping of concern. */
> > +       if (vma->vm_start <= addr_masked)
> > +               return false;
>
> Hmm.
>
> I should have caught this last time, but I didn't.
>
> That test smells bad to me. Or maybe it's just the comment.
>
> I *suspect* that the test is literally just for the stack movement
> case by execve, where it catches the case where we're doing the
> movement entirely within the one vma we set up.

Yes that's right, the test is only for the stack movement case. For
the regular mremap case, I don't think there is a way for it to
trigger.

> But in the *general* case I think the above is horribly wrong: if you
> want to move pages within an existing mapping, the page moving code
> can't just randomly say "I'll expand the area you wanted to move".
> Again, in that general mremap() case (as opposed to the special stack
> moving case for execve), I *think* that the caller has already split
> the vma's at the point of the move, and this test simply cannot ever
> trigger.

Yes, the test simply cannot ever trigger for mremap() but we still
need the test for the stack case. That's why I had considered adding
it and I had indeed reviewed the stack case when adding the test. I
could update the comment to mention that, if you want.

> So I think the _code_ works, but I think the comment in particular is
> questionable, and I'm a bit surprised about the code too,. because I
> thought execve() only expanded to exactly the moving area.

It expands to cover both the new start and the old end of the stack AFAICS:
          /*
           * cover the whole range: [new_start, old_end)
           */
          if (vma_expand(&vmi, vma, new_start, old_end, vma->vm_pgoff, NULL))
                  return -ENOMEM;

In this case, it will trigger for the stack because this same expanded
vma is passed as both the new vma and the old vma to
move_page_tables().

           */
          if (length != move_page_tables(vma, old_start,
                                         vma, new_start, length, false))
                  return -ENOMEM;

So AFAICS, it is possible that old_start will start later than this
newly expanded VMA. So for such a situation, old_start can be
realigned to PMD and the test allows that by saying it need not worry
about aligning down to an existing VMA.

> End result: I think the patch on the whole looks nice, and smaller
> than I expected. I also suspect it works in practice, but I'd like
> that test clarified. Does it *actually* trigger for the stack moving
> case? Because I think it must (never* trigger for the mremap case?

You are right that the test will not trigger for the mremap case. But
from a correctness standpoint, I thought of leaving it there for the
stack (and who knows for what other future reasons the test may be needed).
I can update the comment describing the stack if you like.

> And maybe I'm the one confused here, and all I really need is an
> explanation with small words and simple grammar starting with "No,
> Linus, this is for case xyz"

Hopefully it is clear now and you agree. Let me know if you want me to
do something more. I can make some time next week to trace the stack
case a bit more if you like and report back on any behaviors, however
the mremap tests I did are looking good and working as expected.

thanks,

 - Joel