linux-kernel - Re: [External] [PATCH 3/3] hugetlb: before freeing hugetlb page set dtor to appropriate value

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

Date:   Wed, 14 Jul 2021 10:39:25 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Muchun Song <songmuchun@...edance.com>
Cc:     Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Michal Hocko <mhocko@...e.com>,
        Oscar Salvador <osalvador@...e.de>,
        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: [External] [PATCH 3/3] hugetlb: before freeing hugetlb page set
 dtor to appropriate value

On 7/14/21 3:57 AM, Muchun Song wrote:
> On Sat, Jul 10, 2021 at 8:25 AM Mike Kravetz <mike.kravetz@...cle.com> wrote:
>> +       /*
>> +        * Very subtle
>> +        *
>> +        * For non-gigantic pages set the destructor to the normal compound
>> +        * page dtor.  This is needed in case someone takes an additional
>> +        * temporary ref to the page, and freeing is delayed until they drop
>> +        * their reference.
>> +        *
>> +        * For gigantic pages set the destructor to the null dtor.  This
>> +        * destructor will never be called.  Before freeing the gigantic
>> +        * page destroy_compound_gigantic_page will turn the compound page
>> +        * into a simple group of pages.  After this the destructor does not
>> +        * apply.
>> +        *
>> +        * This handles the case where more than one ref is held when and
>> +        * after update_and_free_page is called.
>> +        */
>>         set_page_refcounted(page);
>> -       set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
>> +       if (hstate_is_gigantic(h))
>> +               set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
>> +       else
>> +               set_compound_page_dtor(page, COMPOUND_PAGE_DTOR);
> 
> Hi Mike,
> 
> The race is really subtle. But we also should remove the WARN from
> free_contig_range, right? Because the refcount of the head page of
> the gigantic page can be greater than one, but free_contig_range has
> the following warning.
> 
> WARN(count != 0, "%lu pages are still in use!\n", count);
> 

I did hit that warning in my testing and thought about removing it.
However, I decided to keep it because non-hugetlb code also makes use of
alloc_contig_range/free_contig_range and it might be useful in those
cases.

My 'guess' is that the warning was added not because of temporary ref
count increases but rather to point out any code that forgot to drop a
reference.

BTW - It is not just the 'head' page which could trigger this warning, but
any 'tail' page as well.  That is because we do not call free_contig_range
with a compound page, but rather a group of pages all with ref count of
at least one.

I'm happy to remove the warning if people do not think it is generally
useful.
-- 
Mike Kravetz