linux-kernel - Re: [PATCH v2 1/4] mm/mremap: Optimize the start addresses in move_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEXW_YTqjGG4Y06brQthe4UMqprTJm=xk=P7i5gTpm2rZRZkXQ@mail.gmail.com>
Date:   Fri, 19 May 2023 23:56:50 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, Shuah Khan <shuah@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michal Hocko <mhocko@...e.com>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Kirill A Shutemov <kirill@...temov.name>,
        "Liam R. Howlett" <liam.howlett@...cle.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH v2 1/4] mm/mremap: Optimize the start addresses in move_page_tables()

On Fri, May 19, 2023 at 11:17 PM Joel Fernandes <joel@...lfernandes.org> wrote:
>
> Hi Linus,
>
> On Fri, May 19, 2023 at 10:34 PM Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> >
> > On Fri, May 19, 2023 at 3:52 PM Joel Fernandes <joel@...lfernandes.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