Re: [PATCH v4] mm/memory: use pmdp_get_lockless() for the pmd read in do_fault_around()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/4/26 10:10, Hui Zhu wrote:
> From: Hui Zhu <zhuhui@kylinos.cn>
> 
> KCSAN reports a data-race in do_read_fault / pmd_install; the same code
> is in mainline:
> 
>   write (marked) to 0xffff8d52a75fb810 of 8 bytes by task 27487 on cpu 5:
>    native_set_pmd arch/x86/include/asm/pgtable_64.h:83 [inline]
>    pmd_populate arch/x86/include/asm/pgalloc.h:84 [inline]
>    pmd_install+0xe0/0x120 mm/memory.c:444
>    finish_fault+0x5bc/0xb00 mm/memory.c:5204
>    do_cow_fault mm/memory.c:5432 [inline]
>    do_fault+0x5d0/0x6e0 mm/memory.c:5526
>    handle_mm_fault+0x211/0x4c0 mm/memory.c:6218
>    __mm_populate+0x144/0x270 mm/gup.c:2207
>    __do_sys_mlockall+0x2cd/0x380 mm/mlock.c:764
> 
>   read to 0xffff8d52a75fb810 of 8 bytes by task 27527 on cpu 9:
>    do_fault_around mm/memory.c:5341 [inline]
>    do_read_fault+0x162/0x670 mm/memory.c:5381
>    do_fault+0x31a/0x6e0 mm/memory.c:5524
>    handle_mm_fault+0x211/0x4c0 mm/memory.c:6218
>    faultin_page_range+0x227/0x530 mm/gup.c:2156
>    __x64_sys_madvise+0x67/0x80 mm/madvise.c:1479
> 
>   value changed: 0x0000000000000000 -> 0x000000001386c067
> 
> Both paths only hold mmap_lock for read, so do_fault_around() tests
> vmf->pmd while pmd_install() populates it under the pmd lock.  The race is
> benign: a stale pmd_none() only wastes the prealloc_pte, which do_fault()
> frees again, and a pmd seen as populated only makes ->map_pages() find no
> PTE and do_read_fault() fall back to __do_fault().
> 
> The load still has to be lockless-safe.  The store side already is: on
> i386-PAE, pmd_install() publishes both halves of the pmd with a single
> cmpxchg8b.  The load side is not.  pmdp_get() is a plain READ_ONCE() over
> a pmd_t that is wider than a word there, so it becomes two 32-bit loads
> and can return the low half of one pmd together with the high half of
> another.  Use pmdp_get_lockless(), the accessor for pmd reads that do not
> hold the pmd lock: it re-reads until both halves come from the same pmd.
> It is also what __pte_offset_map() already uses on this very pmd a few
> instructions later.
> 
> Only X86_32+X86_PAE and SH+X2TLB compile the retry loop, these being the
> GUP_GET_PXX_LOW_HIGH selectors that have more than two page table levels.
> mips32 with 64-bit physical addresses selects GUP_GET_PXX_LOW_HIGH too,
> but is 2-level and so falls back to pmdp_get().  On every other
> configuration, x86-64 included, pmdp_get_lockless() is pmdp_get(): the
> pmd is loaded in one go, the READ_ONCE() silences the report the same
> way, and there is no functional change.
> 
> Fixes: 7267ec008b5c ("mm: postpone page table allocation until we have page to map")
> Cc: stable+noautosel@kernel.org # KCSAN annotation only, no functional change
> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> Acked-by: Lorenzo Stoakes <ljs@kernel.org>
> ---
> Changelog:
> v4:
> Commit message only, the diff is identical to v3.  Spell out the affected
> arches and add the lore links for v1-v3 (Lorenzo Stoakes).  Reword the
> cmpxchg8b sentence: the store is already atomic, it is the load that tears.
> Add the Fixes: tag (David Hildenbrand).
> v3:
> https://lore.kernel.org/all/20261103014749.96764-1-hui.zhu@linux.dev/
> Shorten the comment to one line (David Hildenbrand).
> v2:
> https://lore.kernel.org/all/20261102093120.92654-1-hui.zhu@linux.dev/
> Use pmdp_get_lockless() instead of pmdp_get() (David Hildenbrand).
> v1:
> https://lore.kernel.org/all/20261102031308.66177-1-hui.zhu@linux.dev/
> 
>  mm/memory.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 0fb676dd2bd63..f25c2ce0ef153 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6017,7 +6017,8 @@ static vm_fault_t do_fault_around(struct vm_fault *vmf)
>  	to_pte = min3(from_pte + nr_pages, (pgoff_t)PTRS_PER_PTE,
>  		      pte_off + vma_pages(vmf->vma) - vma_off) - 1;
>  
> -	if (pmd_none(*vmf->pmd)) {
> +	/* pmd_install() rechecks pmd_none() under the pmd lock. */
> +	if (pmd_none(pmdp_get_lockless(vmf->pmd))) {
>  		vmf->prealloc_pte = pte_alloc_one(vmf->vma->vm_mm);
>  		if (!vmf->prealloc_pte)
>  			return VM_FAULT_OOM;

Fun, this breaks some odd m68k configs because they do some legacy weird stuff with pmd_none().

I'll throw in the following cleanup. CCing m68k maintainers: I would drag
this along with this fix here unless you disagree.

From e3c739933a04743e3923f6f39a603e93f2700304 Mon Sep 17 00:00:00 2001
From: "David Hildenbrand (Arm)" <david@kernel.org>
Date: Thu, 5 Nov 2026 12:49:38 +0100
Subject: [PATCH] m68k/mm: simplify
 pmd_none()/pmd_val()/pmd_present()/pmd_bad()

pmd_t on m68k used to contain 16 words. Commit ef22d8abd876 ("m68k:
mm: Restructure Motorola MMU page-table layout") reduced Motorola PMDs
to one word; Sun3 and ColdFire use word-sized folded PMDs.

Use direct member access in pmd_val() and replace the Sun3 and ColdFire
pointer-taking predicate wrappers with inline functions taking values.

This allows us to write something like:

  if (pmd_none(pmdp_get(pmdp)))

While at it remove unused pmd_present2() and commented-out pmd_bad()
on sun3.

Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
---
 arch/m68k/include/asm/mcf_pgtable.h  | 20 +++++++++++++++-----
 arch/m68k/include/asm/page.h         |  2 +-
 arch/m68k/include/asm/sun3_pgtable.h | 23 +++++++++++++++--------
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/arch/m68k/include/asm/mcf_pgtable.h b/arch/m68k/include/asm/mcf_pgtable.h
index f45a882238dbb..4efa8e3e1e98a 100644
--- a/arch/m68k/include/asm/mcf_pgtable.h
+++ b/arch/m68k/include/asm/mcf_pgtable.h
@@ -130,11 +130,21 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr,
 
 #define pte_page(pte)	virt_to_page(__pte_page(pte))
 
-static inline int pmd_none2(pmd_t *pmd) { return !pmd_val(*pmd); }
-#define pmd_none(pmd) pmd_none2(&(pmd))
-static inline int pmd_bad2(pmd_t *pmd) { return 0; }
-#define pmd_bad(pmd) pmd_bad2(&(pmd))
-#define pmd_present(pmd) (!pmd_none2(&(pmd)))
+static inline int pmd_none(pmd_t pmd)
+{
+	return !pmd_val(pmd);
+}
+
+static inline int pmd_bad(pmd_t pmd)
+{
+	return 0;
+}
+
+static inline int pmd_present(pmd_t pmd)
+{
+	return !pmd_none(pmd);
+}
+
 static inline void pmd_clear(pmd_t *pmdp) { pmd_val(*pmdp) = 0; }
 
 
diff --git a/arch/m68k/include/asm/page.h b/arch/m68k/include/asm/page.h
index d30f8b2f15927..09a2c020ec992 100644
--- a/arch/m68k/include/asm/page.h
+++ b/arch/m68k/include/asm/page.h
@@ -17,7 +17,7 @@
  */
 #if !defined(CONFIG_MMU) || CONFIG_PGTABLE_LEVELS == 3
 typedef struct { unsigned long pmd; } pmd_t;
-#define pmd_val(x)	((&x)->pmd)
+#define pmd_val(x)	((x).pmd)
 #define __pmd(x)	((pmd_t) { (x) } )
 #endif
 
diff --git a/arch/m68k/include/asm/sun3_pgtable.h b/arch/m68k/include/asm/sun3_pgtable.h
index 704442a391fd7..5867d24032655 100644
--- a/arch/m68k/include/asm/sun3_pgtable.h
+++ b/arch/m68k/include/asm/sun3_pgtable.h
@@ -109,14 +109,21 @@ static inline void pte_clear (struct mm_struct *mm, unsigned long addr, pte_t *p
 #define pmd_page(pmd)		virt_to_page((void *)pmd_page_vaddr(pmd))
 
 
-static inline int pmd_none2 (pmd_t *pmd) { return !pmd_val (*pmd); }
-#define pmd_none(pmd) pmd_none2(&(pmd))
-//static inline int pmd_bad (pmd_t pmd) { return (pmd_val (pmd) & SUN3_PMD_MASK) != SUN3_PMD_MAGIC; }
-static inline int pmd_bad2 (pmd_t *pmd) { return 0; }
-#define pmd_bad(pmd) pmd_bad2(&(pmd))
-static inline int pmd_present2 (pmd_t *pmd) { return pmd_val (*pmd) & SUN3_PMD_VALID; }
-/* #define pmd_present(pmd) pmd_present2(&(pmd)) */
-#define pmd_present(pmd) (!pmd_none2(&(pmd)))
+static inline int pmd_none(pmd_t pmd)
+{
+	return !pmd_val(pmd);
+}
+
+static inline int pmd_bad(pmd_t pmd)
+{
+	return 0;
+}
+
+static inline int pmd_present(pmd_t pmd)
+{
+	return !pmd_none(pmd);
+}
+
 static inline void pmd_clear (pmd_t *pmdp) { pmd_val (*pmdp) = 0; }
 
 /*
-- 
2.43.0


-- 
Cheers,

David