linux-kernel - Re: [PATCH v2 1/3] hugetlb: simplify prep_compound_gigantic

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]

Date:   Tue, 10 Aug 2021 09:51:03 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Muchun Song <songmuchun@...edance.com>,
        Michal Hocko <mhocko@...e.com>,
        David Hildenbrand <david@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        Mina Almasry <almasrymina@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 1/3] hugetlb: simplify prep_compound_gigantic_page ref
 count racing code

On 8/10/21 2:29 AM, Oscar Salvador wrote:
> On 2021-08-09 20:48, Mike Kravetz wrote:
>> Code in prep_compound_gigantic_page waits for a rcu grace period if it
>> notices a temporarily inflated ref count on a tail page.  This was due
>> to the identified potential race with speculative page cache references
>> which could only last for a rcu grace period.  This is overly complicated
>> as this situation is VERY unlikely to ever happen.  Instead, just quickly
>> return an error.
>> Also, only print a warning in prep_compound_gigantic_page instead of
>> multiple callers.
> 
> The above makes sense to me.

Thanks for taking a look!

> My only question would be: gather_bootmem_prealloc() is an __init function.
> Can we have speculative refcounts due to e.g: pagecache at that early stage?
> I think we cannot, but I am not really sure.

I had the same thought when adding that code.  We cannot have a
speculative refcount this early after boot.  However, further
thinking below...

> 
> We might be able to remove that else() in case we cannot have such scenarios.
> 

Instead of removing the else, I think we should put a BUG_ON() just to
make sure the condition never happens in the future.  Otherwise, we would
just abandon the gigantic page and leave memory (1GB or so) unavailable.

Even if it were possible to have speculative references this early in
the boot process, the likelihood of it happening here is still VERY
small.  So, I would not expect a BUG_ON() to ever be hit in development or
testing.  Rather, with our luck it would show up in some production
environment.

Since handling the race in this routine is so simple, I chose to just
add the code to handle it.
-- 
Mike Kravetz