Message-ID: <CAHk-=wjBszCmj5A3cQ4PBB=x9BR58hyoQqfVNef48v3N=4Z3mQ@mail.gmail.com>
Date: Mon, 17 Nov 2025 17:10:50 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: "David Hildenbrand (Red Hat)" <david@...nel.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 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)

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

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.

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.

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?

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.

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.

Don't make up new names that only makes it harder to grep for things,
and makes things pointlessly have two different names.

Please.

              Linus

View attachment "patch.diff" of type "text/x-patch" (3536 bytes)

Powered by blists - more mailing lists