Re: [PATCH v2 0/2] x86/mm/KASLR: Change the granularity of randomization to PUD size in 5-level

[Kirill A. Shutemov]

On Thu, Feb 28, 2019 at 06:04:19PM +0800, Baoquan He wrote:
> On 02/28/19 at 12:55pm, Kirill A. Shutemov wrote:
> > I cannot apply the first patch too. Hm?
> 
> It's weird. I use 'git cherry-pick' and it can be cleanly applied.
> 
> And git formatted patch can be applied too. Could you try below one? 
> git am works for me with it, based on this commit on linus's tree.
> 7d762d69145a  afs: Fix manually set volume location server list.
> 
> 
> From dcbe8e4846102fdd051deb3c2946125c17b270fb Mon Sep 17 00:00:00 2001
> From: Baoquan He <bhe@xxxxxxxxxx>
> Date: Thu, 21 Feb 2019 11:46:59 +0800
> Subject: [PATCH v3] x86/mm/KASLR: Only build one PUD entry of area for real mode
>  trampoline
> 
> The current code builds identity mapping for real mode treampoline by
> borrowing page tables from the direct mapping section if KASLR is
> enabled. It will copy present entries of the first PUD table in 4-level
> paging mode, or the first P4D table in 5-level paging mode.
> 
> However, there's only a very small area under low 1 MB reserved
> for real mode trampoline in reserve_real_mode(). Makes no sense
> to build up so large area of mapping for it. Since the randomization
> granularity in 4-level is 1 GB, and 512 GB in 5-level, only copying
> one PUD entry is enough.
> 
> Hence, only copy one PUD entry of area where physical address 0
> resides. And this is preparation for later changing the randomization
> granularity of 5-level paging mode from 512 GB to 1 GB.
> 
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> ---
>  arch/x86/mm/kaslr.c | 82 +++++++++++++++++++++------------------------
>  1 file changed, 38 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> index 754b5da91d43..131e08a10a68 100644
> --- a/arch/x86/mm/kaslr.c
> +++ b/arch/x86/mm/kaslr.c
> @@ -226,74 +226,68 @@ void __init kernel_randomize_memory(void)
>  
>  static void __meminit init_trampoline_pud(void)
>  {
> -	unsigned long paddr, paddr_next;
> +	unsigned long paddr, vaddr;
>  	pgd_t *pgd;
> -	pud_t *pud_page, *pud_page_tramp;
> -	int i;
> +

Unneeded empty line.

> +	pud_t *pud_page, *pud_page_tramp, *pud, *pud_tramp;
>  
>  	pud_page_tramp = alloc_low_page();
>  
>  	paddr = 0;
> +	vaddr = (unsigned long)__va(paddr);

Maybe just

	vaddr = PAGE_OFFSET;

?

>  	pgd = pgd_offset_k((unsigned long)__va(paddr));

We do have 'vaddr' already.

	pgd = pgd_offset_k(vaddr);

> -	pud_page = (pud_t *) pgd_page_vaddr(*pgd);
> -
> -	for (i = pud_index(paddr); i < PTRS_PER_PUD; i++, paddr = paddr_next) {
> -		pud_t *pud, *pud_tramp;
> -		unsigned long vaddr = (unsigned long)__va(paddr);
>  
> -		pud_tramp = pud_page_tramp + pud_index(paddr);
> -		pud = pud_page + pud_index(vaddr);
> -		paddr_next = (paddr & PUD_MASK) + PUD_SIZE;
> -
> -		*pud_tramp = *pud;
> -	}
> +	if (pgtable_l5_enabled()) {
> +		p4d_t *p4d_page, *p4d_page_tramp, *p4d, *p4d_tramp;
> +		p4d_page_tramp = alloc_low_page();
>  
> -	set_pgd(&trampoline_pgd_entry,
> -		__pgd(_KERNPG_TABLE | __pa(pud_page_tramp)));
> -}
> +		p4d_page = (p4d_t *) pgd_page_vaddr(*pgd);
> +		p4d = p4d_page + p4d_index(vaddr);
>  
> -static void __meminit init_trampoline_p4d(void)
> -{
> -	unsigned long paddr, paddr_next;
> -	pgd_t *pgd;
> -	p4d_t *p4d_page, *p4d_page_tramp;
> -	int i;
> +		pud_page = (pud_t *) p4d_page_vaddr(*p4d);
> +		pud = pud_page + pud_index(vaddr);
>  
> -	p4d_page_tramp = alloc_low_page();
> +		p4d_tramp = p4d_page_tramp + p4d_index(paddr);
> +		pud_tramp = pud_page_tramp + pud_index(paddr);

p?d_index() has to be called on the virtual address. It shouldn't break
anything, but it's wrong from conceptual point of view.

I don't think need paddr at all in the function.

BTW, any reason we cannot use p?d_offset() instead of playing with
p?d_index() directly?


-- 
 Kirill A. Shutemov