linux-kernel - Re: [PATCH 1/2] mm: huge_memory: disallow hugepages if the system-wide THP sysfs settings are disabled

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: <E330B371-C7DC-4E79-9043-56F4AA9BBE54@nvidia.com>
Date: Thu, 29 May 2025 22:04:30 -0400
From: Zi Yan <ziy@...dia.com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc: akpm@...ux-foundation.org, hughd@...gle.com, david@...hat.com,
 lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, npache@...hat.com,
 ryan.roberts@....com, dev.jain@....com, linux-mm@...ck.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mm: huge_memory: disallow hugepages if the
 system-wide THP sysfs settings are disabled

On 29 May 2025, at 21:51, Baolin Wang wrote:

> On 2025/5/29 23:10, Zi Yan wrote:
>> On 29 May 2025, at 4:23, Baolin Wang wrote:
>>
>>> The MADV_COLLAPSE will ignore the system-wide Anon THP sysfs settings, which
>>> means that even though we have disabled the Anon THP configuration, MADV_COLLAPSE
>>> will still attempt to collapse into a Anon THP. This violates the rule we have
>>> agreed upon: never means never.
>>>
>>> To address this issue, should check whether the Anon THP configuration is disabled
>>> in thp_vma_allowable_orders(), even when the TVA_ENFORCE_SYSFS flag is set.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
>>> ---
>>>   include/linux/huge_mm.h | 23 +++++++++++++++++++----
>>>   1 file changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index 2f190c90192d..199ddc9f04a1 100644
>>> --- a/include/linux/huge_mm.h
>>> +++ b/include/linux/huge_mm.h
>>> @@ -287,20 +287,35 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
>>>   				       unsigned long orders)
>>>   {
>>>   	/* Optimization to check if required orders are enabled early. */
>>> -	if ((tva_flags & TVA_ENFORCE_SYSFS) && vma_is_anonymous(vma)) {
>>> -		unsigned long mask = READ_ONCE(huge_anon_orders_always);
>>> +	if (vma_is_anonymous(vma)) {
>>> +		unsigned long always = READ_ONCE(huge_anon_orders_always);
>>> +		unsigned long madvise = READ_ONCE(huge_anon_orders_madvise);
>>> +		unsigned long inherit = READ_ONCE(huge_anon_orders_inherit);
>>> +		unsigned long mask = always | madvise;
>>> +
>>> +		/*
>>> +		 * If the system-wide THP/mTHP sysfs settings are disabled,
>>> +		 * then we should never allow hugepages.
>>> +		 */
>>> +		if (!(mask & orders) && !(hugepage_global_enabled() && (inherit & orders)))
>>
>> Can you explain the logic here? Is it equivalent to:
>> 1. if THP is set to always, always_mask & orders == 0, or
>> 2. if THP if set to madvise, madvise_mask & order == 0, or
>> 3. if THP is set to inherit, inherit_mask & order == 0?
>>
>> I cannot figure out why (always | madvise) & orders does not check
>> THP enablement case, but inherit & orders checks hugepage_global_enabled().
>
> Sorry for not being clear. Let me try again:
>
> Now we can control per-sized mTHP through ‘huge_anon_orders_always’, so always does not need to rely on the check of hugepage_global_enabled().
>
> For madvise, referring to David's suggestion: “allowing for collapsing in a VM without VM_HUGEPAGE in the "madvise" mode would be fine", so we can just check 'huge_anon_orders_madvise' without relying on hugepage_global_enabled().

Got it. Setting always or madvise knob in per-size mTHP means user wants to
enable that size, so their orders are not limited by the global config.
Setting inherit means user wants to follow the global config.
Now it makes sense to me. I wonder if renaming inherit to inherit_global
and huge_anon_orders_inherit to huge_anon_orders_inherit_global
could make code more clear (We cannot change sysfs names, but changing
kernel variable names should be fine?).

>
> In the case where hugepage_global_enabled() is enabled, we need to check whether the 'inherit' has enabled the corresponding orders.
>
> In summary, the current strategy is:
>
> 1. If always & orders == 0, and madvise & orders == 0, and hugepage_global_enabled() == false (global THP settings are not enabled), it means mTHP of the orders are prohibited from being used, then madvise_collapse() is forbidden.
>
> 2. If always & orders == 0, and madvise & orders == 0, and hugepage_global_enabled() == true (global THP settings are enabled), and inherit & orders == 0, it means mTHP of the orders are still prohibited from being used, and thus madvise_collapse() is not allowed.

Putting the summary in the comment might be very helpful. WDYT?

Otherwise, the patch looks good to me. Thanks.

Reviewed-by: Zi Yan <ziy@...dia.com>

Best Regards,
Yan, Zi