[PATCH V5 mm-hotfixes 0/3] mm, x86: fix crash due to missing page table sync and make it harder to miss

[PATCH V5 mm-hotfixes 0/3] mm, x86: fix crash due to missing page table sync and make it harder to miss

[Harry Yoo]

To x86 folks:
It's not clear whether this should go through the MM tree or the x86
tree as it changes both. We could send it to the MM tree with Acks
from the x86 folks, or we could send it through the x86 tree instead.
What do you think?

This patch series includes only minimal changes necessary for
backporting the fix to -stable. Planned follow-up patches:
- treewide: include linux/pgalloc.h instead of asm/pgalloc.h
  in common code
- MAINTAINERS: add include/linux/pgalloc.h to MM CORE
- x86/mm/64: convert p*d_populate{,_init} to _kernel variants
- x86/mm/64: drop unnecessary calls to sync_global_pgds() and
  fold it into its sole user

v4 -> v5 (Only cosmetic changes in comments and commit messages):
- Updated comment in PGTBL_*_MODIFIED that I missed in the last version. 
- Added Acked-by, Reviewed-by tags (thanks Kiryl, Mike, Lorenzo and Ulad!)
- Updated commit messages of patch 2 and 3 (Lorenzo)
- Added a comment in arch_sync_kernel_mappings() (Lorenzo)
- Rebased onto the latest mm-hotfixes-unstable (e4321cf73f53)

$ git range-diff \
  rework-sync-kernel-pagetables-v4~3..rework-sync-kernel-pagetables-v4 \
  rework-sync-kernel-pagetables-v5~3..rework-sync-kernel-pagetables-v5

1:  003cb4e564d9 ! 1:  79c2286f7739 mm: move page table sync declarations to linux/pgtable.h
    @@ Commit message
     
         Cc: <stable@vger.kernel.org>
         Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
    +    Acked-by: Kiryl Shutsemau <kas@kernel.org>
    +    Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
    +    Reviewed-by: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
    +    Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
         Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
     
      ## include/linux/pgtable.h ##
2:  7493cc93874d ! 2:  c3c34f3ed699 mm: introduce and use {pgd,p4d}_populate_kernel()
    @@ Commit message
         and the actual synchronization is performed by arch_sync_kernel_mappings().
     
         This change currently targets only x86_64, so only PGD and P4D level
    -    helpers are introduced. In theory, PUD and PMD level helpers can be added
    -    later if needed by other architectures.
    +    helpers are introduced. Currently, these helpers are no-ops since no
    +    architecture sets PGTBL_{PGD,P4D}_MODIFIED in ARCH_PAGE_TABLE_SYNC_MASK.
     
    -    Currently this is a no-op, since no architecture sets
    -    PGTBL_{PGD,P4D}_MODIFIED in ARCH_PAGE_TABLE_SYNC_MASK.
    +    In theory, PUD and PMD level helpers can be added later if needed by
    +    other architectures. For now, 32-bit architectures (x86-32 and arm) only
    +    handle PGTBL_PMD_MODIFIED, so p*d_populate_kernel() will never affect
    +    them unless we introduce a PMD level helper.
     
         Cc: <stable@vger.kernel.org>
         Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
         Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
    +    Acked-by: Kiryl Shutsemau <kas@kernel.org>
    +    Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
    +    Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
         Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
     
      ## include/linux/pgalloc.h (new) ##
    @@ include/linux/pgtable.h: static inline void modify_prot_commit_ptes(struct vm_ar
       */
      #ifndef ARCH_PAGE_TABLE_SYNC_MASK
      #define ARCH_PAGE_TABLE_SYNC_MASK 0
    +@@ include/linux/pgtable.h: static inline bool arch_has_pfn_modify_check(void)
    + /*
    +  * Page Table Modification bits for pgtbl_mod_mask.
    +  *
    +- * These are used by the p?d_alloc_track*() set of functions an in the generic
    +- * vmalloc/ioremap code to track at which page-table levels entries have been
    +- * modified. Based on that the code can better decide when vmalloc and ioremap
    +- * mapping changes need to be synchronized to other page-tables in the system.
    ++ * These are used by the p?d_alloc_track*() and p*d_populate_kernel()
    ++ * functions in the generic vmalloc, ioremap and page table update code
    ++ * to track at which page-table levels entries have been modified.
    ++ * Based on that the code can better decide when page table changes need
    ++ * to be synchronized to other page-tables in the system.
    +  */
    + #define		__PGTBL_PGD_MODIFIED	0
    + #define		__PGTBL_P4D_MODIFIED	1
     
      ## mm/kasan/init.c ##
     @@
3:  f199138e40e6 ! 3:  4ad9ce5c09c2 x86/mm/64: define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings()
    @@ Commit message
         x86/mm/64: define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings()
     
         Define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings() to ensure
    -    page tables are properly synchronized when calling p*d_populate_kernel().
    -    It is inteneded to synchronize page tables via pgd_pouplate_kernel() when
    -    5-level paging is in use and via p4d_pouplate_kernel() when 4-level paging
    -    is used.
    +    page tables are properly synchronized when calling
    +    p*d_populate_kernel().
    +
    +    For 5-level paging, synchronization is performed via
    +    pgd_populate_kernel(). In 4-level paging, pgd_populate() is a no-op,
    +    so synchronization is instead performed at the P4D level via
    +    p4d_populate_kernel().
     
         This fixes intermittent boot failures on systems using 4-level paging
         and a large amount of persistent memory:
    @@ Commit message
         Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
         Closes: https://lore.kernel.org/linux-mm/20250311114420.240341-1-gwan-gyeong.mun@intel.com [1]
         Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
    +    Acked-by: Kiryl Shutsemau <kas@kernel.org>
    +    Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
    +    Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
         Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
     
      ## arch/x86/include/asm/pgtable_64_types.h ##
    @@ arch/x86/mm/init_64.c: static void sync_global_pgds(unsigned long start, unsigne
      		sync_global_pgds_l4(start, end);
      }
      
    ++/*
    ++ * Make kernel mappings visible in all page tables in the system.
    ++ * This is necessary except when the init task populates kernel mappings
    ++ * during the boot process. In that case, all processes originating from
    ++ * the init task copies the kernel mappings, so there is no issue.
    ++ * Otherwise, missing synchronization could lead to kernel crashes due
    ++ * to missing page table entries for certain kernel mappings.
    ++ *
    ++ * Synchronization is performed at the top level, which is the PGD in
    ++ * 5-level paging systems. But in 4-level paging systems, however,
    ++ * pgd_populate() is a no-op, so synchronization is done at the P4D level.
    ++ * sync_global_pgds() handles this difference between paging levels.
    ++ */
     +void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
     +{
     +	sync_global_pgds(start, end);

[ The actual cover letter starts here ]

# The problem: It is easy to miss/overlook page table synchronization

Hi all,

During our internal testing, we started observing intermittent boot
failures when the machine uses 4-level paging and has a large amount
of persistent memory:

  BUG: unable to handle page fault for address: ffffe70000000034
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0002) - not-present page
  PGD 0 P4D 0 
  Oops: 0002 [#1] SMP NOPTI
  RIP: 0010:__init_single_page+0x9/0x6d
  Call Trace:
   <TASK>
   __init_zone_device_page+0x17/0x5d
   memmap_init_zone_device+0x154/0x1bb
   pagemap_range+0x2e0/0x40f
   memremap_pages+0x10b/0x2f0
   devm_memremap_pages+0x1e/0x60
   dev_dax_probe+0xce/0x2ec [device_dax]
   dax_bus_probe+0x6d/0xc9
   [... snip ...]
   </TASK>

It turns out that the kernel panics while initializing vmemmap
(struct page array) when the vmemmap region spans two PGD entries,
because the new PGD entry is only installed in init_mm.pgd,
but not in the page tables of other tasks.

And looking at __populate_section_memmap():
  if (vmemmap_can_optimize(altmap, pgmap))                                
          // does not sync top level page tables
          r = vmemmap_populate_compound_pages(pfn, start, end, nid, pgmap);
  else                                                                    
          // sync top level page tables in x86
          r = vmemmap_populate(start, end, nid, altmap);

In the normal path, vmemmap_populate() in arch/x86/mm/init_64.c
synchronizes the top level page table (See commit 9b861528a801
("x86-64, mem: Update all PGDs for direct mapping and vmemmap mapping
changes")) so that all tasks in the system can see the new vmemmap area.

However, when vmemmap_can_optimize() returns true, the optimized path
skips synchronization of top-level page tables. This is because
vmemmap_populate_compound_pages() is implemented in core MM code, which
does not handle synchronization of the top-level page tables. Instead,
the core MM has historically relied on each architecture to perform this
synchronization manually.

We're not the first party to encounter a crash caused by not-sync'd
top level page tables: earlier this year, Gwan-gyeong Mun attempted to
address the issue [1] [2] after hitting a kernel panic when x86 code
accessed the vmemmap area before the corresponding top-level entries
were synced. At that time, the issue was believed to be triggered
only when struct page was enlarged for debugging purposes, and the patch
did not get further updates.

It turns out that current approach of relying on each arch to handle
the page table sync manually is fragile because 1) it's easy to forget
to sync the top level page table, and 2) it's also easy to overlook that
the kernel should not access the vmemmap and direct mapping areas before
the sync.

# The solution: Make page table sync more code robust and harder to miss

To address this, Dave Hansen suggested [3] [4] introducing
{pgd,p4d}_populate_kernel() for updating kernel portion
of the page tables and allow each architecture to explicitly perform
synchronization when installing top-level entries. With this approach,
we no longer need to worry about missing the sync step, reducing the risk
of future regressions.

The new interface reuses existing ARCH_PAGE_TABLE_SYNC_MASK,
PGTBL_P*D_MODIFIED and arch_sync_kernel_mappings() facility used by
vmalloc and ioremap to synchronize page tables.

pgd_populate_kernel() looks like this:
static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
                                       p4d_t *p4d)
{
        pgd_populate(&init_mm, pgd, p4d);
        if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
                arch_sync_kernel_mappings(addr, addr);
}

It is worth noting that vmalloc() and apply_to_range() carefully
synchronizes page tables by calling p*d_alloc_track() and
arch_sync_kernel_mappings(), and thus they are not affected by
this patch series.

This patch series was hugely inspired by Dave Hansen's suggestion and
hence added Suggested-by: Dave Hansen.

Cc stable because lack of this series opens the door to intermittent
boot failures.

[1] https://lore.kernel.org/linux-mm/20250220064105.808339-1-gwan-gyeong.mun@intel.com
[2] https://lore.kernel.org/linux-mm/20250311114420.240341-1-gwan-gyeong.mun@intel.com
[3] https://lore.kernel.org/linux-mm/d1da214c-53d3-45ac-a8b6-51821c5416e4@intel.com
[4] https://lore.kernel.org/linux-mm/4d800744-7b88-41aa-9979-b245e8bf794b@intel.com 

Harry Yoo (3):
  mm: move page table sync declarations to linux/pgtable.h
  mm: introduce and use {pgd,p4d}_populate_kernel()
  x86/mm/64: define ARCH_PAGE_TABLE_SYNC_MASK and
    arch_sync_kernel_mappings()

 arch/x86/include/asm/pgtable_64_types.h |  3 +++
 arch/x86/mm/init_64.c                   | 18 ++++++++++++++++++
 include/linux/pgalloc.h                 | 24 ++++++++++++++++++++++++
 include/linux/pgtable.h                 | 25 +++++++++++++++++++++----
 include/linux/vmalloc.h                 | 16 ----------------
 mm/kasan/init.c                         | 12 ++++++------
 mm/percpu.c                             |  6 +++---
 mm/sparse-vmemmap.c                     |  6 +++---
 8 files changed, 78 insertions(+), 32 deletions(-)
 create mode 100644 include/linux/pgalloc.h

-- 
2.43.0

[PATCH V5 mm-hotfixes 1/3] mm: move page table sync declarations to linux/pgtable.h

[Harry Yoo]

Move ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings() to
linux/pgtable.h so that they can be used outside of vmalloc and ioremap.

Cc: <stable@vger.kernel.org>
Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
Acked-by: Kiryl Shutsemau <kas@kernel.org>
Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Reviewed-by: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---
 include/linux/pgtable.h | 16 ++++++++++++++++
 include/linux/vmalloc.h | 16 ----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 4c035637eeb7..ba699df6ef69 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1467,6 +1467,22 @@ static inline void modify_prot_commit_ptes(struct vm_area_struct *vma, unsigned
 }
 #endif
 
+/*
+ * Architectures can set this mask to a combination of PGTBL_P?D_MODIFIED values
+ * and let generic vmalloc and ioremap code know when arch_sync_kernel_mappings()
+ * needs to be called.
+ */
+#ifndef ARCH_PAGE_TABLE_SYNC_MASK
+#define ARCH_PAGE_TABLE_SYNC_MASK 0
+#endif
+
+/*
+ * There is no default implementation for arch_sync_kernel_mappings(). It is
+ * relied upon the compiler to optimize calls out if ARCH_PAGE_TABLE_SYNC_MASK
+ * is 0.
+ */
+void arch_sync_kernel_mappings(unsigned long start, unsigned long end);
+
 #endif /* CONFIG_MMU */
 
 /*
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index fdc9aeb74a44..2759dac6be44 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -219,22 +219,6 @@ extern int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
 int vmap_pages_range(unsigned long addr, unsigned long end, pgprot_t prot,
 		     struct page **pages, unsigned int page_shift);
 
-/*
- * Architectures can set this mask to a combination of PGTBL_P?D_MODIFIED values
- * and let generic vmalloc and ioremap code know when arch_sync_kernel_mappings()
- * needs to be called.
- */
-#ifndef ARCH_PAGE_TABLE_SYNC_MASK
-#define ARCH_PAGE_TABLE_SYNC_MASK 0
-#endif
-
-/*
- * There is no default implementation for arch_sync_kernel_mappings(). It is
- * relied upon the compiler to optimize calls out if ARCH_PAGE_TABLE_SYNC_MASK
- * is 0.
- */
-void arch_sync_kernel_mappings(unsigned long start, unsigned long end);
-
 /*
  *	Lowlevel-APIs (not for driver use!)
  */
-- 
2.43.0

[PATCH V5 mm-hotfixes 2/3] mm: introduce and use {pgd,p4d}_populate_kernel()

[Harry Yoo]

Introduce and use {pgd,p4d}_populate_kernel() in core MM code when
populating PGD and P4D entries for the kernel address space.
These helpers ensure proper synchronization of page tables when
updating the kernel portion of top-level page tables.

Until now, the kernel has relied on each architecture to handle
synchronization of top-level page tables in an ad-hoc manner.
For example, see commit 9b861528a801 ("x86-64, mem: Update all PGDs for
direct mapping and vmemmap mapping changes").

However, this approach has proven fragile for following reasons:

  1) It is easy to forget to perform the necessary page table
     synchronization when introducing new changes.
     For instance, commit 4917f55b4ef9 ("mm/sparse-vmemmap: improve memory
     savings for compound devmaps") overlooked the need to synchronize
     page tables for the vmemmap area.

  2) It is also easy to overlook that the vmemmap and direct mapping areas
     must not be accessed before explicit page table synchronization.
     For example, commit 8d400913c231 ("x86/vmemmap: handle unpopulated
     sub-pmd ranges")) caused crashes by accessing the vmemmap area
     before calling sync_global_pgds().

To address this, as suggested by Dave Hansen, introduce _kernel() variants
of the page table population helpers, which invoke architecture-specific
hooks to properly synchronize page tables. These are introduced in a new
header file, include/linux/pgalloc.h, so they can be called from common code.

They reuse existing infrastructure for vmalloc and ioremap.
Synchronization requirements are determined by ARCH_PAGE_TABLE_SYNC_MASK,
and the actual synchronization is performed by arch_sync_kernel_mappings().

This change currently targets only x86_64, so only PGD and P4D level
helpers are introduced. Currently, these helpers are no-ops since no
architecture sets PGTBL_{PGD,P4D}_MODIFIED in ARCH_PAGE_TABLE_SYNC_MASK.

In theory, PUD and PMD level helpers can be added later if needed by
other architectures. For now, 32-bit architectures (x86-32 and arm) only
handle PGTBL_PMD_MODIFIED, so p*d_populate_kernel() will never affect
them unless we introduce a PMD level helper.

Cc: <stable@vger.kernel.org>
Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Kiryl Shutsemau <kas@kernel.org>
Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---
 include/linux/pgalloc.h | 24 ++++++++++++++++++++++++
 include/linux/pgtable.h | 13 +++++++------
 mm/kasan/init.c         | 12 ++++++------
 mm/percpu.c             |  6 +++---
 mm/sparse-vmemmap.c     |  6 +++---
 5 files changed, 43 insertions(+), 18 deletions(-)
 create mode 100644 include/linux/pgalloc.h

diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
new file mode 100644
index 000000000000..290ab864320f
--- /dev/null
+++ b/include/linux/pgalloc.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_PGALLOC_H
+#define _LINUX_PGALLOC_H
+
+#include <linux/pgtable.h>
+#include <asm/pgalloc.h>
+
+static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
+				       p4d_t *p4d)
+{
+	pgd_populate(&init_mm, pgd, p4d);
+	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
+		arch_sync_kernel_mappings(addr, addr);
+}
+
+static inline void p4d_populate_kernel(unsigned long addr, p4d_t *p4d,
+				       pud_t *pud)
+{
+	p4d_populate(&init_mm, p4d, pud);
+	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
+		arch_sync_kernel_mappings(addr, addr);
+}
+
+#endif /* _LINUX_PGALLOC_H */
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index ba699df6ef69..2b80fd456c8b 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1469,8 +1469,8 @@ static inline void modify_prot_commit_ptes(struct vm_area_struct *vma, unsigned
 
 /*
  * Architectures can set this mask to a combination of PGTBL_P?D_MODIFIED values
- * and let generic vmalloc and ioremap code know when arch_sync_kernel_mappings()
- * needs to be called.
+ * and let generic vmalloc, ioremap and page table update code know when
+ * arch_sync_kernel_mappings() needs to be called.
  */
 #ifndef ARCH_PAGE_TABLE_SYNC_MASK
 #define ARCH_PAGE_TABLE_SYNC_MASK 0
@@ -1954,10 +1954,11 @@ static inline bool arch_has_pfn_modify_check(void)
 /*
  * Page Table Modification bits for pgtbl_mod_mask.
  *
- * These are used by the p?d_alloc_track*() set of functions an in the generic
- * vmalloc/ioremap code to track at which page-table levels entries have been
- * modified. Based on that the code can better decide when vmalloc and ioremap
- * mapping changes need to be synchronized to other page-tables in the system.
+ * These are used by the p?d_alloc_track*() and p*d_populate_kernel()
+ * functions in the generic vmalloc, ioremap and page table update code
+ * to track at which page-table levels entries have been modified.
+ * Based on that the code can better decide when page table changes need
+ * to be synchronized to other page-tables in the system.
  */
 #define		__PGTBL_PGD_MODIFIED	0
 #define		__PGTBL_P4D_MODIFIED	1
diff --git a/mm/kasan/init.c b/mm/kasan/init.c
index ced6b29fcf76..8fce3370c84e 100644
--- a/mm/kasan/init.c
+++ b/mm/kasan/init.c
@@ -13,9 +13,9 @@
 #include <linux/mm.h>
 #include <linux/pfn.h>
 #include <linux/slab.h>
+#include <linux/pgalloc.h>
 
 #include <asm/page.h>
-#include <asm/pgalloc.h>
 
 #include "kasan.h"
 
@@ -191,7 +191,7 @@ static int __ref zero_p4d_populate(pgd_t *pgd, unsigned long addr,
 			pud_t *pud;
 			pmd_t *pmd;
 
-			p4d_populate(&init_mm, p4d,
+			p4d_populate_kernel(addr, p4d,
 					lm_alias(kasan_early_shadow_pud));
 			pud = pud_offset(p4d, addr);
 			pud_populate(&init_mm, pud,
@@ -212,7 +212,7 @@ static int __ref zero_p4d_populate(pgd_t *pgd, unsigned long addr,
 			} else {
 				p = early_alloc(PAGE_SIZE, NUMA_NO_NODE);
 				pud_init(p);
-				p4d_populate(&init_mm, p4d, p);
+				p4d_populate_kernel(addr, p4d, p);
 			}
 		}
 		zero_pud_populate(p4d, addr, next);
@@ -251,10 +251,10 @@ int __ref kasan_populate_early_shadow(const void *shadow_start,
 			 * puds,pmds, so pgd_populate(), pud_populate()
 			 * is noops.
 			 */
-			pgd_populate(&init_mm, pgd,
+			pgd_populate_kernel(addr, pgd,
 					lm_alias(kasan_early_shadow_p4d));
 			p4d = p4d_offset(pgd, addr);
-			p4d_populate(&init_mm, p4d,
+			p4d_populate_kernel(addr, p4d,
 					lm_alias(kasan_early_shadow_pud));
 			pud = pud_offset(p4d, addr);
 			pud_populate(&init_mm, pud,
@@ -273,7 +273,7 @@ int __ref kasan_populate_early_shadow(const void *shadow_start,
 				if (!p)
 					return -ENOMEM;
 			} else {
-				pgd_populate(&init_mm, pgd,
+				pgd_populate_kernel(addr, pgd,
 					early_alloc(PAGE_SIZE, NUMA_NO_NODE));
 			}
 		}
diff --git a/mm/percpu.c b/mm/percpu.c
index d9cbaee92b60..a56f35dcc417 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -3108,7 +3108,7 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, size_t dyn_size,
 #endif /* BUILD_EMBED_FIRST_CHUNK */
 
 #ifdef BUILD_PAGE_FIRST_CHUNK
-#include <asm/pgalloc.h>
+#include <linux/pgalloc.h>
 
 #ifndef P4D_TABLE_SIZE
 #define P4D_TABLE_SIZE PAGE_SIZE
@@ -3134,13 +3134,13 @@ void __init __weak pcpu_populate_pte(unsigned long addr)
 
 	if (pgd_none(*pgd)) {
 		p4d = memblock_alloc_or_panic(P4D_TABLE_SIZE, P4D_TABLE_SIZE);
-		pgd_populate(&init_mm, pgd, p4d);
+		pgd_populate_kernel(addr, pgd, p4d);
 	}
 
 	p4d = p4d_offset(pgd, addr);
 	if (p4d_none(*p4d)) {
 		pud = memblock_alloc_or_panic(PUD_TABLE_SIZE, PUD_TABLE_SIZE);
-		p4d_populate(&init_mm, p4d, pud);
+		p4d_populate_kernel(addr, p4d, pud);
 	}
 
 	pud = pud_offset(p4d, addr);
diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 41aa0493eb03..dbd8daccade2 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -27,9 +27,9 @@
 #include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
+#include <linux/pgalloc.h>
 
 #include <asm/dma.h>
-#include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
 
 #include "hugetlb_vmemmap.h"
@@ -229,7 +229,7 @@ p4d_t * __meminit vmemmap_p4d_populate(pgd_t *pgd, unsigned long addr, int node)
 		if (!p)
 			return NULL;
 		pud_init(p);
-		p4d_populate(&init_mm, p4d, p);
+		p4d_populate_kernel(addr, p4d, p);
 	}
 	return p4d;
 }
@@ -241,7 +241,7 @@ pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node)
 		void *p = vmemmap_alloc_block_zero(PAGE_SIZE, node);
 		if (!p)
 			return NULL;
-		pgd_populate(&init_mm, pgd, p);
+		pgd_populate_kernel(addr, pgd, p);
 	}
 	return pgd;
 }
-- 
2.43.0

[PATCH V5 mm-hotfixes 3/3] x86/mm/64: define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings()

[Harry Yoo]

Define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings() to ensure
page tables are properly synchronized when calling
p*d_populate_kernel().

For 5-level paging, synchronization is performed via
pgd_populate_kernel(). In 4-level paging, pgd_populate() is a no-op,
so synchronization is instead performed at the P4D level via
p4d_populate_kernel().

This fixes intermittent boot failures on systems using 4-level paging
and a large amount of persistent memory:

  BUG: unable to handle page fault for address: ffffe70000000034
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0002) - not-present page
  PGD 0 P4D 0
  Oops: 0002 [#1] SMP NOPTI
  RIP: 0010:__init_single_page+0x9/0x6d
  Call Trace:
   <TASK>
   __init_zone_device_page+0x17/0x5d
   memmap_init_zone_device+0x154/0x1bb
   pagemap_range+0x2e0/0x40f
   memremap_pages+0x10b/0x2f0
   devm_memremap_pages+0x1e/0x60
   dev_dax_probe+0xce/0x2ec [device_dax]
   dax_bus_probe+0x6d/0xc9
   [... snip ...]
   </TASK>

It also fixes a crash in vmemmap_set_pmd() caused by accessing vmemmap
before sync_global_pgds() [1]:

  BUG: unable to handle page fault for address: ffffeb3ff1200000
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0002) - not-present page
  PGD 0 P4D 0
  Oops: Oops: 0002 [#1] PREEMPT SMP NOPTI
  Tainted: [W]=WARN
  RIP: 0010:vmemmap_set_pmd+0xff/0x230
   <TASK>
   vmemmap_populate_hugepages+0x176/0x180
   vmemmap_populate+0x34/0x80
   __populate_section_memmap+0x41/0x90
   sparse_add_section+0x121/0x3e0
   __add_pages+0xba/0x150
   add_pages+0x1d/0x70
   memremap_pages+0x3dc/0x810
   devm_memremap_pages+0x1c/0x60
   xe_devm_add+0x8b/0x100 [xe]
   xe_tile_init_noalloc+0x6a/0x70 [xe]
   xe_device_probe+0x48c/0x740 [xe]
   [... snip ...]

Cc: <stable@vger.kernel.org>
Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
Closes: https://lore.kernel.org/linux-mm/20250311114420.240341-1-gwan-gyeong.mun@intel.com [1]
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Kiryl Shutsemau <kas@kernel.org>
Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---
 arch/x86/include/asm/pgtable_64_types.h |  3 +++
 arch/x86/mm/init_64.c                   | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
index 4604f924d8b8..7eb61ef6a185 100644
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -36,6 +36,9 @@ static inline bool pgtable_l5_enabled(void)
 #define pgtable_l5_enabled() cpu_feature_enabled(X86_FEATURE_LA57)
 #endif /* USE_EARLY_PGTABLE_L5 */
 
+#define ARCH_PAGE_TABLE_SYNC_MASK \
+	(pgtable_l5_enabled() ? PGTBL_PGD_MODIFIED : PGTBL_P4D_MODIFIED)
+
 extern unsigned int pgdir_shift;
 extern unsigned int ptrs_per_p4d;
 
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 76e33bd7c556..b9426fce5f3e 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -223,6 +223,24 @@ static void sync_global_pgds(unsigned long start, unsigned long end)
 		sync_global_pgds_l4(start, end);
 }
 
+/*
+ * Make kernel mappings visible in all page tables in the system.
+ * This is necessary except when the init task populates kernel mappings
+ * during the boot process. In that case, all processes originating from
+ * the init task copies the kernel mappings, so there is no issue.
+ * Otherwise, missing synchronization could lead to kernel crashes due
+ * to missing page table entries for certain kernel mappings.
+ *
+ * Synchronization is performed at the top level, which is the PGD in
+ * 5-level paging systems. But in 4-level paging systems, however,
+ * pgd_populate() is a no-op, so synchronization is done at the P4D level.
+ * sync_global_pgds() handles this difference between paging levels.
+ */
+void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
+{
+	sync_global_pgds(start, end);
+}
+
 /*
  * NOTE: This function is marked __ref because it calls __init function
  * (alloc_bootmem_pages). It's safe to do it ONLY when after_bootmem == 0.
-- 
2.43.0

Re: [PATCH V5 mm-hotfixes 1/3] mm: move page table sync declarations to linux/pgtable.h

[David Hildenbrand]

On 18.08.25 04:02, Harry Yoo wrote:
> Move ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings() to
> linux/pgtable.h so that they can be used outside of vmalloc and ioremap.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
> Acked-by: Kiryl Shutsemau <kas@kernel.org>
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> Reviewed-by: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH V5 mm-hotfixes 2/3] mm: introduce and use {pgd,p4d}_populate_kernel()

[David Hildenbrand]

On 18.08.25 04:02, Harry Yoo wrote:
> Introduce and use {pgd,p4d}_populate_kernel() in core MM code when
> populating PGD and P4D entries for the kernel address space.
> These helpers ensure proper synchronization of page tables when
> updating the kernel portion of top-level page tables.
> 
> Until now, the kernel has relied on each architecture to handle
> synchronization of top-level page tables in an ad-hoc manner.
> For example, see commit 9b861528a801 ("x86-64, mem: Update all PGDs for
> direct mapping and vmemmap mapping changes").
> 
> However, this approach has proven fragile for following reasons:
> 
>    1) It is easy to forget to perform the necessary page table
>       synchronization when introducing new changes.
>       For instance, commit 4917f55b4ef9 ("mm/sparse-vmemmap: improve memory
>       savings for compound devmaps") overlooked the need to synchronize
>       page tables for the vmemmap area.
> 
>    2) It is also easy to overlook that the vmemmap and direct mapping areas
>       must not be accessed before explicit page table synchronization.
>       For example, commit 8d400913c231 ("x86/vmemmap: handle unpopulated
>       sub-pmd ranges")) caused crashes by accessing the vmemmap area
>       before calling sync_global_pgds().
> 
> To address this, as suggested by Dave Hansen, introduce _kernel() variants
> of the page table population helpers, which invoke architecture-specific
> hooks to properly synchronize page tables. These are introduced in a new
> header file, include/linux/pgalloc.h, so they can be called from common code.
> 
> They reuse existing infrastructure for vmalloc and ioremap.
> Synchronization requirements are determined by ARCH_PAGE_TABLE_SYNC_MASK,
> and the actual synchronization is performed by arch_sync_kernel_mappings().
> 
> This change currently targets only x86_64, so only PGD and P4D level
> helpers are introduced. Currently, these helpers are no-ops since no
> architecture sets PGTBL_{PGD,P4D}_MODIFIED in ARCH_PAGE_TABLE_SYNC_MASK.
> 
> In theory, PUD and PMD level helpers can be added later if needed by
> other architectures. For now, 32-bit architectures (x86-32 and arm) only
> handle PGTBL_PMD_MODIFIED, so p*d_populate_kernel() will never affect
> them unless we introduce a PMD level helper.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> Acked-by: Kiryl Shutsemau <kas@kernel.org>
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> ---


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH V5 mm-hotfixes 3/3] x86/mm/64: define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings()

[David Hildenbrand]

On 18.08.25 04:02, Harry Yoo wrote:
> Define ARCH_PAGE_TABLE_SYNC_MASK and arch_sync_kernel_mappings() to ensure
> page tables are properly synchronized when calling
> p*d_populate_kernel().
> 
> For 5-level paging, synchronization is performed via
> pgd_populate_kernel(). In 4-level paging, pgd_populate() is a no-op,
> so synchronization is instead performed at the P4D level via
> p4d_populate_kernel().
> 
> This fixes intermittent boot failures on systems using 4-level paging
> and a large amount of persistent memory:
> 
>    BUG: unable to handle page fault for address: ffffe70000000034
>    #PF: supervisor write access in kernel mode
>    #PF: error_code(0x0002) - not-present page
>    PGD 0 P4D 0
>    Oops: 0002 [#1] SMP NOPTI
>    RIP: 0010:__init_single_page+0x9/0x6d
>    Call Trace:
>     <TASK>
>     __init_zone_device_page+0x17/0x5d
>     memmap_init_zone_device+0x154/0x1bb
>     pagemap_range+0x2e0/0x40f
>     memremap_pages+0x10b/0x2f0
>     devm_memremap_pages+0x1e/0x60
>     dev_dax_probe+0xce/0x2ec [device_dax]
>     dax_bus_probe+0x6d/0xc9
>     [... snip ...]
>     </TASK>
> 
> It also fixes a crash in vmemmap_set_pmd() caused by accessing vmemmap
> before sync_global_pgds() [1]:
> 
>    BUG: unable to handle page fault for address: ffffeb3ff1200000
>    #PF: supervisor write access in kernel mode
>    #PF: error_code(0x0002) - not-present page
>    PGD 0 P4D 0
>    Oops: Oops: 0002 [#1] PREEMPT SMP NOPTI
>    Tainted: [W]=WARN
>    RIP: 0010:vmemmap_set_pmd+0xff/0x230
>     <TASK>
>     vmemmap_populate_hugepages+0x176/0x180
>     vmemmap_populate+0x34/0x80
>     __populate_section_memmap+0x41/0x90
>     sparse_add_section+0x121/0x3e0
>     __add_pages+0xba/0x150
>     add_pages+0x1d/0x70
>     memremap_pages+0x3dc/0x810
>     devm_memremap_pages+0x1c/0x60
>     xe_devm_add+0x8b/0x100 [xe]
>     xe_tile_init_noalloc+0x6a/0x70 [xe]
>     xe_device_probe+0x48c/0x740 [xe]
>     [... snip ...]
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
> Closes: https://lore.kernel.org/linux-mm/20250311114420.240341-1-gwan-gyeong.mun@intel.com [1]
> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> Acked-by: Kiryl Shutsemau <kas@kernel.org>
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH V5 mm-hotfixes 0/3] mm, x86: fix crash due to missing page table sync and make it harder to miss

[David Hildenbrand]

On 18.08.25 04:02, Harry Yoo wrote:
> To x86 folks:
> It's not clear whether this should go through the MM tree or the x86
> tree as it changes both. We could send it to the MM tree with Acks
> from the x86 folks, or we could send it through the x86 tree instead.
> What do you think?

I think this should go through the MM tree with ACKs on the x86 parts 
from respective maintainers.

-- 
Cheers

David / dhildenb

[PATCH] mm: fix KASAN build error due to p*d_populate_kernel()

[Harry Yoo]

KASAN unconditionally references kasan_early_shadow_{p4d,pud}.
However, these global variables may not exist depending on the number of
page table levels. For example, if CONFIG_PGTABLE_LEVELS=3, both
variables do not exist. Although KASAN may refernce non-existent
variables, it didn't break builds because calls to {pgd,p4d}_populate()
are optimized away at compile time.

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:

ld.lld: error: undefined symbol: kasan_early_shadow_p4d
>>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> did you mean: kasan_early_shadow_pmd
>>> defined in: vmlinux.a(mm/kasan/init.o)

ld.lld: error: undefined symbol: kasan_early_shadow_pud
>>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:200 (/home/hyeyoo/mm-new/mm/kasan/init.c:200)
>>>               mm/kasan/init.o:(zero_p4d_populate) in archive vmlinux.a
>>> referenced 1 more times

Therefore, to allow calls to {pgd,p4d}_populate_kernel() to be optimized
out at compile time, define {pgd,p4d}_populate_kernel() as macros.
This way, when pgd_populate() or p4d_populate() are simply empty macros,
the corresponding *_populate_kernel() functions can also be optimized
away.

Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---

While the description is quite verbose, it is intended to be fold-merged
into patch [1] of the page table sync series V5.

[1] https://lore.kernel.org/linux-mm/20250818020206.4517-3-harry.yoo@oracle.com/

 include/linux/pgalloc.h | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
index 290ab864320f..8812f842978f 100644
--- a/include/linux/pgalloc.h
+++ b/include/linux/pgalloc.h
@@ -5,20 +5,18 @@
 #include <linux/pgtable.h>
 #include <asm/pgalloc.h>
 
-static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
-				       p4d_t *p4d)
-{
-	pgd_populate(&init_mm, pgd, p4d);
-	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
-		arch_sync_kernel_mappings(addr, addr);
-}
+#define pgd_populate_kernel(addr, pgd, p4d)				\
+	do {								\
+		pgd_populate(&init_mm, pgd, p4d);			\
+		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)	\
+			arch_sync_kernel_mappings(addr, addr);		\
+	} while (0)
 
-static inline void p4d_populate_kernel(unsigned long addr, p4d_t *p4d,
-				       pud_t *pud)
-{
-	p4d_populate(&init_mm, p4d, pud);
-	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
-		arch_sync_kernel_mappings(addr, addr);
-}
+#define p4d_populate_kernel(addr, p4d, pud)				\
+	do {								\
+		p4d_populate(&init_mm, p4d, pud);			\
+		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)	\
+			arch_sync_kernel_mappings(addr, addr);		\
+	} while (0)
 
 #endif /* _LINUX_PGALLOC_H */
-- 
2.43.0

Re: [PATCH] mm: fix KASAN build error due to p*d_populate_kernel()

[Lorenzo Stoakes]

On Thu, Aug 21, 2025 at 06:35:42PM +0900, Harry Yoo wrote:
> KASAN unconditionally references kasan_early_shadow_{p4d,pud}.
> However, these global variables may not exist depending on the number of
> page table levels. For example, if CONFIG_PGTABLE_LEVELS=3, both
> variables do not exist. Although KASAN may refernce non-existent
> variables, it didn't break builds because calls to {pgd,p4d}_populate()
> are optimized away at compile time.
>
> 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:
>
> ld.lld: error: undefined symbol: kasan_early_shadow_p4d
> >>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
> >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> >>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
> >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> >>> did you mean: kasan_early_shadow_pmd
> >>> defined in: vmlinux.a(mm/kasan/init.o)
>
> ld.lld: error: undefined symbol: kasan_early_shadow_pud
> >>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
> >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> >>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
> >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> >>> referenced by init.c:200 (/home/hyeyoo/mm-new/mm/kasan/init.c:200)
> >>>               mm/kasan/init.o:(zero_p4d_populate) in archive vmlinux.a
> >>> referenced 1 more times
>
> Therefore, to allow calls to {pgd,p4d}_populate_kernel() to be optimized
> out at compile time, define {pgd,p4d}_populate_kernel() as macros.
> This way, when pgd_populate() or p4d_populate() are simply empty macros,
> the corresponding *_populate_kernel() functions can also be optimized
> away.
>
> Signed-off-by: Harry Yoo <harry.yoo@oracle.com>

This looks good, other than the nit below re: a comment, I think when we
are doing this kind of thing it's necessary to spell out plainly why
exactly we're doing it because it's not obvious at first glance.

Anyway have checked locally and all good and LGTM code-wise so aside from
above:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>
> While the description is quite verbose, it is intended to be fold-merged
> into patch [1] of the page table sync series V5.
>
> [1] https://lore.kernel.org/linux-mm/20250818020206.4517-3-harry.yoo@oracle.com/
>
>  include/linux/pgalloc.h | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
> index 290ab864320f..8812f842978f 100644
> --- a/include/linux/pgalloc.h
> +++ b/include/linux/pgalloc.h
> @@ -5,20 +5,18 @@
>  #include <linux/pgtable.h>
>  #include <asm/pgalloc.h>
>
> -static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
> -				       p4d_t *p4d)
> -{
> -	pgd_populate(&init_mm, pgd, p4d);
> -	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
> -		arch_sync_kernel_mappings(addr, addr);
> -}
> +#define pgd_populate_kernel(addr, pgd, p4d)				\
> +	do {								\
> +		pgd_populate(&init_mm, pgd, p4d);			\
> +		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)	\
> +			arch_sync_kernel_mappings(addr, addr);		\
> +	} while (0)
>
> -static inline void p4d_populate_kernel(unsigned long addr, p4d_t *p4d,
> -				       pud_t *pud)
> -{
> -	p4d_populate(&init_mm, p4d, pud);
> -	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
> -		arch_sync_kernel_mappings(addr, addr);
> -}
> +#define p4d_populate_kernel(addr, p4d, pud)				\
> +	do {								\
> +		p4d_populate(&init_mm, p4d, pud);			\
> +		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)	\
> +			arch_sync_kernel_mappings(addr, addr);		\
> +	} while (0)
>

Can we have a quick comment above these explaining why they have to be
macros? Thanks!

>  #endif /* _LINUX_PGALLOC_H */
> --
> 2.43.0
>

Re: [PATCH] mm: fix KASAN build error due to p*d_populate_kernel()

[Harry Yoo]

On Thu, Aug 21, 2025 at 11:10:39AM +0100, Lorenzo Stoakes wrote:
> On Thu, Aug 21, 2025 at 06:35:42PM +0900, Harry Yoo wrote:
> > KASAN unconditionally references kasan_early_shadow_{p4d,pud}.
> > However, these global variables may not exist depending on the number of
> > page table levels. For example, if CONFIG_PGTABLE_LEVELS=3, both
> > variables do not exist. Although KASAN may refernce non-existent
> > variables, it didn't break builds because calls to {pgd,p4d}_populate()
> > are optimized away at compile time.
> >
> > 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:
> >
> > ld.lld: error: undefined symbol: kasan_early_shadow_p4d
> > >>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
> > >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> > >>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
> > >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> > >>> did you mean: kasan_early_shadow_pmd
> > >>> defined in: vmlinux.a(mm/kasan/init.o)
> >
> > ld.lld: error: undefined symbol: kasan_early_shadow_pud
> > >>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
> > >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> > >>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
> > >>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
> > >>> referenced by init.c:200 (/home/hyeyoo/mm-new/mm/kasan/init.c:200)
> > >>>               mm/kasan/init.o:(zero_p4d_populate) in archive vmlinux.a
> > >>> referenced 1 more times
> >
> > Therefore, to allow calls to {pgd,p4d}_populate_kernel() to be optimized
> > out at compile time, define {pgd,p4d}_populate_kernel() as macros.
> > This way, when pgd_populate() or p4d_populate() are simply empty macros,
> > the corresponding *_populate_kernel() functions can also be optimized
> > away.
> >
> > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> 
> This looks good, other than the nit below re: a comment, I think when we
> are doing this kind of thing it's necessary to spell out plainly why
> exactly we're doing it because it's not obvious at first glance.

Good point, will do:

/*
 * {pgd,p4d}_populate_kernel() are defined as macros to allow
 * compile-time optimization based on the configured page table levels.
 * Without this, linking may fail because callers (e.g., KASAN) may rely
 * on calls to these functions being optimized away when passing symbols
 * that exist only for certain page table levels.
 */ 

> Anyway have checked locally and all good and LGTM code-wise so aside from
> above:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Thanks!

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] mm: fix KASAN build error due to p*d_populate_kernel()

[Lorenzo Stoakes]

On Thu, Aug 21, 2025 at 07:42:06PM +0900, Harry Yoo wrote:
> > > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
> >
> > This looks good, other than the nit below re: a comment, I think when we
> > are doing this kind of thing it's necessary to spell out plainly why
> > exactly we're doing it because it's not obvious at first glance.
>
> Good point, will do:
>
> /*
>  * {pgd,p4d}_populate_kernel() are defined as macros to allow
>  * compile-time optimization based on the configured page table levels.
>  * Without this, linking may fail because callers (e.g., KASAN) may rely
>  * on calls to these functions being optimized away when passing symbols
>  * that exist only for certain page table levels.
>  */

Thanks LGTM!

[PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Harry Yoo]

KASAN unconditionally references kasan_early_shadow_{p4d,pud}.
However, these global variables may not exist depending on the number of
page table levels. For example, if CONFIG_PGTABLE_LEVELS=3, both
variables do not exist. Although KASAN may refernce non-existent
variables, it didn't break builds because calls to {pgd,p4d}_populate()
are optimized away at compile time.

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:

ld.lld: error: undefined symbol: kasan_early_shadow_p4d
>>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> did you mean: kasan_early_shadow_pmd
>>> defined in: vmlinux.a(mm/kasan/init.o)

ld.lld: error: undefined symbol: kasan_early_shadow_pud
>>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:200 (/home/hyeyoo/mm-new/mm/kasan/init.c:200)
>>>               mm/kasan/init.o:(zero_p4d_populate) in archive vmlinux.a
>>> referenced 1 more times

Therefore, to allow calls to {pgd,p4d}_populate_kernel() to be optimized
out at compile time, define {pgd,p4d}_populate_kernel() as macros.
This way, when pgd_populate() or p4d_populate() are simply empty macros,
the corresponding *_populate_kernel() functions can also be optimized
away.

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---

v1 -> v2: added comment per Lorenzo's comment.

 include/linux/pgalloc.h | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
index 290ab864320f..9174fa59bbc5 100644
--- a/include/linux/pgalloc.h
+++ b/include/linux/pgalloc.h
@@ -5,20 +5,25 @@
 #include <linux/pgtable.h>
 #include <asm/pgalloc.h>
 
-static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
-				       p4d_t *p4d)
-{
-	pgd_populate(&init_mm, pgd, p4d);
-	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
-		arch_sync_kernel_mappings(addr, addr);
-}
+/*
+ * {pgd,p4d}_populate_kernel() are defined as macros to allow
+ * compile-time optimization based on the configured page table levels.
+ * Without this, linking may fail because callers (e.g., KASAN) may rely
+ * on calls to these functions being optimized away when passing symbols
+ * that exist only for certain page table levels.
+ */
+#define pgd_populate_kernel(addr, pgd, p4d)				\
+	do {								\
+		pgd_populate(&init_mm, pgd, p4d);			\
+		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)	\
+			arch_sync_kernel_mappings(addr, addr);		\
+	} while (0)
 
-static inline void p4d_populate_kernel(unsigned long addr, p4d_t *p4d,
-				       pud_t *pud)
-{
-	p4d_populate(&init_mm, p4d, pud);
-	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
-		arch_sync_kernel_mappings(addr, addr);
-}
+#define p4d_populate_kernel(addr, p4d, pud)				\
+	do {								\
+		p4d_populate(&init_mm, p4d, pud);			\
+		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)	\
+			arch_sync_kernel_mappings(addr, addr);		\
+	} while (0)
 
 #endif /* _LINUX_PGALLOC_H */
-- 
2.43.0

Re: [PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Dave Hansen]

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:

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

The right way to do it is to have an #ifdef in a header that avoids
compiling in the reference to the symbol.

But just changing the 'static inline' to a #define seems like a fragile
hack to me.

Re: [PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Harry Yoo]

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?

Just to clarify, what should be the exact ifdeffery to cover these cases?
#if CONFIG_PGTABLE_LEVELS == 4 and 5, or
#ifdef __PAGETABLE_P4D_FOLDED and __PAGETABLE_PUD_FOLDED ?

I have no strong opinion on this, let's hear what KASAN folks think.

> But just changing the 'static inline' to a #define seems like a fragile
> hack to me.

At least that's what KASAN has relied on p*d_populate() to do...

-- 
Cheers,
Harry / Hyeonggon

[PATCH v3] mm: fix KASAN build error due to p*d_populate_kernel()

[Harry Yoo]

Address a linker error introduced by a patch currently in mm-hotfixes:
"mm: introduce and use {pgd,p4d}_populate_kernel" [1].

KASAN unconditionally references kasan_early_shadow_{p4d,pud}.
However, these global variables may not exist depending on the number of
page table levels. For example, if CONFIG_PGTABLE_LEVELS=3, both
variables do not exist. Although KASAN may refernce non-existent
variables, it didn't break builds because calls to {pgd,p4d}_populate()
are optimized away at compile time.

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:

ld.lld: error: undefined symbol: kasan_early_shadow_p4d
>>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:260 (/home/hyeyoo/mm-new/mm/kasan/init.c:260)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> did you mean: kasan_early_shadow_pmd
>>> defined in: vmlinux.a(mm/kasan/init.o)

ld.lld: error: undefined symbol: kasan_early_shadow_pud
>>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:263 (/home/hyeyoo/mm-new/mm/kasan/init.c:263)
>>>               mm/kasan/init.o:(kasan_populate_early_shadow) in archive vmlinux.a
>>> referenced by init.c:200 (/home/hyeyoo/mm-new/mm/kasan/init.c:200)
>>>               mm/kasan/init.o:(zero_p4d_populate) in archive vmlinux.a
>>> referenced 1 more times

Therefore, to allow calls to {pgd,p4d}_populate_kernel() to be optimized
out at compile time, define {pgd,p4d}_populate_kernel() as macros.
This way, when pgd_populate() or p4d_populate() are simply empty macros,
the corresponding *_populate_kernel() functions can also be optimized
away.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202508181636.0Rtk0T7x-lkp@intel.com
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/lkml/20250821160515.611d191e@canb.auug.org.au
Link: https://lore.kernel.org/linux-mm/20250818020206.4517-3-harry.yoo@oracle.com [1]
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Signed-off-by: Harry Yoo <harry.yoo@oracle.com>
---

This is intended to be fold-merged into the patch
"mm: introduce and use {pgd,p4d}_populate_kernel". 

v2 -> v3:
- Explained that this fixes a linker error of a patch in mm-hotfixes. 
- Added links to error reports (Closes:) and Reported-by:

 include/linux/pgalloc.h | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
index 290ab864320f..9174fa59bbc5 100644
--- a/include/linux/pgalloc.h
+++ b/include/linux/pgalloc.h
@@ -5,20 +5,25 @@
 #include <linux/pgtable.h>
 #include <asm/pgalloc.h>
 
-static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
-				       p4d_t *p4d)
-{
-	pgd_populate(&init_mm, pgd, p4d);
-	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
-		arch_sync_kernel_mappings(addr, addr);
-}
+/*
+ * {pgd,p4d}_populate_kernel() are defined as macros to allow
+ * compile-time optimization based on the configured page table levels.
+ * Without this, linking may fail because callers (e.g., KASAN) may rely
+ * on calls to these functions being optimized away when passing symbols
+ * that exist only for certain page table levels.
+ */
+#define pgd_populate_kernel(addr, pgd, p4d)				\
+	do {								\
+		pgd_populate(&init_mm, pgd, p4d);			\
+		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)	\
+			arch_sync_kernel_mappings(addr, addr);		\
+	} while (0)
 
-static inline void p4d_populate_kernel(unsigned long addr, p4d_t *p4d,
-				       pud_t *pud)
-{
-	p4d_populate(&init_mm, p4d, pud);
-	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
-		arch_sync_kernel_mappings(addr, addr);
-}
+#define p4d_populate_kernel(addr, p4d, pud)				\
+	do {								\
+		p4d_populate(&init_mm, p4d, pud);			\
+		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)	\
+			arch_sync_kernel_mappings(addr, addr);		\
+	} while (0)
 
 #endif /* _LINUX_PGALLOC_H */
-- 
2.43.0

Re: [PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Andrey Ryabinin]

On 8/22/25 3:11 AM, 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


That's not exactly the case for the kernel. It historically relied on being
compiled with optimization and compiler being able to eliminate unused references.
AFAIR BUILD_BUG_ON() works like that, there are also plenty of code like

if  (IS_ENABLED(CONFIG_SOMETHING))
	ptr = &something;
else
	ptr = &something_else; 

e.g. irq_remaping_prepare();


> 
> 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?
> 
> Just to clarify, what should be the exact ifdeffery to cover these cases?
> #if CONFIG_PGTABLE_LEVELS == 4 and 5, or
> #ifdef __PAGETABLE_P4D_FOLDED and __PAGETABLE_PUD_FOLDED ?
> 

I think ifdef should be the same as for symbol, so '#if CONFIG_PGTABLE_LEVELS > 4'
for *_p4d and '#if CONFIG_PGTABLE_LEVELS > 3' for *_pud


> I have no strong opinion on this, let's hear what KASAN folks think.
> 

So, I think we have following options:

1. Macros as you did.
2. Hide references in function under  '#if CONFIG_PGTABLE_LEVELS > x', like Dave suggested.
3. It should be enough to just add if in code like
            if (CONFIG_PGTABLE_LEVELS > 4)
		pgd_populate_kernel(addr, pgd,
                                          lm_alias(kasan_early_shadow_p4d));
Compiler should be able to optimize it away.

4. I guess that the link error is due to enabled CONFIG_DEBUG_VIRTUAL=y
lm_alias() ends up with __phys_addr_symbol() function call which compiler can't optimize away.
Technically we can declare __phys_addr_symbol() with __attribute__((pure)), so compiler will
be able to optimize away this call, because the result should be unused.
But I'm not sure we really want that, because it's debug function and even if the result is unused
we might want to still have a check if symbol address is correct.


I would probably prefer 3rd option, but I don't really have very strong opinion, so either way is fine.

Re: [PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Dave Hansen]

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

> Just to clarify, what should be the exact ifdeffery to cover these cases?
> #if CONFIG_PGTABLE_LEVELS == 4 and 5, or
> #ifdef __PAGETABLE_P4D_FOLDED and __PAGETABLE_PUD_FOLDED ?
> 
> I have no strong opinion on this, let's hear what KASAN folks think.

I think CONFIG_PGTABLE_LEVELS works, but in the end I'm not picky about
the specific #ifdefs that work.

Re: [PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Andrey Ryabinin]

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.

Re: [PATCH v2] mm: fix KASAN build error due to p*d_populate_kernel()

[Harry Yoo]

On Fri, Aug 22, 2025 at 06:02:40PM +0200, Andrey Ryabinin wrote:
> On 8/22/25 3:11 AM, 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
>  
> That's not exactly the case for the kernel. It historically relied on being
> compiled with optimization and compiler being able to eliminate unused references.
> AFAIR BUILD_BUG_ON() works like that, there are also plenty of code like
> 
> if  (IS_ENABLED(CONFIG_SOMETHING))
> 	ptr = &something;
> else
> 	ptr = &something_else; 
> 
> e.g. irq_remaping_prepare();

Agreed. I've seen this pattern in many places.

> > 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?
> > 
> > Just to clarify, what should be the exact ifdeffery to cover these cases?
> > #if CONFIG_PGTABLE_LEVELS == 4 and 5, or
> > #ifdef __PAGETABLE_P4D_FOLDED and __PAGETABLE_PUD_FOLDED ?
> > 
> 
> I think ifdef should be the same as for symbol, so '#if CONFIG_PGTABLE_LEVELS > 4'
> for *_p4d and '#if CONFIG_PGTABLE_LEVELS > 3' for *_pud

Right.

> > I have no strong opinion on this, let's hear what KASAN folks think.
> > 
> 
> So, I think we have following options:
> 
> 1. Macros as you did.
> 2. Hide references in function under  '#if CONFIG_PGTABLE_LEVELS > x', like Dave suggested.
> 3. It should be enough to just add if in code like
>             if (CONFIG_PGTABLE_LEVELS > 4)
> 		pgd_populate_kernel(addr, pgd,
>                                           lm_alias(kasan_early_shadow_p4d));
> Compiler should be able to optimize it away.
> 
> 4. I guess that the link error is due to enabled CONFIG_DEBUG_VIRTUAL=y
> lm_alias() ends up with __phys_addr_symbol() function call which compiler can't optimize away.
> Technically we can declare __phys_addr_symbol() with __attribute__((pure)), so compiler will
> be able to optimize away this call, because the result should be unused.
> But I'm not sure we really want that, because it's debug function and even if the result is unused
> we might want to still have a check if symbol address is correct.
> 
> 
> I would probably prefer 3rd option, but I don't really have very strong opinion, so either way is fine.

I also prefer 3rd option (but 1st or 2nd is also fine to me)

That's two votes, I'll do 2nd option in the follow-up series unless
Dave or somebody else objects?

-- 
Cheers,
Harry / Hyeonggon