On 8/22/25 7:08 PM, Dave Hansen wrote:
On 8/21/25 18:11, Harry Yoo wrote:
On Thu, Aug 21, 2025 at 10:36:12AM -0700, Dave Hansen wrote:
On 8/21/25 04:57, Harry Yoo wrote:
However, {pgd,p4d}_populate_kernel() is defined as a function regardless of the number of page table levels, so the compiler may not optimize them away. In this case, the following linker error occurs:
Hi, thanks for taking a look, Dave!
First of all, this is a fix-up patch of a mm-hotfixes patch series that fixes a bug (I should have explained that in the changelog) [1].
[1] https://lore.kernel.org/linux-mm/20250818020206.4517-1-harry.yoo@oracle.com
I think we can continue discussing it and perhaps do that as part of a follow-up series, because the current patch series need to be backported to -stable and your suggestion to improve existing code doesn't require -stable backports.
Does that sound fine?
This part of the changelog confused me. I think it's focusing on the wrong thing.
The code that's triggering this is literally:
pgd_populate(&init_mm, pgd, lm_alias(kasan_early_shadow_p4d));It sure _looks_ like it's unconditionally referencing the 'kasan_early_shadow_p4d' symbol. I think it's wrong to hide that with macro magic and just assume that the macros won't reference it.
If a symbol isn't being defined, it shouldn't be referenced in C code.:q
A fair point, and that's what KASAN code has been doing for years.
The right way to do it is to have an #ifdef in a header that avoids compiling in the reference to the symbol.
You mean defining some wrapper functions for p*d_populate_kernel() in KASAN with different implementations based on ifdeffery?
That would work.
So would something like:
#if CONFIG_PGTABLE_LEVELS >= 4 extern p4d_t kasan_early_shadow_p4d[MAX_PTRS_PER_P4D]; #else #define kasan_early_shadow_p4d NULL #endif
This won't work. It will fix the linker error, but will introduce runtime bug instead:
lm_alias(kasan_early_shadow_p4d) -> __va(__phys_addr_symbol(NULL))
On arm64:
phys_addr_t __phys_addr_symbol(unsigned long x) VIRTUAL_BUG_ON(x < (unsigned long) KERNEL_START || x > (unsigned long) KERNEL_END);
And NULL is < KERNEL_START.
Since __phys_addr_symbol() isn't pure or const, compiler has no right to eliminate such call even though the return value is unused.