Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 15 Jun 2021 13:40:28 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Jann Horn <jannh@google.com>
Cc:     Linux-MM <linux-mm@kvack.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Youquan Song <youquan.song@intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Jan Kara <jack@suse.cz>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        John Hubbard <jhubbard@nvidia.com>,
        "Kirill A. Shutemov" <kirill@shutemov.name>
Subject: Re: page refcount race between prep_compound_gigantic_page() and
 __page_cache_add_speculative()?
Message-ID: <YMifvD723USsnWRH@casper.infradead.org>
References: <CAG48ez23q0Jy9cuVnwAe7t_fdhMk2S7N5Hdi-GLcCeq5bsfLxw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAG48ez23q0Jy9cuVnwAe7t_fdhMk2S7N5Hdi-GLcCeq5bsfLxw@mail.gmail.com>
Precedence: bulk

On Tue, Jun 15, 2021 at 01:03:53PM +0200, Jann Horn wrote:
> The messier path, as the original commit describes, is "gigantic" page
> allocation. In that case, we'll go through the following path (if we
> ignore CMA):
> 
>   alloc_fresh_huge_page():
>     alloc_gigantic_page()
>       alloc_contig_pages()
>         __alloc_contig_pages()
>           alloc_contig_range()
>             isolate_freepages_range()
>               split_map_pages()
>                 post_alloc_hook() [FOR EVERY PAGE]
>                   set_page_refcounted()
>                     set_page_count(page, 1)
>     prep_compound_gigantic_page()
>       set_page_count(p, 0) [FOR EVERY TAIL PAGE]
> 
> so all the tail pages are initially allocated with refcount 1 by the
> page allocator, and then we overwrite those refcounts with zeroes.
> 
> 
> Luckily, the only non-__init codepath that can get here is
> __nr_hugepages_store_common(), which is only invoked from privileged
> writes to sysfs/sysctls.

Argh.  What if we passed __GFP_COMP into alloc_contig_pages()?
The current callers of alloc_contig_range() do not pass __GFP_COMP,
so it's no behaviour change for them, and __GFP_COMP implies this
kind of behaviour.  I think that would imply _not_ calling
split_map_pages(), which implies not calling post_alloc_hook(),
which means we probably need to do a lot of the parts of
post_alloc_hook() in alloc_gigantic_page().  Yuck.