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

[Joel Fernandes <joel@joelfernandes.org>]

On Fri, May 19, 2023 at 11:17 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> Hi Linus,
>
> On Fri, May 19, 2023 at 10:34 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Fri, May 19, 2023 at 3:52 PM Joel Fernandes <joel@joelfernandes.org> 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.
>
> Unfortunately, I just found that mremap-ing a range purely within a
> VMA can actually cause the old and new VMA passed to
> move_page_tables() to be the same.
>
> I added a printk to the beginning of move_page_tables that prints all the args:
> printk("move_page_tables(vma=(%lx,%lx), old_addr=%lx,
> new_vma=(%lx,%lx), new_addr=%lx, len=%lx)\n", vma->vm_start,
> vma->vm_end, old_addr, new_vma->vm_start, new_vma->vm_end, new_addr,
> len);
>
> Then I wrote a simple test to move 1MB purely within a 10MB range and
> I found on running the test that the old and new vma passed to
> move_page_tables() are exactly the same.
>
> [   19.697596] move_page_tables(vma=(7f1f985f7000,7f1f98ff7000),
> old_addr=7f1f987f7000, new_vma=(7f1f985f7000,7f1f98ff7000),
> new_addr=7f1f98af7000, len=100000)
>
> That is a bit counter intuitive as I really thought we'd be splitting
> the VMAs with such a move. Any idea what am I missing?
>
> Also, such a usecase will break with my patch as we may accidentally
> overwrite parts of a range that were not part of the mremap request.
> Maybe I should just turn off the optimization if vma == new_vma,
> however that will also turn it off for the stack move so then maybe
> another way is to special case stack moves in move_page_tables().
>
> So this means I have to go back to the drawing board a bit on this
> patch, and also add more tests in mremap_test.c to test such
> within-VMA moving. I believe there are no such existing tests... More
> work to do for me. :-)

I also realize that I don't really need to check whether the masked
source address falls under a VMA neighboring to that of the source's.
I only need to do so for the destination. And for the destination
masked address, I need to forbid the optimization if after masking,
the destination addr will fall within *any* mapping whether it is its
own or a neighbor one. Since we cannot afford to corrupt those. I
believe that will also take care of both the intra-VMA moves as well
as the stack usecase. And also cut down one of the two find_vma_prev()
calls.

I will rewrite the patch to address these soon. Thanks for patience
and all the comments,

Thanks!

 - Joel