arm64: Enable vmalloc-huge with ptdump

Our goal is to move towards enabling vmalloc-huge by default on arm64 so as to reduce TLB pressure. Therefore, we need a way to analyze the portion of block mappings in vmalloc space we can get on a production system; this can be done through ptdump, but currently we disable vmalloc-huge if CONFIG_PTDUMP_DEBUGFS is on. The reason is that lazy freeing of kernel pagetables via vmap_try_huge_pxd() may race with ptdump, so ptdump may dereference a bogus address.

To solve this, we need to synchronize ptdump_walk_pgd() with pud_free_pmd_page() and pmd_free_pte_page().

Since this race is very unlikely to happen in practice, we do not want to penalize other code paths taking the init_mm mmap_lock. Therefore, we use static keys. ptdump will enable the static key - upon observing that, the vmalloc table freeing path will get patched in with an mmap_read_lock/unlock sequence. A code comment explains in detail, how a combination of acquire sematics of static_branch_enable() and the

barriers in __flush_tlb_kernel_pgtable() ensures that ptdump will never get a hold on the address of a freed PMD or PTE table.

For an (almost) rigorous proof of correctness, one may look at:
https://lore.kernel.org/all/[email protected]/

Reviewed-by Ryan Roberts <[email protected]>

Signed-off-by Dev Jain <[email protected]>

---
Rebased on 6.16-rc7.

mm-selfests pass. No issues were observed while parallelly running test_vmalloc.sh (which stresses the vmalloc subsystem) and dumping the kernel pagetable through sysfs in a loop.

v4->v5:
- The arch_vmap_pxd_supported() changes were dropped by mistake in between the revisions, add them back (Anshuman)
- Rewrite cover letter, drop stray change, add arm64_ptdump_walk_pgd() helper, rename ptdump_lock_key -> arm64_ptdump_lock_key, add comment above __pmd_free_pte_page() to explain when the lock won't be taken (Anshuman)
- Rewrite the comment explaining the synchronization logic (Catalin)

v3->v4:
- Lock-unlock immediately
- Simplify includes

v2->v3:
- Use static key mechanism

v1->v2:
- Take lock only when CONFIG_PTDUMP_DEBUGFS is on
- In case of pud_free_pmd_page(), isolate the PMD table to avoid taking the lock 512 times again via pmd_free_pte_page()

---
 arch/arm64/include/asm/ptdump.h  |  2 +
 arch/arm64/include/asm/vmalloc.h |  6 +--
 arch/arm64/mm/mmu.c              | 86 ++++++++++++++++++++++++++++++--
 arch/arm64/mm/ptdump.c           | 11 +++-
 4 files changed, 95 insertions(+), 10 deletions(-)

arch/arm64/include/asm/ptdump.h +2 -0

index fded5358641f..baff24004459 100644
@@ -7,6 +7,8 @@
 
 #include <linux/ptdump.h>
 
+DECLARE_STATIC_KEY_FALSE(arm64_ptdump_lock_key);
+
 #ifdef CONFIG_PTDUMP
 
 #include <linux/mm_types.h>

arch/arm64/include/asm/vmalloc.h +0 -0

index 12f534e8f3ed..e835fd437ae0 100644

arch/arm64/include/asm/vmalloc.h +2 -4

-	return pud_sect_supported() &&
-	       !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
+	return pud_sect_supported();
 }
 
 #define arch_vmap_pmd_supported arch_vmap_pmd_supported
 static inline bool arch_vmap_pmd_supported(pgprot_t prot)
 {
-	/* See arch_vmap_pud_supported() */
-	return !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
+	return true;
 }
 
 #define arch_vmap_pte_range_map_size arch_vmap_pte_range_map_size

arch/arm64/mm/mmu.c +0 -0

index 00ab1d648db6..49932c1dd34e 100644

arch/arm64/mm/mmu.c +1 -0

+
 enum pgtable_type {
 	TABLE_PTE,
 	TABLE_PMD,

arch/arm64/mm/mmu.c +18 -0

 {
 	pte_t *table;
 	pmd_t pmd;

@@ -1279,13 +1286,24 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 		return 1;
 	}
 
+	/* See comment in pud_free_pmd_page for static key logic */
 	table = pte_offset_kernel(pmdp, addr);
 	pmd_clear(pmdp);
 	__flush_tlb_kernel_pgtable(addr);
+	if (static_branch_unlikely(&arm64_ptdump_lock_key) && lock) {
+		mmap_read_lock(&init_mm);
+		mmap_read_unlock(&init_mm);
+	}
+
 	pte_free_kernel(NULL, table);
 	return 1;
 }
 
+int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
+{
+	return __pmd_free_pte_page(pmdp, addr, true);
+}
+
 int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 {
 	pmd_t *table;

@@ -1301,16 +1319,76 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 	}
 
 	table = pmd_offset(pudp, addr);
+
+	/*
+	 * Our objective is to prevent ptdump from reading a PMD table which has
+	 * been freed.  Assume that ptdump_walk_pgd() (call this thread T1)
+	 * executes completely on CPU1 and pud_free_pmd_page() (call this thread
+	 * T2) executes completely on CPU2. Let the region sandwiched by the
+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).

arch/arm64/mm/mmu.c +11 -0

+	 *
+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch
+	 * will be visible to all CPUs before T1 can enter CS. The static
+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),

arch/arm64/mm/mmu.c +8 -3

+	pud_clear(pudp);
+	__flush_tlb_kernel_pgtable(addr);
+	if (static_branch_unlikely(&arm64_ptdump_lock_key)) {
+		mmap_read_lock(&init_mm);
+		mmap_read_unlock(&init_mm);
+	}
+
 	pmdp = table;
 	next = addr;
 	end = addr + PUD_SIZE;
 	do {
 		if (pmd_present(pmdp_get(pmdp)))
-			pmd_free_pte_page(pmdp, next);
+			__pmd_free_pte_page(pmdp, next, false);
 	} while (pmdp++, next += PMD_SIZE, next != end);
 
-	pud_clear(pudp);
-	__flush_tlb_kernel_pgtable(addr);
 	pmd_free(NULL, table);
 	return 1;
 }

arch/arm64/mm/ptdump.c +0 -0

index 421a5de806c6..65335c7ba482 100644

arch/arm64/mm/ptdump.c +3 -2

+
 void ptdump_walk(struct seq_file *s, struct ptdump_info *info)
 {
 	unsigned long end = ~0UL;

@@ -311,7 +318,7 @@ void ptdump_walk(struct seq_file *s, struct ptdump_info *info)
 		}
 	};
 
-	ptdump_walk_pgd(&st.ptdump, info->mm, NULL);
+	arm64_ptdump_walk_pgd(&st.ptdump, info->mm);
 }
 
 static void __init ptdump_initialize(void)

@@ -353,7 +360,7 @@ bool ptdump_check_wx(void)
 		}
 	};
 
-	ptdump_walk_pgd(&st.ptdump, &init_mm, NULL);
+	arm64_ptdump_walk_pgd(&st.ptdump, &init_mm);
 
 	if (st.wx_pages || st.uxn_pages) {
 		pr_warn("Checked W+X mappings: FAILED, %lu W+X pages found, %lu non-UXN pages found\n",

A very small nit -

arm64/mm: Enable vmalloc-huge with ptdump

Quoted 11 lines

Our goal is to move towards enabling vmalloc-huge by default on arm64 so as to reduce TLB pressure. Therefore, we need a way to analyze the portion of block mappings in vmalloc space we can get on a production system; this can be done through ptdump, but currently we disable vmalloc-huge if CONFIG_PTDUMP_DEBUGFS is on. The reason is that lazy freeing of kernel pagetables via vmap_try_huge_pxd() may race with ptdump, so ptdump may dereference a bogus address.

To solve this, we need to synchronize ptdump_walk_pgd() with pud_free_pmd_page() and pmd_free_pte_page().

Quoted 6 lines

Since this race is very unlikely to happen in practice, we do not want to penalize other code paths taking the init_mm mmap_lock. Therefore, we use static keys. ptdump will enable the static key - upon observing that, the vmalloc table freeing path will get patched in with an mmap_read_lock/unlock sequence. A code comment explains in detail, how a combination of acquire sematics of static_branch_enable() and the

typo ^^^^^^^^ s/sematics/semantics

Quoted 62 lines

barriers in __flush_tlb_kernel_pgtable() ensures that ptdump will never get a hold on the address of a freed PMD or PTE table.

For an (almost) rigorous proof of correctness, one may look at:
https://lore.kernel.org/all/[email protected]/

Reviewed-by Ryan Roberts <[email protected]>

Signed-off-by Dev Jain <[email protected]>

---
Rebased on 6.16-rc7.

mm-selfests pass. No issues were observed while parallelly running test_vmalloc.sh (which stresses the vmalloc subsystem) and dumping the kernel pagetable through sysfs in a loop.

v4->v5:
- The arch_vmap_pxd_supported() changes were dropped by mistake in between the revisions, add them back (Anshuman)
- Rewrite cover letter, drop stray change, add arm64_ptdump_walk_pgd() helper, rename ptdump_lock_key -> arm64_ptdump_lock_key, add comment above __pmd_free_pte_page() to explain when the lock won't be taken (Anshuman)
- Rewrite the comment explaining the synchronization logic (Catalin)

v3->v4:
- Lock-unlock immediately
- Simplify includes

v2->v3:
- Use static key mechanism

v1->v2:
- Take lock only when CONFIG_PTDUMP_DEBUGFS is on
- In case of pud_free_pmd_page(), isolate the PMD table to avoid taking the lock 512 times again via pmd_free_pte_page()

---
  arch/arm64/include/asm/ptdump.h  |  2 +
  arch/arm64/include/asm/vmalloc.h |  6 +--
  arch/arm64/mm/mmu.c              | 86 ++++++++++++++++++++++++++++++--
  arch/arm64/mm/ptdump.c           | 11 +++-
  4 files changed, 95 insertions(+), 10 deletions(-)

arch/arm64/include/asm/ptdump.h +2 -0

index fded5358641f..baff24004459 100644
@@ -7,6 +7,8 @@
 
  #include <linux/ptdump.h>
 
+DECLARE_STATIC_KEY_FALSE(arm64_ptdump_lock_key);
+
  #ifdef CONFIG_PTDUMP
 
  #include <linux/mm_types.h>

arch/arm64/include/asm/vmalloc.h +0 -0

index 12f534e8f3ed..e835fd437ae0 100644

arch/arm64/include/asm/vmalloc.h static inline bool arch_vmap_pud_supported(pgprot_t prot) · line 14

Quoted 3 lines

arch/arm64/include/asm/vmalloc.h +0 -0

@@ -12,15 +12,13 @@ static inline bool arch_vmap_pud_supported(pgprot_t prot)
  	/*
  	 * SW table walks can't handle removal of intermediate entries.
  	 */

This above comment can now be dropped.

arch/arm64/include/asm/vmalloc.h static inline bool arch_vmap_pud_supported(pgprot_t prot) · line 16

Quoted 4 lines

arch/arm64/include/asm/vmalloc.h +1 -2

-	return pud_sect_supported() &&
-	       !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
+	return pud_sect_supported();
  }

Quoted 18 lines

arch/arm64/include/asm/vmalloc.h +1 -2

  #define arch_vmap_pmd_supported arch_vmap_pmd_supported
  static inline bool arch_vmap_pmd_supported(pgprot_t prot)
  {
-	/* See arch_vmap_pud_supported() */
-	return !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
+	return true;
  }
 
  #define arch_vmap_pte_range_map_size arch_vmap_pte_range_map_size

arch/arm64/mm/mmu.c +0 -0

index 00ab1d648db6..49932c1dd34e 100644
@@ -46,6 +46,8 @@
  #define NO_CONT_MAPPINGS	BIT(1)
  #define NO_EXEC_MAPPINGS	BIT(2)	/* assumes FEAT_HPDS is not used */
 

arch/arm64/mm/mmu.c line 49

Quoted 1 line

arch/arm64/mm/mmu.c +1 -0

+DEFINE_STATIC_KEY_FALSE(arm64_ptdump_lock_key);

A short introduction about the static key's purpose might be helpful.

Quoted 9 lines

arch/arm64/mm/mmu.c +1 -0

+
  enum pgtable_type {
  	TABLE_PTE,
  	TABLE_PMD,

@@ -1267,7 +1269,12 @@ int pmd_clear_huge(pmd_t *pmdp)
  	return 1;
  }
 

arch/arm64/mm/mmu.c int pmd_clear_huge(pmd_t *pmdp) · line 1276

Quoted 6 lines

arch/arm64/mm/mmu.c +5 -1

-int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
+/*
+ * If PMD has been isolated via pud_free_pmd_page(), ptdump cannot get
+ * a hold to it, so no need to serialize with mmap_lock, hence lock
+ * will be passed as false here. Otherwise, lock will be true.
+ */

This comment should be split into two and added near their respective call sites with and without the kernel mmap_lock requirement.

arch/arm64/mm/mmu.c int pmd_clear_huge(pmd_t *pmdp) · line 1277

Quoted 2 lines

arch/arm64/mm/mmu.c +1 -0

+static int __pmd_free_pte_page(pmd_t *pmdp, unsigned long addr, bool lock)IIUC 'lock' is the need for taking mmap_lock in init_mm. Hence changing

that as 'mmap_lock' or 'kernel_mmap_lock' might be better which will also add some required context.

Quoted 44 lines

arch/arm64/mm/mmu.c +22 -0

  {
  	pte_t *table;
  	pmd_t pmd;

@@ -1279,13 +1286,24 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
  		return 1;
  	}
 
+	/* See comment in pud_free_pmd_page for static key logic */
  	table = pte_offset_kernel(pmdp, addr);
  	pmd_clear(pmdp);
  	__flush_tlb_kernel_pgtable(addr);
+	if (static_branch_unlikely(&arm64_ptdump_lock_key) && lock) {
+		mmap_read_lock(&init_mm);
+		mmap_read_unlock(&init_mm);
+	}
+
  	pte_free_kernel(NULL, table);
  	return 1;
  }
 
+int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
+{
+	return __pmd_free_pte_page(pmdp, addr, true);
+}
+
  int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
  {
  	pmd_t *table;

@@ -1301,16 +1319,76 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
  	}
 
  	table = pmd_offset(pudp, addr);
+
+	/*
+	 * Our objective is to prevent ptdump from reading a PMD table which has
+	 * been freed.  Assume that ptdump_walk_pgd() (call this thread T1)
+	 * executes completely on CPU1 and pud_free_pmd_page() (call this thread
+	 * T2) executes completely on CPU2. Let the region sandwiched by the
+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1338

Quoted 6 lines

arch/arm64/mm/mmu.c +6 -0

+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.
+	 * T2 will block until T1 drops the lock, so pmd_free() will only be
+	 * executed after T1 exits CS.

T2 blocks here because ptdump_walk_pgd() takes mmap_write_lock(). This needs to be mentioned.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1344

Quoted 6 lines

arch/arm64/mm/mmu.c +5 -0

+	 *> +	 * Case 1 (b): T2 acquires the lock before T1 can.
+	 * The sequence of barriers issued in __flush_tlb_kernel_pgtable()
+	 * ensures that an empty PUD (via pud_clear()) is visible to T1 before
+	 * T1 can enter CS, therefore it is impossible for the CS to get hold
+	 * of the address of the isolated PMD table.

Agreed. mmap_write_lock() from ptdump_walk_pgd() will proceed after T2 calls mmap_read_unlock(). So protection from race comes from the fact that T1 can never get hold of isolated PMD table not because of the mmap_lock.

Quoted 16 lines

arch/arm64/mm/mmu.c +15 -0

+	 *
+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch
+	 * will be visible to all CPUs before T1 can enter CS. The static
+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),
+	 * then all CPUs will have observed an empty PUD ... (ii)
+	 * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+	 * before entering CS => it is impossible for the CS to get hold of
+	 * the address of the isolated PMD table. Q.E.D

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1366

Quoted 6 lines

arch/arm64/mm/mmu.c +6 -0

+	 *> +	 * We have proven that the claim is true on the assumption that
+	 * there is no context switch for T1 and T2. Note that the reasoning
+	 * of the proof uses barriers operating on the inner shareable domain,
+	 * which means that they will affect all CPUs, and also a context switch
+	 * will insert extra barriers into the code paths => the claim will
+	 * stand true even if we drop the assumption.

Do these all rest on the fact that static_branch_enable() and mmap_write_lock() have acquire semantics available via a particular class of barrier instructions

? In which case should the generation of those instructions via these functions be asserted for the above T1/T2 guarantee to hold ? Just curious.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1376

Quoted 11 lines

arch/arm64/mm/mmu.c +10 -0

+	 *
+	 * It is also worth reasoning whether something can go wrong via
+	 * pud_free_pmd_page() -> __pmd_free_pte_page(), since the latter
+	 * will be called locklessly on this code path.
+	 *
+	 * For Case 1 (a), T2 will block until CS is finished, so we are safe.
+	 * For Case 1 (b) and Case 2, the PMD table will be isolated before
+	 * T1 can enter CS, therefore it is safe for T2 to operate on the
+	 * PMD table locklessly.
+	 */

Although looks reasonable - the above comment block is going to be difficult to process after time has passed :) Just wondering could it some how be simplified ?

Quoted 27 lines

arch/arm64/mm/mmu.c +8 -3

+	pud_clear(pudp);
+	__flush_tlb_kernel_pgtable(addr);
+	if (static_branch_unlikely(&arm64_ptdump_lock_key)) {
+		mmap_read_lock(&init_mm);
+		mmap_read_unlock(&init_mm);
+	}
+
  	pmdp = table;
  	next = addr;
  	end = addr + PUD_SIZE;
  	do {
  		if (pmd_present(pmdp_get(pmdp)))
-			pmd_free_pte_page(pmdp, next);
+			__pmd_free_pte_page(pmdp, next, false);
  	} while (pmdp++, next += PMD_SIZE, next != end);
 
-	pud_clear(pudp);
-	__flush_tlb_kernel_pgtable(addr);
  	pmd_free(NULL, table);
  	return 1;
  }

arch/arm64/mm/ptdump.c +0 -0

index 421a5de806c6..65335c7ba482 100644

arch/arm64/mm/ptdump.c void note_page_flush(struct ptdump_state *pt_st) · line 291

Quoted 9 lines

arch/arm64/mm/ptdump.c +6 -0

@@ -283,6 +283,13 @@ void note_page_flush(struct ptdump_state *pt_st)
  	note_page(pt_st, 0, -1, pte_val(pte_zero));
  }
 
+static void arm64_ptdump_walk_pgd(struct ptdump_state *st, struct mm_struct *mm)
+{
+	static_branch_enable(&arm64_ptdump_lock_key);
+	ptdump_walk_pgd(st, mm, NULL);
+	static_branch_disable(&arm64_ptdump_lock_key);
+}

Encapsulating generic ptdump_walk_pgd() between arm64 platform specific static key enable/disable requirement does make sense.

Quoted 22 lines

arch/arm64/mm/ptdump.c +3 -2

+
  void ptdump_walk(struct seq_file *s, struct ptdump_info *info)
  {
  	unsigned long end = ~0UL;

@@ -311,7 +318,7 @@ void ptdump_walk(struct seq_file *s, struct ptdump_info *info)
  		}
  	};
 
-	ptdump_walk_pgd(&st.ptdump, info->mm, NULL);
+	arm64_ptdump_walk_pgd(&st.ptdump, info->mm);
  }
 
  static void __init ptdump_initialize(void)

@@ -353,7 +360,7 @@ bool ptdump_check_wx(void)
  		}
  	};
 
-	ptdump_walk_pgd(&st.ptdump, &init_mm, NULL);
+	arm64_ptdump_walk_pgd(&st.ptdump, &init_mm);
 
  	if (st.wx_pages || st.uxn_pages) {
  		pr_warn("Checked W+X mappings: FAILED, %lu W+X pages found, %lu non-UXN pages found\n",

arch/arm64/mm/mmu.c +0 -0

index 00ab1d648db6..49932c1dd34e 100644

On Wed, Jul 23, 2025 at 09:48:27PM +0530, Dev Jain wrote:

Quoted 11 lines

arch/arm64/mm/mmu.c +10 -0

+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:
+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.
+	 * T2 will block until T1 drops the lock, so pmd_free() will only be

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1338

Quoted 1 line

arch/arm64/mm/mmu.c +1 -0

+	 * executed after T1 exits CS.

This assumes that there is some ordering between unlock and pmd_free() (e.g. some poisoning of the old page). The unlock only gives us release semantics, not acquire. It just happens that we have an atomic dec-and-test down the __free_pages() path but I'm not convinced we should rely on it unless free_pages() has clear semantics on ordering related to prior memory writes.

Quoted 1 line · Catalin Marinas

On Wed, Jul 23, 2025 at 09:48:27PM +0530, Dev Jain wrote:

[...]

Quoted 9 lines

Quoted 9 lines

arch/arm64/mm/mmu.c +9 -0

+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:
+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.

Quoted 8 lines

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1338

Quoted 2 lines

arch/arm64/mm/mmu.c +2 -0

+	 * T2 will block until T1 drops the lock, so pmd_free() will only be
+	 * executed after T1 exits CS.

This assumes that there is some ordering between unlock and pmd_free() (e.g. some poisoning of the old page). The unlock only gives us release semantics, not acquire. It just happens that we have an atomic dec-and-test down the __free_pages() path but I'm not convinced we should rely on it unless free_pages() has clear semantics on ordering related to prior memory writes.

I can understand how pmd_free() could be re-ordered before the unlock, but surely it can't be reorded before the lock? I need to go unlearn everything I thought I understood about locking if that's the case...

Quoted 21 lines · Ryan Roberts

Quoted 19 lines · Catalin Marinas

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1338

Quoted 11 lines · Dev Jain

arch/arm64/mm/mmu.c +11 -0

+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:
+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.
+	 * T2 will block until T1 drops the lock, so pmd_free() will only be
+	 * executed after T1 exits CS.

This assumes that there is some ordering between unlock and pmd_free() (e.g. some poisoning of the old page). The unlock only gives us release semantics, not acquire. It just happens that we have an atomic dec-and-test down the __free_pages() path but I'm not convinced we should rely on it unless free_pages() has clear semantics on ordering related to prior memory writes.

Quoted 3 lines

I can understand how pmd_free() could be re-ordered before the unlock, but surely it can't be reorded before the lock? I need to go unlearn everything I thought I understood about locking if that's the case...

You are correct, what Catalin is saying is that my reasoning has a hole. There is no obvious ordering between unlock and free(), but

mmap_write_unlock() will happen before mmap_read_lock() ... (i) mmap_read_lock() will happen before pmd_free() ... (ii)

which lets us conclude that mmap_write_unlock() will happen before pmd_free().

arch/arm64/include/asm/vmalloc.h static inline bool arch_vmap_pud_supported(pgprot_t prot) · line 15

Quoted 11 lines · Dev Jain

arch/arm64/include/asm/vmalloc.h +1 -2

index 12f534e8f3ed..e835fd437ae0 100644
@@ -12,15 +12,13 @@ static inline bool arch_vmap_pud_supported(pgprot_t prot)
  	/*
  	 * SW table walks can't handle removal of intermediate entries.
  	 */
-	return pud_sect_supported() &&
-	       !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
+	return pud_sect_supported();

Does the "SW table walks..." comment still make sense?

Quoted 5 lines

arch/arm64/mm/mmu.c +0 -0

index 00ab1d648db6..49932c1dd34e 100644

[...]

Quoted 3 lines

arch/arm64/mm/mmu.c +0 -0

@@ -1301,16 +1319,76 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
  	}
 

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1327

Quoted 7 lines

arch/arm64/mm/mmu.c +6 -0

  	table = pmd_offset(pudp, addr);
+
+	/*
+	 * Our objective is to prevent ptdump from reading a PMD table which has
+	 * been freed.  Assume that ptdump_walk_pgd() (call this thread T1)
+	 * executes completely on CPU1 and pud_free_pmd_page() (call this thread
+	 * T2) executes completely on CPU2. Let the region sandwiched by the

I think "executes completely on CPU*" is confusing. Just talk about threads as they can be migrated between CPUs and the memory model is preserved.

Quoted 10 lines

arch/arm64/mm/mmu.c +9 -0

+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:
+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1338

Quoted 2 lines

arch/arm64/mm/mmu.c +2 -0

+	 * T2 will block until T1 drops the lock, so pmd_free() will only be
+	 * executed after T1 exits CS.

This assumes that there is some ordering between unlock and pmd_free() (e.g. some poisoning of the old page). The unlock only gives us release semantics, not acquire. It just happens that we have an atomic dec-and-test down the __free_pages() path but I'm not convinced we should rely on it unless free_pages() has clear semantics on ordering related to prior memory writes.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1344

Quoted 7 lines

arch/arm64/mm/mmu.c +6 -0

+	 *
+	 * Case 1 (b): T2 acquires the lock before T1 can.
+	 * The sequence of barriers issued in __flush_tlb_kernel_pgtable()
+	 * ensures that an empty PUD (via pud_clear()) is visible to T1 before
+	 * T1 can enter CS, therefore it is impossible for the CS to get hold
+	 * of the address of the isolated PMD table.

Is this the effect of the barriers in the TLB flushing or the release semantics of the unlock?

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1349

Quoted 6 lines

arch/arm64/mm/mmu.c +5 -0

+	 *
+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch

Does static_branch_disable() have release semantics? I think it does via some atomic_cmpxchg() but it's worth spelling it out.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1350

Quoted 2 lines

arch/arm64/mm/mmu.c +1 -0

+	 * will be visible to all CPUs before T1 can enter CS. The static

As I mentioned on a previous version, this visibility is not absolute. You could say that it will be observed by other CPUs before they observe the write_lock.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1359

Quoted 10 lines

arch/arm64/mm/mmu.c +9 -0

+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),
+	 * then all CPUs will have observed an empty PUD ... (ii)
+	 * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+	 * before entering CS => it is impossible for the CS to get hold of
+	 * the address of the isolated PMD table. Q.E.D

OK, I nearly got lost. That's not a straightforward memory ordering with as we have instruction updates with ISB as part of the TLB flushing. I concluded last time I looked that this branch patching part works because we also have kick_all_cpus_sync() as part of the static branch update.

Stepping back to a simpler model, let's say that the static key is a variable. I wrote this quick test for herd7 and the Linux kernel memory model (fairly easy to play with):

-------------------------------------8<--------------------------------------- C pte_free_ptdump

(*
* $ cd tools/memory-model
* $ herd7 -conf linux-kernel.cfg path/to/pte_free_ptdump.litmus *)

{
	pmd = 1;				(* allocated pmd *)
	pte_page = 1;				(* valid pte page pointed at by the pmd *)
}

// pmd_free_pte_page()
P0(spinlock_t *init_mm, int *ptdump_lock_key, int *pmd, int *pte_page)

{
	WRITE_ONCE(*pmd, 0);			// pmd_clear()
	smp_mb();
	if (READ_ONCE(*ptdump_lock_key)) {	// static_branch() approximation
		spin_lock(init_mm);		// mmap_read_lock()
		spin_unlock(init_mm);		// mmap_read_unlock()
	}
	smp_mb();
	WRITE_ONCE(*pte_page, 0);		// pte_free_kernel()
}

// ptdump_walk_pgd()
P1(spinlock_t *init_mm, int *ptdump_lock_key, int *pmd, int *pte_page)

{
	int val;
	int page;

	WRITE_ONCE(*ptdump_lock_key, 1);	// static_branch_enable()
	smp_mb();
	spin_lock(init_mm);			// mmap_write_lock()
	val = READ_ONCE(*pmd);
	page = READ_ONCE(*pte_page);		// freed pte page?
	spin_unlock(init_mm);			// mmap_write_unlock()
	smp_mb();
	WRITE_ONCE(*ptdump_lock_key, 0);	// static_branch_disable()
}

exists(1:val=1 /\ 1:page=0) -------------------------------------8<---------------------------------------

I sprinkled some necessary smp_mb() but in most cases we have release/acquire semantics. It does show that we need a barrier before the page freeing. We also need to acquire for enabling the static key and release for disabling it.

Next step is to assess that replacing the static key variable read/write with code patching preserves the model.

Quoted 9 lines · Catalin Marinas

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1350

Quoted 5 lines · Dev Jain

arch/arm64/mm/mmu.c +5 -0

+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch
+	 * will be visible to all CPUs before T1 can enter CS. The static

Does static_branch_disable() have release semantics? I think it does via some atomic_cmpxchg() but it's worth spelling it out.

Yes it *should* have, but this proof is already getting complicated so I thought, let's not mention anything which is not required in the reasoning of the proof :)

Quoted 3 lines

As I mentioned on a previous version, this visibility is not absolute. You could say that it will be observed by other CPUs before they observe the write_lock.

Hmm. I probably am bad at explaining all of this in English :)

Quoted 62 lines

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1359

Quoted 9 lines

arch/arm64/mm/mmu.c +9 -0

+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),
+	 * then all CPUs will have observed an empty PUD ... (ii)
+	 * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+	 * before entering CS => it is impossible for the CS to get hold of
+	 * the address of the isolated PMD table. Q.E.D

OK, I nearly got lost. That's not a straightforward memory ordering with as we have instruction updates with ISB as part of the TLB flushing. I concluded last time I looked that this branch patching part works because we also have kick_all_cpus_sync() as part of the static branch update.

Stepping back to a simpler model, let's say that the static key is a variable. I wrote this quick test for herd7 and the Linux kernel memory model (fairly easy to play with):

-------------------------------------8<--------------------------------------- C pte_free_ptdump

(*
* $ cd tools/memory-model
* $ herd7 -conf linux-kernel.cfg path/to/pte_free_ptdump.litmus *)

{
	pmd = 1;				(* allocated pmd *)
	pte_page = 1;				(* valid pte page pointed at by the pmd *)
}

// pmd_free_pte_page()
P0(spinlock_t *init_mm, int *ptdump_lock_key, int *pmd, int *pte_page)

{
	WRITE_ONCE(*pmd, 0);			// pmd_clear()
	smp_mb();
	if (READ_ONCE(*ptdump_lock_key)) {	// static_branch() approximation
		spin_lock(init_mm);		// mmap_read_lock()
		spin_unlock(init_mm);		// mmap_read_unlock()
	}
	smp_mb();
	WRITE_ONCE(*pte_page, 0);		// pte_free_kernel()
}

// ptdump_walk_pgd()
P1(spinlock_t *init_mm, int *ptdump_lock_key, int *pmd, int *pte_page)

{
	int val;
	int page;

	WRITE_ONCE(*ptdump_lock_key, 1);	// static_branch_enable()
	smp_mb();
	spin_lock(init_mm);			// mmap_write_lock()
	val = READ_ONCE(*pmd);
	page = READ_ONCE(*pte_page);		// freed pte page?
	spin_unlock(init_mm);			// mmap_write_unlock()
	smp_mb();
	WRITE_ONCE(*ptdump_lock_key, 0);	// static_branch_disable()
}

Quoted 7 lines

exists(1:val=1 /\ 1:page=0) -------------------------------------8<---------------------------------------

I sprinkled some necessary smp_mb() but in most cases we have release/acquire semantics. It does show that we need a barrier before the page freeing. We also need to acquire for enabling the static key and release for disabling it.

Quoted 2 lines

Next step is to assess that replacing the static key variable read/write with code patching preserves the model.

Thanks for the litmus test!
I ran it and I get the same result. But I still cannot figure out where is the hole in my proof.

I think you raise an objection to Case 2:

T1: T2:

WRITE_ONCE(*pmd, 0) : t1 WRITE_ONCE(*ptdump_lock_key, 1) : t3
smp_mb() // flush_tlb_kernel_pgtable cmpxchg(acquire write lock)

						smp_mb()

READ_ONCE(*ptdump_lock_key) = 0 : t2 val = READ_ONCE(*pmd) : t4
WRITE_ONCE(*pte_page, 0) page = READ_ONCE(*pte_page)

						smp_mb()
						cmpxchg(release write lock)

Let t_i be the global timestamps of the execution of the labelled instructions (note that each labelled instruction is atomic so assigning a timestamp makes sense).

The fact that we read ptdump_lock_key as 0 implies that t2 < t3 ... (i)

Now,
t1 < t2 because of barrier (ii)
t3 < t4 because of barrier (iii)

So we conclude that t1 < t4, so val will be observed as 0.

Quoted 11 lines · Dev Jain

Quoted 8 lines · Catalin Marinas

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1350

Quoted 5 lines · Dev Jain

arch/arm64/mm/mmu.c +5 -0

+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch
+	 * will be visible to all CPUs before T1 can enter CS. The static

Does static_branch_disable() have release semantics? I think it does via some atomic_cmpxchg() but it's worth spelling it out.

Yes it *should* have, but this proof is already getting complicated so I thought, let's not mention anything which is not required in the reasoning of the proof :)

The static branch observability relative to the mmap_lock is essential, both when enabling and disabling the static key (i.e. you don't want the static branch to be seen disabled while T1 is in the critical section). That's why it's worth mentioning.

On the static_branch_enable() path, we have a kick_all_cpus_sync(), so the T1 update of mmap lock will become visible after the branch update (of course, also subject to correct ordering on T2, see more at the end of the email).

On the static_branch_disable() path, we need it to have release semantics _prior_ to disabling the key. We get this via the atomic_cmpxchg() in static_key_disable_cpuslocked().

So I think it's all good here from the T1 perspective, just capture this in a comment so that we remember in a couple of months time.

Quoted 5 lines

Quoted 4 lines

As I mentioned on a previous version, this visibility is not absolute. You could say that it will be observed by other CPUs before they observe the write_lock.

Hmm. I probably am bad at explaining all of this in English :)

It's the other way around - I don't think English is appropriate for explaining this ;).

Quoted 61 lines

Quoted 61 lines

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1359

Quoted 9 lines

arch/arm64/mm/mmu.c +9 -0

+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),
+	 * then all CPUs will have observed an empty PUD ... (ii)
+	 * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+	 * before entering CS => it is impossible for the CS to get hold of
+	 * the address of the isolated PMD table. Q.E.D

OK, I nearly got lost. That's not a straightforward memory ordering with as we have instruction updates with ISB as part of the TLB flushing. I concluded last time I looked that this branch patching part works because we also have kick_all_cpus_sync() as part of the static branch update.

Stepping back to a simpler model, let's say that the static key is a variable. I wrote this quick test for herd7 and the Linux kernel memory model (fairly easy to play with):

-------------------------------------8<--------------------------------------- C pte_free_ptdump

(*
* $ cd tools/memory-model
* $ herd7 -conf linux-kernel.cfg path/to/pte_free_ptdump.litmus *)

{
	pmd = 1;				(* allocated pmd *)
	pte_page = 1;				(* valid pte page pointed at by the pmd *)
}

// pmd_free_pte_page()
P0(spinlock_t *init_mm, int *ptdump_lock_key, int *pmd, int *pte_page)

{
	WRITE_ONCE(*pmd, 0);			// pmd_clear()
	smp_mb();
	if (READ_ONCE(*ptdump_lock_key)) {	// static_branch() approximation
		spin_lock(init_mm);		// mmap_read_lock()
		spin_unlock(init_mm);		// mmap_read_unlock()
	}
	smp_mb();
	WRITE_ONCE(*pte_page, 0);		// pte_free_kernel()
}

// ptdump_walk_pgd()
P1(spinlock_t *init_mm, int *ptdump_lock_key, int *pmd, int *pte_page)

{
	int val;
	int page;

	WRITE_ONCE(*ptdump_lock_key, 1);	// static_branch_enable()
	smp_mb();
	spin_lock(init_mm);			// mmap_write_lock()
	val = READ_ONCE(*pmd);
	page = READ_ONCE(*pte_page);		// freed pte page?
	spin_unlock(init_mm);			// mmap_write_unlock()
	smp_mb();
	WRITE_ONCE(*ptdump_lock_key, 0);	// static_branch_disable()
}

Quoted 7 lines

Quoted 7 lines

exists(1:val=1 /\ 1:page=0) -------------------------------------8<---------------------------------------

I sprinkled some necessary smp_mb() but in most cases we have release/acquire semantics. It does show that we need a barrier before the page freeing. We also need to acquire for enabling the static key and release for disabling it.

The smp_mb() before the write to *pte_page in the model above is only needed on the static key disabled path. As Ryan pointed out, on the other path we take the mmap_lock and we have acquire semantics.

Quoted 5 lines

Quoted 2 lines

Next step is to assess that replacing the static key variable read/write with code patching preserves the model.

Thanks for the litmus test!
I ran it and I get the same result. But I still cannot figure out where is the hole in my proof.

TBH, I couldn't fully follow the proof. The arm memory model is based on relations between accesses (e.g. read-from, coherence order) rather than timestamps. They may be equivalent but that's a whole new (and complex) proof to write.

So I'm happy with a simple English explanation and a formal model along the lines of the one I posted as the equivalent of a proof.

Quoted 1 line

I think you raise an objection to Case 2:

Let's follow your approach, you are missing some timestamps, adding them below:

Quoted 4 lines

T1: T2:

WRITE_ONCE(*pmd, 0) : t1 WRITE_ONCE(*ptdump_lock_key, 1) : t3
smp_mb() // flush_tlb_kernel_pgtable cmpxchg(acquire write lock)

						^^^ : t3'

Quoted 3 lines

						smp_mb()

READ_ONCE(*ptdump_lock_key) = 0 : t2 val = READ_ONCE(*pmd) : t4
WRITE_ONCE(*pte_page, 0) page = READ_ONCE(*pte_page)

^^^ : t2' ^^^ : t4'

Quoted 2 lines

						smp_mb()
						cmpxchg(release write lock)

						^^^ : t5
						smp_mb()
						WRITE_ONCE(*ptdump_lock_key, 0): t6

Quoted 5 lines

Let t_i be the global timestamps of the execution of the labelled instructions (note that each labelled instruction is atomic so assigning a timestamp makes sense).

The fact that we read ptdump_lock_key as 0 implies that t2 < t3 ... (i)

Or that t2 > t6, a plausible scenario in the litmus test above.

Quoted 5 lines

Now,
t1 < t2 because of barrier (ii)
t3 < t4 because of barrier (iii)

So we conclude that t1 < t4, so val will be observed as 0.

Without a barrier between t2 and t2', there's no guarantee that t2' > t2. If you remove the last smp_mb() on P0 in my litmus test, you'll see it failing (run herd7 with something like "-view evince -show prop" for a visualisation of the transitions and relations; you need graphviz installed as well).

The way it fails is that t4 is seeing the pmd=1 prior to t1, t2 is reading from the t6 write and t4' is reading pte_page=0 from t2' (since t2' < t2 is possible without a barrier).

A control dependency would work as well without a barrier, i.e.:

	if (READ_ONCE(*ptdump_lock_key)) {
		mmap_lock();
		mmap_unlock();
		WRITE_ONCE(*pte_page, 0);
	} else {
		WRITE_ONCE(*pte_page, 0);
	}

but the compiler is probably free to only issue a single WRITE_ONCE() irrespective of the ptdump_lock_key check.

Of course, using smp_load_acquire(ptdump_lock_key) would also work.

However, things get fuzzier as we don't have a classic load from the ptdump_lock_key but rather a patched instruction. We need to guarantee that t2' is issued after the t2 branch when the instruction is patched. The kick_all_cpus_sync() on the static key disable path doesn't help us since P0 (T2 in your description) may see the patched branch/nop and go straight to the WRITE_ONCE(*pte_page). Not sure what barrier helps here (after more sleep, I may have a better idea tomorrow).

Quoted 15 lines · Catalin Marinas

A control dependency would work as well without a barrier, i.e.:

	if (READ_ONCE(*ptdump_lock_key)) {
		mmap_lock();
		mmap_unlock();
		WRITE_ONCE(*pte_page, 0);
	} else {
		WRITE_ONCE(*pte_page, 0);
	}

but the compiler is probably free to only issue a single WRITE_ONCE() irrespective of the ptdump_lock_key check.

Of course, using smp_load_acquire(ptdump_lock_key) would also work.

Quoted 7 lines

However, things get fuzzier as we don't have a classic load from the ptdump_lock_key but rather a patched instruction. We need to guarantee that t2' is issued after the t2 branch when the instruction is patched. The kick_all_cpus_sync() on the static key disable path doesn't help us since P0 (T2 in your description) may see the patched branch/nop and go straight to the WRITE_ONCE(*pte_page). Not sure what barrier helps here (after more sleep, I may have a better idea tomorrow).

Got it. The hole in my proof is not with Case 2 but with Case 1: the assumption
in the reasoning is that pmd_free() will be observed after the patched-in
read lock/unlock, but that will happen when patching-in happens, for which
we need to observe the branch before the pmd_free(), but that isn't guaranteed
since there is no barrier between the if block and the pmd_free(), nor is there any control dependency, like you describe above. So, in pud_free_pmd_page, the entire block from "pmdp = table" till "pmd_free()" can be observed before the observation of the branch.

Reading tools/memory-model/Documentation/control-dependencies.txt, I interpret that the compiler is free to hoist out the WRITE_ONCE() out of the control block, and then we have the same problem, BUT I tested with herd and the test passes :)

arch/arm64/mm/mmu.c +45 -0

@@ -1301,16 +1319,76 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 	}
 
 	table = pmd_offset(pudp, addr);
+
+	/*
+	 * Our objective is to prevent ptdump from reading a PMD table which has
+	 * been freed.  Assume that ptdump_walk_pgd() (call this thread T1)
+	 * executes completely on CPU1 and pud_free_pmd_page() (call this thread
+	 * T2) executes completely on CPU2. Let the region sandwiched by the
+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:
+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.
+	 * T2 will block until T1 drops the lock, so pmd_free() will only be
+	 * executed after T1 exits CS.
+	 *
+	 * Case 1 (b): T2 acquires the lock before T1 can.
+	 * The sequence of barriers issued in __flush_tlb_kernel_pgtable()
+	 * ensures that an empty PUD (via pud_clear()) is visible to T1 before
+	 * T1 can enter CS, therefore it is impossible for the CS to get hold
+	 * of the address of the isolated PMD table.
+	 *
+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch
+	 * will be visible to all CPUs before T1 can enter CS. The static
+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),
+	 * then all CPUs will have observed an empty PUD ... (ii)
+	 * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+	 * before entering CS => it is impossible for the CS to get hold of
+	 * the address of the isolated PMD table. Q.E.D
+	 *
+	 * We have proven that the claim is true on the assumption that
+	 * there is no context switch for T1 and T2. Note that the reasoning
+	 * of the proof uses barriers operating on the inner shareable domain,
+	 * which means that they will affect all CPUs, and also a context switch
+	 * will insert extra barriers into the code paths => the claim will
+	 * stand true even if we drop the assumption.

arch/arm64/mm/ptdump.c +0 -0

index 421a5de806c6..65335c7ba482 100644

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Quoted 2 lines · Will Deacon

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Here's what we came up with. There's not a good way to express the IPI from kick_all_cpus_sync() but it turns out that the ISB from the TLB invalidation is sufficient anyway. Does it make sense to you?

Dev,

Quoted 1 line · Will Deacon

On Tue, Sep 16, 2025 at 11:30:26AM +0100, Will Deacon wrote:

Quoted 5 lines

Quoted 2 lines

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Here's what we came up with. There's not a good way to express the IPI from kick_all_cpus_sync() but it turns out that the ISB from the TLB invalidation is sufficient anyway. Does it make sense to you?

Any comments on the litmus test and/or my question about serialisation of the static key? I'll be moving the arm64 tree to "fixes only" soon as far as 6.18 is concerned.

Will

Quoted 2 lines · Will Deacon

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Here's what we came up with. There's not a good way to express the IPI from kick_all_cpus_sync() but it turns out that the ISB from the TLB invalidation is sufficient anyway. Does it make sense to you?

AArch64 ptdump
Variant=Ifetch
{
uint64_t pud=0xa110c;
uint64_t pmd;

0:X0=label:"P1:L0"; 0:X1=instr:"NOP"; 0:X2=lock; 0:X3=pud; 0:X4=pmd;
                     1:X1=0xdead;      1:X2=lock; 1:X3=pud; 1:X4=pmd;
}
  P0				| P1				;
  (* static_key_enable *)	| (* pud_free_pmd_page *)	;
  STR	W1, [X0]		| LDR	X9, [X3]		;
  DC	CVAU,X0			| STR	XZR, [X3]		;
  DSB	ISH			| DSB	ISH			;
  IC	IVAU,X0			| ISB				;
  DSB	ISH			|				;
  ISB				| (* static key *)		;
				| L0:				;
  (* mmap_lock *)		| B	out1			;
  Lwlock:			|				;
  MOV	W7, #1			| (* mmap_lock *)		;
  SWPA	W7, W8, [X2]		| Lrlock:			;
				| MOV	W7, #1			;
				| SWPA	W7, W8, [X2]		;
  (* walk pgtable *)		|				;
  LDR	X9, [X3]		| (* mmap_unlock *)		;
  CBZ	X9, out0		| STLR	WZR, [X2]		;
  EOR	X10, X9, X9		|				;
  LDR	X11, [X4, X10]		| out1:				;
				| EOR	X10, X9, X9		;

out0: | STR X1, [X4, X10] ;

exists (0:X8=0 /\ 1:X8=0 /\ (* Lock acquisitions succeed *)

	0:X9=0xa110c /\		(* P0 sees the valid PUD ...*)
	0:X11=0xdead)		(* ... but the freed PMD *)

Will

Quoted 40 lines · Will Deacon

Quoted 2 lines · Will Deacon

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Here's what we came up with. There's not a good way to express the IPI from kick_all_cpus_sync() but it turns out that the ISB from the TLB invalidation is sufficient anyway. Does it make sense to you?

AArch64 ptdump
Variant=Ifetch
{
uint64_t pud=0xa110c;
uint64_t pmd;

0:X0=label:"P1:L0"; 0:X1=instr:"NOP"; 0:X2=lock; 0:X3=pud; 0:X4=pmd;
                     1:X1=0xdead;      1:X2=lock; 1:X3=pud; 1:X4=pmd;
}
  P0				| P1				;
  (* static_key_enable *)	| (* pud_free_pmd_page *)	;
  STR	W1, [X0]		| LDR	X9, [X3]		;
  DC	CVAU,X0			| STR	XZR, [X3]		;
  DSB	ISH			| DSB	ISH			;
  IC	IVAU,X0			| ISB				;
  DSB	ISH			|				;
  ISB				| (* static key *)		;
				| L0:				;
  (* mmap_lock *)		| B	out1			;
  Lwlock:			|				;
  MOV	W7, #1			| (* mmap_lock *)		;
  SWPA	W7, W8, [X2]		| Lrlock:			;
				| MOV	W7, #1			;
				| SWPA	W7, W8, [X2]		;
  (* walk pgtable *)		|				;
  LDR	X9, [X3]		| (* mmap_unlock *)		;
  CBZ	X9, out0		| STLR	WZR, [X2]		;
  EOR	X10, X9, X9		|				;
  LDR	X11, [X4, X10]		| out1:				;
				| EOR	X10, X9, X9		;

out0: | STR X1, [X4, X10] ;

Quoted 6 lines

exists (0:X8=0 /\ 1:X8=0 /\ (* Lock acquisitions succeed *)

	0:X9=0xa110c /\		(* P0 sees the valid PUD ...*)
	0:X11=0xdead)		(* ... but the freed PMD *)

Will

Is the syntax correct? I cannot use the herd7 command to run this.

Quoted 41 lines · Dev Jain

Quoted 40 lines · Will Deacon

Quoted 2 lines · Will Deacon

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Here's what we came up with. There's not a good way to express the IPI from kick_all_cpus_sync() but it turns out that the ISB from the TLB invalidation is sufficient anyway. Does it make sense to you?

AArch64 ptdump
Variant=Ifetch
{
uint64_t pud=0xa110c;
uint64_t pmd;

0:X0=label:"P1:L0"; 0:X1=instr:"NOP"; 0:X2=lock; 0:X3=pud; 0:X4=pmd;
                      1:X1=0xdead;      1:X2=lock; 1:X3=pud; 1:X4=pmd;
}
   P0				| P1				;
   (* static_key_enable *)	| (* pud_free_pmd_page *)	;
   STR	W1, [X0]		| LDR	X9, [X3]		;
   DC	CVAU,X0			| STR	XZR, [X3]		;
   DSB	ISH			| DSB	ISH			;
   IC	IVAU,X0			| ISB				;
   DSB	ISH			|				;
   ISB				| (* static key *)		;
				| L0:				;
   (* mmap_lock *)		| B	out1			;
   Lwlock:			|				;
   MOV	W7, #1			| (* mmap_lock *)		;
   SWPA	W7, W8, [X2]		| Lrlock:			;
				| MOV	W7, #1			;
				| SWPA	W7, W8, [X2]		;
   (* walk pgtable *)		|				;
   LDR	X9, [X3]		| (* mmap_unlock *)		;
   CBZ	X9, out0		| STLR	WZR, [X2]		;
   EOR	X10, X9, X9		|				;
   LDR	X11, [X4, X10]		| out1:				;
				| EOR	X10, X9, X9		;
   out0:				| STR	X1, [X4, X10]		;

Quoted 7 lines

Quoted 6 lines

exists (0:X8=0 /\ 1:X8=0 /\ (* Lock acquisitions succeed *)

	0:X9=0xa110c /\		(* P0 sees the valid PUD ...*)
	0:X11=0xdead)		(* ... but the freed PMD *)

Will

Is the syntax correct? I cannot use the herd7 command to run this.

Weird, what happens? It runs for me:

$ herd7 -version
7.58+1, Rev: e39a86f5d59dee3174d08d9ab5b13155c75936fd

$ herd7 ptdump.litmus
Test ptdump Allowed
States 5
0:X8=0; 0:X9=0; 0:X11=0; 1:X8=0;
0:X8=0; 0:X9=0; 0:X11=0; 1:X8=1;
0:X8=0; 0:X9=659724; 0:X11=0; 1:X8=1;
0:X8=0; 0:X9=659724; 0:X11=57005; 1:X8=1;
0:X8=1; 0:X9=0; 0:X11=0; 1:X8=0;
No
Witnesses
Positive: 0 Negative: 9
Flag Assuming-common-inner-shareable-domain
Condition exists (0:X8=0 /\ 1:X8=0 /\ 0:X9=659724 /\ 0:X11=57005)
Observation ptdump Never 0 9
Time ptdump 1.65
Hash=238908ee9413a36507c61b92a31a366a

Will

Quoted 53 lines · Will Deacon

Quoted 41 lines · Dev Jain

Quoted 40 lines · Will Deacon

Quoted 2 lines · Will Deacon

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Here's what we came up with. There's not a good way to express the IPI from kick_all_cpus_sync() but it turns out that the ISB from the TLB invalidation is sufficient anyway. Does it make sense to you?

AArch64 ptdump
Variant=Ifetch
{
uint64_t pud=0xa110c;
uint64_t pmd;

0:X0=label:"P1:L0"; 0:X1=instr:"NOP"; 0:X2=lock; 0:X3=pud; 0:X4=pmd;
                      1:X1=0xdead;      1:X2=lock; 1:X3=pud; 1:X4=pmd;
}
   P0				| P1				;
   (* static_key_enable *)	| (* pud_free_pmd_page *)	;
   STR	W1, [X0]		| LDR	X9, [X3]		;
   DC	CVAU,X0			| STR	XZR, [X3]		;
   DSB	ISH			| DSB	ISH			;
   IC	IVAU,X0			| ISB				;
   DSB	ISH			|				;
   ISB				| (* static key *)		;
				| L0:				;
   (* mmap_lock *)		| B	out1			;
   Lwlock:			|				;
   MOV	W7, #1			| (* mmap_lock *)		;
   SWPA	W7, W8, [X2]		| Lrlock:			;
				| MOV	W7, #1			;
				| SWPA	W7, W8, [X2]		;
   (* walk pgtable *)		|				;
   LDR	X9, [X3]		| (* mmap_unlock *)		;
   CBZ	X9, out0		| STLR	WZR, [X2]		;
   EOR	X10, X9, X9		|				;
   LDR	X11, [X4, X10]		| out1:				;
				| EOR	X10, X9, X9		;
   out0:				| STR	X1, [X4, X10]		;

Quoted 6 lines

Quoted 5 lines

exists (0:X8=0 /\ 1:X8=0 /\ (* Lock acquisitions succeed *)

	0:X9=0xa110c /\		(* P0 sees the valid PUD ...*)
	0:X11=0xdead)		(* ... but the freed PMD *)

Is the syntax correct? I cannot use the herd7 command to run this.

Weird, what happens? It runs for me:

$ herd7 -version
7.58+1, Rev: e39a86f5d59dee3174d08d9ab5b13155c75936fd

Quoted 16 lines

$ herd7 ptdump.litmus
Test ptdump Allowed
States 5
0:X8=0; 0:X9=0; 0:X11=0; 1:X8=0;
0:X8=0; 0:X9=0; 0:X11=0; 1:X8=1;
0:X8=0; 0:X9=659724; 0:X11=0; 1:X8=1;
0:X8=0; 0:X9=659724; 0:X11=57005; 1:X8=1;
0:X8=1; 0:X9=0; 0:X11=0; 1:X8=0;
No
Witnesses
Positive: 0 Negative: 9
Flag Assuming-common-inner-shareable-domain
Condition exists (0:X8=0 /\ 1:X8=0 /\ 0:X9=659724 /\ 0:X11=57005)
Observation ptdump Never 0 9
Time ptdump 1.65
Hash=238908ee9413a36507c61b92a31a366a

I was using some other command :) yup this works.

I'll try posting the next version today itself, but will you be okay merging this if I post on Monday?

Hi Dev,

Quoted 49 lines · Dev Jain

arch/arm64/mm/mmu.c +45 -0

@@ -1301,16 +1319,76 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
  	}
 
  	table = pmd_offset(pudp, addr);
+
+	/*
+	 * Our objective is to prevent ptdump from reading a PMD table which has
+	 * been freed.  Assume that ptdump_walk_pgd() (call this thread T1)
+	 * executes completely on CPU1 and pud_free_pmd_page() (call this thread
+	 * T2) executes completely on CPU2. Let the region sandwiched by the
+	 * mmap_write_lock/unlock in T1 be called CS (the critical section).
+	 *
+	 * Claim: The CS of T1 will never operate on a freed PMD table.
+	 *
+	 * Proof:
+	 *
+	 * Case 1: The static branch is visible to T2.
+	 *
+	 * Case 1 (a): T1 acquires the lock before T2 can.
+	 * T2 will block until T1 drops the lock, so pmd_free() will only be
+	 * executed after T1 exits CS.
+	 *
+	 * Case 1 (b): T2 acquires the lock before T1 can.
+	 * The sequence of barriers issued in __flush_tlb_kernel_pgtable()
+	 * ensures that an empty PUD (via pud_clear()) is visible to T1 before
+	 * T1 can enter CS, therefore it is impossible for the CS to get hold
+	 * of the address of the isolated PMD table.
+	 *
+	 * Case 2: The static branch is not visible to T2.
+	 *
+	 * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+	 * have acquire semantics, it is guaranteed that the static branch
+	 * will be visible to all CPUs before T1 can enter CS. The static
+	 * branch not being visible to T2 therefore guarantees that T1 has
+	 * not yet entered CS .... (i)
+	 * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+	 * implies that if the invisibility of the static branch has been
+	 * observed by T2 (i.e static_branch_unlikely() is observed as false),
+	 * then all CPUs will have observed an empty PUD ... (ii)
+	 * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+	 * before entering CS => it is impossible for the CS to get hold of
+	 * the address of the isolated PMD table. Q.E.D
+	 *
+	 * We have proven that the claim is true on the assumption that
+	 * there is no context switch for T1 and T2. Note that the reasoning
+	 * of the proof uses barriers operating on the inner shareable domain,
+	 * which means that they will affect all CPUs, and also a context switch
+	 * will insert extra barriers into the code paths => the claim will
+	 * stand true even if we drop the assumption.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1376

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+	 *
+	 * It is also worth reasoning whether something can go wrong via
+	 * pud_free_pmd_page() -> __pmd_free_pte_page(), since the latter
+	 * will be called locklessly on this code path.
+	 *
+	 * For Case 1 (a), T2 will block until CS is finished, so we are safe.
+	 * For Case 1 (b) and Case 2, the PMD table will be isolated before
+	 * T1 can enter CS, therefore it is safe for T2 to operate on the
+	 * PMD table locklessly.
+	 */

Although I can see that you put a lot of effort into this comment, I think we should just remove it. Instead, we should have a litmus test in the commit message and probably just some small comments here to explain e.g. why the mmap_read_lock() critical section is empty.

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

In the meantime...

Quoted 8 lines

arch/arm64/mm/ptdump.c +0 -0

index 421a5de806c6..65335c7ba482 100644
@@ -283,6 +283,13 @@ void note_page_flush(struct ptdump_state *pt_st)
  	note_page(pt_st, 0, -1, pte_val(pte_zero));
  }
 

arch/arm64/mm/ptdump.c void note_page_flush(struct ptdump_state *pt_st) · line 291

Quoted 6 lines

arch/arm64/mm/ptdump.c +6 -0

+static void arm64_ptdump_walk_pgd(struct ptdump_state *st, struct mm_struct *mm)
+{
+	static_branch_enable(&arm64_ptdump_lock_key);
+	ptdump_walk_pgd(st, mm, NULL);
+	static_branch_disable(&arm64_ptdump_lock_key);
+}

What serialises the toggling of the static key here? For example, I can't see what prevents a kernel page-table dump running concurrently with a check_wx() and the key ending up in the wrong state.

Either we need an additional lock or perhaps using static_branch_{inc,dec}() would work instead? I haven't thought too hard about that but it looks like we need _something_.

Will

Quoted 66 lines · Will Deacon

Hi Dev,

Quoted 48 lines · Dev Jain

arch/arm64/mm/mmu.c +45 -0

@@ -1301,16 +1319,76 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
       }
         table = pmd_offset(pudp, addr);
+
+    /*
+     * Our objective is to prevent ptdump from reading a PMD table which has
+     * been freed.  Assume that ptdump_walk_pgd() (call this thread T1)
+     * executes completely on CPU1 and pud_free_pmd_page() (call this thread
+     * T2) executes completely on CPU2. Let the region sandwiched by the
+     * mmap_write_lock/unlock in T1 be called CS (the critical section).
+     *
+     * Claim: The CS of T1 will never operate on a freed PMD table.
+     *
+     * Proof:
+     *
+     * Case 1: The static branch is visible to T2.
+     *
+     * Case 1 (a): T1 acquires the lock before T2 can.
+     * T2 will block until T1 drops the lock, so pmd_free() will only be
+     * executed after T1 exits CS.
+     *
+     * Case 1 (b): T2 acquires the lock before T1 can.
+     * The sequence of barriers issued in __flush_tlb_kernel_pgtable()
+     * ensures that an empty PUD (via pud_clear()) is visible to T1 before
+     * T1 can enter CS, therefore it is impossible for the CS to get hold
+     * of the address of the isolated PMD table.
+     *
+     * Case 2: The static branch is not visible to T2.
+     *
+     * Since static_branch_enable() (via dmb(ish)) and mmap_write_lock()
+     * have acquire semantics, it is guaranteed that the static branch
+     * will be visible to all CPUs before T1 can enter CS. The static
+     * branch not being visible to T2 therefore guarantees that T1 has
+     * not yet entered CS .... (i)
+     * The sequence of barriers via __flush_tlb_kernel_pgtable() in T2
+     * implies that if the invisibility of the static branch has been
+     * observed by T2 (i.e static_branch_unlikely() is observed as false),
+     * then all CPUs will have observed an empty PUD ... (ii)
+     * Combining (i) and (ii), we conclude that T1 observes an empty PUD
+     * before entering CS => it is impossible for the CS to get hold of
+     * the address of the isolated PMD table. Q.E.D
+     *
+     * We have proven that the claim is true on the assumption that
+     * there is no context switch for T1 and T2. Note that the reasoning
+     * of the proof uses barriers operating on the inner shareable domain,
+     * which means that they will affect all CPUs, and also a context switch
+     * will insert extra barriers into the code paths => the claim will
+     * stand true even if we drop the assumption.

arch/arm64/mm/mmu.c int pud_free_pmd_page(pud_t *pudp, unsigned long addr) · line 1375

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+     *
+     * It is also worth reasoning whether something can go wrong via
+     * pud_free_pmd_page() -> __pmd_free_pte_page(), since the latter
+     * will be called locklessly on this code path.
+     *
+     * For Case 1 (a), T2 will block until CS is finished, so we are safe.
+     * For Case 1 (b) and Case 2, the PMD table will be isolated before
+     * T1 can enter CS, therefore it is safe for T2 to operate on the
+     * PMD table locklessly.
+     */

Although I can see that you put a lot of effort into this comment, I think we should just remove it. Instead, we should have a litmus test in the commit message and probably just some small comments here to explain e.g. why the mmap_read_lock() critical section is empty.

Quoted 2 lines

I'm currently trying to put together a litmus test with James (cc'd) so maybe we can help you out with that part.

Thanks for the effort!

Quoted 12 lines

In the meantime...

Quoted 9 lines

arch/arm64/mm/ptdump.c +0 -0

index 421a5de806c6..65335c7ba482 100644
@@ -283,6 +283,13 @@ void note_page_flush(struct ptdump_state *pt_st)
       note_page(pt_st, 0, -1, pte_val(pte_zero));
   }

  +static void arm64_ptdump_walk_pgd(struct ptdump_state *st, struct mm_struct *mm)

Quoted 8 lines

arch/arm64/mm/ptdump.c void note_page_flush(struct ptdump_state *pt_st) · line 291

Quoted 5 lines

arch/arm64/mm/ptdump.c +5 -0

+{
+    static_branch_enable(&arm64_ptdump_lock_key);
+    ptdump_walk_pgd(st, mm, NULL);
+    static_branch_disable(&arm64_ptdump_lock_key);
+}

What serialises the toggling of the static key here? For example, I can't see what prevents a kernel page-table dump running concurrently with a check_wx() and the key ending up in the wrong state.

Good spot. I believe static_branch_{inc, dec} should work here. The vmalloc code only needs to know that someone is traversing the pagetables, and there is no synchronization needed between check_wx and ptdump. And, static_branch_inc() should have the same semantics as static_branch_enable().

Quoted 7 lines

Either we need an additional lock or perhaps using static_branch_{inc,dec}() would work instead? I haven't thought too hard about that but it looks like we need _something_.

Will