| Message ID | 20210830235927.6443-11-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PKS write protected page tables | expand |
On Mon, Aug 30, 2021 at 04:59:18PM -0700, Rick Edgecombe wrote: > fill_pte(), set_pte_vaddr(), etc allocate page tables with > spp_getpage(). Use alloc_table() for these allocations in order to get > tables from the cache of protected pages when needed. I can't say I tracked all the users of set_pte_vaddr(), but I don't see a fundamental reason why spp_getpage() would need GFP_ATOMIC. Even if there is a caller of set_pte_vaddr() that cannot sleep, it seems that page tables can be prepopulated so that set_pte_vaddr() will not need to allocate anything. > Opportunistically, fix a stale comment. Ack for this one :) > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > arch/x86/mm/init_64.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c > index 3c0323ad99da..de5a785ee89f 100644 > --- a/arch/x86/mm/init_64.c > +++ b/arch/x86/mm/init_64.c > @@ -220,16 +220,19 @@ static void sync_global_pgds(unsigned long start, unsigned long end) > > /* > * NOTE: This function is marked __ref because it calls __init function > - * (alloc_bootmem_pages). It's safe to do it ONLY when after_bootmem == 0. > + * (memblock_alloc). It's safe to do it ONLY when after_bootmem == 0. > */ > static __ref void *spp_getpage(void) > { > void *ptr; > > - if (after_bootmem) > - ptr = (void *) get_zeroed_page(GFP_ATOMIC); > - else > + if (after_bootmem) { > + struct page *page = alloc_table(GFP_ATOMIC | __GFP_ZERO); > + > + ptr = page ? page_address(page) : NULL; > + } else { > ptr = memblock_alloc(PAGE_SIZE, PAGE_SIZE); > + } > > if (!ptr || ((unsigned long)ptr & ~PAGE_MASK)) { > panic("set_pte_phys: cannot allocate page data %s\n", > -- > 2.17.1 >
On Tue, 2021-08-31 at 11:47 +0300, Mike Rapoport wrote: > On Mon, Aug 30, 2021 at 04:59:18PM -0700, Rick Edgecombe wrote: > > fill_pte(), set_pte_vaddr(), etc allocate page tables with > > spp_getpage(). Use alloc_table() for these allocations in order to > > get > > tables from the cache of protected pages when needed. > > > I can't say I tracked all the users of set_pte_vaddr(), but I don't > see a > fundamental reason why spp_getpage() would need GFP_ATOMIC. Yea, I couldn't find why it was done that way in the first place, and there were almost too many callers to audit. I guess I could roll up my sleeves an audit it all, but its not foolproof. Or put a warn for atomic context and pull all of the GFP_ATOMIC code if it doesn't get triggered after awhile. Also seems weird that it just panics here if the allocation fails. > Even if there > is a caller of set_pte_vaddr() that cannot sleep, it seems that page > tables > can be prepopulated so that set_pte_vaddr() will not need to allocate > anything. Hmm, could work for the fixmap callers I guess (maybe already happening in practice). Xen and a few other things seems to use this for non- fixmap things, but it's during init and easier to audit.
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c index 3c0323ad99da..de5a785ee89f 100644 --- a/arch/x86/mm/init_64.c +++ b/arch/x86/mm/init_64.c @@ -220,16 +220,19 @@ static void sync_global_pgds(unsigned long start, unsigned long end) /* * NOTE: This function is marked __ref because it calls __init function - * (alloc_bootmem_pages). It's safe to do it ONLY when after_bootmem == 0. + * (memblock_alloc). It's safe to do it ONLY when after_bootmem == 0. */ static __ref void *spp_getpage(void) { void *ptr; - if (after_bootmem) - ptr = (void *) get_zeroed_page(GFP_ATOMIC); - else + if (after_bootmem) { + struct page *page = alloc_table(GFP_ATOMIC | __GFP_ZERO); + + ptr = page ? page_address(page) : NULL; + } else { ptr = memblock_alloc(PAGE_SIZE, PAGE_SIZE); + } if (!ptr || ((unsigned long)ptr & ~PAGE_MASK)) { panic("set_pte_phys: cannot allocate page data %s\n",
fill_pte(), set_pte_vaddr(), etc allocate page tables with spp_getpage(). Use alloc_table() for these allocations in order to get tables from the cache of protected pages when needed. Opportunistically, fix a stale comment. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- arch/x86/mm/init_64.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)