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

[Lorenzo Stoakes (ARM)]

On Tue, Nov 03, 2026 at 09:47:49AM +0800, Hui Zhu wrote:
> From: Hui Zhu <zhuhui@xxxxxxxxxx>
>
> 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.  pmdp_get() is a plain READ_ONCE()
> and can tear where pmd_t is wider than a word, as on i386-PAE where

Isn't i386 no longer supported? Are there any other arches we are worried
about?

It seems now after Arnd's removals this has to be manually selected by the
user.

Other than that we are worried about mips32 with 64 bit addresses and sh
with x2tlb. More museum-piece stuff...

I hate that we're impacted by this.

Anyway you should probably call out the relevant arches here.

> pmd_install() publishes two words with a single cmpxchg8b.  Use
> pmdp_get_lockless(), the accessor for pmd reads without the pmd lock and
> what __pte_offset_map() already uses on this very pmd a few instructions
> later.  It is pmdp_get() everywhere else, so the report is silenced the
> same way.
>
> No functional change on architectures that can load a pmd in one go.
>
> Signed-off-by: Hui Zhu <zhuhui@xxxxxxxxxx>


> ---
> Changelog:
> v3:
> Shorten the comment to one line (David Hildenbrand).
> v2:
> Use pmdp_get_lockless() instead of pmdp_get() (David Hildenbrand).

Please add links to previous discussion, it's difficult to track stuff
otherwise.

>
>  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)) {

This pattern is definitely bad I think, we have it in many other places so
should probably convert those to pmdp_get() at least?

> +	/* pmd_install() rechecks pmd_none() under the pmd lock. */
> +	if (pmd_none(pmdp_get_lockless(vmf->pmd))) {

It definitely seems right to use at least pmdp_get() here, but I wonder
about the need for pmdp_get_lockless() here.

pmd_none() only checks the lower word, so a torn 64-bit word on 32-bit
(i.e. the PAE style cases we are worried about) makes no difference:

>From x86 pmd_none():

	/* Only check low word on 32-bit platforms, since it might be
	   out of sync with upper half. */

And anyway I guess there is no valid PMD in such systems that would have
the lower word cleared and upper word not cleared.

And we have other instances of this pattern all over the place, when you
read the PMD you don't hold the PMD lock at all so 'lockless' is kinda
questionable here.

I mean yes it's lockless but no more than other cases like this.

So either I think use pmdp_get() or we need to go convert everywhere else
and be strict on pmdp_get() only being used with the PMD lock held.

>  		vmf->prealloc_pte = pte_alloc_one(vmf->vma->vm_mm);
>  		if (!vmf->prealloc_pte)
>  			return VM_FAULT_OOM;
> --
> 2.43.0
>

--
Cheers, Lorenzo