Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220831083024.37138-1-david@redhat.com>
In-Reply-To: <20220831083024.37138-1-david@redhat.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Wed, 31 Aug 2022 10:55:43 -0700
Message-ID: <CAHbLzkqeDAnCdt3q4E2RZw64QEzVaO_pseR3VaoHUhB+rZFcZQ@mail.gmail.com>
Subject: Re: [PATCH v1] mm/ksm: update stale comment in write_protect_page()
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Jason Gunthorpe <jgg@nvidia.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Hugh Dickins <hughd@google.com>, Peter Xu <peterx@redhat.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 31, 2022 at 1:30 AM David Hildenbrand <david@redhat.com> wrote:
>
> The comment is stale, because a TLB flush is no longer sufficient and
> required to synchronize against concurrent GUP-fast. This used to be true
> in the past, whereby a TLB flush would have implied an IPI on architectures
> that support GUP-fast, resulting in GUP-fast that disables local interrupts
> from completing before completing the flush.

Hmm... it seems there might be problem for THP collapse IIUC. THP
collapse clears and flushes pmd before doing anything on pte and
relies on interrupt disable of fast GUP to serialize against fast GUP.
But if TLB flush is no longer sufficient, then we may run into the
below race IIUC:

         CPU A                                                CPU B
THP collapse                                             fast GUP

gup_pmd_range() <-- see valid pmd

gup_pte_range() <-- work on pte
clear pmd and flush TLB
__collapse_huge_page_isolate()
    isolate page <-- before GUP bump refcount

   pin the page
__collapse_huge_page_copy()
    copy data to huge page
    clear pte (don't flush TLB)
Install huge pmd for huge page

return the obsolete page


>
> However, ever since general RCU GUP-fast was introduced in
> commit 2667f50e8b81 ("mm: introduce a general RCU get_user_pages_fast()"),
> this handling no longer applies in general. RCU primarily prevents page
> tables from getting freed while they might still get walked by GUP-fast,
> but we can race with GUP-fast after clearing the PTE and flushing the
> TLB.
>
> Nowadays, we can see a refcount change from GUP-fast at any point in
> time. However, GUP-fast detects concurrent PTE changes by looking up the
> PTE, temporarily grabbing a reference, and dropping the reference again if
> the PTE changed in the meantime.
>
> An explanation by Jason Gunthorpe regarding how existing memory barriers
> should be fine to make sure that concurrent GUP-fast cannot succeed in
> grabbing a reference with write permissions after we cleared the PTE and
> flushed the TLB can be found at [1].
>
> Note that clearing PageAnonExclusive via page_try_share_anon_rmap()
> might still need some explicit memory barriers to not have any possible
> races with RCU GUP-fast.
>
> [1] https://lkml.kernel.org/r/Yw5rwIUPm49XtqOB@nvidia.com
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/ksm.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 42ab153335a2..e88291f63461 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1072,23 +1072,20 @@ static int write_protect_page(struct vm_area_struct *vma, struct page *page,
>                 swapped = PageSwapCache(page);
>                 flush_cache_page(vma, pvmw.address, page_to_pfn(page));
>                 /*
> -                * Ok this is tricky, when get_user_pages_fast() run it doesn't
> -                * take any lock, therefore the check that we are going to make
> -                * with the pagecount against the mapcount is racy and
> -                * O_DIRECT can happen right after the check.
> -                * So we clear the pte and flush the tlb before the check
> -                * this assure us that no O_DIRECT can happen after the check
> -                * or in the middle of the check.
> -                *
> -                * No need to notify as we are downgrading page table to read
> -                * only not changing it to point to a new page.
> +                * Especially if we're downgrading protection, make sure to
> +                * flush the TLB now. No need to notify as we are not changing
> +                * the PTE to point at a different page.
>                  *
>                  * See Documentation/mm/mmu_notifier.rst
>                  */
>                 entry = ptep_clear_flush(vma, pvmw.address, pvmw.pte);
> +
>                 /*
> -                * Check that no O_DIRECT or similar I/O is in progress on the
> -                * page
> +                * Make sure that there are no unexpected references (e.g.,
> +                * concurrent O_DIRECT). Note that while concurrent GUP-fast
> +                * could raise the refcount temporarily to grab a write
> +                * reference, it will observe the changed PTE and drop that
> +                * temporary reference again.
>                  */
>                 if (page_mapcount(page) + 1 + swapped != page_count(page)) {
>                         set_pte_at(mm, pvmw.address, pvmw.pte, entry);
> --
> 2.37.1
>
>