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

Harry Yoo
2025-08-11T12:12:55Z

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().

> > 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>
> > ---
> > 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.

> > + 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())).

> (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