linux-kernel - Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped

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: <d562572c-6b0d-443f-bcae-b862338fa7f7@redhat.com>
Date: Mon, 21 Apr 2025 09:17:37 +0200
From: David Hildenbrand <david@...hat.com>
To: Lance Yang <lance.yang@...ux.dev>,
 Andrew Morton <akpm@...ux-foundation.org>
Cc: mingzhe.yang@...com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 Lance Yang <ioworker0@...il.com>
Subject: Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for
 folio_test_large_maybe_mapped_shared()

On 21.04.25 07:16, Lance Yang wrote:
> April 21, 2025 at 1:13 PM, "Lance Yang" <lance.yang@...ux.dev> wrote:
> 
> 
> 
>>
>> April 21, 2025 at 7:29 AM, "Andrew Morton" <akpm@...ux-foundation.org> wrote:
>>
>>>
>>> On Fri, 18 Apr 2025 23:22:28 +0800 Lance Yang <ioworker0@...il.com> wrote:
>>>
>>>   
>>>
>>>   
>>>
>>>   From: Lance Yang <lance.yang@...ux.dev>
>>>
>>>   
>>>
>>>   
>>>
>>>   
>>>
>>>   To prevent folio_test_large_maybe_mapped_shared() from being used without
>>>
>>>   
>>>
>>>   CONFIG_MM_ID, we add a compile-time check rather than wrapping it in
>>>
>>>   
>>>
>>>   '#ifdef', avoiding even more #ifdef in callers that already use
>>>
>>>   
>>>
>>>   IS_ENABLED(CONFIG_MM_ID).
>>>
>>>   
>>>
>>>   
>>>
>>>   
>>>
>>>   Also, we used plenty of IS_ENABLED() on purpose to keep the code free of
>>>
>>>   
>>>
>>>   '#ifdef' mess.
>>>
>>>   
>>>
>>>   
>>>
>>>   I dunno, this just seems really whacky.
>>>
>>
>> I'd hope David could leave some comments on that.
>>
>>>
>>> --- a/include/linux/page-flags.h
>>>
>>>   
>>>
>>>   +++ b/include/linux/page-flags.h
>>>
>>>   
>>>
>>>   @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio)
>>>
>>>   
>>>
>>>   
>>>
>>>   
>>>
>>>   static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio)
>>>
>>>   
>>>
>>>   {
>>>
>>>   
>>>
>>>   + /* This function should never be called without CONFIG_MM_ID enabled. */
>>>
>>>   
>>>
>>>   
>>>
>>>   A correcter comment would be "This function should never be compiled
>>>
>>>   
>>>
>>>   without CONFIG_MM_ID enabled".
>>>
>>
>> Yes, that is more exact ;)
>>
>>>
>>> Which lets the cat out of the bag. Why the heck is it being compiled
>>>
>>>   
>>>
>>>   with CONFIG_MM_ID=n?? We have tools to prevent that.
>>>
>>>   
>>>
>>>   Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It
>>>
>>>   
>>>
>>>   should have no callers, right? If the linker ends up complaining then
>>>
>>>   
>>>
>>>   something went wrong.
>>>
>>
>> The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared()
>>
>> is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID).
> 
> static inline bool folio_maybe_mapped_shared(struct folio *folio)
> {
>          [...]
>          if (!IS_ENABLED(CONFIG_MM_ID))
>                  return true;
>          [...]
>          return folio_test_large_maybe_mapped_shared(folio);
> }
> 
> folio_maybe_mapped_shared() is always available - doesn't depend on
> CONFIG_MM_ID or CONFIG_TRANSPARENT_HUGEPAGE.

We could #ifdef in folio_maybe_mapped_shared(), which I find rather 
suboptimal ...

or simply inline it into the 4 callers.

That might be the best approach, given that only selected user should be 
using the low-level primitive and everybody else should be using 
folio_maybe_mapped_shared().

-- 
Cheers,

David / dhildenb