On 2/18/25 8:41 PM, Dave Hansen wrote:
On 2/17/25 21:45, Andrew Morton wrote:
When a process leads the addition of a struct page to vmemmap (e.g. hot-plug), the page table update for the newly added vmemmap-based virtual address is updated first in init_mm's page table and then synchronized later. If the vmemmap-based virtual address is accessed through the process's page table before this sync, a page fault will occur.
So, I think we're talking about the loop in vmemmap_populate_hugepages() (with a bunch of context chopped out:
for (addr = start; addr < end; addr = next) { ... pgd = vmemmap_pgd_populate(addr, node); if (!pgd) return -ENOMEM; ... vmemmap_set_pmd(pmd, p, node, addr, next); }This both creates a mapping under 'pgd' and uses the new mapping inside vmemmap_set_pmd(). This is generally a known problem since vmemmap_populate() already does a sync_global_pgds(). The reason it manifests here is that the vmemmap_set_pmd() comes before the sync:
vmemmap_populate() { vmemmap_populate_hugepages() { vmemmap_pgd_populate(addr, node); ... // crash: vmemmap_set_pmd(pmd, p, node, addr, next); } // too late: sync_global_pgds(); }
I really don't like the idea of having the x86 code just be super careful not to use the newly-populated PGD (this patch). That's fragile and further diverges the x86 implementation from the generic code.
The quick and dirty fix would be to just to call sync_global_pgds() all the time, like:
pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node) { pgd_t *pgd = pgd_offset_k(addr); if (pgd_none(*pgd)) { void *p = vmemmap_alloc_block_zero(PAGE_SIZE, node); if (!p) return NULL; pgd_populate(&init_mm, pgd, p);
sync_global_pgds(...); } return pgd;}
That actually mirrors how __kernel_physical_mapping_init() does it: watch for an actual PGD write and sync there. It shouldn't be too slow because it only calls sync_global_pgds() during actual PGD population which is horribly rare.
Could we do something like that, please? It might mean defining a new __weak symbol in mm/sparse-vmemmap.c and then calling out to an x86 implementation like vmemmap_set_pmd().
Hi Dave Hansen,
Thanks for the feedback. I do agree with a generic code rather than a diverse code for x86 implementation. If everyone else agrees, I'll send a new patch in this style.
Is x86 just an oddball with how it populates kernel page tables? I'm a bit surprised nobody else has this problem too.
Presumably, this won't happen on other platforms where they don't have the scenario of accessing that address before synchronizing the pgd. Please correct me if I'm wrong.
Br, G.G.