From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=91.218.175.134 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793934234; b=hbPLKDiEYJn7DqvA46g/v/R92u4yop0HDW4bcOgkNr3bsHDCoG82pZOXgRTqu+MY75f9 K+y7ZCRVa55KatMq9ufp6AzmFp4H2l1T3gnLclMj2xIBWdAu0xnz3SGC2kOn81jAtyPv7 4kVR+/7ebD/BuBTloYPXNuuN/DRPunjpiSAepRyozrixGZK7ul7GhjbYYzW14YufgwzNa QZO1RW4Osbp90/Oc9EBkbGREmC2ZtyyYRSyZTQtMhY3lqPeJmuYrFTP5HtLg4zaRhNsP9 eGqCf//aKEiCzk1Hw8yt9PD5LgOOenedRButNF+m1df1AMq0k1fEBxyKxbhZVG6f/Eg== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793934234; h=DMARC-Filter:X-Envelope-To:DKIM-Signature:X-Envelope-To:Received: X-Mizu-Trace-ID:X-Migadu-Flow:Message-ID:Date:MIME-Version:User-Agent: Subject:To:Cc:References:Content-Language:From:In-Reply-To: Content-Type:Content-Transfer-Encoding; bh=eeUY/7cJGXFEn20K8oROVcv/0XXhUdc/6BlusTj1DNU=; b=gQkhBXUpzDNh76D9Z2uBv0UuwMuQW5JcWB8syXA7E6vQcO77vRz65xsIkcOb0Vx41Ygk BB/lUz/pF3diDUZYipwL2JqucGbwdmeuz0Vr9bfixtNYCGuKnK2YkX21SAlJuBXjYviTi 0ryF4unqtuos8cTE9xXvIKrmFlIMxRByrkfFzUMY3MDdC/3QQZjN1+eaygHy7fnf8lkuL W7FxenWzPLlMZs33CqSfiUG1KicgtePtqN0uLqgqIjt6KKlvsMuKgsq2ob35MjQm1pZ22 sIRPZfZ7ijpuxcxVfnkN4f6YXISLbkAiMIRDarRDnyuntQXPWCoD7ubVZGAaU4McoOg== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=iDKrsO+B; arc=none smtp.remote-ip=91.218.175.134 Received: from mta0.migadu.com (out-134.mta0.migadu.com [91.218.175.134]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 811DF606A32C for ; Fri, 6 Nov 2026 03:03:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org 811DF606A32C Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key, unprotected) header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=iDKrsO+B X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=jLiXUbWh0Cq3K7wdDXTxGIGTG12TPsew4PFOTwWSpWU=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1793934231; v=1; x=1794539031; b=iDKrsO+BXI5lRueVDL1VlGWh5PUtQvd5QPfMcXlcflU/tL2xOd8/J2XNEsJBpoNvINTzAXoK VNjQJmv+UDu8PSkTVvoYMfjtYO9fp1B8+yP5d8BnpKP6L+DPF/nsEH/sP5ajPM7dYrrEkS+Ry6Y a7b/G7AFrSME9IqNeTV/Og0g= X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id b619772cf9920b08; Fri, 06 Nov 2026 03:03:51 +0000 X-Mizu-Trace-ID: b619772cf9920b08 X-Migadu-Flow: FLOW_OUT Message-ID: <88a6bf17-2189-44a8-8255-50b7ee1d80f2@linux.dev> Date: Fri, 6 Nov 2026 11:03:46 +0800 X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 User-Agent: Betterbird (macOS/Intel) Subject: Re: [PATCH v4] mm/memory: use pmdp_get_lockless() for the pmd read in do_fault_around() To: "David Hildenbrand (Arm)" , Andrew Morton , Lorenzo Stoakes , "Liam R. Howlett" , Vlastimil Babka , Mike Rapoport , Suren Baghdasaryan , Michal Hocko , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Geert Uytterhoeven , Sam Creasey Cc: Hui Zhu , stable+noautosel@kernel.org, linux-m68k@lists.linux-m68k.org References: <20261104091050.117076-1-hui.zhu@linux.dev> Content-Language: en-US From: Hui Zhu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 2026/11/5 20:00, David Hildenbrand (Arm) : > On 11/4/26 10:10, Hui Zhu wrote: >> From: Hui Zhu >> >> 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 >> Acked-by: Lorenzo Stoakes >> --- >> 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)" > 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) > --- > 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; } > > /* Hi David, Thanks for picking this up, and for the m68k cleanup! Tested-by: Hui Zhu Tested with gcc-m68k-linux-gnu 13.3.0 (Ubuntu 13.3.0-6ubuntu2~24.04), on three configs that between them cover all three m68k families:   amiga_defconfig                    Motorola MMU, PGTABLE_LEVELS=3   sun3_defconfig                     SUN3,  PGTABLE_LEVELS=2   m5208evb_defconfig + CONFIG_MMU=y  ColdFire,  PGTABLE_LEVELS=2 Without your patch none of the three can build mm/memory.o, and each fails at a different site:   arch/m68k/include/asm/page.h:20:27: error: lvalue required as unary '&' operand   arch/m68k/include/asm/sun3_pgtable.h:113:33: error: lvalue required as unary '&' operand   arch/m68k/include/asm/mcf_pgtable.h:134:33: error: lvalue required as unary '&' operand With it, all three build a full vmlinux with zero warnings. Two things that might save you some time: - m68k looks like the only arch with this pattern. Grepping arch/ and   include/ for '(&x)->pmd' and '(&(pmd))' finds nothing outside arch/m68k/,   and pmd_none2()/pmd_bad2()/pmd_present2() only exist in mcf_pgtable.h and   sun3_pgtable.h. So your three hunks look complete to me. - No m52xx/m54xx defconfig enables CONFIG_MMU, so the ColdFire path through   mcf_pgtable.h has no defconfig coverage at all; you have to flip it on by   hand (scripts/config -e MMU && make olddefconfig) to reach it. That is   probably how this went unnoticed for so long - my patch has been breaking   m68k since v1, because pmdp_get() returns by value just like   pmdp_get_lockless() does. I also checked whether your patch changes code generation, by building the same tree with and without it and comparing every object with objdump -d (plain cmp is useless here, timestamps and link layout shift everything). Counting only objects whose disassembly actually differs:   amiga      3733 objects, 0 changed   sun3       3553 objects, 3 changed: mm/memory.o, mm/pgtable-generic.o,                                       mm/vmalloc.o   m5208evb    716 objects, 1 changed: mm/pgtable-generic.o amiga is the interesting one. It is the only config where pmd_val() itself changed, and it comes out bit-for-bit identical, which also says the build is reproducible enough for this comparison to mean anything. sun3 and ColdFire differ because pmd_none() stops taking the address of its argument, and the direction is an improvement. In mm/vmalloc.o on sun3 one function loses a forced spill:   -  subql #4,%sp   -  movel %d0,%sp@(12)     /* spill the pmd value */   -  lea %sp@(12),%a1       /* &(pmd) */   -  moveal %a1@,%a0        /* read it back */ and turns into a 13 instruction function with no stack slot at all. Instruction counts are otherwise flat (sun3 mm/memory.o 7373 -> 7373, mm/vmalloc.o 5514 -> 5515), so it is not a size regression. Dragging the cleanup along with the fix sounds right to me. It is a hard prerequisite, and sent on its own it would just look like a cleanup with no urgency. I assume it goes in ahead of 6ab55cfd in core-7.5.misc, so that linux-next never sees a broken m68k in between. One red herring in case anyone else pokes at this: m5208evb with its stock defconfig (CONFIG_MMU not set) fails to build mm/memory.o for reasons that have nothing to do with any of this - implicit declarations of pte_is_uffd_wp_marker() and pmd_pgtable(), plus a parse error in include/linux/mm.h. Pre-existing, and it goes away once MMU is on. Best, Hui