* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-18 5:01 ` Jeffy Chen
0 siblings, 0 replies; 42+ messages in thread
From: Jeffy Chen @ 2017-10-18 5:01 UTC (permalink / raw)
To: linux-kernel
Cc: chris.zhong, Jeffy Chen, Ingo Molnar, Ard Biesheuvel,
Russell King, linux-arm-kernel
The zImage file size should be aligned.
Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index b38dcef90756..1636fa259577 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -70,10 +70,6 @@ SECTIONS
.got : { *(.got) }
_got_end = .;
- /* ensure the zImage file size is always a multiple of 64 bits */
- /* (without a dummy byte, ld just ignores the empty section) */
- .pad : { BYTE(0); . = ALIGN(8); }
-
#ifdef CONFIG_EFI_STUB
.data : ALIGN(4096) {
__pecoff_data_start = .;
@@ -93,6 +89,10 @@ SECTIONS
__pecoff_data_rawsize = . - ADDR(.data);
#endif
+ /* ensure the zImage file size is always a multiple of 64 bits */
+ /* (without a dummy byte, ld just ignores the empty section) */
+ .pad : { BYTE(0); . = ALIGN(8); }
+
_edata = .;
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
--
2.11.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-18 5:01 ` Jeffy Chen
0 siblings, 0 replies; 42+ messages in thread
From: Jeffy Chen @ 2017-10-18 5:01 UTC (permalink / raw)
To: linux-arm-kernel
The zImage file size should be aligned.
Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index b38dcef90756..1636fa259577 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -70,10 +70,6 @@ SECTIONS
.got : { *(.got) }
_got_end = .;
- /* ensure the zImage file size is always a multiple of 64 bits */
- /* (without a dummy byte, ld just ignores the empty section) */
- .pad : { BYTE(0); . = ALIGN(8); }
-
#ifdef CONFIG_EFI_STUB
.data : ALIGN(4096) {
__pecoff_data_start = .;
@@ -93,6 +89,10 @@ SECTIONS
__pecoff_data_rawsize = . - ADDR(.data);
#endif
+ /* ensure the zImage file size is always a multiple of 64 bits */
+ /* (without a dummy byte, ld just ignores the empty section) */
+ .pad : { BYTE(0); . = ALIGN(8); }
+
_edata = .;
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
--
2.11.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-18 5:01 ` Jeffy Chen
@ 2017-10-18 6:19 ` Chris Zhong
-1 siblings, 0 replies; 42+ messages in thread
From: Chris Zhong @ 2017-10-18 6:19 UTC (permalink / raw)
To: Jeffy Chen, linux-kernel
Cc: chris.zhong, Ingo Molnar, Ard Biesheuvel, Russell King, linux-arm-kernel
Tested-by: Chris Zhong <zyw@rock-chips.com>
On Wednesday, October 18, 2017 01:01 PM, Jeffy Chen wrote:
> The zImage file size should be aligned.
>
> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..1636fa259577 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -70,10 +70,6 @@ SECTIONS
> .got : { *(.got) }
> _got_end = .;
>
> - /* ensure the zImage file size is always a multiple of 64 bits */
> - /* (without a dummy byte, ld just ignores the empty section) */
> - .pad : { BYTE(0); . = ALIGN(8); }
> -
> #ifdef CONFIG_EFI_STUB
> .data : ALIGN(4096) {
> __pecoff_data_start = .;
> @@ -93,6 +89,10 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> + /* ensure the zImage file size is always a multiple of 64 bits */
> + /* (without a dummy byte, ld just ignores the empty section) */
> + .pad : { BYTE(0); . = ALIGN(8); }
> +
> _edata = .;
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
--
Chris Zhong
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-18 6:19 ` Chris Zhong
0 siblings, 0 replies; 42+ messages in thread
From: Chris Zhong @ 2017-10-18 6:19 UTC (permalink / raw)
To: linux-arm-kernel
Tested-by: Chris Zhong <zyw@rock-chips.com>
On Wednesday, October 18, 2017 01:01 PM, Jeffy Chen wrote:
> The zImage file size should be aligned.
>
> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..1636fa259577 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -70,10 +70,6 @@ SECTIONS
> .got : { *(.got) }
> _got_end = .;
>
> - /* ensure the zImage file size is always a multiple of 64 bits */
> - /* (without a dummy byte, ld just ignores the empty section) */
> - .pad : { BYTE(0); . = ALIGN(8); }
> -
> #ifdef CONFIG_EFI_STUB
> .data : ALIGN(4096) {
> __pecoff_data_start = .;
> @@ -93,6 +89,10 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> + /* ensure the zImage file size is always a multiple of 64 bits */
> + /* (without a dummy byte, ld just ignores the empty section) */
> + .pad : { BYTE(0); . = ALIGN(8); }
> +
> _edata = .;
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
--
Chris Zhong
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-18 5:01 ` Jeffy Chen
@ 2017-10-22 11:01 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-22 11:01 UTC (permalink / raw)
To: Jeffy Chen, Russell King
Cc: linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> The zImage file size should be aligned.
>
> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..1636fa259577 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -70,10 +70,6 @@ SECTIONS
> .got : { *(.got) }
> _got_end = .;
>
> - /* ensure the zImage file size is always a multiple of 64 bits */
> - /* (without a dummy byte, ld just ignores the empty section) */
> - .pad : { BYTE(0); . = ALIGN(8); }
> -
> #ifdef CONFIG_EFI_STUB
> .data : ALIGN(4096) {
> __pecoff_data_start = .;
> @@ -93,6 +89,10 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> + /* ensure the zImage file size is always a multiple of 64 bits */
> + /* (without a dummy byte, ld just ignores the empty section) */
> + .pad : { BYTE(0); . = ALIGN(8); }
> +
> _edata = .;
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> --
> 2.11.0
>
This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
filesize should be rounded up to 512 bytes not 8 bytes. The '. =
ALIGN(512);' in the .data section appears to ensure that, but for some
reason, that appears not to be working.
Could you share the output of
$ readelf -S arch/arm/boot/compressed/vmlinux
please? And could you please check whether this patch
https://marc.info/?l=linux-arm-kernel&m=150488477807353
fixes the issue for you?
Thanks,
Ard.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-22 11:01 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-22 11:01 UTC (permalink / raw)
To: linux-arm-kernel
On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> The zImage file size should be aligned.
>
> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..1636fa259577 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -70,10 +70,6 @@ SECTIONS
> .got : { *(.got) }
> _got_end = .;
>
> - /* ensure the zImage file size is always a multiple of 64 bits */
> - /* (without a dummy byte, ld just ignores the empty section) */
> - .pad : { BYTE(0); . = ALIGN(8); }
> -
> #ifdef CONFIG_EFI_STUB
> .data : ALIGN(4096) {
> __pecoff_data_start = .;
> @@ -93,6 +89,10 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> + /* ensure the zImage file size is always a multiple of 64 bits */
> + /* (without a dummy byte, ld just ignores the empty section) */
> + .pad : { BYTE(0); . = ALIGN(8); }
> +
> _edata = .;
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> --
> 2.11.0
>
This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
filesize should be rounded up to 512 bytes not 8 bytes. The '. =
ALIGN(512);' in the .data section appears to ensure that, but for some
reason, that appears not to be working.
Could you share the output of
$ readelf -S arch/arm/boot/compressed/vmlinux
please? And could you please check whether this patch
https://marc.info/?l=linux-arm-kernel&m=150488477807353
fixes the issue for you?
Thanks,
Ard.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-22 11:01 ` Ard Biesheuvel
@ 2017-10-22 12:47 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-22 12:47 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jeffy Chen, linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> > The zImage file size should be aligned.
> >
> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> > index b38dcef90756..1636fa259577 100644
> > --- a/arch/arm/boot/compressed/vmlinux.lds.S
> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> > @@ -70,10 +70,6 @@ SECTIONS
> > .got : { *(.got) }
> > _got_end = .;
> >
> > - /* ensure the zImage file size is always a multiple of 64 bits */
> > - /* (without a dummy byte, ld just ignores the empty section) */
> > - .pad : { BYTE(0); . = ALIGN(8); }
> > -
> > #ifdef CONFIG_EFI_STUB
> > .data : ALIGN(4096) {
> > __pecoff_data_start = .;
> > @@ -93,6 +89,10 @@ SECTIONS
> > __pecoff_data_rawsize = . - ADDR(.data);
> > #endif
> >
> > + /* ensure the zImage file size is always a multiple of 64 bits */
> > + /* (without a dummy byte, ld just ignores the empty section) */
> > + .pad : { BYTE(0); . = ALIGN(8); }
> > +
> > _edata = .;
> >
> > _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> > --
> > 2.11.0
> >
>
> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
> ALIGN(512);' in the .data section appears to ensure that, but for some
> reason, that appears not to be working.
Actually, the existing .pad section is totally and utterly bogus when
EFI is enabled:
. = ALIGN(4);
_etext = .;
.got.plt : { *(.got.plt) }
_got_start = .;
.got : { *(.got) }
_got_end = .;
The .got.plt and .got are always word-based. This is then followed by
.pad, which does nothing but pad out to a multiple of 64 bit:
/* ensure the zImage file size is always a multiple of 64 bits */
/* (without a dummy byte, ld just ignores the empty section) */
.pad : { BYTE(0); . = ALIGN(8); }
So this may add zero or 4 bytes of padding.
This is then followed by the EFI data:
.data : ALIGN(4096) {
...
. = ALIGN(512);
}
which is aligned to 4K but aligns the end of itself to 512.
So, we have the end of .got aligned to 4, followed by .pad that tries to
align to 8, followed by an optional .data section. This is pointless.
A sane patch would be to choose between the EFI .data section and the
.pad section. So, it should be:
#ifdef CONFIG_EFI_STUB
.data : ALIGN(4096) {
...
. = ALIGN(512);
}
#else
.pad : { BYTE(0); . = ALIGN(8); }
#endif
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-22 12:47 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-22 12:47 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> > The zImage file size should be aligned.
> >
> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> > index b38dcef90756..1636fa259577 100644
> > --- a/arch/arm/boot/compressed/vmlinux.lds.S
> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> > @@ -70,10 +70,6 @@ SECTIONS
> > .got : { *(.got) }
> > _got_end = .;
> >
> > - /* ensure the zImage file size is always a multiple of 64 bits */
> > - /* (without a dummy byte, ld just ignores the empty section) */
> > - .pad : { BYTE(0); . = ALIGN(8); }
> > -
> > #ifdef CONFIG_EFI_STUB
> > .data : ALIGN(4096) {
> > __pecoff_data_start = .;
> > @@ -93,6 +89,10 @@ SECTIONS
> > __pecoff_data_rawsize = . - ADDR(.data);
> > #endif
> >
> > + /* ensure the zImage file size is always a multiple of 64 bits */
> > + /* (without a dummy byte, ld just ignores the empty section) */
> > + .pad : { BYTE(0); . = ALIGN(8); }
> > +
> > _edata = .;
> >
> > _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> > --
> > 2.11.0
> >
>
> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
> ALIGN(512);' in the .data section appears to ensure that, but for some
> reason, that appears not to be working.
Actually, the existing .pad section is totally and utterly bogus when
EFI is enabled:
. = ALIGN(4);
_etext = .;
.got.plt : { *(.got.plt) }
_got_start = .;
.got : { *(.got) }
_got_end = .;
The .got.plt and .got are always word-based. This is then followed by
.pad, which does nothing but pad out to a multiple of 64 bit:
/* ensure the zImage file size is always a multiple of 64 bits */
/* (without a dummy byte, ld just ignores the empty section) */
.pad : { BYTE(0); . = ALIGN(8); }
So this may add zero or 4 bytes of padding.
This is then followed by the EFI data:
.data : ALIGN(4096) {
...
. = ALIGN(512);
}
which is aligned to 4K but aligns the end of itself to 512.
So, we have the end of .got aligned to 4, followed by .pad that tries to
align to 8, followed by an optional .data section. This is pointless.
A sane patch would be to choose between the EFI .data section and the
.pad section. So, it should be:
#ifdef CONFIG_EFI_STUB
.data : ALIGN(4096) {
...
. = ALIGN(512);
}
#else
.pad : { BYTE(0); . = ALIGN(8); }
#endif
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-22 12:47 ` Russell King - ARM Linux
@ 2017-10-22 13:01 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-22 13:01 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jeffy Chen, linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
On 22 October 2017 at 13:47, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>> > The zImage file size should be aligned.
>> >
>> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
>> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> > ---
>> >
>> > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>> > 1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> > index b38dcef90756..1636fa259577 100644
>> > --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> > @@ -70,10 +70,6 @@ SECTIONS
>> > .got : { *(.got) }
>> > _got_end = .;
>> >
>> > - /* ensure the zImage file size is always a multiple of 64 bits */
>> > - /* (without a dummy byte, ld just ignores the empty section) */
>> > - .pad : { BYTE(0); . = ALIGN(8); }
>> > -
>> > #ifdef CONFIG_EFI_STUB
>> > .data : ALIGN(4096) {
>> > __pecoff_data_start = .;
>> > @@ -93,6 +89,10 @@ SECTIONS
>> > __pecoff_data_rawsize = . - ADDR(.data);
>> > #endif
>> >
>> > + /* ensure the zImage file size is always a multiple of 64 bits */
>> > + /* (without a dummy byte, ld just ignores the empty section) */
>> > + .pad : { BYTE(0); . = ALIGN(8); }
>> > +
>> > _edata = .;
>> >
>> > _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> > --
>> > 2.11.0
>> >
>>
>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
>> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
>> ALIGN(512);' in the .data section appears to ensure that, but for some
>> reason, that appears not to be working.
>
> Actually, the existing .pad section is totally and utterly bogus when
> EFI is enabled:
>
> . = ALIGN(4);
> _etext = .;
>
> .got.plt : { *(.got.plt) }
> _got_start = .;
> .got : { *(.got) }
> _got_end = .;
>
> The .got.plt and .got are always word-based. This is then followed by
> .pad, which does nothing but pad out to a multiple of 64 bit:
>
> /* ensure the zImage file size is always a multiple of 64 bits */
> /* (without a dummy byte, ld just ignores the empty section) */
> .pad : { BYTE(0); . = ALIGN(8); }
>
> So this may add zero or 4 bytes of padding.
>
> This is then followed by the EFI data:
>
> .data : ALIGN(4096) {
> ...
> . = ALIGN(512);
> }
>
> which is aligned to 4K but aligns the end of itself to 512.
>
> So, we have the end of .got aligned to 4, followed by .pad that tries to
> align to 8, followed by an optional .data section. This is pointless.
>
> A sane patch would be to choose between the EFI .data section and the
> .pad section. So, it should be:
>
> #ifdef CONFIG_EFI_STUB
> .data : ALIGN(4096) {
> ...
> . = ALIGN(512);
> }
> #else
> .pad : { BYTE(0); . = ALIGN(8); }
> #endif
>
Agreed, the .pad section has no point for EFI_STUB=y. However, it
seems this symptom is caused by the same issues I am trying to address
here
https://marc.info/?l=linux-arm-kernel&m=150488477807353
which is that we have __ksymtab_xxx sections that we should discard,
because the linker will otherwise emit them /after/ .data or .pad.
This is caused by the use of lib/sort.c in the EFI stub, which
contains an EXPORT_SYMBOL().
Would you perhaps prefer that I clone sort.c into its own .c file
specifically for the EFI stub? (under drivers/firmware/efi/libstub)
That should get rid of these spurious sections and thus the
misalignments and/or movements that are causing all of these issues.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-22 13:01 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-22 13:01 UTC (permalink / raw)
To: linux-arm-kernel
On 22 October 2017 at 13:47, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>> > The zImage file size should be aligned.
>> >
>> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
>> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> > ---
>> >
>> > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>> > 1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> > index b38dcef90756..1636fa259577 100644
>> > --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> > @@ -70,10 +70,6 @@ SECTIONS
>> > .got : { *(.got) }
>> > _got_end = .;
>> >
>> > - /* ensure the zImage file size is always a multiple of 64 bits */
>> > - /* (without a dummy byte, ld just ignores the empty section) */
>> > - .pad : { BYTE(0); . = ALIGN(8); }
>> > -
>> > #ifdef CONFIG_EFI_STUB
>> > .data : ALIGN(4096) {
>> > __pecoff_data_start = .;
>> > @@ -93,6 +89,10 @@ SECTIONS
>> > __pecoff_data_rawsize = . - ADDR(.data);
>> > #endif
>> >
>> > + /* ensure the zImage file size is always a multiple of 64 bits */
>> > + /* (without a dummy byte, ld just ignores the empty section) */
>> > + .pad : { BYTE(0); . = ALIGN(8); }
>> > +
>> > _edata = .;
>> >
>> > _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> > --
>> > 2.11.0
>> >
>>
>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
>> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
>> ALIGN(512);' in the .data section appears to ensure that, but for some
>> reason, that appears not to be working.
>
> Actually, the existing .pad section is totally and utterly bogus when
> EFI is enabled:
>
> . = ALIGN(4);
> _etext = .;
>
> .got.plt : { *(.got.plt) }
> _got_start = .;
> .got : { *(.got) }
> _got_end = .;
>
> The .got.plt and .got are always word-based. This is then followed by
> .pad, which does nothing but pad out to a multiple of 64 bit:
>
> /* ensure the zImage file size is always a multiple of 64 bits */
> /* (without a dummy byte, ld just ignores the empty section) */
> .pad : { BYTE(0); . = ALIGN(8); }
>
> So this may add zero or 4 bytes of padding.
>
> This is then followed by the EFI data:
>
> .data : ALIGN(4096) {
> ...
> . = ALIGN(512);
> }
>
> which is aligned to 4K but aligns the end of itself to 512.
>
> So, we have the end of .got aligned to 4, followed by .pad that tries to
> align to 8, followed by an optional .data section. This is pointless.
>
> A sane patch would be to choose between the EFI .data section and the
> .pad section. So, it should be:
>
> #ifdef CONFIG_EFI_STUB
> .data : ALIGN(4096) {
> ...
> . = ALIGN(512);
> }
> #else
> .pad : { BYTE(0); . = ALIGN(8); }
> #endif
>
Agreed, the .pad section has no point for EFI_STUB=y. However, it
seems this symptom is caused by the same issues I am trying to address
here
https://marc.info/?l=linux-arm-kernel&m=150488477807353
which is that we have __ksymtab_xxx sections that we should discard,
because the linker will otherwise emit them /after/ .data or .pad.
This is caused by the use of lib/sort.c in the EFI stub, which
contains an EXPORT_SYMBOL().
Would you perhaps prefer that I clone sort.c into its own .c file
specifically for the EFI stub? (under drivers/firmware/efi/libstub)
That should get rid of these spurious sections and thus the
misalignments and/or movements that are causing all of these issues.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-22 13:01 ` Ard Biesheuvel
@ 2017-10-23 3:26 ` jeffy
-1 siblings, 0 replies; 42+ messages in thread
From: jeffy @ 2017-10-23 3:26 UTC (permalink / raw)
To: Ard Biesheuvel, Russell King - ARM Linux
Cc: linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
Hi Ard,
On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> On 22 October 2017 at 13:47, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
>>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>>> The zImage file size should be aligned.
>>>>
>>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
>>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>>> ---
>>>>
>>>> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>>>> index b38dcef90756..1636fa259577 100644
>>>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>>>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>>>> @@ -70,10 +70,6 @@ SECTIONS
>>>> .got : { *(.got) }
>>>> _got_end = .;
>>>>
>>>> - /* ensure the zImage file size is always a multiple of 64 bits */
>>>> - /* (without a dummy byte, ld just ignores the empty section) */
>>>> - .pad : { BYTE(0); . = ALIGN(8); }
>>>> -
>>>> #ifdef CONFIG_EFI_STUB
>>>> .data : ALIGN(4096) {
>>>> __pecoff_data_start = .;
>>>> @@ -93,6 +89,10 @@ SECTIONS
>>>> __pecoff_data_rawsize = . - ADDR(.data);
>>>> #endif
>>>>
>>>> + /* ensure the zImage file size is always a multiple of 64 bits */
>>>> + /* (without a dummy byte, ld just ignores the empty section) */
>>>> + .pad : { BYTE(0); . = ALIGN(8); }
>>>> +
>>>> _edata = .;
>>>>
>>>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>>>> --
>>>> 2.11.0
>>>>
>>>
>>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
>>> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
>>> ALIGN(512);' in the .data section appears to ensure that, but for some
>>> reason, that appears not to be working.
>>
>> Actually, the existing .pad section is totally and utterly bogus when
>> EFI is enabled:
>>
>> . = ALIGN(4);
>> _etext = .;
>>
>> .got.plt : { *(.got.plt) }
>> _got_start = .;
>> .got : { *(.got) }
>> _got_end = .;
>>
>> The .got.plt and .got are always word-based. This is then followed by
>> .pad, which does nothing but pad out to a multiple of 64 bit:
>>
>> /* ensure the zImage file size is always a multiple of 64 bits */
>> /* (without a dummy byte, ld just ignores the empty section) */
>> .pad : { BYTE(0); . = ALIGN(8); }
>>
>> So this may add zero or 4 bytes of padding.
>>
>> This is then followed by the EFI data:
>>
>> .data : ALIGN(4096) {
>> ...
>> . = ALIGN(512);
>> }
>>
>> which is aligned to 4K but aligns the end of itself to 512.
>>
>> So, we have the end of .got aligned to 4, followed by .pad that tries to
>> align to 8, followed by an optional .data section. This is pointless.
>>
>> A sane patch would be to choose between the EFI .data section and the
>> .pad section. So, it should be:
>>
>> #ifdef CONFIG_EFI_STUB
>> .data : ALIGN(4096) {
>> ...
>> . = ALIGN(512);
>> }
>> #else
>> .pad : { BYTE(0); . = ALIGN(8); }
>> #endif
>>
>
> Agreed, the .pad section has no point for EFI_STUB=y. However, it
> seems this symptom is caused by the same issues I am trying to address
> here
>
> https://marc.info/?l=linux-arm-kernel&m=150488477807353
>
> which is that we have __ksymtab_xxx sections that we should discard,
> because the linker will otherwise emit them /after/ .data or .pad.
> This is caused by the use of lib/sort.c in the EFI stub, which
> contains an EXPORT_SYMBOL().
hmm, right, didn't notice the data is already aligned...
so it's indeed caused by the ksym:
[ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA
0 0 4096
[10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA
0 0 4
[11] .bss NOBITS 006ce208 6d6208 00001c 00 WA
0 0 4
and both of your old([PATCH] ARM: compressed: discard ksym/kcrctab input
section) and new([PATCH] efi/libstub: arm: omit sorting of the UEFI
memory map) patches fix the issue i meet, thanks:)
>
> Would you perhaps prefer that I clone sort.c into its own .c file
> specifically for the EFI stub? (under drivers/firmware/efi/libstub)
> That should get rid of these spurious sections and thus the
> misalignments and/or movements that are causing all of these issues.
>
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-23 3:26 ` jeffy
0 siblings, 0 replies; 42+ messages in thread
From: jeffy @ 2017-10-23 3:26 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ard,
On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> On 22 October 2017 at 13:47, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
>>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>>> The zImage file size should be aligned.
>>>>
>>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
>>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>>> ---
>>>>
>>>> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>>>> index b38dcef90756..1636fa259577 100644
>>>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>>>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>>>> @@ -70,10 +70,6 @@ SECTIONS
>>>> .got : { *(.got) }
>>>> _got_end = .;
>>>>
>>>> - /* ensure the zImage file size is always a multiple of 64 bits */
>>>> - /* (without a dummy byte, ld just ignores the empty section) */
>>>> - .pad : { BYTE(0); . = ALIGN(8); }
>>>> -
>>>> #ifdef CONFIG_EFI_STUB
>>>> .data : ALIGN(4096) {
>>>> __pecoff_data_start = .;
>>>> @@ -93,6 +89,10 @@ SECTIONS
>>>> __pecoff_data_rawsize = . - ADDR(.data);
>>>> #endif
>>>>
>>>> + /* ensure the zImage file size is always a multiple of 64 bits */
>>>> + /* (without a dummy byte, ld just ignores the empty section) */
>>>> + .pad : { BYTE(0); . = ALIGN(8); }
>>>> +
>>>> _edata = .;
>>>>
>>>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>>>> --
>>>> 2.11.0
>>>>
>>>
>>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
>>> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
>>> ALIGN(512);' in the .data section appears to ensure that, but for some
>>> reason, that appears not to be working.
>>
>> Actually, the existing .pad section is totally and utterly bogus when
>> EFI is enabled:
>>
>> . = ALIGN(4);
>> _etext = .;
>>
>> .got.plt : { *(.got.plt) }
>> _got_start = .;
>> .got : { *(.got) }
>> _got_end = .;
>>
>> The .got.plt and .got are always word-based. This is then followed by
>> .pad, which does nothing but pad out to a multiple of 64 bit:
>>
>> /* ensure the zImage file size is always a multiple of 64 bits */
>> /* (without a dummy byte, ld just ignores the empty section) */
>> .pad : { BYTE(0); . = ALIGN(8); }
>>
>> So this may add zero or 4 bytes of padding.
>>
>> This is then followed by the EFI data:
>>
>> .data : ALIGN(4096) {
>> ...
>> . = ALIGN(512);
>> }
>>
>> which is aligned to 4K but aligns the end of itself to 512.
>>
>> So, we have the end of .got aligned to 4, followed by .pad that tries to
>> align to 8, followed by an optional .data section. This is pointless.
>>
>> A sane patch would be to choose between the EFI .data section and the
>> .pad section. So, it should be:
>>
>> #ifdef CONFIG_EFI_STUB
>> .data : ALIGN(4096) {
>> ...
>> . = ALIGN(512);
>> }
>> #else
>> .pad : { BYTE(0); . = ALIGN(8); }
>> #endif
>>
>
> Agreed, the .pad section has no point for EFI_STUB=y. However, it
> seems this symptom is caused by the same issues I am trying to address
> here
>
> https://marc.info/?l=linux-arm-kernel&m=150488477807353
>
> which is that we have __ksymtab_xxx sections that we should discard,
> because the linker will otherwise emit them /after/ .data or .pad.
> This is caused by the use of lib/sort.c in the EFI stub, which
> contains an EXPORT_SYMBOL().
hmm, right, didn't notice the data is already aligned...
so it's indeed caused by the ksym:
[ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA
0 0 4096
[10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA
0 0 4
[11] .bss NOBITS 006ce208 6d6208 00001c 00 WA
0 0 4
and both of your old([PATCH] ARM: compressed: discard ksym/kcrctab input
section) and new([PATCH] efi/libstub: arm: omit sorting of the UEFI
memory map) patches fix the issue i meet, thanks:)
>
> Would you perhaps prefer that I clone sort.c into its own .c file
> specifically for the EFI stub? (under drivers/firmware/efi/libstub)
> That should get rid of these spurious sections and thus the
> misalignments and/or movements that are causing all of these issues.
>
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-23 3:26 ` jeffy
@ 2017-10-23 8:50 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-23 8:50 UTC (permalink / raw)
To: jeffy
Cc: Ard Biesheuvel, linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
On Mon, Oct 23, 2017 at 11:26:49AM +0800, jeffy wrote:
> Hi Ard,
>
> On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> >On 22 October 2017 at 13:47, Russell King - ARM Linux
> ><linux@armlinux.org.uk> wrote:
> >>On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
> >>>On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> >>>>The zImage file size should be aligned.
> >>>>
> >>>>Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> >>>>Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> >>>>---
> >>>>
> >>>> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> >>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>index b38dcef90756..1636fa259577 100644
> >>>>--- a/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>+++ b/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>@@ -70,10 +70,6 @@ SECTIONS
> >>>> .got : { *(.got) }
> >>>> _got_end = .;
> >>>>
> >>>>- /* ensure the zImage file size is always a multiple of 64 bits */
> >>>>- /* (without a dummy byte, ld just ignores the empty section) */
> >>>>- .pad : { BYTE(0); . = ALIGN(8); }
> >>>>-
> >>>> #ifdef CONFIG_EFI_STUB
> >>>> .data : ALIGN(4096) {
> >>>> __pecoff_data_start = .;
> >>>>@@ -93,6 +89,10 @@ SECTIONS
> >>>> __pecoff_data_rawsize = . - ADDR(.data);
> >>>> #endif
> >>>>
> >>>>+ /* ensure the zImage file size is always a multiple of 64 bits */
> >>>>+ /* (without a dummy byte, ld just ignores the empty section) */
> >>>>+ .pad : { BYTE(0); . = ALIGN(8); }
> >>>>+
> >>>> _edata = .;
> >>>>
> >>>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> >>>>--
> >>>>2.11.0
> >>>>
> >>>
> >>>This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
> >>>filesize should be rounded up to 512 bytes not 8 bytes. The '. =
> >>>ALIGN(512);' in the .data section appears to ensure that, but for some
> >>>reason, that appears not to be working.
> >>
> >>Actually, the existing .pad section is totally and utterly bogus when
> >>EFI is enabled:
> >>
> >> . = ALIGN(4);
> >> _etext = .;
> >>
> >> .got.plt : { *(.got.plt) }
> >> _got_start = .;
> >> .got : { *(.got) }
> >> _got_end = .;
> >>
> >>The .got.plt and .got are always word-based. This is then followed by
> >>.pad, which does nothing but pad out to a multiple of 64 bit:
> >>
> >> /* ensure the zImage file size is always a multiple of 64 bits */
> >> /* (without a dummy byte, ld just ignores the empty section) */
> >> .pad : { BYTE(0); . = ALIGN(8); }
> >>
> >>So this may add zero or 4 bytes of padding.
> >>
> >>This is then followed by the EFI data:
> >>
> >> .data : ALIGN(4096) {
> >> ...
> >> . = ALIGN(512);
> >> }
> >>
> >>which is aligned to 4K but aligns the end of itself to 512.
> >>
> >>So, we have the end of .got aligned to 4, followed by .pad that tries to
> >>align to 8, followed by an optional .data section. This is pointless.
> >>
> >>A sane patch would be to choose between the EFI .data section and the
> >>.pad section. So, it should be:
> >>
> >>#ifdef CONFIG_EFI_STUB
> >> .data : ALIGN(4096) {
> >> ...
> >> . = ALIGN(512);
> >> }
> >>#else
> >> .pad : { BYTE(0); . = ALIGN(8); }
> >>#endif
> >>
> >
> >Agreed, the .pad section has no point for EFI_STUB=y. However, it
> >seems this symptom is caused by the same issues I am trying to address
> >here
> >
> >https://marc.info/?l=linux-arm-kernel&m=150488477807353
> >
> >which is that we have __ksymtab_xxx sections that we should discard,
> >because the linker will otherwise emit them /after/ .data or .pad.
> >This is caused by the use of lib/sort.c in the EFI stub, which
> >contains an EXPORT_SYMBOL().
>
> hmm, right, didn't notice the data is already aligned...
> so it's indeed caused by the ksym:
>
> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
> 0 4096
> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
> 0 4
> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
> 0 4
It's earlier - look for __ksymtab_strings.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-23 8:50 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-23 8:50 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 23, 2017 at 11:26:49AM +0800, jeffy wrote:
> Hi Ard,
>
> On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> >On 22 October 2017 at 13:47, Russell King - ARM Linux
> ><linux@armlinux.org.uk> wrote:
> >>On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
> >>>On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> >>>>The zImage file size should be aligned.
> >>>>
> >>>>Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> >>>>Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> >>>>---
> >>>>
> >>>> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> >>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>index b38dcef90756..1636fa259577 100644
> >>>>--- a/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>+++ b/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>@@ -70,10 +70,6 @@ SECTIONS
> >>>> .got : { *(.got) }
> >>>> _got_end = .;
> >>>>
> >>>>- /* ensure the zImage file size is always a multiple of 64 bits */
> >>>>- /* (without a dummy byte, ld just ignores the empty section) */
> >>>>- .pad : { BYTE(0); . = ALIGN(8); }
> >>>>-
> >>>> #ifdef CONFIG_EFI_STUB
> >>>> .data : ALIGN(4096) {
> >>>> __pecoff_data_start = .;
> >>>>@@ -93,6 +89,10 @@ SECTIONS
> >>>> __pecoff_data_rawsize = . - ADDR(.data);
> >>>> #endif
> >>>>
> >>>>+ /* ensure the zImage file size is always a multiple of 64 bits */
> >>>>+ /* (without a dummy byte, ld just ignores the empty section) */
> >>>>+ .pad : { BYTE(0); . = ALIGN(8); }
> >>>>+
> >>>> _edata = .;
> >>>>
> >>>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> >>>>--
> >>>>2.11.0
> >>>>
> >>>
> >>>This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
> >>>filesize should be rounded up to 512 bytes not 8 bytes. The '. =
> >>>ALIGN(512);' in the .data section appears to ensure that, but for some
> >>>reason, that appears not to be working.
> >>
> >>Actually, the existing .pad section is totally and utterly bogus when
> >>EFI is enabled:
> >>
> >> . = ALIGN(4);
> >> _etext = .;
> >>
> >> .got.plt : { *(.got.plt) }
> >> _got_start = .;
> >> .got : { *(.got) }
> >> _got_end = .;
> >>
> >>The .got.plt and .got are always word-based. This is then followed by
> >>.pad, which does nothing but pad out to a multiple of 64 bit:
> >>
> >> /* ensure the zImage file size is always a multiple of 64 bits */
> >> /* (without a dummy byte, ld just ignores the empty section) */
> >> .pad : { BYTE(0); . = ALIGN(8); }
> >>
> >>So this may add zero or 4 bytes of padding.
> >>
> >>This is then followed by the EFI data:
> >>
> >> .data : ALIGN(4096) {
> >> ...
> >> . = ALIGN(512);
> >> }
> >>
> >>which is aligned to 4K but aligns the end of itself to 512.
> >>
> >>So, we have the end of .got aligned to 4, followed by .pad that tries to
> >>align to 8, followed by an optional .data section. This is pointless.
> >>
> >>A sane patch would be to choose between the EFI .data section and the
> >>.pad section. So, it should be:
> >>
> >>#ifdef CONFIG_EFI_STUB
> >> .data : ALIGN(4096) {
> >> ...
> >> . = ALIGN(512);
> >> }
> >>#else
> >> .pad : { BYTE(0); . = ALIGN(8); }
> >>#endif
> >>
> >
> >Agreed, the .pad section has no point for EFI_STUB=y. However, it
> >seems this symptom is caused by the same issues I am trying to address
> >here
> >
> >https://marc.info/?l=linux-arm-kernel&m=150488477807353
> >
> >which is that we have __ksymtab_xxx sections that we should discard,
> >because the linker will otherwise emit them /after/ .data or .pad.
> >This is caused by the use of lib/sort.c in the EFI stub, which
> >contains an EXPORT_SYMBOL().
>
> hmm, right, didn't notice the data is already aligned...
> so it's indeed caused by the ksym:
>
> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
> 0 4096
> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
> 0 4
> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
> 0 4
It's earlier - look for __ksymtab_strings.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-23 8:50 ` Russell King - ARM Linux
@ 2017-10-23 10:24 ` jeffy
-1 siblings, 0 replies; 42+ messages in thread
From: jeffy @ 2017-10-23 10:24 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Ard Biesheuvel, linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
Hi Russell,
Thanks for your reply.
On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
>> >
>> >hmm, right, didn't notice the data is already aligned...
>> >so it's indeed caused by the ksym:
>> >
>> > [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
>> >0 4096
>> > [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
>> >0 4
>> > [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
>> >0 4
> It's earlier - look for __ksymtab_strings.
the problem i meet is the appended dtb code found dtb invalid. i thought
that is because of unaligned zImage size, but i was wrong...
it looks like the size is still aligned, and after add more logs, it
seems the problem is due to _edata not matched the real file size, which
is because of the unexpected ___ksymtab+sort:
currently:
zImage size is 6d6208:
-rwxr-xr-x 1 root root 7135752 Oct 23 18:12 zImage
_edata is 006ce200:
006ce200 0 NOTYPE GLOBAL DEFAULT 9 _edata
vmlinux sections:
Section Headers:
[Nr] Name Type Addr Off Size ES Flg
Lk Inf Al
[ 0] NULL 00000000 000000 000000 00
0 0 0
[ 1] .text PROGBITS 00000000 008000 00b7a0 00 AX
0 0 4096
[ 2] .table PROGBITS 0000b7a0 0137a0 000014 00 WA
0 0 4
[ 3] .rodata PROGBITS 0000b7b4 0137b4 0015ef 00 A
0 0 2
[ 4] __ksymtab_strings PROGBITS 0000cda3 014da3 000005 00 A
0 0 1
[ 5] .piggydata PROGBITS 0000cda8 014da8 6c026f 00 A
0 0 1
[ 6] .got.plt PROGBITS 006cd018 6d5018 00000c 04 WA
0 0 4
[ 7] .got PROGBITS 006cd024 6d5024 000028 00 WA
0 0 4
[ 8] .pad PROGBITS 006cd04c 6d504c 000004 00 WA
0 0 1
[ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA
0 0 4096
[10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA
0 0 4
[11] .bss NOBITS 006ce208 6d6208 00001c 00 WA
0 0 4
and it turns out moving around .pad section only hide the problem by
placing the .pad after the ___ksymtab+sort:
Section Headers:
[Nr] Name Type Addr Off Size ES Flg
Lk Inf Al
[ 0] NULL 00000000 000000 000000 00
0 0 0
[ 1] .text PROGBITS 00000000 008000 00b7a0 00 AX
0 0 4096
[ 2] .table PROGBITS 0000b7a0 0137a0 000014 00 WA
0 0 4
[ 3] .rodata PROGBITS 0000b7b4 0137b4 0015ef 00 A
0 0 2
[ 4] __ksymtab_strings PROGBITS 0000cda3 014da3 000005 00 A
0 0 1
[ 5] .piggydata PROGBITS 0000cda8 014da8 6c026f 00 A
0 0 1
[ 6] .got.plt PROGBITS 006cd018 6d5018 00000c 04 WA
0 0 4
[ 7] .got PROGBITS 006cd024 6d5024 000028 00 WA
0 0 4
[ 8] .data PROGBITS 006ce000 6d6000 000200 00 WA
0 0 4096
[ 9] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA
0 0 4
[10] .pad PROGBITS 006ce208 6d6208 000008 00 WA
0 0 1
[11] .bss NOBITS 006ce210 6d6210 00001c 00 WA
0 0 4
-rwxr-xr-x 1 root root 7135760 Oct 23 18:09 zImage
006ce210 0 NOTYPE GLOBAL DEFAULT 10 _edata
and i think Ard's new patch could be the right way to fix it :)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-23 10:24 ` jeffy
0 siblings, 0 replies; 42+ messages in thread
From: jeffy @ 2017-10-23 10:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi Russell,
Thanks for your reply.
On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
>> >
>> >hmm, right, didn't notice the data is already aligned...
>> >so it's indeed caused by the ksym:
>> >
>> > [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
>> >0 4096
>> > [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
>> >0 4
>> > [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
>> >0 4
> It's earlier - look for __ksymtab_strings.
the problem i meet is the appended dtb code found dtb invalid. i thought
that is because of unaligned zImage size, but i was wrong...
it looks like the size is still aligned, and after add more logs, it
seems the problem is due to _edata not matched the real file size, which
is because of the unexpected ___ksymtab+sort:
currently:
zImage size is 6d6208:
-rwxr-xr-x 1 root root 7135752 Oct 23 18:12 zImage
_edata is 006ce200:
006ce200 0 NOTYPE GLOBAL DEFAULT 9 _edata
vmlinux sections:
Section Headers:
[Nr] Name Type Addr Off Size ES Flg
Lk Inf Al
[ 0] NULL 00000000 000000 000000 00
0 0 0
[ 1] .text PROGBITS 00000000 008000 00b7a0 00 AX
0 0 4096
[ 2] .table PROGBITS 0000b7a0 0137a0 000014 00 WA
0 0 4
[ 3] .rodata PROGBITS 0000b7b4 0137b4 0015ef 00 A
0 0 2
[ 4] __ksymtab_strings PROGBITS 0000cda3 014da3 000005 00 A
0 0 1
[ 5] .piggydata PROGBITS 0000cda8 014da8 6c026f 00 A
0 0 1
[ 6] .got.plt PROGBITS 006cd018 6d5018 00000c 04 WA
0 0 4
[ 7] .got PROGBITS 006cd024 6d5024 000028 00 WA
0 0 4
[ 8] .pad PROGBITS 006cd04c 6d504c 000004 00 WA
0 0 1
[ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA
0 0 4096
[10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA
0 0 4
[11] .bss NOBITS 006ce208 6d6208 00001c 00 WA
0 0 4
and it turns out moving around .pad section only hide the problem by
placing the .pad after the ___ksymtab+sort:
Section Headers:
[Nr] Name Type Addr Off Size ES Flg
Lk Inf Al
[ 0] NULL 00000000 000000 000000 00
0 0 0
[ 1] .text PROGBITS 00000000 008000 00b7a0 00 AX
0 0 4096
[ 2] .table PROGBITS 0000b7a0 0137a0 000014 00 WA
0 0 4
[ 3] .rodata PROGBITS 0000b7b4 0137b4 0015ef 00 A
0 0 2
[ 4] __ksymtab_strings PROGBITS 0000cda3 014da3 000005 00 A
0 0 1
[ 5] .piggydata PROGBITS 0000cda8 014da8 6c026f 00 A
0 0 1
[ 6] .got.plt PROGBITS 006cd018 6d5018 00000c 04 WA
0 0 4
[ 7] .got PROGBITS 006cd024 6d5024 000028 00 WA
0 0 4
[ 8] .data PROGBITS 006ce000 6d6000 000200 00 WA
0 0 4096
[ 9] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA
0 0 4
[10] .pad PROGBITS 006ce208 6d6208 000008 00 WA
0 0 1
[11] .bss NOBITS 006ce210 6d6210 00001c 00 WA
0 0 4
-rwxr-xr-x 1 root root 7135760 Oct 23 18:09 zImage
006ce210 0 NOTYPE GLOBAL DEFAULT 10 _edata
and i think Ard's new patch could be the right way to fix it :)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-23 10:24 ` jeffy
@ 2017-10-23 10:50 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-23 10:50 UTC (permalink / raw)
To: jeffy
Cc: Ard Biesheuvel, linux-kernel, chris.zhong, Ingo Molnar, linux-arm-kernel
On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
> Hi Russell,
>
> Thanks for your reply.
>
> On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
> >>>
> >>>hmm, right, didn't notice the data is already aligned...
> >>>so it's indeed caused by the ksym:
> >>>
> >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
> >>>0 4096
> >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
> >>>0 4
> >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
> >>>0 4
> >It's earlier - look for __ksymtab_strings.
>
> the problem i meet is the appended dtb code found dtb invalid. i thought
> that is because of unaligned zImage size, but i was wrong...
Hmm, you really ought not to be using the appended dtb code for modern
systems - the appended dtb system is there for old boot loaders that
are incapable of dealing with a dtb. As is said in the option's help
text:
This is meant as a backward compatibility convenience for those
systems with a bootloader that can't be upgraded to accommodate
the documented boot protocol using a device tree.
Beware that there is very little in terms of protection against
this option being confused by leftover garbage in memory that might
look like a DTB header after a reboot if no actual DTB is appended
to zImage. Do not leave this option active in a production kernel
if you don't intend to always append a DTB. Proper passing of the
location into r2 of a bootloader provided DTB is always preferable
to this option.
If you rely on it, and you have something that looks like a dtb after
the image, then things will go wrong, so it's better _not_ to use it
and to keep it disabled.
That aside, thanks for doing a more in-depth analysis of what is going
on, which helps to understand /why/ Ard's fix works (whereas before
it was rather nebulous.)
I wonder whether we ought to tell the linker to discard any unknown
sections by adding at the bottom:
/DISCARD/ { *(*) }
but I do think we need to document this, specifically that _edata must
point to the first byte after the binary file, and that the only
sections after it are allowed to be the .bss and stack sections.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-23 10:50 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-23 10:50 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
> Hi Russell,
>
> Thanks for your reply.
>
> On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
> >>>
> >>>hmm, right, didn't notice the data is already aligned...
> >>>so it's indeed caused by the ksym:
> >>>
> >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
> >>>0 4096
> >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
> >>>0 4
> >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
> >>>0 4
> >It's earlier - look for __ksymtab_strings.
>
> the problem i meet is the appended dtb code found dtb invalid. i thought
> that is because of unaligned zImage size, but i was wrong...
Hmm, you really ought not to be using the appended dtb code for modern
systems - the appended dtb system is there for old boot loaders that
are incapable of dealing with a dtb. As is said in the option's help
text:
This is meant as a backward compatibility convenience for those
systems with a bootloader that can't be upgraded to accommodate
the documented boot protocol using a device tree.
Beware that there is very little in terms of protection against
this option being confused by leftover garbage in memory that might
look like a DTB header after a reboot if no actual DTB is appended
to zImage. Do not leave this option active in a production kernel
if you don't intend to always append a DTB. Proper passing of the
location into r2 of a bootloader provided DTB is always preferable
to this option.
If you rely on it, and you have something that looks like a dtb after
the image, then things will go wrong, so it's better _not_ to use it
and to keep it disabled.
That aside, thanks for doing a more in-depth analysis of what is going
on, which helps to understand /why/ Ard's fix works (whereas before
it was rather nebulous.)
I wonder whether we ought to tell the linker to discard any unknown
sections by adding at the bottom:
/DISCARD/ { *(*) }
but I do think we need to document this, specifically that _edata must
point to the first byte after the binary file, and that the only
sections after it are allowed to be the .bss and stack sections.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-23 10:50 ` Russell King - ARM Linux
@ 2017-10-23 11:45 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-23 11:45 UTC (permalink / raw)
To: jeffy
Cc: Ingo Molnar, chris.zhong, linux-kernel, linux-arm-kernel, Ard Biesheuvel
On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
> > Hi Russell,
> >
> > Thanks for your reply.
> >
> > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
> > >>>
> > >>>hmm, right, didn't notice the data is already aligned...
> > >>>so it's indeed caused by the ksym:
> > >>>
> > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
> > >>>0 4096
> > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
> > >>>0 4
> > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
> > >>>0 4
> > >It's earlier - look for __ksymtab_strings.
> >
> > the problem i meet is the appended dtb code found dtb invalid. i thought
> > that is because of unaligned zImage size, but i was wrong...
>
> Hmm, you really ought not to be using the appended dtb code for modern
> systems - the appended dtb system is there for old boot loaders that
> are incapable of dealing with a dtb. As is said in the option's help
> text:
>
> This is meant as a backward compatibility convenience for those
> systems with a bootloader that can't be upgraded to accommodate
> the documented boot protocol using a device tree.
>
> Beware that there is very little in terms of protection against
> this option being confused by leftover garbage in memory that might
> look like a DTB header after a reboot if no actual DTB is appended
> to zImage. Do not leave this option active in a production kernel
> if you don't intend to always append a DTB. Proper passing of the
> location into r2 of a bootloader provided DTB is always preferable
> to this option.
>
> If you rely on it, and you have something that looks like a dtb after
> the image, then things will go wrong, so it's better _not_ to use it
> and to keep it disabled.
>
> That aside, thanks for doing a more in-depth analysis of what is going
> on, which helps to understand /why/ Ard's fix works (whereas before
> it was rather nebulous.)
>
> I wonder whether we ought to tell the linker to discard any unknown
> sections by adding at the bottom:
>
> /DISCARD/ { *(*) }
>
> but I do think we need to document this, specifically that _edata must
> point to the first byte after the binary file, and that the only
> sections after it are allowed to be the .bss and stack sections.
Short of adding the discard (which I think is itself risky, we've had
problems in the main kernel's vmlinux.lds in this area), I think we
ought to verify the size of the zImage file, so that the build fails
when we generate a zImage which is wrong, rather than producing a
zImage that is incorrect. Maybe something like this?
8<=====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] ARM: verify size of zImage
The linker can sometimes add additional sections to the zImage ELF file
which results in the zImage binary being larger than expected. This
causes appended DT blobs to fail.
Verify that the zImage binary is the expected size, and fail the build
if this is not the case.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
arch/arm/boot/Makefile | 6 +++++-
arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
create mode 100755 arch/arm/boot/verify_zimage.sh
diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
index a3af4dc08c3e..1290875556ae 100644
--- a/arch/arm/boot/Makefile
+++ b/arch/arm/boot/Makefile
@@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE
else
+quiet_cmd_mkzimage = ZIMAGE $@
+cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \
+ '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }'
+
$(obj)/xipImage: FORCE
@echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)'
@false
@@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE
$(Q)$(MAKE) $(build)=$(obj)/compressed $@
$(obj)/zImage: $(obj)/compressed/vmlinux FORCE
- $(call if_changed,objcopy)
+ $(call if_changed,mkzimage)
endif
diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh
new file mode 100755
index 000000000000..922a93e61aa7
--- /dev/null
+++ b/arch/arm/boot/verify_zimage.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+set -e
+
+vmlinuz="$1"
+zimage="$2"
+nm="$3"
+
+magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) {
+ $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/;
+ $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/;
+}; printf "%d\n", $magic_end - $magic_start;')
+
+zimage_size=$(stat -c '%s' "$zimage")
+
+# Verify that the resulting binary matches the size contained within
+# the binary (iow, the linker has not added any additional sections.)
+if [ $magic_size -ne $zimage_size ]; then
+ echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2
+ exit 1
+fi
+exit 0
--
2.7.4
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-23 11:45 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-23 11:45 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
> > Hi Russell,
> >
> > Thanks for your reply.
> >
> > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
> > >>>
> > >>>hmm, right, didn't notice the data is already aligned...
> > >>>so it's indeed caused by the ksym:
> > >>>
> > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
> > >>>0 4096
> > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
> > >>>0 4
> > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
> > >>>0 4
> > >It's earlier - look for __ksymtab_strings.
> >
> > the problem i meet is the appended dtb code found dtb invalid. i thought
> > that is because of unaligned zImage size, but i was wrong...
>
> Hmm, you really ought not to be using the appended dtb code for modern
> systems - the appended dtb system is there for old boot loaders that
> are incapable of dealing with a dtb. As is said in the option's help
> text:
>
> This is meant as a backward compatibility convenience for those
> systems with a bootloader that can't be upgraded to accommodate
> the documented boot protocol using a device tree.
>
> Beware that there is very little in terms of protection against
> this option being confused by leftover garbage in memory that might
> look like a DTB header after a reboot if no actual DTB is appended
> to zImage. Do not leave this option active in a production kernel
> if you don't intend to always append a DTB. Proper passing of the
> location into r2 of a bootloader provided DTB is always preferable
> to this option.
>
> If you rely on it, and you have something that looks like a dtb after
> the image, then things will go wrong, so it's better _not_ to use it
> and to keep it disabled.
>
> That aside, thanks for doing a more in-depth analysis of what is going
> on, which helps to understand /why/ Ard's fix works (whereas before
> it was rather nebulous.)
>
> I wonder whether we ought to tell the linker to discard any unknown
> sections by adding at the bottom:
>
> /DISCARD/ { *(*) }
>
> but I do think we need to document this, specifically that _edata must
> point to the first byte after the binary file, and that the only
> sections after it are allowed to be the .bss and stack sections.
Short of adding the discard (which I think is itself risky, we've had
problems in the main kernel's vmlinux.lds in this area), I think we
ought to verify the size of the zImage file, so that the build fails
when we generate a zImage which is wrong, rather than producing a
zImage that is incorrect. Maybe something like this?
8<=====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] ARM: verify size of zImage
The linker can sometimes add additional sections to the zImage ELF file
which results in the zImage binary being larger than expected. This
causes appended DT blobs to fail.
Verify that the zImage binary is the expected size, and fail the build
if this is not the case.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
arch/arm/boot/Makefile | 6 +++++-
arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
create mode 100755 arch/arm/boot/verify_zimage.sh
diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
index a3af4dc08c3e..1290875556ae 100644
--- a/arch/arm/boot/Makefile
+++ b/arch/arm/boot/Makefile
@@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE
else
+quiet_cmd_mkzimage = ZIMAGE $@
+cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \
+ '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }'
+
$(obj)/xipImage: FORCE
@echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)'
@false
@@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE
$(Q)$(MAKE) $(build)=$(obj)/compressed $@
$(obj)/zImage: $(obj)/compressed/vmlinux FORCE
- $(call if_changed,objcopy)
+ $(call if_changed,mkzimage)
endif
diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh
new file mode 100755
index 000000000000..922a93e61aa7
--- /dev/null
+++ b/arch/arm/boot/verify_zimage.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+set -e
+
+vmlinuz="$1"
+zimage="$2"
+nm="$3"
+
+magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) {
+ $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/;
+ $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/;
+}; printf "%d\n", $magic_end - $magic_start;')
+
+zimage_size=$(stat -c '%s' "$zimage")
+
+# Verify that the resulting binary matches the size contained within
+# the binary (iow, the linker has not added any additional sections.)
+if [ $magic_size -ne $zimage_size ]; then
+ echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2
+ exit 1
+fi
+exit 0
--
2.7.4
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-23 11:45 ` Russell King - ARM Linux
@ 2017-10-24 8:09 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 8:09 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: jeffy, Ingo Molnar, Chris Zhong, linux-kernel, linux-arm-kernel
On 23 October 2017 at 12:45, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote:
>> On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
>> > Hi Russell,
>> >
>> > Thanks for your reply.
>> >
>> > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
>> > >>>
>> > >>>hmm, right, didn't notice the data is already aligned...
>> > >>>so it's indeed caused by the ksym:
>> > >>>
>> > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
>> > >>>0 4096
>> > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
>> > >>>0 4
>> > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
>> > >>>0 4
>> > >It's earlier - look for __ksymtab_strings.
>> >
>> > the problem i meet is the appended dtb code found dtb invalid. i thought
>> > that is because of unaligned zImage size, but i was wrong...
>>
>> Hmm, you really ought not to be using the appended dtb code for modern
>> systems - the appended dtb system is there for old boot loaders that
>> are incapable of dealing with a dtb. As is said in the option's help
>> text:
>>
>> This is meant as a backward compatibility convenience for those
>> systems with a bootloader that can't be upgraded to accommodate
>> the documented boot protocol using a device tree.
>>
>> Beware that there is very little in terms of protection against
>> this option being confused by leftover garbage in memory that might
>> look like a DTB header after a reboot if no actual DTB is appended
>> to zImage. Do not leave this option active in a production kernel
>> if you don't intend to always append a DTB. Proper passing of the
>> location into r2 of a bootloader provided DTB is always preferable
>> to this option.
>>
>> If you rely on it, and you have something that looks like a dtb after
>> the image, then things will go wrong, so it's better _not_ to use it
>> and to keep it disabled.
>>
>> That aside, thanks for doing a more in-depth analysis of what is going
>> on, which helps to understand /why/ Ard's fix works (whereas before
>> it was rather nebulous.)
>>
>> I wonder whether we ought to tell the linker to discard any unknown
>> sections by adding at the bottom:
>>
>> /DISCARD/ { *(*) }
>>
>> but I do think we need to document this, specifically that _edata must
>> point to the first byte after the binary file, and that the only
>> sections after it are allowed to be the .bss and stack sections.
>
> Short of adding the discard (which I think is itself risky, we've had
> problems in the main kernel's vmlinux.lds in this area), I think we
> ought to verify the size of the zImage file, so that the build fails
> when we generate a zImage which is wrong, rather than producing a
> zImage that is incorrect. Maybe something like this?
>
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] ARM: verify size of zImage
>
> The linker can sometimes add additional sections to the zImage ELF file
> which results in the zImage binary being larger than expected. This
> causes appended DT blobs to fail.
>
> Verify that the zImage binary is the expected size, and fail the build
> if this is not the case.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> arch/arm/boot/Makefile | 6 +++++-
> arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++
> 2 files changed, 26 insertions(+), 1 deletion(-)
> create mode 100755 arch/arm/boot/verify_zimage.sh
>
> diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
> index a3af4dc08c3e..1290875556ae 100644
> --- a/arch/arm/boot/Makefile
> +++ b/arch/arm/boot/Makefile
> @@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE
>
> else
>
> +quiet_cmd_mkzimage = ZIMAGE $@
> +cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \
> + '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }'
> +
> $(obj)/xipImage: FORCE
> @echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)'
> @false
> @@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE
> $(Q)$(MAKE) $(build)=$(obj)/compressed $@
>
> $(obj)/zImage: $(obj)/compressed/vmlinux FORCE
> - $(call if_changed,objcopy)
> + $(call if_changed,mkzimage)
>
> endif
>
> diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh
> new file mode 100755
> index 000000000000..922a93e61aa7
> --- /dev/null
> +++ b/arch/arm/boot/verify_zimage.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +set -e
> +
> +vmlinuz="$1"
> +zimage="$2"
> +nm="$3"
> +
> +magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) {
> + $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/;
> + $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/;
> +}; printf "%d\n", $magic_end - $magic_start;')
> +
> +zimage_size=$(stat -c '%s' "$zimage")
> +
> +# Verify that the resulting binary matches the size contained within
> +# the binary (iow, the linker has not added any additional sections.)
> +if [ $magic_size -ne $zimage_size ]; then
> + echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2
> + exit 1
> +fi
> +exit 0
> --
> 2.7.4
>
The following patch appears to fix the issue as well:
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..0e0f504e256e 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -83,7 +83,9 @@ SECTIONS
__pecoff_data_rawsize = . - ADDR(.data);
#endif
- _edata = .;
+ .edata (NOLOAD) : {
+ _edata = .;
+ }
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
_magic_start = ZIMAGE_MAGIC(_start);
because the NOLOAD triggers the linker to emit all PROGBITS sections
before _edata, including unknown ones.
E.g., in my binary, it gives me
00798020 D __pecoff_data_end
00798200 d __ksymtab_sort
00798208 B __bss_start
00798208 D _edata
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 8:09 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 8:09 UTC (permalink / raw)
To: linux-arm-kernel
On 23 October 2017 at 12:45, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote:
>> On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
>> > Hi Russell,
>> >
>> > Thanks for your reply.
>> >
>> > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
>> > >>>
>> > >>>hmm, right, didn't notice the data is already aligned...
>> > >>>so it's indeed caused by the ksym:
>> > >>>
>> > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0
>> > >>>0 4096
>> > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0
>> > >>>0 4
>> > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0
>> > >>>0 4
>> > >It's earlier - look for __ksymtab_strings.
>> >
>> > the problem i meet is the appended dtb code found dtb invalid. i thought
>> > that is because of unaligned zImage size, but i was wrong...
>>
>> Hmm, you really ought not to be using the appended dtb code for modern
>> systems - the appended dtb system is there for old boot loaders that
>> are incapable of dealing with a dtb. As is said in the option's help
>> text:
>>
>> This is meant as a backward compatibility convenience for those
>> systems with a bootloader that can't be upgraded to accommodate
>> the documented boot protocol using a device tree.
>>
>> Beware that there is very little in terms of protection against
>> this option being confused by leftover garbage in memory that might
>> look like a DTB header after a reboot if no actual DTB is appended
>> to zImage. Do not leave this option active in a production kernel
>> if you don't intend to always append a DTB. Proper passing of the
>> location into r2 of a bootloader provided DTB is always preferable
>> to this option.
>>
>> If you rely on it, and you have something that looks like a dtb after
>> the image, then things will go wrong, so it's better _not_ to use it
>> and to keep it disabled.
>>
>> That aside, thanks for doing a more in-depth analysis of what is going
>> on, which helps to understand /why/ Ard's fix works (whereas before
>> it was rather nebulous.)
>>
>> I wonder whether we ought to tell the linker to discard any unknown
>> sections by adding at the bottom:
>>
>> /DISCARD/ { *(*) }
>>
>> but I do think we need to document this, specifically that _edata must
>> point to the first byte after the binary file, and that the only
>> sections after it are allowed to be the .bss and stack sections.
>
> Short of adding the discard (which I think is itself risky, we've had
> problems in the main kernel's vmlinux.lds in this area), I think we
> ought to verify the size of the zImage file, so that the build fails
> when we generate a zImage which is wrong, rather than producing a
> zImage that is incorrect. Maybe something like this?
>
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] ARM: verify size of zImage
>
> The linker can sometimes add additional sections to the zImage ELF file
> which results in the zImage binary being larger than expected. This
> causes appended DT blobs to fail.
>
> Verify that the zImage binary is the expected size, and fail the build
> if this is not the case.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> arch/arm/boot/Makefile | 6 +++++-
> arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++
> 2 files changed, 26 insertions(+), 1 deletion(-)
> create mode 100755 arch/arm/boot/verify_zimage.sh
>
> diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
> index a3af4dc08c3e..1290875556ae 100644
> --- a/arch/arm/boot/Makefile
> +++ b/arch/arm/boot/Makefile
> @@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE
>
> else
>
> +quiet_cmd_mkzimage = ZIMAGE $@
> +cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \
> + '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }'
> +
> $(obj)/xipImage: FORCE
> @echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)'
> @false
> @@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE
> $(Q)$(MAKE) $(build)=$(obj)/compressed $@
>
> $(obj)/zImage: $(obj)/compressed/vmlinux FORCE
> - $(call if_changed,objcopy)
> + $(call if_changed,mkzimage)
>
> endif
>
> diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh
> new file mode 100755
> index 000000000000..922a93e61aa7
> --- /dev/null
> +++ b/arch/arm/boot/verify_zimage.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +set -e
> +
> +vmlinuz="$1"
> +zimage="$2"
> +nm="$3"
> +
> +magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) {
> + $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/;
> + $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/;
> +}; printf "%d\n", $magic_end - $magic_start;')
> +
> +zimage_size=$(stat -c '%s' "$zimage")
> +
> +# Verify that the resulting binary matches the size contained within
> +# the binary (iow, the linker has not added any additional sections.)
> +if [ $magic_size -ne $zimage_size ]; then
> + echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2
> + exit 1
> +fi
> +exit 0
> --
> 2.7.4
>
The following patch appears to fix the issue as well:
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..0e0f504e256e 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -83,7 +83,9 @@ SECTIONS
__pecoff_data_rawsize = . - ADDR(.data);
#endif
- _edata = .;
+ .edata (NOLOAD) : {
+ _edata = .;
+ }
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
_magic_start = ZIMAGE_MAGIC(_start);
because the NOLOAD triggers the linker to emit all PROGBITS sections
before _edata, including unknown ones.
E.g., in my binary, it gives me
00798020 D __pecoff_data_end
00798200 d __ksymtab_sort
00798208 B __bss_start
00798208 D _edata
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 8:09 ` Ard Biesheuvel
@ 2017-10-24 9:09 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel, jeffy, Ingo Molnar, linux-arm-kernel, Chris Zhong
On Tue, Oct 24, 2017 at 09:09:52AM +0100, Ard Biesheuvel wrote:
> The following patch appears to fix the issue as well:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..0e0f504e256e 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -83,7 +83,9 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> - _edata = .;
> + .edata (NOLOAD) : {
> + _edata = .;
> + }
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> _magic_start = ZIMAGE_MAGIC(_start);
>
> because the NOLOAD triggers the linker to emit all PROGBITS sections
> before _edata, including unknown ones.
>
> E.g., in my binary, it gives me
>
> 00798020 D __pecoff_data_end
> 00798200 d __ksymtab_sort
> 00798208 B __bss_start
> 00798208 D _edata
The question is: do we want to know when additional sections get
emitted into the binary?
You've already said that for EFI, you need the size of the binary
to be a multiple of 512, so I guess the answer to that is "yes".
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:09 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 24, 2017 at 09:09:52AM +0100, Ard Biesheuvel wrote:
> The following patch appears to fix the issue as well:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..0e0f504e256e 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -83,7 +83,9 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> - _edata = .;
> + .edata (NOLOAD) : {
> + _edata = .;
> + }
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> _magic_start = ZIMAGE_MAGIC(_start);
>
> because the NOLOAD triggers the linker to emit all PROGBITS sections
> before _edata, including unknown ones.
>
> E.g., in my binary, it gives me
>
> 00798020 D __pecoff_data_end
> 00798200 d __ksymtab_sort
> 00798208 B __bss_start
> 00798208 D _edata
The question is: do we want to know when additional sections get
emitted into the binary?
You've already said that for EFI, you need the size of the binary
to be a multiple of 512, so I guess the answer to that is "yes".
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:09 ` Russell King - ARM Linux
@ 2017-10-24 9:13 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:13 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-kernel, jeffy, Ingo Molnar, linux-arm-kernel, Chris Zhong
On 24 October 2017 at 10:09, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 09:09:52AM +0100, Ard Biesheuvel wrote:
>> The following patch appears to fix the issue as well:
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
>> b/arch/arm/boot/compressed/vmlinux.lds.S
>> index 7a4c59154361..0e0f504e256e 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -83,7 +83,9 @@ SECTIONS
>> __pecoff_data_rawsize = . - ADDR(.data);
>> #endif
>>
>> - _edata = .;
>> + .edata (NOLOAD) : {
>> + _edata = .;
>> + }
>>
>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> _magic_start = ZIMAGE_MAGIC(_start);
>>
>> because the NOLOAD triggers the linker to emit all PROGBITS sections
>> before _edata, including unknown ones.
>>
>> E.g., in my binary, it gives me
>>
>> 00798020 D __pecoff_data_end
>> 00798200 d __ksymtab_sort
>> 00798208 B __bss_start
>> 00798208 D _edata
>
> The question is: do we want to know when additional sections get
> emitted into the binary?
>
Well, we need to know whether the size of zImage is a multiple of 512
bytes. We could check that separately by adding the following as well
#ifdef CONFIG_EFI_STUB
ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
#endif
> You've already said that for EFI, you need the size of the binary
> to be a multiple of 512, so I guess the answer to that is "yes".
>
Indeed.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:13 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:13 UTC (permalink / raw)
To: linux-arm-kernel
On 24 October 2017 at 10:09, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 09:09:52AM +0100, Ard Biesheuvel wrote:
>> The following patch appears to fix the issue as well:
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
>> b/arch/arm/boot/compressed/vmlinux.lds.S
>> index 7a4c59154361..0e0f504e256e 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -83,7 +83,9 @@ SECTIONS
>> __pecoff_data_rawsize = . - ADDR(.data);
>> #endif
>>
>> - _edata = .;
>> + .edata (NOLOAD) : {
>> + _edata = .;
>> + }
>>
>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> _magic_start = ZIMAGE_MAGIC(_start);
>>
>> because the NOLOAD triggers the linker to emit all PROGBITS sections
>> before _edata, including unknown ones.
>>
>> E.g., in my binary, it gives me
>>
>> 00798020 D __pecoff_data_end
>> 00798200 d __ksymtab_sort
>> 00798208 B __bss_start
>> 00798208 D _edata
>
> The question is: do we want to know when additional sections get
> emitted into the binary?
>
Well, we need to know whether the size of zImage is a multiple of 512
bytes. We could check that separately by adding the following as well
#ifdef CONFIG_EFI_STUB
ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
#endif
> You've already said that for EFI, you need the size of the binary
> to be a multiple of 512, so I guess the answer to that is "yes".
>
Indeed.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 8:09 ` Ard Biesheuvel
@ 2017-10-24 9:16 ` jeffy
-1 siblings, 0 replies; 42+ messages in thread
From: jeffy @ 2017-10-24 9:16 UTC (permalink / raw)
To: Ard Biesheuvel, Russell King - ARM Linux
Cc: Ingo Molnar, Chris Zhong, linux-kernel, linux-arm-kernel
Hi guys,
On 10/24/2017 04:09 PM, Ard Biesheuvel wrote:
> The following patch appears to fix the issue as well:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..0e0f504e256e 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -83,7 +83,9 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> - _edata = .;
> + .edata (NOLOAD) : {
> + _edata = .;
> + }
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> _magic_start = ZIMAGE_MAGIC(_start);
>
> because the NOLOAD triggers the linker to emit all PROGBITS sections
> before _edata, including unknown ones.
>
> E.g., in my binary, it gives me
>
> 00798020 D __pecoff_data_end
> 00798200 d __ksymtab_sort
> 00798208 B __bss_start
> 00798208 D _edata
this works too:
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -70,10 +70,6 @@ SECTIONS
.got : { *(.got) }
_got_end = .;
- /* ensure the zImage file size is always a multiple of 64 bits */
- /* (without a dummy byte, ld just ignores the empty section) */
- .pad : { BYTE(0); . = ALIGN(8); }
-
#ifdef CONFIG_EFI_STUB
.data : ALIGN(4096) {
__pecoff_data_start = .;
@@ -93,7 +89,10 @@ SECTIONS
__pecoff_data_rawsize = . - ADDR(.data);
#endif
- _edata = .;
+ /* ensure the zImage file size is always a multiple of 64 bits */
+ .edata : ALIGN(8) {
+ _edata = .;
+ }
339: 006cf200 8 OBJECT LOCAL DEFAULT 9 __ksymtab_sort
...
528: 006cf208 0 NOTYPE GLOBAL DEFAULT 9 _edata
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:16 ` jeffy
0 siblings, 0 replies; 42+ messages in thread
From: jeffy @ 2017-10-24 9:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi guys,
On 10/24/2017 04:09 PM, Ard Biesheuvel wrote:
> The following patch appears to fix the issue as well:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..0e0f504e256e 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -83,7 +83,9 @@ SECTIONS
> __pecoff_data_rawsize = . - ADDR(.data);
> #endif
>
> - _edata = .;
> + .edata (NOLOAD) : {
> + _edata = .;
> + }
>
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> _magic_start = ZIMAGE_MAGIC(_start);
>
> because the NOLOAD triggers the linker to emit all PROGBITS sections
> before _edata, including unknown ones.
>
> E.g., in my binary, it gives me
>
> 00798020 D __pecoff_data_end
> 00798200 d __ksymtab_sort
> 00798208 B __bss_start
> 00798208 D _edata
this works too:
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -70,10 +70,6 @@ SECTIONS
.got : { *(.got) }
_got_end = .;
- /* ensure the zImage file size is always a multiple of 64 bits */
- /* (without a dummy byte, ld just ignores the empty section) */
- .pad : { BYTE(0); . = ALIGN(8); }
-
#ifdef CONFIG_EFI_STUB
.data : ALIGN(4096) {
__pecoff_data_start = .;
@@ -93,7 +89,10 @@ SECTIONS
__pecoff_data_rawsize = . - ADDR(.data);
#endif
- _edata = .;
+ /* ensure the zImage file size is always a multiple of 64 bits */
+ .edata : ALIGN(8) {
+ _edata = .;
+ }
339: 006cf200 8 OBJECT LOCAL DEFAULT 9 __ksymtab_sort
...
528: 006cf208 0 NOTYPE GLOBAL DEFAULT 9 _edata
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:13 ` Ard Biesheuvel
@ 2017-10-24 9:22 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:22 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:09, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > The question is: do we want to know when additional sections get
> > emitted into the binary?
>
> Well, we need to know whether the size of zImage is a multiple of 512
> bytes. We could check that separately by adding the following as well
>
> #ifdef CONFIG_EFI_STUB
> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
This would only catch them when EFI is enabled, which doesn't give very
good build coverage. I still prefer my solution over adding the assert
and a section for _edata - my solution catches it with EFI disabled as
well.
A variant on that would be this, which should catch additional sections
for EFI and non-EFI:
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index b38dcef90756..9d0c5d80979a 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -95,6 +95,10 @@ SECTIONS
_edata = .;
+ .image_end (NOLOAD) : {
+ _image_end = .;
+ }
+
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
_magic_start = ZIMAGE_MAGIC(_start);
_magic_end = ZIMAGE_MAGIC(_edata);
@@ -119,3 +123,5 @@ SECTIONS
.stab.indexstr 0 : { *(.stab.indexstr) }
.comment 0 : { *(.comment) }
}
+
+ASSERT(image_end == end, "zImage file size is incorrect")
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:22 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:22 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:09, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > The question is: do we want to know when additional sections get
> > emitted into the binary?
>
> Well, we need to know whether the size of zImage is a multiple of 512
> bytes. We could check that separately by adding the following as well
>
> #ifdef CONFIG_EFI_STUB
> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
This would only catch them when EFI is enabled, which doesn't give very
good build coverage. I still prefer my solution over adding the assert
and a section for _edata - my solution catches it with EFI disabled as
well.
A variant on that would be this, which should catch additional sections
for EFI and non-EFI:
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index b38dcef90756..9d0c5d80979a 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -95,6 +95,10 @@ SECTIONS
_edata = .;
+ .image_end (NOLOAD) : {
+ _image_end = .;
+ }
+
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
_magic_start = ZIMAGE_MAGIC(_start);
_magic_end = ZIMAGE_MAGIC(_edata);
@@ -119,3 +123,5 @@ SECTIONS
.stab.indexstr 0 : { *(.stab.indexstr) }
.comment 0 : { *(.comment) }
}
+
+ASSERT(image_end == end, "zImage file size is incorrect")
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:22 ` Russell King - ARM Linux
@ 2017-10-24 9:26 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:26 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On 24 October 2017 at 10:22, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > The question is: do we want to know when additional sections get
>> > emitted into the binary?
>>
>> Well, we need to know whether the size of zImage is a multiple of 512
>> bytes. We could check that separately by adding the following as well
>>
>> #ifdef CONFIG_EFI_STUB
>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>
> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>
> This would only catch them when EFI is enabled, which doesn't give very
> good build coverage. I still prefer my solution over adding the assert
> and a section for _edata - my solution catches it with EFI disabled as
> well.
>
> A variant on that would be this, which should catch additional sections
> for EFI and non-EFI:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..9d0c5d80979a 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -95,6 +95,10 @@ SECTIONS
>
> _edata = .;
>
> + .image_end (NOLOAD) : {
> + _image_end = .;
> + }
> +
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> _magic_start = ZIMAGE_MAGIC(_start);
> _magic_end = ZIMAGE_MAGIC(_edata);
> @@ -119,3 +123,5 @@ SECTIONS
> .stab.indexstr 0 : { *(.stab.indexstr) }
> .comment 0 : { *(.comment) }
> }
> +
> +ASSERT(image_end == end, "zImage file size is incorrect")
>
I take it you mean
_image_end == _edata
here? That works for me:
Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
(with CONFIG_EFI enabled)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:26 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:26 UTC (permalink / raw)
To: linux-arm-kernel
On 24 October 2017 at 10:22, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > The question is: do we want to know when additional sections get
>> > emitted into the binary?
>>
>> Well, we need to know whether the size of zImage is a multiple of 512
>> bytes. We could check that separately by adding the following as well
>>
>> #ifdef CONFIG_EFI_STUB
>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>
> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>
> This would only catch them when EFI is enabled, which doesn't give very
> good build coverage. I still prefer my solution over adding the assert
> and a section for _edata - my solution catches it with EFI disabled as
> well.
>
> A variant on that would be this, which should catch additional sections
> for EFI and non-EFI:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..9d0c5d80979a 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -95,6 +95,10 @@ SECTIONS
>
> _edata = .;
>
> + .image_end (NOLOAD) : {
> + _image_end = .;
> + }
> +
> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> _magic_start = ZIMAGE_MAGIC(_start);
> _magic_end = ZIMAGE_MAGIC(_edata);
> @@ -119,3 +123,5 @@ SECTIONS
> .stab.indexstr 0 : { *(.stab.indexstr) }
> .comment 0 : { *(.comment) }
> }
> +
> +ASSERT(image_end == end, "zImage file size is incorrect")
>
I take it you mean
_image_end == _edata
here? That works for me:
Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
(with CONFIG_EFI enabled)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:26 ` Ard Biesheuvel
@ 2017-10-24 9:30 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:30 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 24 October 2017 at 10:22, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>>> <linux@armlinux.org.uk> wrote:
>>> > The question is: do we want to know when additional sections get
>>> > emitted into the binary?
>>>
>>> Well, we need to know whether the size of zImage is a multiple of 512
>>> bytes. We could check that separately by adding the following as well
>>>
>>> #ifdef CONFIG_EFI_STUB
>>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>>
>> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>>
>> This would only catch them when EFI is enabled, which doesn't give very
>> good build coverage. I still prefer my solution over adding the assert
>> and a section for _edata - my solution catches it with EFI disabled as
>> well.
>>
>> A variant on that would be this, which should catch additional sections
>> for EFI and non-EFI:
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> index b38dcef90756..9d0c5d80979a 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -95,6 +95,10 @@ SECTIONS
>>
>> _edata = .;
>>
>> + .image_end (NOLOAD) : {
>> + _image_end = .;
>> + }
>> +
>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> _magic_start = ZIMAGE_MAGIC(_start);
>> _magic_end = ZIMAGE_MAGIC(_edata);
>> @@ -119,3 +123,5 @@ SECTIONS
>> .stab.indexstr 0 : { *(.stab.indexstr) }
>> .comment 0 : { *(.comment) }
>> }
>> +
>> +ASSERT(image_end == end, "zImage file size is incorrect")
>>
>
> I take it you mean
>
> _image_end == _edata
>
> here? That works for me:
>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> (with CONFIG_EFI enabled)
BTW you could simplify that to
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..204d4580a04f 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -85,6 +85,10 @@ SECTIONS
_edata = .;
+ .image_end (NOLOAD) : {
+ ASSERT(. == _edata, "zImage file size is incorrect");
+ }
+
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
_magic_start = ZIMAGE_MAGIC(_start);
_magic_end = ZIMAGE_MAGIC(_edata);
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:30 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:30 UTC (permalink / raw)
To: linux-arm-kernel
On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 24 October 2017 at 10:22, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>>> <linux@armlinux.org.uk> wrote:
>>> > The question is: do we want to know when additional sections get
>>> > emitted into the binary?
>>>
>>> Well, we need to know whether the size of zImage is a multiple of 512
>>> bytes. We could check that separately by adding the following as well
>>>
>>> #ifdef CONFIG_EFI_STUB
>>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>>
>> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>>
>> This would only catch them when EFI is enabled, which doesn't give very
>> good build coverage. I still prefer my solution over adding the assert
>> and a section for _edata - my solution catches it with EFI disabled as
>> well.
>>
>> A variant on that would be this, which should catch additional sections
>> for EFI and non-EFI:
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> index b38dcef90756..9d0c5d80979a 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -95,6 +95,10 @@ SECTIONS
>>
>> _edata = .;
>>
>> + .image_end (NOLOAD) : {
>> + _image_end = .;
>> + }
>> +
>> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> _magic_start = ZIMAGE_MAGIC(_start);
>> _magic_end = ZIMAGE_MAGIC(_edata);
>> @@ -119,3 +123,5 @@ SECTIONS
>> .stab.indexstr 0 : { *(.stab.indexstr) }
>> .comment 0 : { *(.comment) }
>> }
>> +
>> +ASSERT(image_end == end, "zImage file size is incorrect")
>>
>
> I take it you mean
>
> _image_end == _edata
>
> here? That works for me:
>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> (with CONFIG_EFI enabled)
BTW you could simplify that to
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..204d4580a04f 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -85,6 +85,10 @@ SECTIONS
_edata = .;
+ .image_end (NOLOAD) : {
+ ASSERT(. == _edata, "zImage file size is incorrect");
+ }
+
_magic_sig = ZIMAGE_MAGIC(0x016f2818);
_magic_start = ZIMAGE_MAGIC(_start);
_magic_end = ZIMAGE_MAGIC(_edata);
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:30 ` Ard Biesheuvel
@ 2017-10-24 9:38 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:38 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On Tue, Oct 24, 2017 at 10:30:41AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 24 October 2017 at 10:22, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> >> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
> >>> On 24 October 2017 at 10:09, Russell King - ARM Linux
> >>> <linux@armlinux.org.uk> wrote:
> >>> > The question is: do we want to know when additional sections get
> >>> > emitted into the binary?
> >>>
> >>> Well, we need to know whether the size of zImage is a multiple of 512
> >>> bytes. We could check that separately by adding the following as well
> >>>
> >>> #ifdef CONFIG_EFI_STUB
> >>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
> >>
> >> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
> >>
> >> This would only catch them when EFI is enabled, which doesn't give very
> >> good build coverage. I still prefer my solution over adding the assert
> >> and a section for _edata - my solution catches it with EFI disabled as
> >> well.
> >>
> >> A variant on that would be this, which should catch additional sections
> >> for EFI and non-EFI:
> >>
> >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> >> index b38dcef90756..9d0c5d80979a 100644
> >> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> >> @@ -95,6 +95,10 @@ SECTIONS
> >>
> >> _edata = .;
> >>
> >> + .image_end (NOLOAD) : {
> >> + _image_end = .;
> >> + }
> >> +
> >> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> >> _magic_start = ZIMAGE_MAGIC(_start);
> >> _magic_end = ZIMAGE_MAGIC(_edata);
> >> @@ -119,3 +123,5 @@ SECTIONS
> >> .stab.indexstr 0 : { *(.stab.indexstr) }
> >> .comment 0 : { *(.comment) }
> >> }
> >> +
> >> +ASSERT(image_end == end, "zImage file size is incorrect")
> >>
> >
> > I take it you mean
> >
> > _image_end == _edata
> >
> > here? That works for me:
> >
> > Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > (with CONFIG_EFI enabled)
>
>
> BTW you could simplify that to
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..204d4580a04f 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -85,6 +85,10 @@ SECTIONS
>
> _edata = .;
>
> + .image_end (NOLOAD) : {
> + ASSERT(. == _edata, "zImage file size is incorrect");
> + }
Yep. Okay, so now we have a patch from Arnd to fix the alignment problem
(so don't need alignment of the piggydata), and we have a patch to catch
the additional sections. That leaves a patch to sort out the additional
sections.
Do you have any preference - I'd prefer one that I can merge along with
these changes? One way forward would be to temporarily add the /DISCARD/
for the ksym sections, which can then be removed once your sort() removal
gets added to the EFI trees.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:38 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:38 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 24, 2017 at 10:30:41AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 24 October 2017 at 10:22, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> >> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
> >>> On 24 October 2017 at 10:09, Russell King - ARM Linux
> >>> <linux@armlinux.org.uk> wrote:
> >>> > The question is: do we want to know when additional sections get
> >>> > emitted into the binary?
> >>>
> >>> Well, we need to know whether the size of zImage is a multiple of 512
> >>> bytes. We could check that separately by adding the following as well
> >>>
> >>> #ifdef CONFIG_EFI_STUB
> >>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
> >>
> >> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
> >>
> >> This would only catch them when EFI is enabled, which doesn't give very
> >> good build coverage. I still prefer my solution over adding the assert
> >> and a section for _edata - my solution catches it with EFI disabled as
> >> well.
> >>
> >> A variant on that would be this, which should catch additional sections
> >> for EFI and non-EFI:
> >>
> >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> >> index b38dcef90756..9d0c5d80979a 100644
> >> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> >> @@ -95,6 +95,10 @@ SECTIONS
> >>
> >> _edata = .;
> >>
> >> + .image_end (NOLOAD) : {
> >> + _image_end = .;
> >> + }
> >> +
> >> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> >> _magic_start = ZIMAGE_MAGIC(_start);
> >> _magic_end = ZIMAGE_MAGIC(_edata);
> >> @@ -119,3 +123,5 @@ SECTIONS
> >> .stab.indexstr 0 : { *(.stab.indexstr) }
> >> .comment 0 : { *(.comment) }
> >> }
> >> +
> >> +ASSERT(image_end == end, "zImage file size is incorrect")
> >>
> >
> > I take it you mean
> >
> > _image_end == _edata
> >
> > here? That works for me:
> >
> > Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > (with CONFIG_EFI enabled)
>
>
> BTW you could simplify that to
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..204d4580a04f 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -85,6 +85,10 @@ SECTIONS
>
> _edata = .;
>
> + .image_end (NOLOAD) : {
> + ASSERT(. == _edata, "zImage file size is incorrect");
> + }
Yep. Okay, so now we have a patch from Arnd to fix the alignment problem
(so don't need alignment of the piggydata), and we have a patch to catch
the additional sections. That leaves a patch to sort out the additional
sections.
Do you have any preference - I'd prefer one that I can merge along with
these changes? One way forward would be to temporarily add the /DISCARD/
for the ksym sections, which can then be removed once your sort() removal
gets added to the EFI trees.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:38 ` Russell King - ARM Linux
@ 2017-10-24 9:44 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:44 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On 24 October 2017 at 10:38, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:30:41AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > On 24 October 2017 at 10:22, Russell King - ARM Linux
>> > <linux@armlinux.org.uk> wrote:
>> >> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>> >>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>> >>> <linux@armlinux.org.uk> wrote:
>> >>> > The question is: do we want to know when additional sections get
>> >>> > emitted into the binary?
>> >>>
>> >>> Well, we need to know whether the size of zImage is a multiple of 512
>> >>> bytes. We could check that separately by adding the following as well
>> >>>
>> >>> #ifdef CONFIG_EFI_STUB
>> >>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>> >>
>> >> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>> >>
>> >> This would only catch them when EFI is enabled, which doesn't give very
>> >> good build coverage. I still prefer my solution over adding the assert
>> >> and a section for _edata - my solution catches it with EFI disabled as
>> >> well.
>> >>
>> >> A variant on that would be this, which should catch additional sections
>> >> for EFI and non-EFI:
>> >>
>> >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> >> index b38dcef90756..9d0c5d80979a 100644
>> >> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> >> @@ -95,6 +95,10 @@ SECTIONS
>> >>
>> >> _edata = .;
>> >>
>> >> + .image_end (NOLOAD) : {
>> >> + _image_end = .;
>> >> + }
>> >> +
>> >> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> >> _magic_start = ZIMAGE_MAGIC(_start);
>> >> _magic_end = ZIMAGE_MAGIC(_edata);
>> >> @@ -119,3 +123,5 @@ SECTIONS
>> >> .stab.indexstr 0 : { *(.stab.indexstr) }
>> >> .comment 0 : { *(.comment) }
>> >> }
>> >> +
>> >> +ASSERT(image_end == end, "zImage file size is incorrect")
>> >>
>> >
>> > I take it you mean
>> >
>> > _image_end == _edata
>> >
>> > here? That works for me:
>> >
>> > Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > (with CONFIG_EFI enabled)
>>
>>
>> BTW you could simplify that to
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
>> b/arch/arm/boot/compressed/vmlinux.lds.S
>> index 7a4c59154361..204d4580a04f 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -85,6 +85,10 @@ SECTIONS
>>
>> _edata = .;
>>
>> + .image_end (NOLOAD) : {
>> + ASSERT(. == _edata, "zImage file size is incorrect");
>> + }
>
> Yep. Okay, so now we have a patch from Arnd to fix the alignment problem
> (so don't need alignment of the piggydata), and we have a patch to catch
> the additional sections. That leaves a patch to sort out the additional
> sections.
>
> Do you have any preference - I'd prefer one that I can merge along with
> these changes? One way forward would be to temporarily add the /DISCARD/
> for the ksym sections, which can then be removed once your sort() removal
> gets added to the EFI trees.
>
Well, I can respin that patch to only discard the ksymtab/kcrctab
sections (and drop the alignment of piggydata), but I'd prefer to make
it permanent if you don't mind. I still intend to remove the sort()
call, given that ARM doesn't need it, but it seems more future proof
to me to always discard those sections, and if we end up incorporating
more core kernel code into the EFI stub, we won't need to revisit this
(although I am not aware of any reasons why we would be doing that in
the near future)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:44 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 9:44 UTC (permalink / raw)
To: linux-arm-kernel
On 24 October 2017 at 10:38, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:30:41AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > On 24 October 2017 at 10:22, Russell King - ARM Linux
>> > <linux@armlinux.org.uk> wrote:
>> >> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>> >>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>> >>> <linux@armlinux.org.uk> wrote:
>> >>> > The question is: do we want to know when additional sections get
>> >>> > emitted into the binary?
>> >>>
>> >>> Well, we need to know whether the size of zImage is a multiple of 512
>> >>> bytes. We could check that separately by adding the following as well
>> >>>
>> >>> #ifdef CONFIG_EFI_STUB
>> >>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>> >>
>> >> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>> >>
>> >> This would only catch them when EFI is enabled, which doesn't give very
>> >> good build coverage. I still prefer my solution over adding the assert
>> >> and a section for _edata - my solution catches it with EFI disabled as
>> >> well.
>> >>
>> >> A variant on that would be this, which should catch additional sections
>> >> for EFI and non-EFI:
>> >>
>> >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> >> index b38dcef90756..9d0c5d80979a 100644
>> >> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> >> @@ -95,6 +95,10 @@ SECTIONS
>> >>
>> >> _edata = .;
>> >>
>> >> + .image_end (NOLOAD) : {
>> >> + _image_end = .;
>> >> + }
>> >> +
>> >> _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> >> _magic_start = ZIMAGE_MAGIC(_start);
>> >> _magic_end = ZIMAGE_MAGIC(_edata);
>> >> @@ -119,3 +123,5 @@ SECTIONS
>> >> .stab.indexstr 0 : { *(.stab.indexstr) }
>> >> .comment 0 : { *(.comment) }
>> >> }
>> >> +
>> >> +ASSERT(image_end == end, "zImage file size is incorrect")
>> >>
>> >
>> > I take it you mean
>> >
>> > _image_end == _edata
>> >
>> > here? That works for me:
>> >
>> > Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > (with CONFIG_EFI enabled)
>>
>>
>> BTW you could simplify that to
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
>> b/arch/arm/boot/compressed/vmlinux.lds.S
>> index 7a4c59154361..204d4580a04f 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -85,6 +85,10 @@ SECTIONS
>>
>> _edata = .;
>>
>> + .image_end (NOLOAD) : {
>> + ASSERT(. == _edata, "zImage file size is incorrect");
>> + }
>
> Yep. Okay, so now we have a patch from Arnd to fix the alignment problem
> (so don't need alignment of the piggydata), and we have a patch to catch
> the additional sections. That leaves a patch to sort out the additional
> sections.
>
> Do you have any preference - I'd prefer one that I can merge along with
> these changes? One way forward would be to temporarily add the /DISCARD/
> for the ksym sections, which can then be removed once your sort() removal
> gets added to the EFI trees.
>
Well, I can respin that patch to only discard the ksymtab/kcrctab
sections (and drop the alignment of piggydata), but I'd prefer to make
it permanent if you don't mind. I still intend to remove the sort()
call, given that ARM doesn't need it, but it seems more future proof
to me to always discard those sections, and if we end up incorporating
more core kernel code into the EFI stub, we won't need to revisit this
(although I am not aware of any reasons why we would be doing that in
the near future)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:44 ` Ard Biesheuvel
@ 2017-10-24 9:54 ` Russell King - ARM Linux
-1 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:54 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On Tue, Oct 24, 2017 at 10:44:00AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:38, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Do you have any preference - I'd prefer one that I can merge along with
> > these changes? One way forward would be to temporarily add the /DISCARD/
> > for the ksym sections, which can then be removed once your sort() removal
> > gets added to the EFI trees.
> >
>
> Well, I can respin that patch to only discard the ksymtab/kcrctab
> sections (and drop the alignment of piggydata), but I'd prefer to make
> it permanent if you don't mind. I still intend to remove the sort()
> call, given that ARM doesn't need it, but it seems more future proof
> to me to always discard those sections, and if we end up incorporating
> more core kernel code into the EFI stub, we won't need to revisit this
> (although I am not aware of any reasons why we would be doing that in
> the near future)
I don't think it's future-proof. The decompressor is a particularly
special environment due to its runtime relocation support, where we
(eg) don't really support initialised pointers in the .data section.
Stuffing code from other parts of the kernel into the decompressor
isn't going to work reliably, and I'd much rather discourage the
practice.
Hence, there's really no reason to keep that discard, and I'd much
rather have the build fail if the sections re-appear as a pointer
towards something not being correct.
A solution to this would be to link the decompressor ELF with -r
and wrap that up in a mini-linker, but I suspect that will need
quite a re-write because of the self-relocation that the image
does if it detects overlap between itself and the destination for
the decompressed kernel image.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 9:54 ` Russell King - ARM Linux
0 siblings, 0 replies; 42+ messages in thread
From: Russell King - ARM Linux @ 2017-10-24 9:54 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 24, 2017 at 10:44:00AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:38, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Do you have any preference - I'd prefer one that I can merge along with
> > these changes? One way forward would be to temporarily add the /DISCARD/
> > for the ksym sections, which can then be removed once your sort() removal
> > gets added to the EFI trees.
> >
>
> Well, I can respin that patch to only discard the ksymtab/kcrctab
> sections (and drop the alignment of piggydata), but I'd prefer to make
> it permanent if you don't mind. I still intend to remove the sort()
> call, given that ARM doesn't need it, but it seems more future proof
> to me to always discard those sections, and if we end up incorporating
> more core kernel code into the EFI stub, we won't need to revisit this
> (although I am not aware of any reasons why we would be doing that in
> the near future)
I don't think it's future-proof. The decompressor is a particularly
special environment due to its runtime relocation support, where we
(eg) don't really support initialised pointers in the .data section.
Stuffing code from other parts of the kernel into the decompressor
isn't going to work reliably, and I'd much rather discourage the
practice.
Hence, there's really no reason to keep that discard, and I'd much
rather have the build fail if the sections re-appear as a pointer
towards something not being correct.
A solution to this would be to link the decompressor ELF with -r
and wrap that up in a mini-linker, but I suspect that will need
quite a re-write because of the self-relocation that the image
does if it detects overlap between itself and the destination for
the decompressed kernel image.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
2017-10-24 9:54 ` Russell King - ARM Linux
@ 2017-10-24 10:03 ` Ard Biesheuvel
-1 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 10:03 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Chris Zhong, jeffy, linux-kernel, linux-arm-kernel, Ingo Molnar
On 24 October 2017 at 10:54, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:44:00AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:38, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > Do you have any preference - I'd prefer one that I can merge along with
>> > these changes? One way forward would be to temporarily add the /DISCARD/
>> > for the ksym sections, which can then be removed once your sort() removal
>> > gets added to the EFI trees.
>> >
>>
>> Well, I can respin that patch to only discard the ksymtab/kcrctab
>> sections (and drop the alignment of piggydata), but I'd prefer to make
>> it permanent if you don't mind. I still intend to remove the sort()
>> call, given that ARM doesn't need it, but it seems more future proof
>> to me to always discard those sections, and if we end up incorporating
>> more core kernel code into the EFI stub, we won't need to revisit this
>> (although I am not aware of any reasons why we would be doing that in
>> the near future)
>
> I don't think it's future-proof. The decompressor is a particularly
> special environment due to its runtime relocation support, where we
> (eg) don't really support initialised pointers in the .data section.
> Stuffing code from other parts of the kernel into the decompressor
> isn't going to work reliably, and I'd much rather discourage the
> practice.
>
Fair enough.
> Hence, there's really no reason to keep that discard, and I'd much
> rather have the build fail if the sections re-appear as a pointer
> towards something not being correct.
>
OK. You're welcome to take that sort() removal patch through the ARM
tree if you want to keep these patches together. Or I could respin the
/discard/ one. Whichever you prefer (although I am leaning towards the
former, so we can merge them and be done with it)
> A solution to this would be to link the decompressor ELF with -r
> and wrap that up in a mini-linker, but I suspect that will need
> quite a re-write because of the self-relocation that the image
> does if it detects overlap between itself and the destination for
> the decompressed kernel image.
>
It was already quite tricky to get the hybrid PE-COFF/bare metal image
working, especially because the stub code is shared between x86, arm64
and ARM, all of which have their own quirks when it comes to the early
environment this code executes in.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled
@ 2017-10-24 10:03 ` Ard Biesheuvel
0 siblings, 0 replies; 42+ messages in thread
From: Ard Biesheuvel @ 2017-10-24 10:03 UTC (permalink / raw)
To: linux-arm-kernel
On 24 October 2017 at 10:54, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:44:00AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:38, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > Do you have any preference - I'd prefer one that I can merge along with
>> > these changes? One way forward would be to temporarily add the /DISCARD/
>> > for the ksym sections, which can then be removed once your sort() removal
>> > gets added to the EFI trees.
>> >
>>
>> Well, I can respin that patch to only discard the ksymtab/kcrctab
>> sections (and drop the alignment of piggydata), but I'd prefer to make
>> it permanent if you don't mind. I still intend to remove the sort()
>> call, given that ARM doesn't need it, but it seems more future proof
>> to me to always discard those sections, and if we end up incorporating
>> more core kernel code into the EFI stub, we won't need to revisit this
>> (although I am not aware of any reasons why we would be doing that in
>> the near future)
>
> I don't think it's future-proof. The decompressor is a particularly
> special environment due to its runtime relocation support, where we
> (eg) don't really support initialised pointers in the .data section.
> Stuffing code from other parts of the kernel into the decompressor
> isn't going to work reliably, and I'd much rather discourage the
> practice.
>
Fair enough.
> Hence, there's really no reason to keep that discard, and I'd much
> rather have the build fail if the sections re-appear as a pointer
> towards something not being correct.
>
OK. You're welcome to take that sort() removal patch through the ARM
tree if you want to keep these patches together. Or I could respin the
/discard/ one. Whichever you prefer (although I am leaning towards the
former, so we can merge them and be done with it)
> A solution to this would be to link the decompressor ELF with -r
> and wrap that up in a mini-linker, but I suspect that will need
> quite a re-write because of the self-relocation that the image
> does if it detects overlap between itself and the destination for
> the decompressed kernel image.
>
It was already quite tricky to get the hybrid PE-COFF/bare metal image
working, especially because the stub code is shared between x86, arm64
and ARM, all of which have their own quirks when it comes to the early
environment this code executes in.
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2017-10-24 10:03 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-18 5:01 [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled Jeffy Chen
2017-10-18 5:01 ` Jeffy Chen
2017-10-18 6:19 ` Chris Zhong
2017-10-18 6:19 ` Chris Zhong
2017-10-22 11:01 ` Ard Biesheuvel
2017-10-22 11:01 ` Ard Biesheuvel
2017-10-22 12:47 ` Russell King - ARM Linux
2017-10-22 12:47 ` Russell King - ARM Linux
2017-10-22 13:01 ` Ard Biesheuvel
2017-10-22 13:01 ` Ard Biesheuvel
2017-10-23 3:26 ` jeffy
2017-10-23 3:26 ` jeffy
2017-10-23 8:50 ` Russell King - ARM Linux
2017-10-23 8:50 ` Russell King - ARM Linux
2017-10-23 10:24 ` jeffy
2017-10-23 10:24 ` jeffy
2017-10-23 10:50 ` Russell King - ARM Linux
2017-10-23 10:50 ` Russell King - ARM Linux
2017-10-23 11:45 ` Russell King - ARM Linux
2017-10-23 11:45 ` Russell King - ARM Linux
2017-10-24 8:09 ` Ard Biesheuvel
2017-10-24 8:09 ` Ard Biesheuvel
2017-10-24 9:09 ` Russell King - ARM Linux
2017-10-24 9:09 ` Russell King - ARM Linux
2017-10-24 9:13 ` Ard Biesheuvel
2017-10-24 9:13 ` Ard Biesheuvel
2017-10-24 9:22 ` Russell King - ARM Linux
2017-10-24 9:22 ` Russell King - ARM Linux
2017-10-24 9:26 ` Ard Biesheuvel
2017-10-24 9:26 ` Ard Biesheuvel
2017-10-24 9:30 ` Ard Biesheuvel
2017-10-24 9:30 ` Ard Biesheuvel
2017-10-24 9:38 ` Russell King - ARM Linux
2017-10-24 9:38 ` Russell King - ARM Linux
2017-10-24 9:44 ` Ard Biesheuvel
2017-10-24 9:44 ` Ard Biesheuvel
2017-10-24 9:54 ` Russell King - ARM Linux
2017-10-24 9:54 ` Russell King - ARM Linux
2017-10-24 10:03 ` Ard Biesheuvel
2017-10-24 10:03 ` Ard Biesheuvel
2017-10-24 9:16 ` jeffy
2017-10-24 9:16 ` jeffy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.