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

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

On 11/2/26 04:13, 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
> 
> do_fault_around() reads vmf->pmd without the pmd lock to decide whether
> ->map_pages() needs a preallocated PTE page, while pmd_install() publishes
> the entry with WRITE_ONCE() under the pmd lock.  Both paths above only hold
> mmap_lock for read, so they can fault into the same VMA concurrently.
> 
> The race is benign.  The pmd is a naturally aligned 8 byte object, the
> store is a single WRITE_ONCE() and pmd_none() only tests the low word, so
> no torn value is possible.  A stale none pmd only allocates an extra
> prealloc_pte, which pmd_install() leaves unused because it rechecks
> pmd_none() under the pmd lock, and do_fault() frees it on the error path.
> A pmd already seen as populated makes ->map_pages() find no PTE, and
> do_read_fault() then falls back to __do_fault() and finish_fault(), which
> allocate the page table properly.
> 
> Read the entry with pmdp_get() so that the load becomes a READ_ONCE()
> pairing with the WRITE_ONCE() in pmd_install().
> 
> No functional change intended.
> 
> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> ---
>  mm/memory.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 0fb676dd2bd63..90afbd124d503 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6017,7 +6017,11 @@ 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)) {
> +	/*
> +	 * Lockless hint: pmd_install() may populate this pmd concurrently and
> +	 * rechecks it under the pmd lock, so a stale value is harmless.
> +	 */
> +	if (pmd_none(pmdp_get(vmf->pmd))) {
>  		vmf->prealloc_pte = pte_alloc_one(vmf->vma->vm_mm);
>  		if (!vmf->prealloc_pte)
>  			return VM_FAULT_OOM;

If it's an lockless access, shouldn't we be using pmdp_get_lockless()?

-- 
Cheers,

David