Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453331 > unrolled thread

[PATCH v1 0/2] x86/power/64: Make KASLR memory randomization compatible with hibernation

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/2] x86/power/64: Make KASLR memory randomization compatible with hibernation Thomas Garnier <thgarnie@google.com> - 2016-08-01 19:10 +0200
    [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping Thomas Garnier <thgarnie@google.com> - 2016-08-01 19:10 +0200
      Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-02 02:40 +0200
        Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for  temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-02 21:10 +0200
      Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for  temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-02 20:00 +0200
        Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for  temporary mapping Thomas Garnier <thgarnie@google.com> - 2016-08-02 20:00 +0200
          Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for  temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-02 22:00 +0200
            Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for  temporary mapping Thomas Garnier <thgarnie@google.com> - 2016-08-03 18:40 +0200
            [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-03 23:00 +0200
              Re: [PATCH v2] x86/power/64: Support unaligned addresses for  temporary mapping "Rafael J. Wysocki" <rafael@kernel.org> - 2016-08-04 00:40 +0200
                Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-07 03:00 +0200
                  Re: [PATCH v2] x86/power/64: Support unaligned addresses for  temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-07 07:00 +0200
                    Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-08 01:20 +0200
                      Re: [PATCH v2] x86/power/64: Support unaligned addresses for  temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-08 09:10 +0200
                        Re: [PATCH v2] x86/power/64: Support unaligned addresses for  temporary mapping Yinghai Lu <yinghai@kernel.org> - 2016-08-08 09:30 +0200
                          Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-08 15:20 +0200
    Re: [PATCH v1 0/2] x86/power/64: Make KASLR memory randomization compatible with hibernation "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-02 01:50 +0200
      Re: [PATCH v1 0/2] x86/power/64: Make KASLR memory randomization compatible with hibernation "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-08-02 02:50 +0200

#1453331 — [PATCH v1 0/2] x86/power/64: Make KASLR memory randomization compatible with hibernation

***Background:
KASLR memory randomization for x86_64 was added when KASLR did not support hibernation. Now that it does, some changes are needed.

***Problems that needed solving:
Hibernation was failing on reboot with a GP fault when CONFIG_RANDOMIZE_MEMORY was enabled. Two issues were identified.

The original fault was due to a wrong physical address assigned to cr3. The problem was introduced with __PAGE_OFFSET becoming a global variable when randomized. The fix uses a define to use the glbobal or immediate value based on config settings.

The second isssue was that the temporary page table mapping did not support virtual addresses not aligned on PGD level. KASLR memory randomization will generated a random address aligned on PUD level. The fix correctly calculates the offset on all levels of the temporary page table.

***Parts:
 - 01/02: Support unaligned addresses (second issue)
 - 02/02: Fix __PAGE_OFFSET usage on assembly (first issue)

[toc] | [next] | [standalone]

#1453334 — [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

Correctly setup the temporary mapping for hibernation. Previous
implementation assumed the address was aligned on the PGD level. With
KASLR memory randomization enabled, the address is randomized on the PUD
level. This change supports unaligned address up to PMD.

Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
 arch/x86/mm/ident_map.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c
index ec21796..ea1ebf1 100644
--- a/arch/x86/mm/ident_map.c
+++ b/arch/x86/mm/ident_map.c
@@ -3,15 +3,16 @@
  * included by both the compressed kernel and the regular kernel.
  */
 
-static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
+static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
 			   unsigned long addr, unsigned long end)
 {
-	addr &= PMD_MASK;
-	for (; addr < end; addr += PMD_SIZE) {
-		pmd_t *pmd = pmd_page + pmd_index(addr);
+	int off = info->kernel_mapping ? pmd_index(__PAGE_OFFSET) : 0;
+
+	for (addr &= PMD_MASK; addr < end; addr += PMD_SIZE) {
+		pmd_t *pmd = pmd_page + pmd_index(addr) + off;
 
 		if (!pmd_present(*pmd))
-			set_pmd(pmd, __pmd(addr | pmd_flag));
+			set_pmd(pmd, __pmd(addr | info->pmd_flag));
 	}
 }
 
@@ -19,9 +20,10 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
 			  unsigned long addr, unsigned long end)
 {
 	unsigned long next;
+	int off = info->kernel_mapping ? pud_index(__PAGE_OFFSET) : 0;
 
 	for (; addr < end; addr = next) {
-		pud_t *pud = pud_page + pud_index(addr);
+		pud_t *pud = pud_page + pud_index(addr) + off;
 		pmd_t *pmd;
 
 		next = (addr & PUD_MASK) + PUD_SIZE;
@@ -30,13 +32,13 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
 
 		if (pud_present(*pud)) {
 			pmd = pmd_offset(pud, 0);
-			ident_pmd_init(info->pmd_flag, pmd, addr, next);
+			ident_pmd_init(info, pmd, addr, next);
 			continue;
 		}
 		pmd = (pmd_t *)info->alloc_pgt_page(info->context);
 		if (!pmd)
 			return -ENOMEM;
-		ident_pmd_init(info->pmd_flag, pmd, addr, next);
+		ident_pmd_init(info, pmd, addr, next);
 		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
 	}
 
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1453520 — Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

On Monday, August 01, 2016 10:07:59 AM Thomas Garnier wrote:
> Correctly setup the temporary mapping for hibernation. Previous
> implementation assumed the address was aligned on the PGD level. With
> KASLR memory randomization enabled, the address is randomized on the PUD
> level. This change supports unaligned address up to PMD.
> 
> Signed-off-by: Thomas Garnier <thgarnie@google.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

This code is shared with kexec AFAICS, so it likely is better to push it
through tip rather than through the PM tree.

> ---
>  arch/x86/mm/ident_map.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c
> index ec21796..ea1ebf1 100644
> --- a/arch/x86/mm/ident_map.c
> +++ b/arch/x86/mm/ident_map.c
> @@ -3,15 +3,16 @@
>   * included by both the compressed kernel and the regular kernel.
>   */
>  
> -static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
> +static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
>  			   unsigned long addr, unsigned long end)
>  {
> -	addr &= PMD_MASK;
> -	for (; addr < end; addr += PMD_SIZE) {
> -		pmd_t *pmd = pmd_page + pmd_index(addr);
> +	int off = info->kernel_mapping ? pmd_index(__PAGE_OFFSET) : 0;
> +
> +	for (addr &= PMD_MASK; addr < end; addr += PMD_SIZE) {
> +		pmd_t *pmd = pmd_page + pmd_index(addr) + off;
>  
>  		if (!pmd_present(*pmd))
> -			set_pmd(pmd, __pmd(addr | pmd_flag));
> +			set_pmd(pmd, __pmd(addr | info->pmd_flag));
>  	}
>  }
>  
> @@ -19,9 +20,10 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
>  			  unsigned long addr, unsigned long end)
>  {
>  	unsigned long next;
> +	int off = info->kernel_mapping ? pud_index(__PAGE_OFFSET) : 0;
>  
>  	for (; addr < end; addr = next) {
> -		pud_t *pud = pud_page + pud_index(addr);
> +		pud_t *pud = pud_page + pud_index(addr) + off;
>  		pmd_t *pmd;
>  
>  		next = (addr & PUD_MASK) + PUD_SIZE;
> @@ -30,13 +32,13 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
>  
>  		if (pud_present(*pud)) {
>  			pmd = pmd_offset(pud, 0);
> -			ident_pmd_init(info->pmd_flag, pmd, addr, next);
> +			ident_pmd_init(info, pmd, addr, next);
>  			continue;
>  		}
>  		pmd = (pmd_t *)info->alloc_pgt_page(info->context);
>  		if (!pmd)
>  			return -ENOMEM;
> -		ident_pmd_init(info->pmd_flag, pmd, addr, next);
> +		ident_pmd_init(info, pmd, addr, next);
>  		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
>  	}
>  
> 

[toc] | [prev] | [next] | [standalone]

#1455405 — Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

On Mon, Aug 1, 2016 at 5:36 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, August 01, 2016 10:07:59 AM Thomas Garnier wrote:
>> Correctly setup the temporary mapping for hibernation. Previous
>> implementation assumed the address was aligned on the PGD level. With
>> KASLR memory randomization enabled, the address is randomized on the PUD
>> level. This change supports unaligned address up to PMD.
>
> This code is shared with kexec AFAICS, so it likely is better to push it
> through tip rather than through the PM tree.

Only calling path via arch/x86/power/hibernate_64.c have
   kernel_mapping = true;
other two paths: arch/x86/boot/compressed/pagetable.c and
arch/x86/kernel/machine_kexec_64.c
all have kernel_mapping as false.

maybe that path need simplified kernel_physical_mapping_init() instead?

Thanks

Yinghai

[toc] | [prev] | [next] | [standalone]

#1455209 — Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

On Mon, Aug 1, 2016 at 10:07 AM, Thomas Garnier <thgarnie@google.com> wrote:
> Correctly setup the temporary mapping for hibernation. Previous
> implementation assumed the address was aligned on the PGD level. With
> KASLR memory randomization enabled, the address is randomized on the PUD
> level. This change supports unaligned address up to PMD.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
>  arch/x86/mm/ident_map.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c
> index ec21796..ea1ebf1 100644
> --- a/arch/x86/mm/ident_map.c
> +++ b/arch/x86/mm/ident_map.c
> @@ -3,15 +3,16 @@
>   * included by both the compressed kernel and the regular kernel.
>   */
>
> -static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
> +static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
>                            unsigned long addr, unsigned long end)
>  {
> -       addr &= PMD_MASK;
> -       for (; addr < end; addr += PMD_SIZE) {
> -               pmd_t *pmd = pmd_page + pmd_index(addr);
> +       int off = info->kernel_mapping ? pmd_index(__PAGE_OFFSET) : 0;
> +
> +       for (addr &= PMD_MASK; addr < end; addr += PMD_SIZE) {
> +               pmd_t *pmd = pmd_page + pmd_index(addr) + off;
>
>                 if (!pmd_present(*pmd))
> -                       set_pmd(pmd, __pmd(addr | pmd_flag));
> +                       set_pmd(pmd, __pmd(addr | info->pmd_flag));
>         }
>  }
>
> @@ -19,9 +20,10 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
>                           unsigned long addr, unsigned long end)
>  {
>         unsigned long next;
> +       int off = info->kernel_mapping ? pud_index(__PAGE_OFFSET) : 0;
>
>         for (; addr < end; addr = next) {
> -               pud_t *pud = pud_page + pud_index(addr);
> +               pud_t *pud = pud_page + pud_index(addr) + off;
>                 pmd_t *pmd;
>
>                 next = (addr & PUD_MASK) + PUD_SIZE;

Is there any chance for (pud_index(addr) + off) or (pmd_index(addr) + off)
bigger than 512?

Looks like we need to change the loop from phys address to virtual
address instead.
to avoid the overflow.

Thanks

Yinghai

[toc] | [prev] | [next] | [standalone]

#1455211 — Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

On Tue, Aug 2, 2016 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Aug 1, 2016 at 10:07 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> Correctly setup the temporary mapping for hibernation. Previous
>> implementation assumed the address was aligned on the PGD level. With
>> KASLR memory randomization enabled, the address is randomized on the PUD
>> level. This change supports unaligned address up to PMD.
>>
>> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> ---
>>  arch/x86/mm/ident_map.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c
>> index ec21796..ea1ebf1 100644
>> --- a/arch/x86/mm/ident_map.c
>> +++ b/arch/x86/mm/ident_map.c
>> @@ -3,15 +3,16 @@
>>   * included by both the compressed kernel and the regular kernel.
>>   */
>>
>> -static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
>> +static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
>>                            unsigned long addr, unsigned long end)
>>  {
>> -       addr &= PMD_MASK;
>> -       for (; addr < end; addr += PMD_SIZE) {
>> -               pmd_t *pmd = pmd_page + pmd_index(addr);
>> +       int off = info->kernel_mapping ? pmd_index(__PAGE_OFFSET) : 0;
>> +
>> +       for (addr &= PMD_MASK; addr < end; addr += PMD_SIZE) {
>> +               pmd_t *pmd = pmd_page + pmd_index(addr) + off;
>>
>>                 if (!pmd_present(*pmd))
>> -                       set_pmd(pmd, __pmd(addr | pmd_flag));
>> +                       set_pmd(pmd, __pmd(addr | info->pmd_flag));
>>         }
>>  }
>>
>> @@ -19,9 +20,10 @@ static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
>>                           unsigned long addr, unsigned long end)
>>  {
>>         unsigned long next;
>> +       int off = info->kernel_mapping ? pud_index(__PAGE_OFFSET) : 0;
>>
>>         for (; addr < end; addr = next) {
>> -               pud_t *pud = pud_page + pud_index(addr);
>> +               pud_t *pud = pud_page + pud_index(addr) + off;
>>                 pmd_t *pmd;
>>
>>                 next = (addr & PUD_MASK) + PUD_SIZE;
>
> Is there any chance for (pud_index(addr) + off) or (pmd_index(addr) + off)
> bigger than 512?
>
> Looks like we need to change the loop from phys address to virtual
> address instead.
> to avoid the overflow.
>

That's a good point. I will take a look at it.

> Thanks
>
> Yinghai

[toc] | [prev] | [next] | [standalone]

#1455438 — Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 2, 2016 at 10:48 AM, Thomas Garnier <thgarnie@google.com> wrote:
> On Tue, Aug 2, 2016 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>> Looks like we need to change the loop from phys address to virtual
>> address instead.
>> to avoid the overflow.

something like attached.

[toc] | [prev] | [next] | [standalone]

#1455890 — Re: [PATCH v1 1/2] x86/power/64: Support unaligned addresses for temporary mapping

On Tue, Aug 2, 2016 at 12:55 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Aug 2, 2016 at 10:48 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> On Tue, Aug 2, 2016 at 10:36 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>
>>> Looks like we need to change the loop from phys address to virtual
>>> address instead.
>>> to avoid the overflow.
>
> something like attached.

I tested it and it worked well. I just got this warning on build:

In file included from arch/x86/mm/init_64.c:60:0:
arch/x86/mm/ident_map.c: In function ‘ident_pmd_init’:
arch/x86/mm/ident_map.c:18:29: warning: suggest parentheses around
arithmetic in operand of ‘|’ [-Wparentheses]
    set_pmd(pmd, __pmd(vaddr - off | info->pmd_flag));

Do you want to resend your version for integration?

[toc] | [prev] | [next] | [standalone]

#1456023 — [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

From: Thomas Garnier <thgarnie@google.com>

Correctly setup the temporary mapping for hibernation. Previous
implementation assumed the offset between KVA and PA was aligned on the PGD level.
With KASLR memory randomization enabled, the offset is randomized on the PUD
level. This change supports unaligned up to PMD.

Signed-off-by: Thomas Garnier <thgarnie@google.com>
[yinghai: change loop to virtual address]
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/mm/ident_map.c |   54 ++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

Index: linux-2.6/arch/x86/mm/ident_map.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/ident_map.c
+++ linux-2.6/arch/x86/mm/ident_map.c
@@ -3,40 +3,47 @@
  * included by both the compressed kernel and the regular kernel.
  */
 
-static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
+static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
 			   unsigned long addr, unsigned long end)
 {
-	addr &= PMD_MASK;
-	for (; addr < end; addr += PMD_SIZE) {
-		pmd_t *pmd = pmd_page + pmd_index(addr);
+	unsigned long off = info->kernel_mapping ? __PAGE_OFFSET : 0;
+	unsigned long vaddr = addr + off;
+	unsigned long vend = end + off;
+
+	vaddr &= PMD_MASK;
+	for (; vaddr < vend; vaddr += PMD_SIZE) {
+		pmd_t *pmd = pmd_page + pmd_index(vaddr);
 
 		if (!pmd_present(*pmd))
-			set_pmd(pmd, __pmd(addr | pmd_flag));
+			set_pmd(pmd, __pmd((vaddr - off) | info->pmd_flag));
 	}
 }
 
 static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
 			  unsigned long addr, unsigned long end)
 {
-	unsigned long next;
+	unsigned long off = info->kernel_mapping ? __PAGE_OFFSET : 0;
+	unsigned long vaddr = addr + off;
+	unsigned long vend = end + off;
+	unsigned long vnext;
 
-	for (; addr < end; addr = next) {
-		pud_t *pud = pud_page + pud_index(addr);
+	for (; vaddr < vend; vaddr = vnext) {
+		pud_t *pud = pud_page + pud_index(vaddr);
 		pmd_t *pmd;
 
-		next = (addr & PUD_MASK) + PUD_SIZE;
-		if (next > end)
-			next = end;
+		vnext = (vaddr & PUD_MASK) + PUD_SIZE;
+		if (vnext > vend)
+			vnext = vend;
 
 		if (pud_present(*pud)) {
 			pmd = pmd_offset(pud, 0);
-			ident_pmd_init(info->pmd_flag, pmd, addr, next);
+			ident_pmd_init(info, pmd, vaddr - off, vnext - off);
 			continue;
 		}
 		pmd = (pmd_t *)info->alloc_pgt_page(info->context);
 		if (!pmd)
 			return -ENOMEM;
-		ident_pmd_init(info->pmd_flag, pmd, addr, next);
+		ident_pmd_init(info, pmd, vaddr - off, vnext - off);
 		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
 	}
 
@@ -46,21 +53,24 @@ static int ident_pud_init(struct x86_map
 int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
 			      unsigned long addr, unsigned long end)
 {
-	unsigned long next;
 	int result;
-	int off = info->kernel_mapping ? pgd_index(__PAGE_OFFSET) : 0;
+	unsigned long off = info->kernel_mapping ? __PAGE_OFFSET : 0;
+	unsigned long vaddr = addr + off;
+	unsigned long vend = end + off;
+	unsigned long vnext;
 
-	for (; addr < end; addr = next) {
-		pgd_t *pgd = pgd_page + pgd_index(addr) + off;
+	for (; vaddr < vend; vaddr = vnext) {
+		pgd_t *pgd = pgd_page + pgd_index(vaddr);
 		pud_t *pud;
 
-		next = (addr & PGDIR_MASK) + PGDIR_SIZE;
-		if (next > end)
-			next = end;
+		vnext = (vaddr & PGDIR_MASK) + PGDIR_SIZE;
+		if (vnext > vend)
+			vnext = vend;
 
 		if (pgd_present(*pgd)) {
 			pud = pud_offset(pgd, 0);
-			result = ident_pud_init(info, pud, addr, next);
+			result = ident_pud_init(info, pud, vaddr - off,
+						vnext - off);
 			if (result)
 				return result;
 			continue;
@@ -69,7 +79,7 @@ int kernel_ident_mapping_init(struct x86
 		pud = (pud_t *)info->alloc_pgt_page(info->context);
 		if (!pud)
 			return -ENOMEM;
-		result = ident_pud_init(info, pud, addr, next);
+		result = ident_pud_init(info, pud, vaddr - off, vnext - off);
 		if (result)
 			return result;
 		set_pgd(pgd, __pgd(__pa(pud) | _KERNPG_TABLE));

[toc] | [prev] | [next] | [standalone]

#1456046 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Wed, Aug 3, 2016 at 8:23 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> From: Thomas Garnier <thgarnie@google.com>
>
> Correctly setup the temporary mapping for hibernation. Previous
> implementation assumed the offset between KVA and PA was aligned on the PGD level.
> With KASLR memory randomization enabled, the offset is randomized on the PUD
> level. This change supports unaligned up to PMD.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> [yinghai: change loop to virtual address]
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  arch/x86/mm/ident_map.c |   54 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
>
> Index: linux-2.6/arch/x86/mm/ident_map.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/ident_map.c
> +++ linux-2.6/arch/x86/mm/ident_map.c
> @@ -3,40 +3,47 @@
>   * included by both the compressed kernel and the regular kernel.
>   */
>
> -static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
> +static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
>                            unsigned long addr, unsigned long end)
>  {
> -       addr &= PMD_MASK;
> -       for (; addr < end; addr += PMD_SIZE) {
> -               pmd_t *pmd = pmd_page + pmd_index(addr);
> +       unsigned long off = info->kernel_mapping ? __PAGE_OFFSET : 0;
> +       unsigned long vaddr = addr + off;
> +       unsigned long vend = end + off;
> +
> +       vaddr &= PMD_MASK;
> +       for (; vaddr < vend; vaddr += PMD_SIZE) {
> +               pmd_t *pmd = pmd_page + pmd_index(vaddr);
>
>                 if (!pmd_present(*pmd))
> -                       set_pmd(pmd, __pmd(addr | pmd_flag));
> +                       set_pmd(pmd, __pmd((vaddr - off) | info->pmd_flag));
>         }
>  }
>
>  static int ident_pud_init(struct x86_mapping_info *info, pud_t *pud_page,
>                           unsigned long addr, unsigned long end)
>  {
> -       unsigned long next;
> +       unsigned long off = info->kernel_mapping ? __PAGE_OFFSET : 0;
> +       unsigned long vaddr = addr + off;
> +       unsigned long vend = end + off;
> +       unsigned long vnext;
>
> -       for (; addr < end; addr = next) {
> -               pud_t *pud = pud_page + pud_index(addr);
> +       for (; vaddr < vend; vaddr = vnext) {
> +               pud_t *pud = pud_page + pud_index(vaddr);
>                 pmd_t *pmd;
>
> -               next = (addr & PUD_MASK) + PUD_SIZE;
> -               if (next > end)
> -                       next = end;
> +               vnext = (vaddr & PUD_MASK) + PUD_SIZE;
> +               if (vnext > vend)
> +                       vnext = vend;
>
>                 if (pud_present(*pud)) {
>                         pmd = pmd_offset(pud, 0);
> -                       ident_pmd_init(info->pmd_flag, pmd, addr, next);
> +                       ident_pmd_init(info, pmd, vaddr - off, vnext - off);
>                         continue;
>                 }
>                 pmd = (pmd_t *)info->alloc_pgt_page(info->context);
>                 if (!pmd)
>                         return -ENOMEM;
> -               ident_pmd_init(info->pmd_flag, pmd, addr, next);
> +               ident_pmd_init(info, pmd, vaddr - off, vnext - off);
>                 set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
>         }
>
> @@ -46,21 +53,24 @@ static int ident_pud_init(struct x86_map
>  int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
>                               unsigned long addr, unsigned long end)
>  {
> -       unsigned long next;
>         int result;
> -       int off = info->kernel_mapping ? pgd_index(__PAGE_OFFSET) : 0;
> +       unsigned long off = info->kernel_mapping ? __PAGE_OFFSET : 0;
> +       unsigned long vaddr = addr + off;
> +       unsigned long vend = end + off;
> +       unsigned long vnext;
>
> -       for (; addr < end; addr = next) {
> -               pgd_t *pgd = pgd_page + pgd_index(addr) + off;
> +       for (; vaddr < vend; vaddr = vnext) {
> +               pgd_t *pgd = pgd_page + pgd_index(vaddr);
>                 pud_t *pud;
>
> -               next = (addr & PGDIR_MASK) + PGDIR_SIZE;
> -               if (next > end)
> -                       next = end;
> +               vnext = (vaddr & PGDIR_MASK) + PGDIR_SIZE;
> +               if (vnext > vend)
> +                       vnext = vend;
>
>                 if (pgd_present(*pgd)) {
>                         pud = pud_offset(pgd, 0);
> -                       result = ident_pud_init(info, pud, addr, next);
> +                       result = ident_pud_init(info, pud, vaddr - off,
> +                                               vnext - off);
>                         if (result)
>                                 return result;
>                         continue;
> @@ -69,7 +79,7 @@ int kernel_ident_mapping_init(struct x86
>                 pud = (pud_t *)info->alloc_pgt_page(info->context);
>                 if (!pud)
>                         return -ENOMEM;
> -               result = ident_pud_init(info, pud, addr, next);
> +               result = ident_pud_init(info, pud, vaddr - off, vnext - off);
>                 if (result)
>                         return result;
>                 set_pgd(pgd, __pgd(__pa(pud) | _KERNPG_TABLE));
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1457412 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Wednesday, August 03, 2016 11:28:48 PM Rafael J. Wysocki wrote:
> On Wed, Aug 3, 2016 at 8:23 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > From: Thomas Garnier <thgarnie@google.com>
> >
> > Correctly setup the temporary mapping for hibernation. Previous
> > implementation assumed the offset between KVA and PA was aligned on the PGD level.
> > With KASLR memory randomization enabled, the offset is randomized on the PUD
> > level. This change supports unaligned up to PMD.
> >
> > Signed-off-by: Thomas Garnier <thgarnie@google.com>
> > [yinghai: change loop to virtual address]
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On a second thought, it seems to be better to follow your suggestion to simply
provide a special version of kernel_ident_mapping_init() for hibernation,
because it is sufficiently distinct from the other users of the code in
ident_map.c.

The patch below does just that (lightly tested).

Thomas, can you please test this one too?

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] x86/power/64: Always create temporary identity mapping correctly

The low-level resume-from-hibernation code on x86-64 uses
kernel_ident_mapping_init() to create the temoprary identity mapping,
but that function assumes that the offset between kernel virtual
addresses and physical addresses is aligned on the PGD level.

However, with a randomized identity mapping base, it may be aligned
on the PUD level and if that happens, the temporary identity mapping
created by set_up_temporary_mappings() will not reflect the actual
kernel identity mapping and the image restoration will fail as a
result (leading to a kernel panic most of the time).

To fix this problem, provide simplified routines for creating the
temporary identity mapping during resume from hibernation on x86-64
that support unaligned offsets between KVA and PA up to the PMD
level.

Although kernel_ident_mapping_init() might be made work in that
case too, using hibernation-specific code for that is way simpler.

Reported-by: Thomas Garnier <thgarnie@google.com>
Suggested-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/x86/power/hibernate_64.c |   61 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 53 insertions(+), 8 deletions(-)

Index: linux-pm/arch/x86/power/hibernate_64.c
===================================================================
--- linux-pm.orig/arch/x86/power/hibernate_64.c
+++ linux-pm/arch/x86/power/hibernate_64.c
@@ -77,18 +77,63 @@ static int set_up_temporary_text_mapping
 	return 0;
 }
 
-static void *alloc_pgt_page(void *context)
+static void ident_pmd_init(pmd_t *pmd, unsigned long addr, unsigned long end)
 {
-	return (void *)get_safe_page(GFP_ATOMIC);
+	for (; addr < end; addr += PMD_SIZE)
+		set_pmd(pmd + pmd_index(addr),
+			__pmd((addr - __PAGE_OFFSET) | __PAGE_KERNEL_LARGE_EXEC));
+}
+
+static int ident_pud_init(pud_t *pud, unsigned long addr, unsigned long end)
+{
+	unsigned long next;
+
+	for (; addr < end; addr = next) {
+		pmd_t *pmd;
+
+		pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
+		if (!pmd)
+			return -ENOMEM;
+
+		next = (addr & PUD_MASK) + PUD_SIZE;
+		if (next > end)
+			next = end;
+
+		ident_pmd_init(pmd, addr & PMD_MASK, next);
+		set_pud(pud + pud_index(addr), __pud(__pa(pmd) | _KERNPG_TABLE));
+	}
+	return 0;
+}
+
+static int ident_mapping_init(pgd_t *pgd, unsigned long mstart, unsigned long mend)
+{
+	unsigned long addr = mstart + __PAGE_OFFSET;
+	unsigned long end = mend + __PAGE_OFFSET;
+	unsigned long next;
+
+	for (; addr < end; addr = next) {
+		pud_t *pud;
+		int result;
+
+		pud = (pud_t *)get_safe_page(GFP_ATOMIC);
+		if (!pud)
+			return -ENOMEM;
+
+		next = (addr & PGDIR_MASK) + PGDIR_SIZE;
+		if (next > end)
+			next = end;
+
+		result = ident_pud_init(pud, addr, next);
+		if (result)
+			return result;
+
+		set_pgd(pgd + pgd_index(addr), __pgd(__pa(pud) | _KERNPG_TABLE));
+	}
+	return 0;
 }
 
 static int set_up_temporary_mappings(void)
 {
-	struct x86_mapping_info info = {
-		.alloc_pgt_page	= alloc_pgt_page,
-		.pmd_flag	= __PAGE_KERNEL_LARGE_EXEC,
-		.kernel_mapping = true,
-	};
 	unsigned long mstart, mend;
 	pgd_t *pgd;
 	int result;
@@ -108,7 +153,7 @@ static int set_up_temporary_mappings(voi
 		mstart = pfn_mapped[i].start << PAGE_SHIFT;
 		mend   = pfn_mapped[i].end << PAGE_SHIFT;
 
-		result = kernel_ident_mapping_init(&info, pgd, mstart, mend);
+		result = ident_mapping_init(pgd, mstart, mend);
 		if (result)
 			return result;
 	}

[toc] | [prev] | [next] | [standalone]

#1457431 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Sat, Aug 6, 2016 at 6:03 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, August 03, 2016 11:28:48 PM Rafael J. Wysocki wrote:
>
> On a second thought, it seems to be better to follow your suggestion to simply
> provide a special version of kernel_ident_mapping_init() for hibernation,
> because it is sufficiently distinct from the other users of the code in
> ident_map.c.
>
> The patch below does just that (lightly tested).
>
>
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] x86/power/64: Always create temporary identity mapping correctly
>
> The low-level resume-from-hibernation code on x86-64 uses
> kernel_ident_mapping_init() to create the temoprary identity mapping,
> but that function assumes that the offset between kernel virtual
> addresses and physical addresses is aligned on the PGD level.
>
> However, with a randomized identity mapping base, it may be aligned
> on the PUD level and if that happens, the temporary identity mapping
> created by set_up_temporary_mappings() will not reflect the actual
> kernel identity mapping and the image restoration will fail as a
> result (leading to a kernel panic most of the time).
>
> To fix this problem, provide simplified routines for creating the
> temporary identity mapping during resume from hibernation on x86-64
> that support unaligned offsets between KVA and PA up to the PMD
> level.
>
> Although kernel_ident_mapping_init() might be made work in that
> case too, using hibernation-specific code for that is way simpler.
>
> Reported-by: Thomas Garnier <thgarnie@google.com>
> Suggested-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  arch/x86/power/hibernate_64.c |   61 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 53 insertions(+), 8 deletions(-)
>
> Index: linux-pm/arch/x86/power/hibernate_64.c
> ===================================================================
> --- linux-pm.orig/arch/x86/power/hibernate_64.c
> +++ linux-pm/arch/x86/power/hibernate_64.c
> @@ -77,18 +77,63 @@ static int set_up_temporary_text_mapping
>         return 0;
>  }
>
> -static void *alloc_pgt_page(void *context)
> +static void ident_pmd_init(pmd_t *pmd, unsigned long addr, unsigned long end)
>  {
> -       return (void *)get_safe_page(GFP_ATOMIC);
> +       for (; addr < end; addr += PMD_SIZE)
> +               set_pmd(pmd + pmd_index(addr),
> +                       __pmd((addr - __PAGE_OFFSET) | __PAGE_KERNEL_LARGE_EXEC));
> +}
> +
> +static int ident_pud_init(pud_t *pud, unsigned long addr, unsigned long end)
> +{
> +       unsigned long next;
> +
> +       for (; addr < end; addr = next) {
> +               pmd_t *pmd;
> +
> +               pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
> +               if (!pmd)
> +                       return -ENOMEM;
> +
> +               next = (addr & PUD_MASK) + PUD_SIZE;
> +               if (next > end)
> +                       next = end;
> +
> +               ident_pmd_init(pmd, addr & PMD_MASK, next);
> +               set_pud(pud + pud_index(addr), __pud(__pa(pmd) | _KERNPG_TABLE));
> +       }
> +       return 0;
> +}
> +
> +static int ident_mapping_init(pgd_t *pgd, unsigned long mstart, unsigned long mend)
> +{
> +       unsigned long addr = mstart + __PAGE_OFFSET;
> +       unsigned long end = mend + __PAGE_OFFSET;
> +       unsigned long next;
> +
> +       for (; addr < end; addr = next) {
> +               pud_t *pud;
> +               int result;
> +
> +               pud = (pud_t *)get_safe_page(GFP_ATOMIC);
> +               if (!pud)
> +                       return -ENOMEM;
> +
> +               next = (addr & PGDIR_MASK) + PGDIR_SIZE;
> +               if (next > end)
> +                       next = end;
> +
> +               result = ident_pud_init(pud, addr, next);
> +               if (result)
> +                       return result;
> +
> +               set_pgd(pgd + pgd_index(addr), __pgd(__pa(pud) | _KERNPG_TABLE));
> +       }
> +       return 0;
>  }
>
>  static int set_up_temporary_mappings(void)
>  {
> -       struct x86_mapping_info info = {
> -               .alloc_pgt_page = alloc_pgt_page,
> -               .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
> -               .kernel_mapping = true,
> -       };
>         unsigned long mstart, mend;
>         pgd_t *pgd;
>         int result;
> @@ -108,7 +153,7 @@ static int set_up_temporary_mappings(voi
>                 mstart = pfn_mapped[i].start << PAGE_SHIFT;
>                 mend   = pfn_mapped[i].end << PAGE_SHIFT;
>
> -               result = kernel_ident_mapping_init(&info, pgd, mstart, mend);
> +               result = ident_mapping_init(pgd, mstart, mend);
>                 if (result)
>                         return result;
>         }
>

Hi Rafael,

Your version seems not considering different pfn_mapped range could
share same PGD (512G) or even PUD(1G), or even same PMD (2M) range.

so just keep on using kernel_ident_mapping_init() for that.

At the same time, set_up_temporary_text_mapping could be replaced with
kernel_ident_mapping_init() too if restore_jump_address is KVA for
jump_address_phys.

Thanks

Yinghai

[toc] | [prev] | [next] | [standalone]

#1457497 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Saturday, August 06, 2016 09:53:50 PM Yinghai Lu wrote:
> On Sat, Aug 6, 2016 at 6:03 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, August 03, 2016 11:28:48 PM Rafael J. Wysocki wrote:
> >
> > On a second thought, it seems to be better to follow your suggestion to simply
> > provide a special version of kernel_ident_mapping_init() for hibernation,
> > because it is sufficiently distinct from the other users of the code in
> > ident_map.c.
> >
> > The patch below does just that (lightly tested).
> >
> >
> > ---
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: [PATCH] x86/power/64: Always create temporary identity mapping correctly
> >
> > The low-level resume-from-hibernation code on x86-64 uses
> > kernel_ident_mapping_init() to create the temoprary identity mapping,
> > but that function assumes that the offset between kernel virtual
> > addresses and physical addresses is aligned on the PGD level.
> >
> > However, with a randomized identity mapping base, it may be aligned
> > on the PUD level and if that happens, the temporary identity mapping
> > created by set_up_temporary_mappings() will not reflect the actual
> > kernel identity mapping and the image restoration will fail as a
> > result (leading to a kernel panic most of the time).
> >
> > To fix this problem, provide simplified routines for creating the
> > temporary identity mapping during resume from hibernation on x86-64
> > that support unaligned offsets between KVA and PA up to the PMD
> > level.
> >
> > Although kernel_ident_mapping_init() might be made work in that
> > case too, using hibernation-specific code for that is way simpler.
> >
> > Reported-by: Thomas Garnier <thgarnie@google.com>
> > Suggested-by: Yinghai Lu <yinghai@kernel.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  arch/x86/power/hibernate_64.c |   61 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 53 insertions(+), 8 deletions(-)
> >
> > Index: linux-pm/arch/x86/power/hibernate_64.c
> > ===================================================================
> > --- linux-pm.orig/arch/x86/power/hibernate_64.c
> > +++ linux-pm/arch/x86/power/hibernate_64.c
> > @@ -77,18 +77,63 @@ static int set_up_temporary_text_mapping
> >         return 0;
> >  }
> >
> > -static void *alloc_pgt_page(void *context)
> > +static void ident_pmd_init(pmd_t *pmd, unsigned long addr, unsigned long end)
> >  {
> > -       return (void *)get_safe_page(GFP_ATOMIC);
> > +       for (; addr < end; addr += PMD_SIZE)
> > +               set_pmd(pmd + pmd_index(addr),
> > +                       __pmd((addr - __PAGE_OFFSET) | __PAGE_KERNEL_LARGE_EXEC));
> > +}
> > +
> > +static int ident_pud_init(pud_t *pud, unsigned long addr, unsigned long end)
> > +{
> > +       unsigned long next;
> > +
> > +       for (; addr < end; addr = next) {
> > +               pmd_t *pmd;
> > +
> > +               pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
> > +               if (!pmd)
> > +                       return -ENOMEM;
> > +
> > +               next = (addr & PUD_MASK) + PUD_SIZE;
> > +               if (next > end)
> > +                       next = end;
> > +
> > +               ident_pmd_init(pmd, addr & PMD_MASK, next);
> > +               set_pud(pud + pud_index(addr), __pud(__pa(pmd) | _KERNPG_TABLE));
> > +       }
> > +       return 0;
> > +}
> > +
> > +static int ident_mapping_init(pgd_t *pgd, unsigned long mstart, unsigned long mend)
> > +{
> > +       unsigned long addr = mstart + __PAGE_OFFSET;
> > +       unsigned long end = mend + __PAGE_OFFSET;
> > +       unsigned long next;
> > +
> > +       for (; addr < end; addr = next) {
> > +               pud_t *pud;
> > +               int result;
> > +
> > +               pud = (pud_t *)get_safe_page(GFP_ATOMIC);
> > +               if (!pud)
> > +                       return -ENOMEM;
> > +
> > +               next = (addr & PGDIR_MASK) + PGDIR_SIZE;
> > +               if (next > end)
> > +                       next = end;
> > +
> > +               result = ident_pud_init(pud, addr, next);
> > +               if (result)
> > +                       return result;
> > +
> > +               set_pgd(pgd + pgd_index(addr), __pgd(__pa(pud) | _KERNPG_TABLE));
> > +       }
> > +       return 0;
> >  }
> >
> >  static int set_up_temporary_mappings(void)
> >  {
> > -       struct x86_mapping_info info = {
> > -               .alloc_pgt_page = alloc_pgt_page,
> > -               .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
> > -               .kernel_mapping = true,
> > -       };
> >         unsigned long mstart, mend;
> >         pgd_t *pgd;
> >         int result;
> > @@ -108,7 +153,7 @@ static int set_up_temporary_mappings(voi
> >                 mstart = pfn_mapped[i].start << PAGE_SHIFT;
> >                 mend   = pfn_mapped[i].end << PAGE_SHIFT;
> >
> > -               result = kernel_ident_mapping_init(&info, pgd, mstart, mend);
> > +               result = ident_mapping_init(pgd, mstart, mend);
> >                 if (result)
> >                         return result;
> >         }
> >
> 
> Hi Rafael,
> 
> Your version seems not considering different pfn_mapped range could
> share same PGD (512G) or even PUD(1G), or even same PMD (2M) range.

Good point!

> so just keep on using kernel_ident_mapping_init() for that.

But then playing with offsets in ident_pud_init() is not necessary,
because that function works on virtual addresses only, so the appended patch
should be sufficient to fix the problem, shouldn't it?

> At the same time, set_up_temporary_text_mapping could be replaced with
> kernel_ident_mapping_init() too if restore_jump_address is KVA for
> jump_address_phys.

I see no reason to do that.

First, it is not guaranteed that restore_jump_address will always be a KVA for
jump_address_phys and second, it really is only necessary to map one PMD in
there.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH v2] x86/power/64: Always create temporary identity mapping correctly

The low-level resume-from-hibernation code on x86-64 uses
kernel_ident_mapping_init() to create the temoprary identity mapping,
but that function assumes that the offset between kernel virtual
addresses and physical addresses is aligned on the PGD level.

However, with a randomized identity mapping base, it may be aligned
on the PUD level and if that happens, the temporary identity mapping
created by set_up_temporary_mappings() will not reflect the actual
kernel identity mapping and the image restoration will fail as a
result (leading to a kernel panic most of the time).

To fix this problem, rework kernel_ident_mapping_init() to support
unaligned offsets between KVA and PA up to the PMD level and make
set_up_temporary_mappings() use it as approprtiate.

Reported-by: Thomas Garnier <thgarnie@google.com>
Suggested-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/x86/include/asm/init.h   |    4 ++--
 arch/x86/mm/ident_map.c       |   19 +++++++++++--------
 arch/x86/power/hibernate_64.c |    2 +-
 3 files changed, 14 insertions(+), 11 deletions(-)

Index: linux-pm/arch/x86/include/asm/init.h
===================================================================
--- linux-pm.orig/arch/x86/include/asm/init.h
+++ linux-pm/arch/x86/include/asm/init.h
@@ -5,10 +5,10 @@ struct x86_mapping_info {
 	void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
 	void *context;			 /* context for alloc_pgt_page */
 	unsigned long pmd_flag;		 /* page flag for PMD entry */
-	bool kernel_mapping;		 /* kernel mapping or ident mapping */
+	unsigned long offset;		 /* ident mapping offset */
 };
 
 int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
-				unsigned long addr, unsigned long end);
+				unsigned long pstart, unsigned long pend);
 
 #endif /* _ASM_X86_INIT_H */
Index: linux-pm/arch/x86/mm/ident_map.c
===================================================================
--- linux-pm.orig/arch/x86/mm/ident_map.c
+++ linux-pm/arch/x86/mm/ident_map.c
@@ -3,15 +3,17 @@
  * included by both the compressed kernel and the regular kernel.
  */
 
-static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
+static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
 			   unsigned long addr, unsigned long end)
 {
 	addr &= PMD_MASK;
 	for (; addr < end; addr += PMD_SIZE) {
 		pmd_t *pmd = pmd_page + pmd_index(addr);
 
-		if (!pmd_present(*pmd))
-			set_pmd(pmd, __pmd(addr | pmd_flag));
+		if (pmd_present(*pmd))
+			continue;
+
+		set_pmd(pmd, __pmd((addr - info->offset) | info->pmd_flag));
 	}
 }
 
@@ -30,13 +32,13 @@ static int ident_pud_init(struct x86_map
 
 		if (pud_present(*pud)) {
 			pmd = pmd_offset(pud, 0);
-			ident_pmd_init(info->pmd_flag, pmd, addr, next);
+			ident_pmd_init(info, pmd, addr, next);
 			continue;
 		}
 		pmd = (pmd_t *)info->alloc_pgt_page(info->context);
 		if (!pmd)
 			return -ENOMEM;
-		ident_pmd_init(info->pmd_flag, pmd, addr, next);
+		ident_pmd_init(info, pmd, addr, next);
 		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
 	}
 
@@ -44,14 +46,15 @@ static int ident_pud_init(struct x86_map
 }
 
 int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
-			      unsigned long addr, unsigned long end)
+			      unsigned long pstart, unsigned long pend)
 {
+	unsigned long addr = pstart + info->offset;
+	unsigned long end = pend + info->offset;
 	unsigned long next;
 	int result;
-	int off = info->kernel_mapping ? pgd_index(__PAGE_OFFSET) : 0;
 
 	for (; addr < end; addr = next) {
-		pgd_t *pgd = pgd_page + pgd_index(addr) + off;
+		pgd_t *pgd = pgd_page + pgd_index(addr);
 		pud_t *pud;
 
 		next = (addr & PGDIR_MASK) + PGDIR_SIZE;
Index: linux-pm/arch/x86/power/hibernate_64.c
===================================================================
--- linux-pm.orig/arch/x86/power/hibernate_64.c
+++ linux-pm/arch/x86/power/hibernate_64.c
@@ -87,7 +87,7 @@ static int set_up_temporary_mappings(voi
 	struct x86_mapping_info info = {
 		.alloc_pgt_page	= alloc_pgt_page,
 		.pmd_flag	= __PAGE_KERNEL_LARGE_EXEC,
-		.kernel_mapping = true,
+		.offset		= __PAGE_OFFSET,
 	};
 	unsigned long mstart, mend;
 	pgd_t *pgd;

[toc] | [prev] | [next] | [standalone]

#1457580 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Sun, Aug 7, 2016 at 4:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Saturday, August 06, 2016 09:53:50 PM Yinghai Lu wrote:
>> On Sat, Aug 6, 2016 at 6:03 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, August 03, 2016 11:28:48 PM Rafael J. Wysocki wrote:
>> >
>> > On a second thought, it seems to be better to follow your suggestion to simply
>> > provide a special version of kernel_ident_mapping_init() for hibernation,
>> > because it is sufficiently distinct from the other users of the code in
>> > ident_map.c.
>> >
>> > The patch below does just that (lightly tested).
>> >
>> >
>> > ---
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > Subject: [PATCH] x86/power/64: Always create temporary identity mapping correctly
>> >
>> > The low-level resume-from-hibernation code on x86-64 uses
>> > kernel_ident_mapping_init() to create the temoprary identity mapping,
>> > but that function assumes that the offset between kernel virtual
>> > addresses and physical addresses is aligned on the PGD level.
>> >
>> > However, with a randomized identity mapping base, it may be aligned
>> > on the PUD level and if that happens, the temporary identity mapping
>> > created by set_up_temporary_mappings() will not reflect the actual
>> > kernel identity mapping and the image restoration will fail as a
>> > result (leading to a kernel panic most of the time).
>> >
>> > To fix this problem, provide simplified routines for creating the
>> > temporary identity mapping during resume from hibernation on x86-64
>> > that support unaligned offsets between KVA and PA up to the PMD
>> > level.
>> >
>> > Although kernel_ident_mapping_init() might be made work in that
>> > case too, using hibernation-specific code for that is way simpler.
>> >
>> > Reported-by: Thomas Garnier <thgarnie@google.com>
>> > Suggested-by: Yinghai Lu <yinghai@kernel.org>
>> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > ---
>> >  arch/x86/power/hibernate_64.c |   61 ++++++++++++++++++++++++++++++++++++------
>> >  1 file changed, 53 insertions(+), 8 deletions(-)
>> >
>> > Index: linux-pm/arch/x86/power/hibernate_64.c
>> > ===================================================================
>> > --- linux-pm.orig/arch/x86/power/hibernate_64.c
>> > +++ linux-pm/arch/x86/power/hibernate_64.c
>> > @@ -77,18 +77,63 @@ static int set_up_temporary_text_mapping
>> >         return 0;
>> >  }
>> >
>> > -static void *alloc_pgt_page(void *context)
>> > +static void ident_pmd_init(pmd_t *pmd, unsigned long addr, unsigned long end)
>> >  {
>> > -       return (void *)get_safe_page(GFP_ATOMIC);
>> > +       for (; addr < end; addr += PMD_SIZE)
>> > +               set_pmd(pmd + pmd_index(addr),
>> > +                       __pmd((addr - __PAGE_OFFSET) | __PAGE_KERNEL_LARGE_EXEC));
>> > +}
>> > +
>> > +static int ident_pud_init(pud_t *pud, unsigned long addr, unsigned long end)
>> > +{
>> > +       unsigned long next;
>> > +
>> > +       for (; addr < end; addr = next) {
>> > +               pmd_t *pmd;
>> > +
>> > +               pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
>> > +               if (!pmd)
>> > +                       return -ENOMEM;
>> > +
>> > +               next = (addr & PUD_MASK) + PUD_SIZE;
>> > +               if (next > end)
>> > +                       next = end;
>> > +
>> > +               ident_pmd_init(pmd, addr & PMD_MASK, next);
>> > +               set_pud(pud + pud_index(addr), __pud(__pa(pmd) | _KERNPG_TABLE));
>> > +       }
>> > +       return 0;
>> > +}
>> > +
>> > +static int ident_mapping_init(pgd_t *pgd, unsigned long mstart, unsigned long mend)
>> > +{
>> > +       unsigned long addr = mstart + __PAGE_OFFSET;
>> > +       unsigned long end = mend + __PAGE_OFFSET;
>> > +       unsigned long next;
>> > +
>> > +       for (; addr < end; addr = next) {
>> > +               pud_t *pud;
>> > +               int result;
>> > +
>> > +               pud = (pud_t *)get_safe_page(GFP_ATOMIC);
>> > +               if (!pud)
>> > +                       return -ENOMEM;
>> > +
>> > +               next = (addr & PGDIR_MASK) + PGDIR_SIZE;
>> > +               if (next > end)
>> > +                       next = end;
>> > +
>> > +               result = ident_pud_init(pud, addr, next);
>> > +               if (result)
>> > +                       return result;
>> > +
>> > +               set_pgd(pgd + pgd_index(addr), __pgd(__pa(pud) | _KERNPG_TABLE));
>> > +       }
>> > +       return 0;
>> >  }
>> >
>> >  static int set_up_temporary_mappings(void)
>> >  {
>> > -       struct x86_mapping_info info = {
>> > -               .alloc_pgt_page = alloc_pgt_page,
>> > -               .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
>> > -               .kernel_mapping = true,
>> > -       };
>> >         unsigned long mstart, mend;
>> >         pgd_t *pgd;
>> >         int result;
>> > @@ -108,7 +153,7 @@ static int set_up_temporary_mappings(voi
>> >                 mstart = pfn_mapped[i].start << PAGE_SHIFT;
>> >                 mend   = pfn_mapped[i].end << PAGE_SHIFT;
>> >
>> > -               result = kernel_ident_mapping_init(&info, pgd, mstart, mend);
>> > +               result = ident_mapping_init(pgd, mstart, mend);
>> >                 if (result)
>> >                         return result;
>> >         }
>> >
>>
>> Hi Rafael,
>>
>> Your version seems not considering different pfn_mapped range could
>> share same PGD (512G) or even PUD(1G), or even same PMD (2M) range.
>
> Good point!
>
>> so just keep on using kernel_ident_mapping_init() for that.
>
> But then playing with offsets in ident_pud_init() is not necessary,
> because that function works on virtual addresses only, so the appended patch
> should be sufficient to fix the problem, shouldn't it?

I agree.

>
>> At the same time, set_up_temporary_text_mapping could be replaced with
>> kernel_ident_mapping_init() too if restore_jump_address is KVA for
>> jump_address_phys.
>
> I see no reason to do that.
>
> First, it is not guaranteed that restore_jump_address will always be a KVA for
> jump_address_phys and second, it really is only necessary to map one PMD in
> there.

With your v2 version, you could pass difference between restore_jump_address and
jump_address_phys as info->off ?
With that, we can kill more lines if replace with
set_up_temporary_text_mapping with
kernel_ident_mapping_init() and make code more readable.

But just keep that in separated patch after your v2 patch.

> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH v2] x86/power/64: Always create temporary identity mapping correctly
>
> The low-level resume-from-hibernation code on x86-64 uses
> kernel_ident_mapping_init() to create the temoprary identity mapping,
> but that function assumes that the offset between kernel virtual
> addresses and physical addresses is aligned on the PGD level.
>
> However, with a randomized identity mapping base, it may be aligned
> on the PUD level and if that happens, the temporary identity mapping
> created by set_up_temporary_mappings() will not reflect the actual
> kernel identity mapping and the image restoration will fail as a
> result (leading to a kernel panic most of the time).
>
> To fix this problem, rework kernel_ident_mapping_init() to support
> unaligned offsets between KVA and PA up to the PMD level and make
> set_up_temporary_mappings() use it as approprtiate.
>
> Reported-by: Thomas Garnier <thgarnie@google.com>
> Suggested-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

it should replace

[PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

Acked-by: Yinghai Lu <yinghai@kernel.org>

> ---
>  arch/x86/include/asm/init.h   |    4 ++--
>  arch/x86/mm/ident_map.c       |   19 +++++++++++--------
>  arch/x86/power/hibernate_64.c |    2 +-
>  3 files changed, 14 insertions(+), 11 deletions(-)
>
> Index: linux-pm/arch/x86/include/asm/init.h
> ===================================================================
> --- linux-pm.orig/arch/x86/include/asm/init.h
> +++ linux-pm/arch/x86/include/asm/init.h
> @@ -5,10 +5,10 @@ struct x86_mapping_info {
>         void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
>         void *context;                   /* context for alloc_pgt_page */
>         unsigned long pmd_flag;          /* page flag for PMD entry */
> -       bool kernel_mapping;             /* kernel mapping or ident mapping */
> +       unsigned long offset;            /* ident mapping offset */
>  };
>
>  int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> -                               unsigned long addr, unsigned long end);
> +                               unsigned long pstart, unsigned long pend);
>
>  #endif /* _ASM_X86_INIT_H */
> Index: linux-pm/arch/x86/mm/ident_map.c
> ===================================================================
> --- linux-pm.orig/arch/x86/mm/ident_map.c
> +++ linux-pm/arch/x86/mm/ident_map.c
> @@ -3,15 +3,17 @@
>   * included by both the compressed kernel and the regular kernel.
>   */
>
> -static void ident_pmd_init(unsigned long pmd_flag, pmd_t *pmd_page,
> +static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
>                            unsigned long addr, unsigned long end)
>  {
>         addr &= PMD_MASK;
>         for (; addr < end; addr += PMD_SIZE) {
>                 pmd_t *pmd = pmd_page + pmd_index(addr);
>
> -               if (!pmd_present(*pmd))
> -                       set_pmd(pmd, __pmd(addr | pmd_flag));
> +               if (pmd_present(*pmd))
> +                       continue;
> +
> +               set_pmd(pmd, __pmd((addr - info->offset) | info->pmd_flag));
>         }
>  }
>
> @@ -30,13 +32,13 @@ static int ident_pud_init(struct x86_map
>
>                 if (pud_present(*pud)) {
>                         pmd = pmd_offset(pud, 0);
> -                       ident_pmd_init(info->pmd_flag, pmd, addr, next);
> +                       ident_pmd_init(info, pmd, addr, next);
>                         continue;
>                 }
>                 pmd = (pmd_t *)info->alloc_pgt_page(info->context);
>                 if (!pmd)
>                         return -ENOMEM;
> -               ident_pmd_init(info->pmd_flag, pmd, addr, next);
> +               ident_pmd_init(info, pmd, addr, next);
>                 set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
>         }
>
> @@ -44,14 +46,15 @@ static int ident_pud_init(struct x86_map
>  }
>
>  int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> -                             unsigned long addr, unsigned long end)
> +                             unsigned long pstart, unsigned long pend)
>  {
> +       unsigned long addr = pstart + info->offset;
> +       unsigned long end = pend + info->offset;
>         unsigned long next;
>         int result;
> -       int off = info->kernel_mapping ? pgd_index(__PAGE_OFFSET) : 0;
>
>         for (; addr < end; addr = next) {
> -               pgd_t *pgd = pgd_page + pgd_index(addr) + off;
> +               pgd_t *pgd = pgd_page + pgd_index(addr);
>                 pud_t *pud;
>
>                 next = (addr & PGDIR_MASK) + PGDIR_SIZE;
> Index: linux-pm/arch/x86/power/hibernate_64.c
> ===================================================================
> --- linux-pm.orig/arch/x86/power/hibernate_64.c
> +++ linux-pm/arch/x86/power/hibernate_64.c
> @@ -87,7 +87,7 @@ static int set_up_temporary_mappings(voi
>         struct x86_mapping_info info = {
>                 .alloc_pgt_page = alloc_pgt_page,
>                 .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
> -               .kernel_mapping = true,
> +               .offset         = __PAGE_OFFSET,
>         };
>         unsigned long mstart, mend;
>         pgd_t *pgd;
>

[toc] | [prev] | [next] | [standalone]

#1457585 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Mon, Aug 8, 2016 at 12:06 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>>> At the same time, set_up_temporary_text_mapping could be replaced with
>>> kernel_ident_mapping_init() too if restore_jump_address is KVA for
>>> jump_address_phys.
>>
>> I see no reason to do that.
>>
>> First, it is not guaranteed that restore_jump_address will always be a KVA for
>> jump_address_phys and second, it really is only necessary to map one PMD in
>> there.
>
> With your v2 version, you could pass difference between restore_jump_address and
> jump_address_phys as info->off ?
> With that, we can kill more lines if replace with
> set_up_temporary_text_mapping with
> kernel_ident_mapping_init() and make code more readable.
>
> But just keep that in separated patch after your v2 patch.

like:

---
 arch/x86/power/hibernate_64.c |   55 ++++++++++++------------------------------
 1 file changed, 17 insertions(+), 38 deletions(-)

Index: linux-2.6/arch/x86/power/hibernate_64.c
===================================================================
--- linux-2.6.orig/arch/x86/power/hibernate_64.c
+++ linux-2.6/arch/x86/power/hibernate_64.c
@@ -41,42 +41,6 @@ unsigned long temp_level4_pgt __visible;

 unsigned long relocated_restore_code __visible;

-static int set_up_temporary_text_mapping(pgd_t *pgd)
-{
-    pmd_t *pmd;
-    pud_t *pud;
-
-    /*
-     * The new mapping only has to cover the page containing the image
-     * kernel's entry point (jump_address_phys), because the switch over to
-     * it is carried out by relocated code running from a page allocated
-     * specifically for this purpose and covered by the identity mapping, so
-     * the temporary kernel text mapping is only needed for the final jump.
-     * Moreover, in that mapping the virtual address of the image kernel's
-     * entry point must be the same as its virtual address in the image
-     * kernel (restore_jump_address), so the image kernel's
-     * restore_registers() code doesn't find itself in a different area of
-     * the virtual address space after switching over to the original page
-     * tables used by the image kernel.
-     */
-    pud = (pud_t *)get_safe_page(GFP_ATOMIC);
-    if (!pud)
-        return -ENOMEM;
-
-    pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
-    if (!pmd)
-        return -ENOMEM;
-
-    set_pmd(pmd + pmd_index(restore_jump_address),
-        __pmd((jump_address_phys & PMD_MASK) | __PAGE_KERNEL_LARGE_EXEC));
-    set_pud(pud + pud_index(restore_jump_address),
-        __pud(__pa(pmd) | _KERNPG_TABLE));
-    set_pgd(pgd + pgd_index(restore_jump_address),
-        __pgd(__pa(pud) | _KERNPG_TABLE));
-
-    return 0;
-}
-
 static void *alloc_pgt_page(void *context)
 {
     return (void *)get_safe_page(GFP_ATOMIC);
@@ -87,7 +51,6 @@ static int set_up_temporary_mappings(voi
     struct x86_mapping_info info = {
         .alloc_pgt_page    = alloc_pgt_page,
         .pmd_flag    = __PAGE_KERNEL_LARGE_EXEC,
-        .offset        = __PAGE_OFFSET,
     };
     unsigned long mstart, mend;
     pgd_t *pgd;
@@ -99,11 +62,27 @@ static int set_up_temporary_mappings(voi
         return -ENOMEM;

     /* Prepare a temporary mapping for the kernel text */
-    result = set_up_temporary_text_mapping(pgd);
+    /*
+     * The new mapping only has to cover the page containing the image
+     * kernel's entry point (jump_address_phys), because the switch over to
+     * it is carried out by relocated code running from a page allocated
+     * specifically for this purpose and covered by the identity mapping, so
+     * the temporary kernel text mapping is only needed for the final jump.
+     * Moreover, in that mapping the virtual address of the image kernel's
+     * entry point must be the same as its virtual address in the image
+     * kernel (restore_jump_address), so the image kernel's
+     * restore_registers() code doesn't find itself in a different area of
+     * the virtual address space after switching over to the original page
+     * tables used by the image kernel.
+     */
+    info.offset = restore_jump_address - jump_address_phys;
+    result = kernel_ident_mapping_init(&info, pgd, jump_address_phys,
+                       jump_address_phys + PMD_SIZE);
     if (result)
         return result;

     /* Set up the direct mapping from scratch */
+    info.offset = __PAGE_OFFSET;
     for (i = 0; i < nr_pfn_mapped; i++) {
         mstart = pfn_mapped[i].start << PAGE_SHIFT;
         mend   = pfn_mapped[i].end << PAGE_SHIFT;

[toc] | [prev] | [next] | [standalone]

#1457780 — Re: [PATCH v2] x86/power/64: Support unaligned addresses for temporary mapping

On Monday, August 08, 2016 12:23:33 AM Yinghai Lu wrote:
> On Mon, Aug 8, 2016 at 12:06 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>
> >>> At the same time, set_up_temporary_text_mapping could be replaced with
> >>> kernel_ident_mapping_init() too if restore_jump_address is KVA for
> >>> jump_address_phys.
> >>
> >> I see no reason to do that.
> >>
> >> First, it is not guaranteed that restore_jump_address will always be a KVA for
> >> jump_address_phys and second, it really is only necessary to map one PMD in
> >> there.
> >
> > With your v2 version, you could pass difference between restore_jump_address and
> > jump_address_phys as info->off ?
> > With that, we can kill more lines if replace with
> > set_up_temporary_text_mapping with
> > kernel_ident_mapping_init() and make code more readable.
> >
> > But just keep that in separated patch after your v2 patch.
> 
> like:
> 
> ---
>  arch/x86/power/hibernate_64.c |   55 ++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 38 deletions(-)
> 
> Index: linux-2.6/arch/x86/power/hibernate_64.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/power/hibernate_64.c
> +++ linux-2.6/arch/x86/power/hibernate_64.c
> @@ -41,42 +41,6 @@ unsigned long temp_level4_pgt __visible;
> 
>  unsigned long relocated_restore_code __visible;
> 
> -static int set_up_temporary_text_mapping(pgd_t *pgd)
> -{
> -    pmd_t *pmd;
> -    pud_t *pud;
> -
> -    /*
> -     * The new mapping only has to cover the page containing the image
> -     * kernel's entry point (jump_address_phys), because the switch over to
> -     * it is carried out by relocated code running from a page allocated
> -     * specifically for this purpose and covered by the identity mapping, so
> -     * the temporary kernel text mapping is only needed for the final jump.
> -     * Moreover, in that mapping the virtual address of the image kernel's
> -     * entry point must be the same as its virtual address in the image
> -     * kernel (restore_jump_address), so the image kernel's
> -     * restore_registers() code doesn't find itself in a different area of
> -     * the virtual address space after switching over to the original page
> -     * tables used by the image kernel.
> -     */
> -    pud = (pud_t *)get_safe_page(GFP_ATOMIC);
> -    if (!pud)
> -        return -ENOMEM;
> -
> -    pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
> -    if (!pmd)
> -        return -ENOMEM;
> -
> -    set_pmd(pmd + pmd_index(restore_jump_address),
> -        __pmd((jump_address_phys & PMD_MASK) | __PAGE_KERNEL_LARGE_EXEC));
> -    set_pud(pud + pud_index(restore_jump_address),
> -        __pud(__pa(pmd) | _KERNPG_TABLE));
> -    set_pgd(pgd + pgd_index(restore_jump_address),
> -        __pgd(__pa(pud) | _KERNPG_TABLE));
> -
> -    return 0;
> -}
> -
>  static void *alloc_pgt_page(void *context)
>  {
>      return (void *)get_safe_page(GFP_ATOMIC);
> @@ -87,7 +51,6 @@ static int set_up_temporary_mappings(voi
>      struct x86_mapping_info info = {
>          .alloc_pgt_page    = alloc_pgt_page,
>          .pmd_flag    = __PAGE_KERNEL_LARGE_EXEC,
> -        .offset        = __PAGE_OFFSET,
>      };
>      unsigned long mstart, mend;
>      pgd_t *pgd;
> @@ -99,11 +62,27 @@ static int set_up_temporary_mappings(voi
>          return -ENOMEM;
> 
>      /* Prepare a temporary mapping for the kernel text */
> -    result = set_up_temporary_text_mapping(pgd);
> +    /*
> +     * The new mapping only has to cover the page containing the image
> +     * kernel's entry point (jump_address_phys), because the switch over to
> +     * it is carried out by relocated code running from a page allocated
> +     * specifically for this purpose and covered by the identity mapping, so
> +     * the temporary kernel text mapping is only needed for the final jump.
> +     * Moreover, in that mapping the virtual address of the image kernel's
> +     * entry point must be the same as its virtual address in the image
> +     * kernel (restore_jump_address), so the image kernel's
> +     * restore_registers() code doesn't find itself in a different area of
> +     * the virtual address space after switching over to the original page
> +     * tables used by the image kernel.
> +     */
> +    info.offset = restore_jump_address - jump_address_phys;
> +    result = kernel_ident_mapping_init(&info, pgd, jump_address_phys,
> +                       jump_address_phys + PMD_SIZE);
>      if (result)
>          return result;
> 
>      /* Set up the direct mapping from scratch */
> +    info.offset = __PAGE_OFFSET;
>      for (i = 0; i < nr_pfn_mapped; i++) {
>          mstart = pfn_mapped[i].start << PAGE_SHIFT;
>          mend   = pfn_mapped[i].end << PAGE_SHIFT;

OK, I see what you mean.

Looks like a good idea to me.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1453512

On Monday, August 01, 2016 10:07:58 AM Thomas Garnier wrote:
> ***Background:
> KASLR memory randomization for x86_64 was added when KASLR did not support hibernation. Now that it does, some changes are needed.
> 
> ***Problems that needed solving:
> Hibernation was failing on reboot with a GP fault when CONFIG_RANDOMIZE_MEMORY was enabled. Two issues were identified.
> 
> The original fault was due to a wrong physical address assigned to cr3. The problem was introduced with __PAGE_OFFSET becoming a global variable when randomized. The fix uses a define to use the glbobal or immediate value based on config settings.
> 
> The second isssue was that the temporary page table mapping did not support virtual addresses not aligned on PGD level. KASLR memory randomization will generated a random address aligned on PUD level. The fix correctly calculates the offset on all levels of the temporary page table.
> 
> ***Parts:
>  - 01/02: Support unaligned addresses (second issue)
>  - 02/02: Fix __PAGE_OFFSET usage on assembly (first issue)

Thanks a lot for taking care of this!

Patch [2/2] looks good to me, but I'd to the [1/2] differently (more details
will follow).

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1453523

On Tuesday, August 02, 2016 01:48:13 AM Rafael J. Wysocki wrote:
> On Monday, August 01, 2016 10:07:58 AM Thomas Garnier wrote:
> > ***Background:
> > KASLR memory randomization for x86_64 was added when KASLR did not support hibernation. Now that it does, some changes are needed.
> > 
> > ***Problems that needed solving:
> > Hibernation was failing on reboot with a GP fault when CONFIG_RANDOMIZE_MEMORY was enabled. Two issues were identified.
> > 
> > The original fault was due to a wrong physical address assigned to cr3. The problem was introduced with __PAGE_OFFSET becoming a global variable when randomized. The fix uses a define to use the glbobal or immediate value based on config settings.
> > 
> > The second isssue was that the temporary page table mapping did not support virtual addresses not aligned on PGD level. KASLR memory randomization will generated a random address aligned on PUD level. The fix correctly calculates the offset on all levels of the temporary page table.
> > 
> > ***Parts:
> >  - 01/02: Support unaligned addresses (second issue)
> >  - 02/02: Fix __PAGE_OFFSET usage on assembly (first issue)
> 
> Thanks a lot for taking care of this!
> 
> Patch [2/2] looks good to me, but I'd to the [1/2] differently (more details
> will follow).

Well, I got this the other way around, sorry.

I've just sent an ACK for the [1/2] and an alternative patch for the [2/2].

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web