linux-kernel - Re: page refcount race between prep_compound_gigantic_page() and __page_cache_add

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] [day] [month] [year] [list]

Message-ID: <41f698d6-a099-105d-e922-170fbb3e1798@oracle.com>
Date:   Tue, 15 Jun 2021 11:27:14 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Matthew Wilcox <willy@...radead.org>, Jann Horn <jannh@...gle.com>
Cc:     Linux-MM <linux-mm@...ck.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Youquan Song <youquan.song@...el.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Jan Kara <jack@...e.cz>, John Hubbard <jhubbard@...dia.com>,
        "Kirill A. Shutemov" <kirill@...temov.name>
Subject: Re: page refcount race between prep_compound_gigantic_page() and
 __page_cache_add_speculative()?

On 6/15/21 5:40 AM, Matthew Wilcox wrote:
> 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.

Thanks for spotting this Jann!

> 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.

That might work.  We would need to do something 'like' split_map_pages
to split the compound free pages in the allocated range.  Then, stitch
them together into one big compound page.  We 'should' be able to call
post_alloc_hook on the resulting big compound page.  Of course, that is
all theory without digging into the details.

Note that in the general case alloc_contig_range/alloc_contig_pages can
be called to request a non-power of two number of pages.  In such cases
__GFP_COMP would make little sense.
-- 
Mike Kravetz