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: <20221128180252.1684965-1-jannh@google.com> <20221128180252.1684965-2-jannh@google.com>
 <CAHbLzkp7+ZrXkoYcVtqrd2mQN3FZ4Y6tyeZCd31Oubz=+esaJQ@mail.gmail.com>
 <CAG48ez0iS2BZd9BAXZLBA3D0fzNePSLWoXqAbYWsTig4nN5FrQ@mail.gmail.com>
 <CAHbLzkrAsFvUoavXrF_R4aZZm-02Fa7tF0E24TGEsAn8owbDuw@mail.gmail.com>
 <CAG48ez2CdMMOWsD3yo8-EEDc9x19Jdp6B8fJxtsRMNccF3_xCg@mail.gmail.com> <CAHbLzkrfx4TNTFaG2J4DxRT8kXvcV=0mJQ9g64eOZvofazEdEw@mail.gmail.com>
In-Reply-To: <CAHbLzkrfx4TNTFaG2J4DxRT8kXvcV=0mJQ9g64eOZvofazEdEw@mail.gmail.com>
From:   Jann Horn <jannh@google.com>
Date:   Tue, 29 Nov 2022 16:30:26 +0100
Message-ID: <CAG48ez09vFC1+0y1z=2xgZ3ZV+Peo28o5u64ZNT-iBS8xv=-4w@mail.gmail.com>
Subject: Re: [PATCH v4 2/3] mm/khugepaged: Fix GUP-fast interaction by sending IPI
To:     Yang Shi <shy828301@gmail.com>
Cc:     security@kernel.org, Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Peter Xu <peterx@redhat.com>,
        John Hubbard <jhubbard@nvidia.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 28, 2022 at 11:10 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Mon, Nov 28, 2022 at 12:12 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Mon, Nov 28, 2022 at 9:10 PM Yang Shi <shy828301@gmail.com> wrote:
> > > On Mon, Nov 28, 2022 at 11:57 AM Jann Horn <jannh@google.com> wrote:
> > > >
> > > > On Mon, Nov 28, 2022 at 8:54 PM Yang Shi <shy828301@gmail.com> wrote:
> > > > >
> > > > > On Mon, Nov 28, 2022 at 10:03 AM Jann Horn <jannh@google.com> wrote:
> > > > > >
> > > > > > Since commit 70cbc3cc78a99 ("mm: gup: fix the fast GUP race against THP
> > > > > > collapse"), the lockless_pages_from_mm() fastpath rechecks the pmd_t to
> > > > > > ensure that the page table was not removed by khugepaged in between.
> > > > > >
> > > > > > However, lockless_pages_from_mm() still requires that the page table is not
> > > > > > concurrently freed or reused to store non-PTE data. Otherwise, problems
> > > > > > can occur because:
> > > > > >
> > > > > >  - deposited page tables can be freed when a THP page somewhere in the
> > > > > >    mm is removed
> > > > > >  - some architectures store non-PTE information inside deposited page
> > > > > >    tables (see radix__pgtable_trans_huge_deposit())
> > > > > >
> > > > > > Additionally, lockless_pages_from_mm() is also somewhat brittle with
> > > > > > regards to page tables being repeatedly moved back and forth, but
> > > > > > that shouldn't be an issue in practice.
> > > > > >
> > > > > > Fix it by sending IPIs (if the architecture uses
> > > > > > semi-RCU-style page table freeing) before freeing/reusing page tables.
> > > > > >
> > > > > > As noted in mm/gup.c, on configs that define CONFIG_HAVE_FAST_GUP,
> > > > > > there are two possible cases:
> > > > > >
> > > > > >  1. CONFIG_MMU_GATHER_RCU_TABLE_FREE is set, causing
> > > > > >     tlb_remove_table_sync_one() to send an IPI to synchronize with
> > > > > >     lockless_pages_from_mm().
> > > > > >  2. CONFIG_MMU_GATHER_RCU_TABLE_FREE is unset, indicating that all
> > > > > >     TLB flushes are already guaranteed to send IPIs.
> > > > > >     tlb_remove_table_sync_one() will do nothing, but we've already
> > > > > >     run pmdp_collapse_flush(), which did a TLB flush, which must have
> > > > > >     involved IPIs.
> > > > >
> > > > > I'm trying to catch up with the discussion after the holiday break. I
> > > > > understand you switched from always allocating a new page table page
> > > > > (we decided before) to sending IPIs to serialize against fast-GUP,
> > > > > this is fine to me.
> > > > >
> > > > > So the code now looks like:
> > > > >     pmdp_collapse_flush()
> > > > >     sending IPI
> > > > >
> > > > > But the missing part is how we reached "TLB flushes are already
> > > > > guaranteed to send IPIs" when CONFIG_MMU_GATHER_RCU_TABLE_FREE is
> > > > > unset? ARM64 doesn't do it IIRC. Or did I miss something?
> > > >
> > > > From arch/arm64/Kconfig:
> > > >
> > > > select MMU_GATHER_RCU_TABLE_FREE
> > > >
> > > > CONFIG_MMU_GATHER_RCU_TABLE_FREE is not a config option that the user
> > > > can freely toggle; it is an option selected by the architecture.
> > >
> > > Aha, I see :-) BTW, shall we revert "mm: gup: fix the fast GUP race
> > > against THP collapse"? It seems not necessary anymore if this approach
> > > is used IIUC.
> >
> > Yeah, I agree.
>
> Since this patch could solve two problems: the use-after-free of the
> data page (pinned by fast-GUP) and the page table page and my patch
> will be reverted, so could you please catch both issues in this
> patch's commit log? I'd like to preserve the description of the issue
> fixed by my patch. I think that it is helpful to see the information
> about all the fixed problems in one commit instead of digging into
> another reverted commit.

OK, I will rewrite the commit message to describe the overall problem,
including the part addressed by your patch.