From mboxrd@z Thu Jan 1 00:00:00 1970 From: jeffy.chen@rock-chips.com (jeffy) Date: Mon, 23 Oct 2017 11:26:49 +0800 Subject: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled In-Reply-To: References: <20171018050108.10352-1-jeffy.chen@rock-chips.com> <20171022124757.GL20805@n2100.armlinux.org.uk> Message-ID: <59ED6179.5020301@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Ard, On 10/22/2017 09:01 PM, Ard Biesheuvel wrote: > On 22 October 2017 at 13:47, Russell King - ARM Linux > wrote: >> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote: >>> On 18 October 2017 at 06:01, 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 >>>> --- >>>> >>>> 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. > > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751417AbdJWD1M (ORCPT ); Sun, 22 Oct 2017 23:27:12 -0400 Received: from regular1.263xmail.com ([211.150.99.138]:60288 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751082AbdJWD1K (ORCPT ); Sun, 22 Oct 2017 23:27:10 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: ard.biesheuvel@linaro.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <59ED6179.5020301@rock-chips.com> Date: Mon, 23 Oct 2017 11:26:49 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Ard Biesheuvel , Russell King - ARM Linux CC: "linux-kernel@vger.kernel.org" , chris.zhong@rock-chips.com, Ingo Molnar , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled References: <20171018050108.10352-1-jeffy.chen@rock-chips.com> <20171022124757.GL20805@n2100.armlinux.org.uk> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ard, On 10/22/2017 09:01 PM, Ard Biesheuvel wrote: > On 22 October 2017 at 13:47, Russell King - ARM Linux > wrote: >> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote: >>> On 18 October 2017 at 06:01, 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 >>>> --- >>>> >>>> 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. > > >