linux-kernel - Re: [PATCH mm-unstable v5 01/10] mm: add folio dtor and order setter functions

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]

Message-Id: <3F95112C-C1B3-4774-9E21-998CADEC38D4@linux.dev>
Date:   Thu, 8 Dec 2022 10:19:24 +0800
From:   Muchun Song <muchun.song@...ux.dev>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Sidhartha Kumar <sidhartha.kumar@...cle.com>,
        linux-kernel@...r.kernel.org,
        Linux Memory Management List <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Muchun Song <songmuchun@...edance.com>,
        Matthew Wilcox <willy@...radead.org>,
        Mina Almasry <almasrymina@...gle.com>,
        Miaohe Lin <linmiaohe@...wei.com>, hughd@...gle.com,
        tsahu@...ux.ibm.com, jhubbard@...dia.com,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH mm-unstable v5 01/10] mm: add folio dtor and order setter
 functions



> On Dec 8, 2022, at 03:25, Mike Kravetz <mike.kravetz@...cle.com> wrote:
> 
> On 12/07/22 11:05, Sidhartha Kumar wrote:
>> On 12/7/22 10:49 AM, Sidhartha Kumar wrote:
>>> On 12/7/22 10:12 AM, Mike Kravetz wrote:
>>>> On 12/07/22 12:11, Muchun Song wrote:
>>>>>> On Dec 7, 2022, at 11:42, Mike Kravetz <mike.kravetz@...cle.com> wrote:
>>>>>> On 12/07/22 11:34, Muchun Song wrote:
>>>>> 
>>>>> Agree. It has confused me a lot. I suggest changing the code to the
>>>>> followings. The folio_test_large() check is still to avoid unexpected
>>>>> users for OOB.
>>>>> 
>>>>> static inline void folio_set_compound_order(struct folio *folio,
>>>>>                         unsigned int order)
>>>>> {
>>>>>     VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
>>>>>     // or
>>>>>     // if (!folio_test_large(folio))
>>>>>     //     return;
>>>>> 
>>>>>     folio->_folio_order = order;
>>>>> #ifdef CONFIG_64BIT
>>>>>     folio->_folio_nr_pages = order ? 1U << order : 0;
>>>>> #endif
>>>>> }
>>>> 
>>>> I think the VM_BUG_ON_FOLIO is appropriate as it would at least flag
>>>> data corruption.
>>>> 
>>> As Mike pointed out, my intention with supporting the 0 case was to
>>> cleanup the __destroy_compound_gigantic_page code by moving the ifdef
>>> CONFIG_64BIT lines to folio_set_compound_order(). I'll add the
>>> VM_BUG_ON_FOLIO line as well as a comment to make it clear it is not
>>> normally supported.
>>> 
>>>> Thinking about this some more, it seems that hugetlb is the only caller
>>>> that abuses folio_set_compound_order (and previously set_compound_order)
>>>> by passing in a zero order.  Since it is unlikely that anyone knows of
>>>> this abuse, it might be good to add a comment to the routine to note
>>>> why it handles the zero case.  This might help prevent changes which
>>>> would potentially break hugetlb.
>>> 
>>> +/*
>>> + * _folio_nr_pages and _folio_order are invalid for
>>> + * order-zero pages. An exception is hugetlb, which passes
>>> + * in a zero order in __destroy_compound_gigantic_page().
>>> + */
>>>  static inline void folio_set_compound_order(struct folio *folio,
>>>                 unsigned int order)
>>>  {
>>> +       VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
>>> +
>>>         folio->_folio_order = order;
>>>  #ifdef CONFIG_64BIT
>>>         folio->_folio_nr_pages = order ? 1U << order : 0;
>>> 
>>> Does this comment work?
>>> 
>>> 
>> 
>> I will change the comment from referencing
>> __destory_compound_gigantic_page()
>> to __destroy_compound_gigantic_folio, although
>> __prep_compound_gigantic_folio() is another user of
>> folio_set_compound_order(folio, 0). Should the sentence just be "An
>> exception is hugetlb, which passes in a zero order"?
> 
> How about a comment like this?
> 
> /*
> * folio_set_compound_order is generally passed a non-zero order to
> * set up/create a large folio.  However, hugetlb code abuses this by
> * passing in zero when 'dissolving' a large folio.
> */

How about adding a new helper like "folio_dissolve_compound(struct folio *folio)"?
then it may be unnecessary to add a comment.

Thanks.

> 
> My only concern is that someone may modify the routine such that it no
> longer works when passed zero order.  It is not likely as anyone should
> notice the special case for zero, and look for callers.
> -- 
> Mike Kravetz