Re: [PATCH V4 mm-hotfixes 2/3] mm: introduce and use {pgd,p4d}_populate_kernel()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Aug 11, 2025 at 09:12:08PM +0900, Harry Yoo wrote:
> On Mon, Aug 11, 2025 at 12:38:37PM +0100, Lorenzo Stoakes wrote:
> > On Mon, Aug 11, 2025 at 02:34:19PM +0900, Harry Yoo wrote:
> > > Introduce and use {pgd,p4d}_populate_kernel() in core MM code when
> > > populating PGD and P4D entries for the kernel address space.
> > > These helpers ensure proper synchronization of page tables when
> > > updating the kernel portion of top-level page tables.
> > >
> > > Until now, the kernel has relied on each architecture to handle
> > > synchronization of top-level page tables in an ad-hoc manner.
> > > For example, see commit 9b861528a801 ("x86-64, mem: Update all PGDs for
> > > direct mapping and vmemmap mapping changes").
> > >
> > > However, this approach has proven fragile for following reasons:
> > >
> > >   1) It is easy to forget to perform the necessary page table
> > >      synchronization when introducing new changes.
> > >      For instance, commit 4917f55b4ef9 ("mm/sparse-vmemmap: improve memory
> > >      savings for compound devmaps") overlooked the need to synchronize
> > >      page tables for the vmemmap area.
> > >
> > >   2) It is also easy to overlook that the vmemmap and direct mapping areas
> > >      must not be accessed before explicit page table synchronization.
> > >      For example, commit 8d400913c231 ("x86/vmemmap: handle unpopulated
> > >      sub-pmd ranges")) caused crashes by accessing the vmemmap area
> > >      before calling sync_global_pgds().
> > >
> > > To address this, as suggested by Dave Hansen, introduce _kernel() variants
> > > of the page table population helpers, which invoke architecture-specific
> > > hooks to properly synchronize page tables. These are introduced in a new
> > > header file, include/linux/pgalloc.h, so they can be called from common code.
> > >
> > > They reuse existing infrastructure for vmalloc and ioremap.
> > > Synchronization requirements are determined by ARCH_PAGE_TABLE_SYNC_MASK,
> > > and the actual synchronization is performed by arch_sync_kernel_mappings().
> > >
> > > This change currently targets only x86_64, so only PGD and P4D level
>
> Hi Lorenzo, thanks for looking at this!
>
> > Well, arm defines ARCH_PAGE_TABLE_SYNC_MASK in arch/arm/include/asm/page.h. But
> > it aliases this to PGTBL_PMD_MODIFIED so will remain unaffected :)
>
> Oh, here I just intended to explain why I didn't implement
> {pud,pmd}_populate_kernel().

I'd add that arm handles PGTBL_PMD_MODIFIED and therefore remains unaffected
just to be super clear.

>
> > > helpers are introduced. In theory, PUD and PMD level helpers can be added
> > > later if needed by other architectures.
> > >
> > > Currently this is a no-op, since no architecture sets
> > > PGTBL_{PGD,P4D}_MODIFIED in ARCH_PAGE_TABLE_SYNC_MASK.
> > >
> > > Cc: <stable@vger.kernel.org>
> > > Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
> > > Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> > > Signed-off-by: Harry Yoo <harry.yoo@oracle.com>

Given that I missed you fixed the vmalloc.h thing, this LGTM so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> > > ---
> > >  include/linux/pgalloc.h | 24 ++++++++++++++++++++++++
> > >  include/linux/pgtable.h |  4 ++--
> > >  mm/kasan/init.c         | 12 ++++++------
> > >  mm/percpu.c             |  6 +++---
> > >  mm/sparse-vmemmap.c     |  6 +++---
> > >  5 files changed, 38 insertions(+), 14 deletions(-)
> > >  create mode 100644 include/linux/pgalloc.h
> > >
> > > diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
> > > new file mode 100644
> > > index 000000000000..290ab864320f
> > > --- /dev/null
> > > +++ b/include/linux/pgalloc.h
> > > @@ -0,0 +1,24 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _LINUX_PGALLOC_H
> > > +#define _LINUX_PGALLOC_H
> > > +
> > > +#include <linux/pgtable.h>
> > > +#include <asm/pgalloc.h>
> > > +
> > > +static inline void pgd_populate_kernel(unsigned long addr, pgd_t *pgd,
> > > +				       p4d_t *p4d)
> > > +{
> > > +	pgd_populate(&init_mm, pgd, p4d);
> > > +	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
> >
> > Hm, ARCH_PAGE_TABLE_SYNC_MASK is only defined for x86 2, 3 page level and arm. I see:
> >
> > #ifndef ARCH_PAGE_TABLE_SYNC_MASK
> > #define ARCH_PAGE_TABLE_SYNC_MASK 0
> > #endif
> >
> > In linux/vmalloc.h, but you're not importing that?
>
> Patch 1 moves it from linux/vmalloc.h to linux/pgtable.h,
> and linux/pgalloc.h includes linux/pgtable.h.
>
> > It sucks that that there is there, but maybe you need to #include
> > <linux/vmalloc.h> for this otherwise this could be broken on other arches?
> >
> > You may be getting lucky with nested header includes that causes this to be
> > picked up somewhere for you, or having it only declared for arches that define
> > it, but we should probably make this explicit.
>
> ...so I don't think I'm missing necessary header includes even on
> other architectures?
>
> > Also arch_sync_kernel_mappings() is defined in linux/vmalloc.h so seems
> > sensible.
>
> Also moved to linux/pgtable.h.

Ah yeah damn, I missed that you do that there, ok well that's fine then :)

>
> > > +		arch_sync_kernel_mappings(addr, addr);
> > > +}
> > > +
> > > +static inline void p4d_populate_kernel(unsigned long addr, p4d_t *p4d,
> > > +				       pud_t *pud)
> > > +{
> > > +	p4d_populate(&init_mm, p4d, pud);
> > > +	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
> > > +		arch_sync_kernel_mappings(addr, addr);
> >
> > It's kind of weird we don't have this defined as a function for many arches,
>
> That's really a mystery :)
>
> I have no idea why other architectures don't handle this.
>
> (At least on 64 bit arches) In theory I think only a few architectures
> (like arm64 where a kernel page table is shared between tasks) don't have
> to implement this.
>
> Probably because it's a bit niche bug to hit?
> (vmemmap, direct mapping, vmalloc/vmap area can span multiple PGD ranges)
> AND (populating some PGD entries is done after boot process (e.g. memory
> hot-plug or vmalloc())).

No comment is more why we don't just do a standard:

#ifndef xxx
#define xxx (0)
#endif

Or something. Just odd.

>
> > (weird as well that we declare it in... vmalloc.h but I guess one for follow up
> > cleanups that).
> >
> > But I see from the comment:
> >
> > /*
> >  * There is no default implementation for arch_sync_kernel_mappings(). It is
> >  * relied upon the compiler to optimize calls out if ARCH_PAGE_TABLE_SYNC_MASK
> >  * is 0.
> >  */
> >
> > So this seems intended... :)
>
> > The rest of this seems sensible, nice cleanup!
>
> Thanks for looking at!
>
> --
> Cheers,
> Harry / Hyeonggon
>