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

[Harry Yoo]

On Mon, Aug 11, 2025 at 01:18:12PM +0100, Lorenzo Stoakes wrote:
> 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.

Will do:

This change currently targets only x86_64, so only PGD and P4D level
helpers are introduced. Currently, these helpers are no-ops since no
architecture sets PGTBL_{PGD,P4D}_MODIFIED in ARCH_PAGE_TABLE_SYNC_MASK.

In theory, PUD and PMD level helpers can be added later if needed by
other architectures. For now, 32-bit architectures (x86-32 and arm)
only handle PGTBL_PMD_MODIFIED, so p*d_populate_kernel() will never
affect them unless we introduce a PMD level helper.

> > > > 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@xxxxxxxxxxxxxxx>
> > > > Fixes: 8d400913c231 ("x86/vmemmap: handle unpopulated sub-pmd ranges")
> > > > Suggested-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Harry Yoo <harry.yoo@xxxxxxxxxx>
> 
> Given that I missed you fixed the vmalloc.h thing, this LGTM so:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>

Thanks!

-- 
Cheers,
Harry / Hyeonggon