Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 14 Aug 2023 12:10:59 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Jann Horn <jannh@google.com>
Cc:     "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Suren Baghdasaryan <surenb@google.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 15/15] mm/mmap: Change vma iteration order in
 do_vmi_align_munmap()
Message-Id: <20230814121059.8b6046595d69284a62b876e9@linux-foundation.org>
In-Reply-To: <CAG48ez09ELhVYZftGtcxrvUaW6pF+k9RzwFtjRs-pcRx1aUweQ@mail.gmail.com>
References: <20230724183157.3939892-1-Liam.Howlett@oracle.com>
        <20230724183157.3939892-16-Liam.Howlett@oracle.com>
        <CAG48ez09ELhVYZftGtcxrvUaW6pF+k9RzwFtjRs-pcRx1aUweQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Mon, 14 Aug 2023 17:43:39 +0200 Jann Horn <jannh@google.com> wrote:

> @akpm
> 
> On Mon, Jul 24, 2023 at 8:31 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > Since prev will be set later in the function, it is better to reverse
> > the splitting direction of the start VMA (modify the new_below argument
> > to __split_vma).
> 
> It might be a good idea to reorder "mm: always lock new vma before
> inserting into vma tree" before this patch.
> 
> If you apply this patch without "mm: always lock new vma before
> inserting into vma tree", I think move_vma(), when called with a start
> address in the middle of a VMA, will behave like this:
> 
>  - vma_start_write() [lock the VMA to be moved]
>  - move_page_tables() [moves page table entries]
>  - do_vmi_munmap()
>    - do_vmi_align_munmap()
>      - __split_vma()
>        - creates a new VMA **covering the moved range** that is **not locked**
>        - stores the new VMA in the VMA tree **without locking it** [1]
>      - new VMA is locked and removed again [2]
> [...]
> 
> So after the page tables in the region have already been moved, I
> believe there will be a brief window (between [1] and [2]) where page
> faults in the region can happen again, which could probably cause new
> page tables and PTEs to be created in the region again in that window.
> (This can't happen in Linus' current tree because the new VMA created
> by __split_vma() only covers the range that is not being moved.)
> 
> Though I guess that's not going to lead to anything bad, since
> do_vmi_munmap() anyway cleans up PTEs and page tables in the region?
> So maybe it's not that important.

Thanks.  I'd of course prefer not to rebuild mm-stable.  If this ends
up being a hard-to-hit issue during git-bisect searches, I think we can
live with that.