Message-ID: <ca1ca90a-7349-49e1-86d1-5db64e3e187e@kernel.org>
Date: Tue, 18 Nov 2025 08:28:11 +0100
From: "David Hildenbrand (Red Hat)" <david@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: David Wang <00107082@....com>, catalin.marinas@....com,
 lance.yang@...ux.dev, b-padhi@...com, akpm@...ux-foundation.org,
 linux-kernel@...r.kernel.org, Jan Polensky <japo@...ux.ibm.com>
Subject: Re: Linux 6.18-rc6

On 18.11.25 02:10, Linus Torvalds wrote:
> On Mon, 17 Nov 2025 at 11:17, David Hildenbrand (Red Hat)
> <david@...nel.org> wrote:
>>
>> So, I briefly tried on x86 with KASAN and the one-liner. I was assuming
>> that KASAN would complain because we are clearing the page before doing
>> the kasan_unpoison_pages() (IOW, writing to a KASAN-poisoned page).
>>
>> It didn't trigger, and I assume it is because clear_highpage() on x86
>> will not be instrumented by KASAN (my theory).
>>
>> The comment in kernel_init_pages() indicates that s390x uses memset()
>> for that purpose and I would assume that that one would be instrumented.
> 
> So I have thought about this some more, and I am not entirely happy
> about any of this, but I think the way forward is to
> 
>   (a) make tag_clear_highpage() just do multiple pages in one go (and
> rename it as tag_clear_highpage*s*() in the process)

That sounds reasonable given that the only caller we have wants to iterate.

> 
>   (b) make it have an actually return value to indicate whether it
> initialized things

Works for me.

> 
> which means that the post_alloc_hook() code just becomes
> 
>          if (zero_tags)
>                  init = tag_clear_highpages(page, 1 << order);
> 
> and then the generic fallback becomes just
> 
>    static inline bool tag_clear_highpages(struct page *page, int numpages)
>    {
>           return false;
>    }
> 
> which makes this all a complete no-op for architectures that don't do
> this memory tagging.
> 
> And the one architecture that *does* do it - arm64 - actually
> simplifies too, because now instead of being called in a loop - and
> having that
> 
>          if (!system_supports_mte()) {
>                   clear_highpage(page);
>                   return;
>          }
> 
> in every iteration of the loop, it now just gets called *once*, and it
> instead just does
> 
>          if (!system_supports_mte())
>                  return false;
> 
> and then it does the *clearing* in a loop instead.

Ack.

> 
> End result: that all looks much saner to me, and should avoid all the
> issues with KASAN (well, arm64 currently clearly depends on
> mte_zero_clear_page_tags() being assembly code that doesn't trigger
> KASAN anyway).
> 
> But maybe it looks saner to me just because I've written that code now.

:)

It should optimize out on !arm64 and optimize arm64 as well (less 
function calls for higher-order pages), so that's clearly better.

> 
> Anyway, here's my suggested patch. I still prefer this over having
> more config variables and #ifdef's. I'd much rather have code that
> just does the right thing and becomes null and void when it's
> effecitlvely disabled by not having hardware support.
> 
> Comments?

Works for me and saves me from continuing my fight with KASAN on s390x I 
started yesterday evening to find out if the one-liner would be 
problematic with KASAN poisoning.

I very much prefer to let kernel_init_pages() handle ordinary (non-tag) 
initialization after KASAN did its unpoison magic.


Do you want to quickly send that patch with linux-mm on CC or do you 
just want to commit it? If you're busy I can quickly send it around.

In any case, feel free to add my

Reviewed-by: David Hildenbrand (Red Hat) <david@...nel.org>

> 
> This is all entirely untested, but I did build it on both x86-64 and
> arm64. So it must be perfect. Right?
> 
> Side note: I really *detest* that stupid "__HAVE_ARCH_XYZ" pattern. I
> hate it. Why do people insist on that stupid pattern? We *have* a name
> already: the name of the thing that the architecture implements. Don't
> make up a new one with all caps and a __HAVE_ARCH_ prefix. If an
> architecture implements the feature "xyz", it should just do "define
> xyz xyz" and be done with it, and then code can test whether it is
> implemented by just doing "#ifdef xyz".
> 
> But I did *not* change that stupid existing pattern. I left it alone,
> and just added the 'S' since now it's multiple pages.  But I really do
> want to bring this up again, because it's so silly to make up new
> names to say "I defined that other name". Just *use* the name.

I stumbled over that just recently myself, and it's just done extremely 
inconsistently even within MM.

Maybe this one is worth spelling out in the coding style, as I was 
recently also unsure what the best practice is in the end. Let me see if 
I can find time for that.

> 
> If you implement "xyz" as a macro, you're done. And if it's
> implemented as an inline function, just add the "#define xyz xyz" to
> show that you did it.

I general, I agree if it's about real "features" that consist of a 
single function. I think it's a different story once a feature actually 
consists of multiple functions that can be cleanly abstracted in a 
config option.


-- 
Cheers

David

Powered by blists - more mailing lists