diff mbox

[v3] ARM: xip: Use correct symbol for end of ROM marker

Message ID 1447697140-4099-1-git-send-email-chris.brandt@renesas.com (mailing list archive)
State Changes Requested
Delegated to: Simon Horman
Headers show

Commit Message

Chris Brandt Nov. 16, 2015, 6:05 p.m. UTC
For an XIP build, _edata_loc, not _etext, represents the end of the
binary image that will be programmed into ROM and mapped into the
MODULES_VADDR area.
With an XIP kernel, nothing is loaded into RAM before boot, meaning
you have to take into account the size of the entire binary image
that was programmed, including the init data values that will be copied
to RAM during kernel boot.

This fixes the bug where you might lose the end of your kernel area
after page table setup is complete.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v3:
* Removed sections.h from Kbuild
v2:
* Added change for MODULES_VADDR
* Moved extern to new file asm/sections.h
---
 arch/arm/include/asm/Kbuild     |    1 -
 arch/arm/include/asm/sections.h |    8 ++++++++
 arch/arm/kernel/module.c        |    2 +-
 arch/arm/mm/mmu.c               |    4 ++--
 4 files changed, 11 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/include/asm/sections.h

Comments

Russell King - ARM Linux Nov. 16, 2015, 6:17 p.m. UTC | #1
Nico,

As you originally created the XIP stuff, I hope you can remember
the details - can you check this patch please?

I'm thinking that we need a new symbol around here:

#ifdef CONFIG_XIP_KERNEL
        __data_loc = ALIGN(4);          /* location in binary */
					<=== here
        . = PAGE_OFFSET + TEXT_OFFSET;
#else

to denote the end of the XIP kernel image which must remain
accessible after boot.  We don't need the data sections because
they will have been copied to RAM, and we probably don't want to
keep those exposed (it's potentially useful for attackers.)

On Mon, Nov 16, 2015 at 01:05:40PM -0500, Chris Brandt wrote:
> For an XIP build, _edata_loc, not _etext, represents the end of the
> binary image that will be programmed into ROM and mapped into the
> MODULES_VADDR area.
> With an XIP kernel, nothing is loaded into RAM before boot, meaning
> you have to take into account the size of the entire binary image
> that was programmed, including the init data values that will be copied
> to RAM during kernel boot.
> 
> This fixes the bug where you might lose the end of your kernel area
> after page table setup is complete.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v3:
> * Removed sections.h from Kbuild
> v2:
> * Added change for MODULES_VADDR
> * Moved extern to new file asm/sections.h
> ---
>  arch/arm/include/asm/Kbuild     |    1 -
>  arch/arm/include/asm/sections.h |    8 ++++++++
>  arch/arm/kernel/module.c        |    2 +-
>  arch/arm/mm/mmu.c               |    4 ++--
>  4 files changed, 11 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm/include/asm/sections.h
> 
> diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild
> index be648eb..6d3da22 100644
> --- a/arch/arm/include/asm/Kbuild
> +++ b/arch/arm/include/asm/Kbuild
> @@ -21,7 +21,6 @@ generic-y += preempt.h
>  generic-y += resource.h
>  generic-y += rwsem.h
>  generic-y += seccomp.h
> -generic-y += sections.h
>  generic-y += segment.h
>  generic-y += sembuf.h
>  generic-y += serial.h
> diff --git a/arch/arm/include/asm/sections.h b/arch/arm/include/asm/sections.h
> new file mode 100644
> index 0000000..401eb3c2
> --- /dev/null
> +++ b/arch/arm/include/asm/sections.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASM_ARM_SECTIONS_H
> +#define _ASM_ARM_SECTIONS_H
> +
> +#include <asm-generic/sections.h>
> +
> +extern char _edata_loc[];
> +
> +#endif	/* _ASM_ARM_SECTIONS_H */
> diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
> index efdddcb..41ae2cc 100644
> --- a/arch/arm/kernel/module.c
> +++ b/arch/arm/kernel/module.c
> @@ -34,7 +34,7 @@
>   * recompiling the whole kernel when CONFIG_XIP_KERNEL is turned on/off.
>   */
>  #undef MODULES_VADDR
> -#define MODULES_VADDR	(((unsigned long)_etext + ~PMD_MASK) & PMD_MASK)
> +#define MODULES_VADDR	(((unsigned long)_edata_loc + ~PMD_MASK) & PMD_MASK)
>  #endif
>  
>  #ifdef CONFIG_MMU
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 4867f5d..dd5a56b 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1210,7 +1210,7 @@ static inline void prepare_page_table(void)
>  
>  #ifdef CONFIG_XIP_KERNEL
>  	/* The XIP kernel is mapped in the module area -- skip over it */
> -	addr = ((unsigned long)_etext + PMD_SIZE - 1) & PMD_MASK;
> +	addr = ((unsigned long)_edata_loc + PMD_SIZE - 1) & PMD_MASK;
>  #endif
>  	for ( ; addr < PAGE_OFFSET; addr += PMD_SIZE)
>  		pmd_clear(pmd_off_k(addr));
> @@ -1292,7 +1292,7 @@ static void __init devicemaps_init(const struct machine_desc *mdesc)
>  #ifdef CONFIG_XIP_KERNEL
>  	map.pfn = __phys_to_pfn(CONFIG_XIP_PHYS_ADDR & SECTION_MASK);
>  	map.virtual = MODULES_VADDR;
> -	map.length = ((unsigned long)_etext - map.virtual + ~SECTION_MASK) & SECTION_MASK;
> +	map.length = ((unsigned long)_edata_loc - map.virtual + ~SECTION_MASK) & SECTION_MASK;
>  	map.type = MT_ROM;
>  	create_mapping(&map);
>  #endif
> -- 
> 1.7.9.5
> 
>
Chris Brandt Nov. 16, 2015, 7:46 p.m. UTC | #2
> We don't need the data sections because they will have been copied to RAM, and
> we probably don't want to keep those exposed (it's potentially useful for
> attackers.)

The init sections also hang around after boot as well (it's XIP code, so there is nothing to 'free' in terms of executable init code).
Any potential security issues there as well? Should the data and init-text sections be put in a separate section that gets blown away after init-data is freed?

Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Nov. 16, 2015, 7:53 p.m. UTC | #3
On Mon, Nov 16, 2015 at 07:46:05PM +0000, Chris Brandt wrote:
> > We don't need the data sections because they will have been copied to RAM, and
> > we probably don't want to keep those exposed (it's potentially useful for
> > attackers.)
> 
> The init sections also hang around after boot as well (it's XIP code, so
> there is nothing to 'free' in terms of executable init code).
> Any potential security issues there as well? Should the data and init-
> text sections be put in a separate section that gets blown away after
> init-data is freed?

That's much harder to do - generally for XIP, people are space limited
(which is why they're using XIP rather than putting the kernel in RAM.)
They won't take kindly to having the kernel image bloated by 1MB just
to pad it out so that the init stuff can be unmapped.

However, from the security point of view, the less that's mapped at
known addresses, the better.
Chris Brandt Nov. 16, 2015, 8:18 p.m. UTC | #4
> That's much harder to do - generally for XIP, people are space limited 
> (which is why they're using XIP rather than putting the kernel in RAM.)
> They won't take kindly to having the kernel image bloated by 1MB just to
> pad it out so that the init stuff can be unmapped.
>
> However, from the security point of view, the less that's mapped at known
> addresses, the better.


Maybe the only true solution is to have some additional XIP Kconfig option that tells the kernel to not copy any of the init or data sections at all at boot, but instead leave it up to pre-boot to preload the physical locations with the correct values. That would get rid of your 1MB bloated boundary and you could even save some valuable XIP-able ROM/Flash space (you could pre-load from serial flash).

However, while that might provide more flexibility and ROM savings, it means now the pre-boot needs to know what values need to get pre-loaded where...adding more complexity to what right now is a very simple boot (set the PC and run).


- Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 16, 2015, 8:27 p.m. UTC | #5
On Mon, 16 Nov 2015, Chris Brandt wrote:

> For an XIP build, _edata_loc, not _etext, represents the end of the
> binary image that will be programmed into ROM and mapped into the
> MODULES_VADDR area.

This statement is wrong.  The kernel data is copied to RAM before the 
MMU is turned on and that's the copy that the kernel must use.  There is 
no reason having anything past _etext accessible from ROM.

> With an XIP kernel, nothing is loaded into RAM before boot, meaning
> you have to take into account the size of the entire binary image
> that was programmed, including the init data values that will be copied
> to RAM during kernel boot.

But this is done way before the code you're modifying is executed.

> This fixes the bug where you might lose the end of your kernel area
> after page table setup is complete.

Could you elaborate about a possible bug please?

> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v3:
> * Removed sections.h from Kbuild
> v2:
> * Added change for MODULES_VADDR
> * Moved extern to new file asm/sections.h
> ---
>  arch/arm/include/asm/Kbuild     |    1 -
>  arch/arm/include/asm/sections.h |    8 ++++++++
>  arch/arm/kernel/module.c        |    2 +-
>  arch/arm/mm/mmu.c               |    4 ++--
>  4 files changed, 11 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm/include/asm/sections.h
> 
> diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild
> index be648eb..6d3da22 100644
> --- a/arch/arm/include/asm/Kbuild
> +++ b/arch/arm/include/asm/Kbuild
> @@ -21,7 +21,6 @@ generic-y += preempt.h
>  generic-y += resource.h
>  generic-y += rwsem.h
>  generic-y += seccomp.h
> -generic-y += sections.h
>  generic-y += segment.h
>  generic-y += sembuf.h
>  generic-y += serial.h
> diff --git a/arch/arm/include/asm/sections.h b/arch/arm/include/asm/sections.h
> new file mode 100644
> index 0000000..401eb3c2
> --- /dev/null
> +++ b/arch/arm/include/asm/sections.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASM_ARM_SECTIONS_H
> +#define _ASM_ARM_SECTIONS_H
> +
> +#include <asm-generic/sections.h>
> +
> +extern char _edata_loc[];
> +
> +#endif	/* _ASM_ARM_SECTIONS_H */
> diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
> index efdddcb..41ae2cc 100644
> --- a/arch/arm/kernel/module.c
> +++ b/arch/arm/kernel/module.c
> @@ -34,7 +34,7 @@
>   * recompiling the whole kernel when CONFIG_XIP_KERNEL is turned on/off.
>   */
>  #undef MODULES_VADDR
> -#define MODULES_VADDR	(((unsigned long)_etext + ~PMD_MASK) & PMD_MASK)
> +#define MODULES_VADDR	(((unsigned long)_edata_loc + ~PMD_MASK) & PMD_MASK)
>  #endif
>  
>  #ifdef CONFIG_MMU
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 4867f5d..dd5a56b 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1210,7 +1210,7 @@ static inline void prepare_page_table(void)
>  
>  #ifdef CONFIG_XIP_KERNEL
>  	/* The XIP kernel is mapped in the module area -- skip over it */
> -	addr = ((unsigned long)_etext + PMD_SIZE - 1) & PMD_MASK;
> +	addr = ((unsigned long)_edata_loc + PMD_SIZE - 1) & PMD_MASK;
>  #endif
>  	for ( ; addr < PAGE_OFFSET; addr += PMD_SIZE)
>  		pmd_clear(pmd_off_k(addr));
> @@ -1292,7 +1292,7 @@ static void __init devicemaps_init(const struct machine_desc *mdesc)
>  #ifdef CONFIG_XIP_KERNEL
>  	map.pfn = __phys_to_pfn(CONFIG_XIP_PHYS_ADDR & SECTION_MASK);
>  	map.virtual = MODULES_VADDR;
> -	map.length = ((unsigned long)_etext - map.virtual + ~SECTION_MASK) & SECTION_MASK;
> +	map.length = ((unsigned long)_edata_loc - map.virtual + ~SECTION_MASK) & SECTION_MASK;
>  	map.type = MT_ROM;
>  	create_mapping(&map);
>  #endif
> -- 
> 1.7.9.5
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Nov. 16, 2015, 8:30 p.m. UTC | #6
Please wrap your messages at or before column 72, thanks.

On Mon, Nov 16, 2015 at 08:18:55PM +0000, Chris Brandt wrote:
> Maybe the only true solution is to have some additional XIP Kconfig
> option that tells the kernel to not copy any of the init or data
> sections at all at boot,

An XIP kernel will copy the .data section from ROM into RAM for you.
You don't need additional code external to the kernel to perfor this.
Nicolas Pitre Nov. 16, 2015, 8:57 p.m. UTC | #7
On Mon, 16 Nov 2015, Russell King - ARM Linux wrote:

> Nico,
> 
> As you originally created the XIP stuff, I hope you can remember
> the details - can you check this patch please?
> 
> I'm thinking that we need a new symbol around here:
> 
> #ifdef CONFIG_XIP_KERNEL
>         __data_loc = ALIGN(4);          /* location in binary */
> 					<=== here
>         . = PAGE_OFFSET + TEXT_OFFSET;
> #else
> 
> to denote the end of the XIP kernel image which must remain
> accessible after boot.  We don't need the data sections because
> they will have been copied to RAM, and we probably don't want to
> keep those exposed (it's potentially useful for attackers.)

The _etext symbol is already used for that purpose.

Now we round it up to the next section mapping which might leave quite a 
lot of data content exposed in ROM.  But given it is more or less the 
same data present in RAM except for those bits that are modified at run 
time, I don't see what an attacker would gain from the data in ROM   
that cannot already be obtained from kernel RAM.  If the section mapping 
extends to part of the ROM that is no longer kernel data then maybe this 
would expose sensitive data.  Is that what you're worried about?


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 16, 2015, 9:02 p.m. UTC | #8
>> For an XIP build, _edata_loc, not _etext, represents the end of the 
>> binary image that will be programmed into ROM and mapped into the 
>> MODULES_VADDR area.
>
> This statement is wrong.  The kernel data is copied to RAM before the
> MMU is turned on and that's the copy that the kernel must use.  There
> is no reason having anything past _etext accessible from ROM.

If I remember correctly, when I first ran into an issue was my
initramfs got cut off because it spanned the 1MB boundary after
_etext.

Also, your init code resides after _etext, so that get cut off
too.


>> With an XIP kernel, nothing is loaded into RAM before boot, meaning 
>> you have to take into account the size of the entire binary image that 
>> was programmed, including the init data values that will be copied to 
>> RAM during kernel boot.
>
> But this is done way before the code you're modifying is executed.

If the MMU is set up to cut everything off after _etext early on, then
anything in the init section gets blown away, and there are lots
of init functions in the kernel that get run at various times during
boot.


>> This fixes the bug where you might lose the end of your kernel area 
>> after page table setup is complete.
>
>Could you elaborate about a possible bug please?

During boot, the kernel attempts to execute code that is not mapped
anywhere, and hence crashes.



-Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 16, 2015, 9:09 p.m. UTC | #9
> If the section mapping extends to part of the ROM that is no longer
> kernel data then maybe this would expose sensitive data.  Is that
> what you're worried about?

That is a good point that I did not consider before. Thank you.

Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 16, 2015, 9:47 p.m. UTC | #10
On Mon, 16 Nov 2015, Chris Brandt wrote:

> >> For an XIP build, _edata_loc, not _etext, represents the end of the 
> >> binary image that will be programmed into ROM and mapped into the 
> >> MODULES_VADDR area.
> >
> > This statement is wrong.  The kernel data is copied to RAM before the
> > MMU is turned on and that's the copy that the kernel must use.  There
> > is no reason having anything past _etext accessible from ROM.
> 
> If I remember correctly, when I first ran into an issue was my
> initramfs got cut off because it spanned the 1MB boundary after
> _etext.

If you are convinced that you need kernel XIP, then you normally would 
want _not_ to use initramfs at all.

> Also, your init code resides after _etext, so that get cut off
> too.

/me looks at the linker script and cry

Forget it.  XIP kernel is simply completely broken at the moment.  

There used to be conditional placement of .init.text that was located in 
the .text section when XIP_KERNEL was selected, or in the .init section 
otherwise.  This particular section was abstracted away into an 
INIT_TEXT_SECTION macro defined in include/asm-generic/vmlinux.lds.h and 
the XIP conditional placement got lost.  New sections (lots of them) 
were added as well since the introduction of XIP kernel with no 
consideration for XIP usage.

The old method with conditional placement in the linker script is broken 
beyond repair.  There is simply way too much stuff these days to do this 
sanely.  I think the only way to fix it properly is to have a separate 
linker script for XIP usage with proper (and obvious) section placement.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 16, 2015, 10:19 p.m. UTC | #11
> If you are convinced that you need kernel XIP, then you normally would
> want _not_ to use initramfs at all.

True, but in trying to test upstream kernels, initramfs was easier than
fighting with device drivers (although now I've just switched to using
memory mapped mtd-rom).


> Forget it.  XIP kernel is simply completely broken at the moment.  

Yes, there were multiple things wrong that took me a while to find, but
once you get them all working, it's pretty cool. Note, the Kconfig
issues are going to be a bit harder to get approval.


> New sections (lots of them) were added as well since the introduction
> of XIP kernel with no consideration for XIP usage.

So I've noticed. As far as I can tell, the XIP kernel stuff broke years
ago.


> I think the only way to fix it properly is to have a separate linker
> script for XIP usage with proper (and obvious) section placement.

I guess then the parts of the kernel that refer to section locations
need to be looked at again. As you can see from my other patches,
there's a bit of miss-match that I was trying to align.


So at this point...I'm assume all my XIP 'hacks' are being rejected.

As for making a new XIP-only linker script, note that I really only
work with the ARMv7. I know the M4 and M5 guys do XIP_KERNEL builds,
but I've got no way of testing their XIP_KERNEL builds or knowing
what will break.


Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 17, 2015, 12:48 a.m. UTC | #12
On Mon, 16 Nov 2015, Chris Brandt wrote:

> > New sections (lots of them) were added as well since the introduction
> > of XIP kernel with no consideration for XIP usage.
> 
> So I've noticed. As far as I can tell, the XIP kernel stuff broke years
> ago.

That, of course, brings the question about keeping XIP around.

> > I think the only way to fix it properly is to have a separate linker
> > script for XIP usage with proper (and obvious) section placement.
> 
> I guess then the parts of the kernel that refer to section locations
> need to be looked at again. As you can see from my other patches,
> there's a bit of miss-match that I was trying to align.

I don't think that's a big issue. Those section locations are obtained 
from symbols that the linker script creates explicitly.

> So at this point...I'm assume all my XIP 'hacks' are being rejected.

Pretty much.  Everything can be fixed at once with a new linker script.

> As for making a new XIP-only linker script, note that I really only
> work with the ARMv7. I know the M4 and M5 guys do XIP_KERNEL builds,
> but I've got no way of testing their XIP_KERNEL builds or knowing
> what will break.

Things can hardly be worse than they are right now.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 17, 2015, 2:11 a.m. UTC | #13
>> So I've noticed. As far as I can tell, the XIP kernel stuff broke 
>> years ago.
>
> That, of course, brings the question about keeping XIP around.

Well, as the email address suggests, I work for Renesas. And since
the ARCH_R7S72100 has up to 10MB internal RAM and can XIP directly from
QSPI Flash, that's a pretty simple BOM.

I also revisted the AXFS file system (that never made it into the
kernel) and like the kernel, required some fixing up.
However, I can get a functioning and usefull ARMv7 system up and
running in under 3MB of total system RAM.
That's got to be worth something to someone ;)


> Everything can be fixed at once with a new linker script.

I do have to agree that a separate linker script would be a cleaner
solution for the section arrangements.
Of course, there are still some other XIP patches needed (like you can't
use text areas as a temporary stack on boot), but the sections are a
pretty logical place to start.


Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 17, 2015, 2:37 a.m. UTC | #14
On Tue, 17 Nov 2015, Chris Brandt wrote:

> >> So I've noticed. As far as I can tell, the XIP kernel stuff broke 
> >> years ago.
> >
> > That, of course, brings the question about keeping XIP around.
> 
> Well, as the email address suggests, I work for Renesas. And since
> the ARCH_R7S72100 has up to 10MB internal RAM and can XIP directly from
> QSPI Flash, that's a pretty simple BOM.
> 
> I also revisted the AXFS file system (that never made it into the
> kernel) and like the kernel, required some fixing up.
> However, I can get a functioning and usefull ARMv7 system up and
> running in under 3MB of total system RAM.
> That's got to be worth something to someone ;)

Absolutely.  I'll be glad to review your patches.

> > Everything can be fixed at once with a new linker script.
> 
> I do have to agree that a separate linker script would be a cleaner
> solution for the section arrangements.
> Of course, there are still some other XIP patches needed (like you can't
> use text areas as a temporary stack on boot), but the sections are a
> pretty logical place to start.

Maybe this can be prevented automatically by the linker with some 
read-only memory regions or the like.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Nov. 17, 2015, 4:56 p.m. UTC | #15
> Absolutely.  I'll be glad to review your patches.

What do you think about this submission?
I think it still holds true regardless of the incorrect sectioning.

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/384354.html



>> Of course, there are still some other XIP patches needed (like you
>> can't use text areas as a temporary stack on boot), but the sections
>> are a pretty logical place to start.
>
> Maybe this can be prevented automatically by the linker with some
> read-only memory regions or the like.

I think this one is more of a coding issue.
These were the attempts to fix the temporary stack issue:

My first patch:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357106.html


Magnus's try:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/383394.html



Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre Nov. 17, 2015, 5:24 p.m. UTC | #16
On Tue, 17 Nov 2015, Chris Brandt wrote:

> > Absolutely.  I'll be glad to review your patches.
> 
> What do you think about this submission?
> I think it still holds true regardless of the incorrect sectioning.
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/384354.html

This patch is correct.

> >> Of course, there are still some other XIP patches needed (like you
> >> can't use text areas as a temporary stack on boot), but the sections
> >> are a pretty logical place to start.
> >
> > Maybe this can be prevented automatically by the linker with some
> > read-only memory regions or the like.
> 
> I think this one is more of a coding issue.
> These were the attempts to fix the temporary stack issue:
> 
> My first patch:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357106.html

I think this ought to be fixed regardless of the XIP issue.

> Magnus's try:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/383394.html

This one is wrong as it clobbers r11.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Nov. 17, 2015, 5:33 p.m. UTC | #17
On Mon, Nov 16, 2015 at 04:47:12PM -0500, Nicolas Pitre wrote:
> The old method with conditional placement in the linker script is broken 
> beyond repair.  There is simply way too much stuff these days to do this 
> sanely.  I think the only way to fix it properly is to have a separate 
> linker script for XIP usage with proper (and obvious) section placement.

Absolutely the right thing to do.

The XIP stuff has been broken and getting in the way for a long time
now because of the preprocessor mess.  I've made several changes to
the linker script over the years, and each time I've ended up deciding
to I'll do the best job I can for XIP, but I'm not going to spend
too long making sure that it's correct - especially as there appeared
to be no users of it.  I wouldn't describe the current file as being
"maintainable" in any way - It has devolved into a total mess, and I
would like to see the XIP stuff gone from the non-XIP linker script
- and doing that should mean it's less likely for XIP to break.

I'd also like to see a lot more of the preprocessor gunk gone (or at
least cleaned up) in that file too...
diff mbox

Patch

diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild
index be648eb..6d3da22 100644
--- a/arch/arm/include/asm/Kbuild
+++ b/arch/arm/include/asm/Kbuild
@@ -21,7 +21,6 @@  generic-y += preempt.h
 generic-y += resource.h
 generic-y += rwsem.h
 generic-y += seccomp.h
-generic-y += sections.h
 generic-y += segment.h
 generic-y += sembuf.h
 generic-y += serial.h
diff --git a/arch/arm/include/asm/sections.h b/arch/arm/include/asm/sections.h
new file mode 100644
index 0000000..401eb3c2
--- /dev/null
+++ b/arch/arm/include/asm/sections.h
@@ -0,0 +1,8 @@ 
+#ifndef _ASM_ARM_SECTIONS_H
+#define _ASM_ARM_SECTIONS_H
+
+#include <asm-generic/sections.h>
+
+extern char _edata_loc[];
+
+#endif	/* _ASM_ARM_SECTIONS_H */
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index efdddcb..41ae2cc 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -34,7 +34,7 @@ 
  * recompiling the whole kernel when CONFIG_XIP_KERNEL is turned on/off.
  */
 #undef MODULES_VADDR
-#define MODULES_VADDR	(((unsigned long)_etext + ~PMD_MASK) & PMD_MASK)
+#define MODULES_VADDR	(((unsigned long)_edata_loc + ~PMD_MASK) & PMD_MASK)
 #endif
 
 #ifdef CONFIG_MMU
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 4867f5d..dd5a56b 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1210,7 +1210,7 @@  static inline void prepare_page_table(void)
 
 #ifdef CONFIG_XIP_KERNEL
 	/* The XIP kernel is mapped in the module area -- skip over it */
-	addr = ((unsigned long)_etext + PMD_SIZE - 1) & PMD_MASK;
+	addr = ((unsigned long)_edata_loc + PMD_SIZE - 1) & PMD_MASK;
 #endif
 	for ( ; addr < PAGE_OFFSET; addr += PMD_SIZE)
 		pmd_clear(pmd_off_k(addr));
@@ -1292,7 +1292,7 @@  static void __init devicemaps_init(const struct machine_desc *mdesc)
 #ifdef CONFIG_XIP_KERNEL
 	map.pfn = __phys_to_pfn(CONFIG_XIP_PHYS_ADDR & SECTION_MASK);
 	map.virtual = MODULES_VADDR;
-	map.length = ((unsigned long)_etext - map.virtual + ~SECTION_MASK) & SECTION_MASK;
+	map.length = ((unsigned long)_edata_loc - map.virtual + ~SECTION_MASK) & SECTION_MASK;
 	map.type = MT_ROM;
 	create_mapping(&map);
 #endif