[PATCH 0/2] ARM: shmobile: Move written variables from .text to .data

[PATCH 0/2] ARM: shmobile: Move written variables from .text to .data

[Geert Uytterhoeven]

	Hi Simon, Magnus,

When CONFIG_ARM_KERNMEM_PERMS=y and CONFIG_DEBUG_RODATA=y, the kernel
crashes during system suspend:
    
    Freezing user space processes ... (elapsed 0.004 seconds) done.
    Freezing remaining freezable tasks ... (elapsed 0.002 seconds)
    done.
    PM: suspend of devices complete after 111.948 msecs
    PM: late suspend of devices complete after 1.086 msecs
    PM: noirq suspend of devices complete after 11.576 msecs
    Disabling non-boot CPUs ...
    Kernel panic - not syncing: Attempted to kill the idle task!
    1014ec ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
    CPU0: stopping

This happens because the shmobile assembler sources have several
variables that are written to in the .text section, while .text is
mapped read-only after kernel bootup if CONFIG_DEBUG_RODATA=y.

This series fixes this by moving variables from .text to .data.
Note that there's still an issue with shmobile_boot_fn and
shmobile_boot_arg in arch/arm/mach-shmobile/headsmp.S.
So far I didn't manage to fix this (the code and data are copied to
SRAM on some SoCs).  However, currently this are harmless, as these are
written during early kernel boot up only, before .text is marked
read-only.

These issues were uncovered by "[PATCH v2] ARM: mm: flip priority of
CONFIG_DEBUG_RODATA". As that patch is planned for queue up (again)
soon, it may be a good idea to queue these fixes for v4.5, to avoid a
dependency with the arm tree.

Tested hard on sh73a0/kzm9g, r8a7791/koelsch.
Tested lighter on emev2/kzm9d[*], r8a73a4/ape6evm, r8a7740/armadillo, and
r8a7779/marzen[*] ([*] = no remote resume).

Thanks!

Geert Uytterhoeven (2):
  ARM: shmobile: Move shmobile_scu_base from .text to .data
  ARM: shmobile: Move shmobile_smp_{mpidr,fn,arg}[] from .text to .data

 arch/arm/mach-shmobile/headsmp-scu.S |  6 ------
 arch/arm/mach-shmobile/headsmp.S     | 22 ++++++++++++++++------
 arch/arm/mach-shmobile/platsmp-scu.c |  3 +++
 3 files changed, 19 insertions(+), 12 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/2] ARM: shmobile: Move shmobile_scu_base from .text to .data

[Geert Uytterhoeven]

shmobile_scu_base is being written to, so it doesn't belong in the .text
section. Fix this by moving it from asm .text to C .data, as it's no
longer used from asm code since commit 4f6da36f7edd5790 ("ARM: shmobile:
Remove old SCU boot code").

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/mach-shmobile/headsmp-scu.S | 6 ------
 arch/arm/mach-shmobile/platsmp-scu.c | 3 +++
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-shmobile/headsmp-scu.S b/arch/arm/mach-shmobile/headsmp-scu.S
index fa5248c52399c9b5..5e503d91ad70ddf2 100644
--- a/arch/arm/mach-shmobile/headsmp-scu.S
+++ b/arch/arm/mach-shmobile/headsmp-scu.S
@@ -38,9 +38,3 @@ ENTRY(shmobile_boot_scu)
 
 	b	secondary_startup
 ENDPROC(shmobile_boot_scu)
-
-	.text
-	.align	2
-	.globl	shmobile_scu_base
-shmobile_scu_base:
-	.space	4
diff --git a/arch/arm/mach-shmobile/platsmp-scu.c b/arch/arm/mach-shmobile/platsmp-scu.c
index 64663110ab6ca0e1..f2198c9c74359736 100644
--- a/arch/arm/mach-shmobile/platsmp-scu.c
+++ b/arch/arm/mach-shmobile/platsmp-scu.c
@@ -17,6 +17,9 @@
 #include <asm/smp_scu.h>
 #include "common.h"
 
+
+void __iomem *shmobile_scu_base;
+
 static int shmobile_smp_scu_notifier_call(struct notifier_block *nfb,
 					  unsigned long action, void *hcpu)
 {
-- 
1.9.1

[PATCH 2/2] ARM: shmobile: Move shmobile_smp_{mpidr, fn, arg}[] from .text to .data

[Geert Uytterhoeven]

When CONFIG_ARM_KERNMEM_PERMS=y and CONFIG_DEBUG_RODATA=y, the kernel
crashes during system suspend:

    Freezing user space processes ... (elapsed 0.004 seconds) done.
    Freezing remaining freezable tasks ... (elapsed 0.002 seconds)
    done.
    PM: suspend of devices complete after 111.948 msecs
    PM: late suspend of devices complete after 1.086 msecs
    PM: noirq suspend of devices complete after 11.576 msecs
    Disabling non-boot CPUs ...
    Kernel panic - not syncing: Attempted to kill the idle task!
    1014ec ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
    CPU0: stopping

This happens because the .text section is marked read-only, while the
arrays shmobile_smp_mpidr[], shmobile_smp_fn[], and shmobile_smp_arg[]
are being written to.

Fix this by moving these arrays from the .text to the .data section.
This requires accessing them through a pointer after virtual to physical
translation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/mach-shmobile/headsmp.S | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-shmobile/headsmp.S b/arch/arm/mach-shmobile/headsmp.S
index 330c1fc63197df89..f924273ceaf476b4 100644
--- a/arch/arm/mach-shmobile/headsmp.S
+++ b/arch/arm/mach-shmobile/headsmp.S
@@ -50,9 +50,12 @@ ENTRY(shmobile_smp_boot)
 	mrc	p15, 0, r1, c0, c0, 5		@ r1 = MPIDR
 	and	r0, r1, r0			@ r0 = cpu_logical_map() value
 	mov	r1, #0				@ r1 = CPU index
-	adr	r5, 1f				@ array of per-cpu mpidr values
-	adr	r6, 2f				@ array of per-cpu functions
-	adr	r7, 3f				@ array of per-cpu arguments
+	adr	r2, 1f
+	ldmia	r2, {r4, r5, r6, r7}
+	sub	r2, r2, r4			@ virt->phys offset
+	add	r5, r5, r2			@ array of per-cpu mpidr values
+	add	r6, r6, r2			@ array of per-cpu functions
+	add	r7, r7, r2			@ array of per-cpu arguments
 
 shmobile_smp_boot_find_mpidr:
 	ldr	r8, [r5, r1, lsl #2]
@@ -80,12 +83,19 @@ ENTRY(shmobile_smp_sleep)
 	b	shmobile_smp_boot
 ENDPROC(shmobile_smp_sleep)
 
+	.align	2
+1:	.long	.
+	.long	shmobile_smp_mpidr
+	.long	shmobile_smp_fn
+	.long	shmobile_smp_arg
+
+	.data
 	.globl	shmobile_smp_mpidr
 shmobile_smp_mpidr:
-1:	.space	NR_CPUS * 4
+	.space	NR_CPUS * 4
 	.globl	shmobile_smp_fn
 shmobile_smp_fn:
-2:	.space	NR_CPUS * 4
+	.space	NR_CPUS * 4
 	.globl	shmobile_smp_arg
 shmobile_smp_arg:
-3:	.space	NR_CPUS * 4
+	.space	NR_CPUS * 4
-- 
1.9.1

[PATCH 0/2] ARM: shmobile: Move written variables from .text to .data

[Kees Cook]

On Thu, Jan 28, 2016 at 7:04 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>         Hi Simon, Magnus,
>
> When CONFIG_ARM_KERNMEM_PERMS=y and CONFIG_DEBUG_RODATA=y, the kernel
> crashes during system suspend:
>
>     Freezing user space processes ... (elapsed 0.004 seconds) done.
>     Freezing remaining freezable tasks ... (elapsed 0.002 seconds)
>     done.
>     PM: suspend of devices complete after 111.948 msecs
>     PM: late suspend of devices complete after 1.086 msecs
>     PM: noirq suspend of devices complete after 11.576 msecs
>     Disabling non-boot CPUs ...
>     Kernel panic - not syncing: Attempted to kill the idle task!
>     1014ec ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>     CPU0: stopping
>
> This happens because the shmobile assembler sources have several
> variables that are written to in the .text section, while .text is
> mapped read-only after kernel bootup if CONFIG_DEBUG_RODATA=y.
>
> This series fixes this by moving variables from .text to .data.
> Note that there's still an issue with shmobile_boot_fn and
> shmobile_boot_arg in arch/arm/mach-shmobile/headsmp.S.
> So far I didn't manage to fix this (the code and data are copied to
> SRAM on some SoCs).  However, currently this are harmless, as these are
> written during early kernel boot up only, before .text is marked
> read-only.
>
> These issues were uncovered by "[PATCH v2] ARM: mm: flip priority of
> CONFIG_DEBUG_RODATA". As that patch is planned for queue up (again)
> soon, it may be a good idea to queue these fixes for v4.5, to avoid a
> dependency with the arm tree.
>
> Tested hard on sh73a0/kzm9g, r8a7791/koelsch.
> Tested lighter on emev2/kzm9d[*], r8a73a4/ape6evm, r8a7740/armadillo, and
> r8a7779/marzen[*] ([*] = no remote resume).
>
> Thanks!
>
> Geert Uytterhoeven (2):
>   ARM: shmobile: Move shmobile_scu_base from .text to .data
>   ARM: shmobile: Move shmobile_smp_{mpidr,fn,arg}[] from .text to .data
>
>  arch/arm/mach-shmobile/headsmp-scu.S |  6 ------
>  arch/arm/mach-shmobile/headsmp.S     | 22 ++++++++++++++++------
>  arch/arm/mach-shmobile/platsmp-scu.c |  3 +++
>  3 files changed, 19 insertions(+), 12 deletions(-)

Thanks!

Reviewed-by: Kees Cook <keescook@chromium.org>

And FWIW, I've now got the RODATA patch back in the patch queue, since
we're at -rc1:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8501/1

-Kees

>
> --
> 1.9.1
>
> Gr{oetje,eeting}s,
>
>                                                 Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                                             -- Linus Torvalds



-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 1/2] ARM: shmobile: Move shmobile_scu_base from .text to .data

[Sergei Shtylyov]

Hello.

On 01/28/2016 06:04 PM, Geert Uytterhoeven wrote:

> shmobile_scu_base is being written to, so it doesn't belong in the .text
> section. Fix this by moving it from asm .text to C .data, as it's no
> longer used from asm code since commit 4f6da36f7edd5790 ("ARM: shmobile:
> Remove old SCU boot code").
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[...]

> diff --git a/arch/arm/mach-shmobile/platsmp-scu.c b/arch/arm/mach-shmobile/platsmp-scu.c
> index 64663110ab6ca0e1..f2198c9c74359736 100644
> --- a/arch/arm/mach-shmobile/platsmp-scu.c
> +++ b/arch/arm/mach-shmobile/platsmp-scu.c
> @@ -17,6 +17,9 @@
>   #include <asm/smp_scu.h>
>   #include "common.h"
>
> +

    Why add 2nd empty line?

> +void __iomem *shmobile_scu_base;
> +
>   static int shmobile_smp_scu_notifier_call(struct notifier_block *nfb,
>   					  unsigned long action, void *hcpu)
>   {

MBR, Sergei

[PATCH 2/2] ARM: shmobile: Move shmobile_smp_{mpidr,fn,arg}[] from .text to .data

[Nicolas Pitre]

On Thu, 28 Jan 2016, Geert Uytterhoeven wrote:

> When CONFIG_ARM_KERNMEM_PERMS=y and CONFIG_DEBUG_RODATA=y, the kernel
> crashes during system suspend:
> 
>     Freezing user space processes ... (elapsed 0.004 seconds) done.
>     Freezing remaining freezable tasks ... (elapsed 0.002 seconds)
>     done.
>     PM: suspend of devices complete after 111.948 msecs
>     PM: late suspend of devices complete after 1.086 msecs
>     PM: noirq suspend of devices complete after 11.576 msecs
>     Disabling non-boot CPUs ...
>     Kernel panic - not syncing: Attempted to kill the idle task!
>     1014ec ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>     CPU0: stopping
> 
> This happens because the .text section is marked read-only, while the
> arrays shmobile_smp_mpidr[], shmobile_smp_fn[], and shmobile_smp_arg[]
> are being written to.
> 
> Fix this by moving these arrays from the .text to the .data section.
> This requires accessing them through a pointer after virtual to physical
> translation.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  arch/arm/mach-shmobile/headsmp.S | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/headsmp.S b/arch/arm/mach-shmobile/headsmp.S
> index 330c1fc63197df89..f924273ceaf476b4 100644
> --- a/arch/arm/mach-shmobile/headsmp.S
> +++ b/arch/arm/mach-shmobile/headsmp.S
> @@ -50,9 +50,12 @@ ENTRY(shmobile_smp_boot)
>  	mrc	p15, 0, r1, c0, c0, 5		@ r1 = MPIDR
>  	and	r0, r1, r0			@ r0 = cpu_logical_map() value
>  	mov	r1, #0				@ r1 = CPU index
> -	adr	r5, 1f				@ array of per-cpu mpidr values
> -	adr	r6, 2f				@ array of per-cpu functions
> -	adr	r7, 3f				@ array of per-cpu arguments
> +	adr	r2, 1f
> +	ldmia	r2, {r4, r5, r6, r7}
> +	sub	r2, r2, r4			@ virt->phys offset
> +	add	r5, r5, r2			@ array of per-cpu mpidr values
> +	add	r6, r6, r2			@ array of per-cpu functions
> +	add	r7, r7, r2			@ array of per-cpu arguments

You could do something slightly more efficient like this:

	adr	r2, 1f
	ldmia	r2, {r5, r6, r7}
	add	r5, r5, r2
	add	r6, r6, r2
	add	r7, r7, r2

[...]

1:	.long	shmobile_smp_mpidr - .
	.long	shmobile_smp_fn - 1b
	.long	shmobile_smp_arg - 1b


Nicolas

[PATCH 1/2] ARM: shmobile: Move shmobile_scu_base from .text to .data

[Geert Uytterhoeven]

On Thu, Jan 28, 2016 at 4:37 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
>> diff --git a/arch/arm/mach-shmobile/platsmp-scu.c
>> b/arch/arm/mach-shmobile/platsmp-scu.c
>> index 64663110ab6ca0e1..f2198c9c74359736 100644
>> --- a/arch/arm/mach-shmobile/platsmp-scu.c
>> +++ b/arch/arm/mach-shmobile/platsmp-scu.c
>> @@ -17,6 +17,9 @@
>>   #include <asm/smp_scu.h>
>>   #include "common.h"
>>
>> +
>
>    Why add 2nd empty line?

I always leave two blank lines between different sections, and one blank
line between function or data blocks.

Is that a problem?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 0/2] ARM: shmobile: Move written variables from .text to .data

[Russell King - ARM Linux]

On Thu, Jan 28, 2016 at 07:19:34AM -0800, Kees Cook wrote:
> On Thu, Jan 28, 2016 at 7:04 AM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> >         Hi Simon, Magnus,
> >
> > When CONFIG_ARM_KERNMEM_PERMS=y and CONFIG_DEBUG_RODATA=y, the kernel
> > crashes during system suspend:
> >
> >     Freezing user space processes ... (elapsed 0.004 seconds) done.
> >     Freezing remaining freezable tasks ... (elapsed 0.002 seconds)
> >     done.
> >     PM: suspend of devices complete after 111.948 msecs
> >     PM: late suspend of devices complete after 1.086 msecs
> >     PM: noirq suspend of devices complete after 11.576 msecs
> >     Disabling non-boot CPUs ...
> >     Kernel panic - not syncing: Attempted to kill the idle task!
> >     1014ec ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> >     CPU0: stopping
> >
> > This happens because the shmobile assembler sources have several
> > variables that are written to in the .text section, while .text is
> > mapped read-only after kernel bootup if CONFIG_DEBUG_RODATA=y.
> >
> > This series fixes this by moving variables from .text to .data.
> > Note that there's still an issue with shmobile_boot_fn and
> > shmobile_boot_arg in arch/arm/mach-shmobile/headsmp.S.
> > So far I didn't manage to fix this (the code and data are copied to
> > SRAM on some SoCs).  However, currently this are harmless, as these are
> > written during early kernel boot up only, before .text is marked
> > read-only.
> >
> > These issues were uncovered by "[PATCH v2] ARM: mm: flip priority of
> > CONFIG_DEBUG_RODATA". As that patch is planned for queue up (again)
> > soon, it may be a good idea to queue these fixes for v4.5, to avoid a
> > dependency with the arm tree.
> >
> > Tested hard on sh73a0/kzm9g, r8a7791/koelsch.
> > Tested lighter on emev2/kzm9d[*], r8a73a4/ape6evm, r8a7740/armadillo, and
> > r8a7779/marzen[*] ([*] = no remote resume).
> >
> > Thanks!
> >
> > Geert Uytterhoeven (2):
> >   ARM: shmobile: Move shmobile_scu_base from .text to .data
> >   ARM: shmobile: Move shmobile_smp_{mpidr,fn,arg}[] from .text to .data
> >
> >  arch/arm/mach-shmobile/headsmp-scu.S |  6 ------
> >  arch/arm/mach-shmobile/headsmp.S     | 22 ++++++++++++++++------
> >  arch/arm/mach-shmobile/platsmp-scu.c |  3 +++
> >  3 files changed, 19 insertions(+), 12 deletions(-)
> 
> Thanks!
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> And FWIW, I've now got the RODATA patch back in the patch queue, since
> we're at -rc1:
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8501/1

And I'll be dropping it into linux-next for next week (iow, I'll commit
it tomorrow.)

Unfortunately, -rc1 broke some platforms, so Olof's builder isn't going
to get full coverage of it while building only my tree.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/2] ARM: shmobile: Move written variables from .text to .data

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [160128 09:50]:
> On Thu, Jan 28, 2016 at 07:19:34AM -0800, Kees Cook wrote:
> > 
> > And FWIW, I've now got the RODATA patch back in the patch queue, since
> > we're at -rc1:
> > http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8501/1
> 
> And I'll be dropping it into linux-next for next week (iow, I'll commit
> it tomorrow.)
> 
> Unfortunately, -rc1 broke some platforms, so Olof's builder isn't going
> to get full coverage of it while building only my tree.

FYI I sent a pull request for the omap related fixes earlier today. So
we should be good to go from that point of view for enabling rodata by
default for v4.6.

Regards,

Tony