Re: [PATCH] kasan: fix runtime shadow page table synchronization

[Muchun Song]

> On Oct 16, 2026, at 22:42, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> wrote:
> 
> Muchun Song <songmuchun@xxxxxxxxxxxxx> writes:
> 
>> kasan_populate_early_shadow() is also used after slab becomes available
>> to install shadow mappings for hot-added memory. When this path
>> allocates a new P4D or PUD table, p4d_alloc() and pud_alloc() publish it
>> only in init_mm.
>> 
>> On x86-64, runtime kernel mappings that add a top-level entry must call
>> arch_sync_kernel_mappings() so existing process page tables can see it.
>> Without this synchronization, the KASAN shadow mapping can remain
>> unreachable and fault when the hot-added memory is accessed.
>> 
> 
> Did you actually hit this crash or it's just LLM's/code inspection finding?

Just code inspection finding.

Because my recent work is converting the vmemmap population path to use the
generic pxd_alloc() for page table allocation. I ran into a similar problem,
so I wanted to refer to how KASAN solved it, and then I found the bug.

> 
>> Add p4d_alloc_kernel() and pud_alloc_kernel() to perform the required
>> synchronization, and use them for runtime KASAN shadow population.
>> 
>> Once copied, a top-level entry remains in other process page tables.
>> Clearing only init_mm and freeing its target table during teardown would
>> leave stale pointers behind. Retain these shared levels instead. On x86,
>> this keeps PUD tables with four-level paging and P4D tables with
>> five-level paging, while lower-level tables can still be freed.
>> 
>> Fixes: 0207df4fa1a8 ("kernel/memremap, kasan: make ZONE_DEVICE with work with KASAN")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Assisted-by: LLM
>> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>> ---
>> include/linux/mm.h | 24 ++++++++++++++++++++++++
>> mm/kasan/init.c    | 10 ++++++++--
>> 2 files changed, 32 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 5cca90fc1df6..5453d3e42780 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -3855,6 +3855,18 @@ static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd,
>> 		NULL : p4d_offset(pgd, address);
>> }
>> 
>> +static inline p4d_t *p4d_alloc_kernel(pgd_t *pgd, unsigned long address)
>> +{
>> + 	if (unlikely(pgd_none(*pgd))) {
>> + 		if (__p4d_alloc(&init_mm, pgd, address))
>> + 			return NULL;
>> + 		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
>> + 			arch_sync_kernel_mappings(address, address);
>> + 	}
>> +
>> + 	return p4d_offset(pgd, address);
>> +}
>> +
>> static inline pud_t *pud_alloc(struct mm_struct *mm, p4d_t *p4d,
>> 		unsigned long address)
>> {
>> @@ -3862,6 +3874,18 @@ static inline pud_t *pud_alloc(struct mm_struct *mm, p4d_t *p4d,
>> 		NULL : pud_offset(p4d, address);
>> }
>> 
>> +static inline pud_t *pud_alloc_kernel(p4d_t *p4d, unsigned long address)
>> +{
>> + 	if (unlikely(p4d_none(*p4d))) {
>> + 		if (__pud_alloc(&init_mm, p4d, address))
>> + 			return NULL;
>> + 		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
>> + 			arch_sync_kernel_mappings(address, address);
>> + 	}
>> +
>> + 	return pud_offset(p4d, address);
>> +}
>> +
>> static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
>> {
>> 	return (unlikely(pud_none(*pud)) && __pmd_alloc(mm, pud, address))?
>> diff --git a/mm/kasan/init.c b/mm/kasan/init.c
>> index 66a883887987..93a74a1fcdec 100644
>> --- a/mm/kasan/init.c
>> +++ b/mm/kasan/init.c
>> @@ -206,7 +206,7 @@ static int __ref zero_p4d_populate(pgd_t *pgd, unsigned long addr,
>> 			pud_t *p;
>> 
>> 			if (slab_is_available()) {
>> - 				p = pud_alloc(&init_mm, p4d, addr);
>> + 				p = pud_alloc_kernel(p4d, addr);
> 
> I think it would be better to reuse pud_alloc_track() here.
> Pass a pgtbl_mod_mask through zero_p4d/pud/pmd_populate(), and call
> arch_sync_kernel_mappings()
> only once at the end of kasan_populate_early_shadow()

OK. No problem.

>> 				if (!p)
>> 					return -ENOMEM;
>> 			} else {
>> @@ -268,7 +268,7 @@ int __ref kasan_populate_early_shadow(const void *shadow_start,
>> 		if (pgd_none(*pgd)) {
>> 
>> 			if (slab_is_available()) {
>> - 				if (!p4d_alloc(&init_mm, pgd, addr))
>> + 				if (!p4d_alloc_kernel(pgd, addr))
> 
> Ditto

Yeah.

> 
>> 					return -ENOMEM;
>> 			} else {
>> 				pgd_populate_kernel(addr, pgd,
>> @@ -322,6 +322,9 @@ static void kasan_free_pud(pud_t *pud_start, p4d_t *p4d)
>> 			return;
>> 	}
>> 
>> + 	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
>> + 		return;
>> +
> 
> With 4-level paging the PUD table is now kept here after it has been
> synced to every pgd. A later kasan_add_zero_shadow() call can overwrite
> it in init_mm only. This might cause a problem, because the aligned

Good catch.

> shortcuts in kasan_populate_early_shadow() and zero_p4d_populate()
> don't check whether the entry is already populated:
> 
> mm/kasan/init.c:kasan_populate_early_shadow() {
>    ...
>                if (IS_ALIGNED(addr, PGDIR_SIZE) && end - addr >= PGDIR_SIZE) {
>            ...
>                        p4d_populate_kernel(addr, p4d,
>                                        lm_alias(kasan_early_shadow_pud));
>    ...
> }
> 
> So sequence of kasan_add_zero_shadow(), kasan_remove_zero_shadow(),
> kasan_add_zero_shadow(), arch_sync_kernel_mappings()
> might trigger the BUG_ON() in sync_global_pgds_l4():
> 
> arch/x86/mm/init_64.c:sync_global_pgds_l4() {
>    ...
>                        if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
>                                BUG_ON(p4d_pgtable(*p4d)
>                                       != p4d_pgtable(*p4d_ref));
>    ...
> }
> 
> Perhaps the aligned populate paths should skip an entry that is
> already populated.

I think so.

I'll update a new version to fix this.

Thanks,
Muchun

> 
>> 	pud_free(&init_mm, pud_start);
>> 	p4d_clear(p4d);
>> }