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

[Linus Torvalds]

On Fri, May 19, 2023 at 3:52 PM Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> >
> > 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.

So I feel the test is simply redundant.

For the regular mremap case, it never triggers.

And for the stack movement case by execve, I don't think it matters if
you just were to change the logic of the subsequent checks a bit.

In particular, you do this:

        /* If the masked address is within vma, there is no prev
mapping of concern. */
        if (vma->vm_start <= addr_masked)
                return false;

        /*
         * Attempt to find vma before prev that contains the address.
         * On any issue, assume the address is within a previous mapping.
         * @mmap write lock is held here, so the lookup is safe.
         */
        cur = find_vma_prev(vma->vm_mm, vma->vm_start, &prev);
        if (!cur || cur != vma || !prev)
                return true;

        /* The masked address fell within a previous mapping. */
        if (prev->vm_end > addr_masked)
                return true;

        return false;

And I think that

        if (!cur || cur != vma || !prev)
                return true;

is actively wrong, because if there is no 'prev', then you should return false.

So I *think* all of the above could just be replaced with this instead:

        find_vma_prev(vma->vm_mm, vma->vm_start, &prev);
        return prev && prev->vm_end  > addr_masked;

because only if we have a 'prev', and the prev is into that masked
address, do we need to avoid doing the masking.

With that simplified test, do you even care about that whole "the
masked address was already in the vma"? Not that I can see.

And we don't even care about the return value of 'find_vma_prev()',
because it had better be 'vma'. We're giving it 'vma->vm_start' as an
address, for chrissake!

So if you *really* wanted to, you could do something like

        cur = find_vma_prev(..);
        if (WARN_ON_ONCE(cut != vma))
                return true;

but even that WARN_ON_ONCE() seems pretty bogus. If it triggers, we
have some serious corruption going on.

So I stil find that whole "vma->vm_start <= addr_masked" test a bit
confusing, since it seems entirely redundant.

Is it just because you wanted to avoid calling "find_vma_prev()" at
all? Maybe just say that in the comment.

                  Linus