Patchwork [lucid] UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from hibernation

login
register
mail settings
Submitter Eric Miao
Date April 23, 2010, 7:14 a.m.
Message ID <i2wf17812d71004230014wfd919c34i5d73c0b909c63b9@mail.gmail.com>
Download mbox | patch
Permalink /patch/50793/
State Superseded
Delegated to: Stefan Bader
Headers show

Comments

Eric Miao - April 23, 2010, 7:14 a.m.
BTW, this may apply to other ARM variants as well, but the hibernation
feature should really be made common first of all in that sense.

commit 1f3ebd28c0e8adf7f7a1fc85377a57d8dbc56267
Author: Eric Miao <eric.miao@canonical.com>
Date:   Fri Apr 23 14:16:17 2010 +0800

    UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
hibernation

    BugLink: http://bugs.launchpad.net/bugs/509006

    Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
    specified on the kernel command line, but it fails if scripts in
    initramfs are used to trigger the resume. It turned out to be page
    table being overwritten when restoring the memory content because
    it's using a normal user process's page table in the latter case,
    which is not safe and could be overwritten. Fix this by using the
    safe swapper_pg_dir during restoring.

    Signed-off-by: Eric Miao <eric.miao@canonical.com>
Stefan Bader - April 23, 2010, 2:18 p.m.
Eric Miao wrote:
> BTW, this may apply to other ARM variants as well, but the hibernation
> feature should really be made common first of all in that sense.
> 
> commit 1f3ebd28c0e8adf7f7a1fc85377a57d8dbc56267
> Author: Eric Miao <eric.miao@canonical.com>
> Date:   Fri Apr 23 14:16:17 2010 +0800
> 
>     UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
> hibernation
> 
>     BugLink: http://bugs.launchpad.net/bugs/509006
> 
>     Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
>     specified on the kernel command line, but it fails if scripts in
>     initramfs are used to trigger the resume. It turned out to be page
>     table being overwritten when restoring the memory content because
>     it's using a normal user process's page table in the latter case,
>     which is not safe and could be overwritten. Fix this by using the
>     safe swapper_pg_dir during restoring.
> 
>     Signed-off-by: Eric Miao <eric.miao@canonical.com>
> 
> diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
> index 0be1e1c..c5c028f 100755
> --- a/arch/arm/mach-dove/Makefile
> +++ b/arch/arm/mach-dove/Makefile
> @@ -1,3 +1,5 @@
> +AFLAGS_swsusp.o			:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> +
>  obj-y				+= clock.o common.o addr-map.o irq.o pcie.o mpp.o \
>  				sdhci_cam_mbus.o
>  obj-$(CONFIG_MACH_DOVE_RD_AVNG)	+= dove-rd-avng-setup.o
> diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
> index 4f4a884..8e308d8 100644
> --- a/arch/arm/mach-dove/swsusp.S
> +++ b/arch/arm/mach-dove/swsusp.S
> @@ -28,6 +28,7 @@
>   */
> 
>  #include <linux/linkage.h>
> +#include <asm/memory.h>
>  #include <asm/segment.h>
>  #include <asm/page.h>
>  #include <asm/asm-offsets.h>
> @@ -209,8 +210,14 @@ FUNC(swsusp_arch_suspend)
> 
>  FUNC_END(swsusp_arch_suspend)
> 
> +#define KERNEL_RAM_PADDR	(PHYS_OFFSET + TEXT_OFFSET)
> +
>  FUNC(swsusp_arch_resume)
>  	/* set page table if needed */
> +	ldr	r0, =(KERNEL_RAM_PADDR - 0x4000)
> +	mcr	p15, 0, r0, c2, c0, 0		@ load page table pointer
> +	mcr	p15, 0, r0, c8, c7, 0		@ invalidate I,D TLBs
> +	mcr	p15, 0, r0, c7, c5, 4		@ ISB
> 
>  	/*
>  	 * retore "nr_copy_pages" pages which are saved and specified
> 
The patch probably is correct, but from the reviewing point of view I do not
know how I make the connection between swapper_pg_dir in the comment and the
patch itself. Is that only me?

Stefan
John Johansen - April 23, 2010, 4:40 p.m.
On 04/23/2010 07:18 AM, Stefan Bader wrote:
> Eric Miao wrote:
>> BTW, this may apply to other ARM variants as well, but the hibernation
>> feature should really be made common first of all in that sense.
>>
>> commit 1f3ebd28c0e8adf7f7a1fc85377a57d8dbc56267
>> Author: Eric Miao <eric.miao@canonical.com>
>> Date:   Fri Apr 23 14:16:17 2010 +0800
>>
>>     UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
>> hibernation
>>
>>     BugLink: http://bugs.launchpad.net/bugs/509006
>>
>>     Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
>>     specified on the kernel command line, but it fails if scripts in
>>     initramfs are used to trigger the resume. It turned out to be page
>>     table being overwritten when restoring the memory content because
>>     it's using a normal user process's page table in the latter case,
>>     which is not safe and could be overwritten. Fix this by using the
>>     safe swapper_pg_dir during restoring.
>>
>>     Signed-off-by: Eric Miao <eric.miao@canonical.com>
>>
>> diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
>> index 0be1e1c..c5c028f 100755
>> --- a/arch/arm/mach-dove/Makefile
>> +++ b/arch/arm/mach-dove/Makefile
>> @@ -1,3 +1,5 @@
>> +AFLAGS_swsusp.o			:= -DTEXT_OFFSET=$(TEXT_OFFSET)
>> +
>>  obj-y				+= clock.o common.o addr-map.o irq.o pcie.o mpp.o \
>>  				sdhci_cam_mbus.o
>>  obj-$(CONFIG_MACH_DOVE_RD_AVNG)	+= dove-rd-avng-setup.o
>> diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
>> index 4f4a884..8e308d8 100644
>> --- a/arch/arm/mach-dove/swsusp.S
>> +++ b/arch/arm/mach-dove/swsusp.S
>> @@ -28,6 +28,7 @@
>>   */
>>
>>  #include <linux/linkage.h>
>> +#include <asm/memory.h>
>>  #include <asm/segment.h>
>>  #include <asm/page.h>
>>  #include <asm/asm-offsets.h>
>> @@ -209,8 +210,14 @@ FUNC(swsusp_arch_suspend)
>>
>>  FUNC_END(swsusp_arch_suspend)
>>
>> +#define KERNEL_RAM_PADDR	(PHYS_OFFSET + TEXT_OFFSET)
>> +
>>  FUNC(swsusp_arch_resume)
>>  	/* set page table if needed */
>> +	ldr	r0, =(KERNEL_RAM_PADDR - 0x4000)
>> +	mcr	p15, 0, r0, c2, c0, 0		@ load page table pointer
>> +	mcr	p15, 0, r0, c8, c7, 0		@ invalidate I,D TLBs
>> +	mcr	p15, 0, r0, c7, c5, 4		@ ISB
>>
>>  	/*
>>  	 * retore "nr_copy_pages" pages which are saved and specified
>>
> The patch probably is correct, but from the reviewing point of view I do not
> know how I make the connection between swapper_pg_dir in the comment and the
> patch itself. Is that only me?
> 
No, I also would like to see a little more information.
Eric Miao - April 23, 2010, 10:19 p.m.
On Sat, Apr 24, 2010 at 12:40 AM, John Johansen
<john.johansen@canonical.com> wrote:
> On 04/23/2010 07:18 AM, Stefan Bader wrote:
>> Eric Miao wrote:
>>> BTW, this may apply to other ARM variants as well, but the hibernation
>>> feature should really be made common first of all in that sense.
>>>
>>> commit 1f3ebd28c0e8adf7f7a1fc85377a57d8dbc56267
>>> Author: Eric Miao <eric.miao@canonical.com>
>>> Date:   Fri Apr 23 14:16:17 2010 +0800
>>>
>>>     UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
>>> hibernation
>>>
>>>     BugLink: http://bugs.launchpad.net/bugs/509006
>>>
>>>     Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
>>>     specified on the kernel command line, but it fails if scripts in
>>>     initramfs are used to trigger the resume. It turned out to be page
>>>     table being overwritten when restoring the memory content because
>>>     it's using a normal user process's page table in the latter case,
>>>     which is not safe and could be overwritten. Fix this by using the
>>>     safe swapper_pg_dir during restoring.
>>>
>>>     Signed-off-by: Eric Miao <eric.miao@canonical.com>
>>>
>>> diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
>>> index 0be1e1c..c5c028f 100755
>>> --- a/arch/arm/mach-dove/Makefile
>>> +++ b/arch/arm/mach-dove/Makefile
>>> @@ -1,3 +1,5 @@
>>> +AFLAGS_swsusp.o                     := -DTEXT_OFFSET=$(TEXT_OFFSET)
>>> +
>>>  obj-y                               += clock.o common.o addr-map.o irq.o pcie.o mpp.o \
>>>                              sdhci_cam_mbus.o
>>>  obj-$(CONFIG_MACH_DOVE_RD_AVNG)     += dove-rd-avng-setup.o
>>> diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
>>> index 4f4a884..8e308d8 100644
>>> --- a/arch/arm/mach-dove/swsusp.S
>>> +++ b/arch/arm/mach-dove/swsusp.S
>>> @@ -28,6 +28,7 @@
>>>   */
>>>
>>>  #include <linux/linkage.h>
>>> +#include <asm/memory.h>
>>>  #include <asm/segment.h>
>>>  #include <asm/page.h>
>>>  #include <asm/asm-offsets.h>
>>> @@ -209,8 +210,14 @@ FUNC(swsusp_arch_suspend)
>>>
>>>  FUNC_END(swsusp_arch_suspend)
>>>
>>> +#define KERNEL_RAM_PADDR    (PHYS_OFFSET + TEXT_OFFSET)
>>> +
>>>  FUNC(swsusp_arch_resume)
>>>      /* set page table if needed */
>>> +    ldr     r0, =(KERNEL_RAM_PADDR - 0x4000)
>>> +    mcr     p15, 0, r0, c2, c0, 0           @ load page table pointer
>>> +    mcr     p15, 0, r0, c8, c7, 0           @ invalidate I,D TLBs
>>> +    mcr     p15, 0, r0, c7, c5, 4           @ ISB
>>>
>>>      /*
>>>       * retore "nr_copy_pages" pages which are saved and specified
>>>
>> The patch probably is correct, but from the reviewing point of view I do not
>> know how I make the connection between swapper_pg_dir in the comment and the
>> patch itself. Is that only me?
>>
> No, I also would like to see a little more information.
>

Ah, sorry for the confusion. swapper_pg_dir is the initial page table for
ARM kernel, which is physically located at KERNEL_RAM_PADDR - 0x4000.
I should have made it clear by a macro or something. The following code is
excerpted from arch/arm/kernel/head.S:

/*
 * swapper_pg_dir is the virtual address of the initial page table.
 * We place the page tables 16K below KERNEL_RAM_VADDR.  Therefore, we must
 * make sure that KERNEL_RAM_VADDR is correctly set.  Currently, we expect
 * the least significant 16 bits to be 0x8000, but we could probably
 * relax this restriction to KERNEL_RAM_VADDR >= PAGE_OFFSET + 0x4000.
 */
#if (KERNEL_RAM_VADDR & 0xffff) != 0x8000
#error KERNEL_RAM_VADDR must start at 0xXXXX8000
#endif

	.globl	swapper_pg_dir
	.equ	swapper_pg_dir, KERNEL_RAM_VADDR - 0x4000

	.macro	pgtbl, rd
	ldr	\rd, =(KERNEL_RAM_PADDR - 0x4000)
	.endm
Stefan Bader - April 27, 2010, 10:08 a.m.
Eric Miao wrote:
> On Sat, Apr 24, 2010 at 12:40 AM, John Johansen
> <john.johansen@canonical.com> wrote:
>> On 04/23/2010 07:18 AM, Stefan Bader wrote:
>>> Eric Miao wrote:
>>>> BTW, this may apply to other ARM variants as well, but the hibernation
>>>> feature should really be made common first of all in that sense.
>>>>
>>>> commit 1f3ebd28c0e8adf7f7a1fc85377a57d8dbc56267
>>>> Author: Eric Miao <eric.miao@canonical.com>
>>>> Date:   Fri Apr 23 14:16:17 2010 +0800
>>>>
>>>>     UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
>>>> hibernation
>>>>
>>>>     BugLink: http://bugs.launchpad.net/bugs/509006
>>>>
>>>>     Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
>>>>     specified on the kernel command line, but it fails if scripts in
>>>>     initramfs are used to trigger the resume. It turned out to be page
>>>>     table being overwritten when restoring the memory content because
>>>>     it's using a normal user process's page table in the latter case,
>>>>     which is not safe and could be overwritten. Fix this by using the
>>>>     safe swapper_pg_dir during restoring.
>>>>
>>>>     Signed-off-by: Eric Miao <eric.miao@canonical.com>
>>>>
>>>> diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
>>>> index 0be1e1c..c5c028f 100755
>>>> --- a/arch/arm/mach-dove/Makefile
>>>> +++ b/arch/arm/mach-dove/Makefile
>>>> @@ -1,3 +1,5 @@
>>>> +AFLAGS_swsusp.o                     := -DTEXT_OFFSET=$(TEXT_OFFSET)
>>>> +
>>>>  obj-y                               += clock.o common.o addr-map.o irq.o pcie.o mpp.o \
>>>>                              sdhci_cam_mbus.o
>>>>  obj-$(CONFIG_MACH_DOVE_RD_AVNG)     += dove-rd-avng-setup.o
>>>> diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
>>>> index 4f4a884..8e308d8 100644
>>>> --- a/arch/arm/mach-dove/swsusp.S
>>>> +++ b/arch/arm/mach-dove/swsusp.S
>>>> @@ -28,6 +28,7 @@
>>>>   */
>>>>
>>>>  #include <linux/linkage.h>
>>>> +#include <asm/memory.h>
>>>>  #include <asm/segment.h>
>>>>  #include <asm/page.h>
>>>>  #include <asm/asm-offsets.h>
>>>> @@ -209,8 +210,14 @@ FUNC(swsusp_arch_suspend)
>>>>
>>>>  FUNC_END(swsusp_arch_suspend)
>>>>
>>>> +#define KERNEL_RAM_PADDR    (PHYS_OFFSET + TEXT_OFFSET)
>>>> +
>>>>  FUNC(swsusp_arch_resume)
>>>>      /* set page table if needed */
>>>> +    ldr     r0, =(KERNEL_RAM_PADDR - 0x4000)
>>>> +    mcr     p15, 0, r0, c2, c0, 0           @ load page table pointer
>>>> +    mcr     p15, 0, r0, c8, c7, 0           @ invalidate I,D TLBs
>>>> +    mcr     p15, 0, r0, c7, c5, 4           @ ISB
>>>>
>>>>      /*
>>>>       * retore "nr_copy_pages" pages which are saved and specified
>>>>
>>> The patch probably is correct, but from the reviewing point of view I do not
>>> know how I make the connection between swapper_pg_dir in the comment and the
>>> patch itself. Is that only me?
>>>
>> No, I also would like to see a little more information.
>>
> 
> Ah, sorry for the confusion. swapper_pg_dir is the initial page table for
> ARM kernel, which is physically located at KERNEL_RAM_PADDR - 0x4000.
> I should have made it clear by a macro or something. The following code is
> excerpted from arch/arm/kernel/head.S:
> 
> /*
>  * swapper_pg_dir is the virtual address of the initial page table.
>  * We place the page tables 16K below KERNEL_RAM_VADDR.  Therefore, we must
>  * make sure that KERNEL_RAM_VADDR is correctly set.  Currently, we expect
>  * the least significant 16 bits to be 0x8000, but we could probably
>  * relax this restriction to KERNEL_RAM_VADDR >= PAGE_OFFSET + 0x4000.
>  */
> #if (KERNEL_RAM_VADDR & 0xffff) != 0x8000
> #error KERNEL_RAM_VADDR must start at 0xXXXX8000
> #endif
> 
> 	.globl	swapper_pg_dir
> 	.equ	swapper_pg_dir, KERNEL_RAM_VADDR - 0x4000
> 
> 	.macro	pgtbl, rd
> 	ldr	\rd, =(KERNEL_RAM_PADDR - 0x4000)
> 	.endm

Hm, wouldn't it then be possible to make the patch actually use the macro name
instead of the hard coded values?

Stefan

Patch

diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
index 0be1e1c..c5c028f 100755
--- a/arch/arm/mach-dove/Makefile
+++ b/arch/arm/mach-dove/Makefile
@@ -1,3 +1,5 @@ 
+AFLAGS_swsusp.o			:= -DTEXT_OFFSET=$(TEXT_OFFSET)
+
 obj-y				+= clock.o common.o addr-map.o irq.o pcie.o mpp.o \
 				sdhci_cam_mbus.o
 obj-$(CONFIG_MACH_DOVE_RD_AVNG)	+= dove-rd-avng-setup.o
diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
index 4f4a884..8e308d8 100644
--- a/arch/arm/mach-dove/swsusp.S
+++ b/arch/arm/mach-dove/swsusp.S
@@ -28,6 +28,7 @@ 
  */

 #include <linux/linkage.h>
+#include <asm/memory.h>
 #include <asm/segment.h>
 #include <asm/page.h>
 #include <asm/asm-offsets.h>
@@ -209,8 +210,14 @@  FUNC(swsusp_arch_suspend)

 FUNC_END(swsusp_arch_suspend)

+#define KERNEL_RAM_PADDR	(PHYS_OFFSET + TEXT_OFFSET)
+
 FUNC(swsusp_arch_resume)
 	/* set page table if needed */
+	ldr	r0, =(KERNEL_RAM_PADDR - 0x4000)
+	mcr	p15, 0, r0, c2, c0, 0		@ load page table pointer
+	mcr	p15, 0, r0, c8, c7, 0		@ invalidate I,D TLBs
+	mcr	p15, 0, r0, c7, c5, 4		@ ISB

 	/*
 	 * retore "nr_copy_pages" pages which are saved and specified