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