[PATCH v5 0/2] kexec: Fix kexec_file_load for llvm16

[PATCH v5 0/2] kexec: Fix kexec_file_load for llvm16

[Ricardo Ribalda]

When upreving llvm I realised that kexec stopped working on my test
platform. This patch fixes it.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v5:
- Add warning when multiple text sections are found. Thanks Simon!
- Add Fixes tag.
- Link to v4: https://lore.kernel.org/r/20230321-kexec_clang16-v4-0-1340518f98e9@chromium.org

Changes in v4:
- Add Cc: stable
- Add linker script for x86
- Add a warning when the kernel image has overlapping sections.
- Link to v3: https://lore.kernel.org/r/20230321-kexec_clang16-v3-0-5f016c8d0e87@chromium.org

Changes in v3:
- Fix initial value. Thanks Ross!
- Link to v2: https://lore.kernel.org/r/20230321-kexec_clang16-v2-0-d10e5d517869@chromium.org

Changes in v2:
- Fix if condition. Thanks Steven!.
- Update Philipp email. Thanks Baoquan.
- Link to v1: https://lore.kernel.org/r/20230321-kexec_clang16-v1-0-a768fc2c7c4d@chromium.org

---
Ricardo Ribalda (2):
      kexec: Support purgatories with .text.hot sections
      x86/purgatory: Add linker script

 arch/x86/purgatory/.gitignore        |  2 ++
 arch/x86/purgatory/Makefile          | 20 +++++++++----
 arch/x86/purgatory/kexec-purgatory.S |  2 +-
 arch/x86/purgatory/purgatory.lds.S   | 57 ++++++++++++++++++++++++++++++++++++
 kernel/kexec_file.c                  | 14 ++++++++-
 5 files changed, 87 insertions(+), 8 deletions(-)
---
base-commit: 17214b70a159c6547df9ae204a6275d983146f6b
change-id: 20230321-kexec_clang16-4510c23d129c

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v5 1/2] kexec: Support purgatories with .text.hot sections

[Ricardo Ribalda]

Clang16 links the purgatory text in two sections:

  [ 1] .text             PROGBITS         0000000000000000  00000040
       00000000000011a1  0000000000000000  AX       0     0     16
  [ 2] .rela.text        RELA             0000000000000000  00003498
       0000000000000648  0000000000000018   I      24     1     8
  ...
  [17] .text.hot.        PROGBITS         0000000000000000  00003220
       000000000000020b  0000000000000000  AX       0     0     1
  [18] .rela.text.hot.   RELA             0000000000000000  00004428
       0000000000000078  0000000000000018   I      24    17     8

And both of them have their range [sh_addr ... sh_addr+sh_size] on the
area pointed by `e_entry`.

This causes that image->start is calculated twice, once for .text and
another time for .text.hot. The second calculation leaves image->start
in a random location.

Because of this, the system crashes immediately after:

kexec_core: Starting new kernel

Cc: stable@vger.kernel.org
Fixes: 930457057abe ("kernel/kexec_file.c: split up __kexec_load_puragory")
Reviewed-by: Ross Zwisler <zwisler@google.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 kernel/kexec_file.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f1a0e4e3fb5c..c7a0e51a6d87 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -901,10 +901,22 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
 		}
 
 		offset = ALIGN(offset, align);
+
+		/*
+		 * Check if the segment contains the entry point, if so,
+		 * calculate the value of image->start based on it.
+		 * If the compiler has produced more than one .text section
+		 * (Eg: .text.hot), they are generally after the main .text
+		 * section, and they shall not be used to calculate
+		 * image->start. So do not re-calculate image->start if it
+		 * is not set to the initial value, and warn the user so they
+		 * have a chance to fix their purgatory's linker script.
+		 */
 		if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
 		    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
 		    pi->ehdr->e_entry < (sechdrs[i].sh_addr
-					 + sechdrs[i].sh_size)) {
+					 + sechdrs[i].sh_size) &&
+		    !WARN_ON(kbuf->image->start != pi->ehdr->e_entry)) {
 			kbuf->image->start -= sechdrs[i].sh_addr;
 			kbuf->image->start += kbuf->mem + offset;
 		}

-- 
2.40.0.348.gf938b09366-goog

[PATCH v5 2/2] x86/purgatory: Add linker script

[Ricardo Ribalda]

Make sure that the .text section is not divided in multiple overlapping
sections. This is not supported by kexec_file.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 arch/x86/purgatory/.gitignore        |  2 ++
 arch/x86/purgatory/Makefile          | 20 +++++++++----
 arch/x86/purgatory/kexec-purgatory.S |  2 +-
 arch/x86/purgatory/purgatory.lds.S   | 57 ++++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/arch/x86/purgatory/.gitignore b/arch/x86/purgatory/.gitignore
index d2be1500671d..1fe71fe5945d 100644
--- a/arch/x86/purgatory/.gitignore
+++ b/arch/x86/purgatory/.gitignore
@@ -1 +1,3 @@
 purgatory.chk
+purgatory.lds
+purgatory
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 17f09dc26381..4dc96d409bec 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -16,10 +16,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
 
 # When linking purgatory.ro with -r unresolved symbols are not checked,
 # also link a purgatory.chk binary without -r to check for unresolved symbols.
-PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
-LDFLAGS_purgatory.ro := -r $(PURGATORY_LDFLAGS)
-LDFLAGS_purgatory.chk := $(PURGATORY_LDFLAGS)
-targets += purgatory.ro purgatory.chk
+PURGATORY_LDFLAGS := -nostdlib -z nodefaultlib
+LDFLAGS_purgatory := -r $(PURGATORY_LDFLAGS) -T
+LDFLAGS_purgatory.chk := -e purgatory_start $(PURGATORY_LDFLAGS)
+
+targets += purgatory.lds purgatory.ro purgatory.chk
 
 # Sanitizer, etc. runtimes are unavailable and cannot be linked here.
 GCOV_PROFILE	:= n
@@ -72,10 +73,17 @@ CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
 AFLAGS_REMOVE_setup-x86_$(BITS).o	+= -Wa,-gdwarf-2
 AFLAGS_REMOVE_entry64.o			+= -Wa,-gdwarf-2
 
-$(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
+OBJCOPYFLAGS_purgatory.ro := -O elf64-x86-64
+OBJCOPYFLAGS_purgatory.ro += --remove-section='*debug*'
+OBJCOPYFLAGS_purgatory.ro += --remove-section='.comment'
+OBJCOPYFLAGS_purgatory.ro += --remove-section='.note.*'
+$(obj)/purgatory.ro: $(obj)/purgatory FORCE
+		$(call if_changed,objcopy)
+
+$(obj)/purgatory.chk: $(obj)/purgatory FORCE
 		$(call if_changed,ld)
 
-$(obj)/purgatory.chk: $(obj)/purgatory.ro FORCE
+$(obj)/purgatory: $(obj)/purgatory.lds $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)
 
 $(obj)/kexec-purgatory.o: $(obj)/purgatory.ro $(obj)/purgatory.chk
diff --git a/arch/x86/purgatory/kexec-purgatory.S b/arch/x86/purgatory/kexec-purgatory.S
index 8530fe93b718..54b0d0b4dc42 100644
--- a/arch/x86/purgatory/kexec-purgatory.S
+++ b/arch/x86/purgatory/kexec-purgatory.S
@@ -5,7 +5,7 @@
 	.align	8
 kexec_purgatory:
 	.globl	kexec_purgatory
-	.incbin	"arch/x86/purgatory/purgatory.ro"
+	.incbin	"arch/x86/purgatory/purgatory"
 .Lkexec_purgatory_end:
 
 	.align	8
diff --git a/arch/x86/purgatory/purgatory.lds.S b/arch/x86/purgatory/purgatory.lds.S
new file mode 100644
index 000000000000..610da88aafa0
--- /dev/null
+++ b/arch/x86/purgatory/purgatory.lds.S
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <asm-generic/vmlinux.lds.h>
+
+OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT)
+
+#undef i386
+
+#include <asm/cache.h>
+#include <asm/page_types.h>
+
+ENTRY(purgatory_start)
+
+SECTIONS
+{
+	. = 0;
+	.head.text : {
+		_head = . ;
+		HEAD_TEXT
+		_ehead = . ;
+	}
+	.rodata : {
+		_rodata = . ;
+		*(.rodata)	 /* read-only data */
+		*(.rodata.*)
+		_erodata = . ;
+	}
+	.text :	{
+		_text = .; 	/* Text */
+		*(.text)
+		*(.text.*)
+		*(.noinstr.text)
+		_etext = . ;
+	}
+	.data :	{
+		_data = . ;
+		*(.data)
+		*(.data.*)
+		*(.bss.efistub)
+		_edata = . ;
+	}
+	. = ALIGN(L1_CACHE_BYTES);
+	.bss : {
+		_bss = . ;
+		*(.bss)
+		*(.bss.*)
+		*(COMMON)
+		. = ALIGN(8);	/* For convenience during zeroing */
+		_ebss = .;
+	}
+
+	/* Sections to be discarded */
+	/DISCARD/ : {
+		*(.eh_frame)
+		*(*__ksymtab*)
+		*(___kcrctab*)
+	}
+}

-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH v5 1/2] kexec: Support purgatories with .text.hot sections

[Steven Rostedt]

On Thu, 30 Mar 2023 11:44:47 +0200
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Clang16 links the purgatory text in two sections:
> 
>   [ 1] .text             PROGBITS         0000000000000000  00000040
>        00000000000011a1  0000000000000000  AX       0     0     16
>   [ 2] .rela.text        RELA             0000000000000000  00003498
>        0000000000000648  0000000000000018   I      24     1     8
>   ...
>   [17] .text.hot.        PROGBITS         0000000000000000  00003220
>        000000000000020b  0000000000000000  AX       0     0     1
>   [18] .rela.text.hot.   RELA             0000000000000000  00004428
>        0000000000000078  0000000000000018   I      24    17     8
> 
> And both of them have their range [sh_addr ... sh_addr+sh_size] on the
> area pointed by `e_entry`.
> 
> This causes that image->start is calculated twice, once for .text and
> another time for .text.hot. The second calculation leaves image->start
> in a random location.
> 
> Because of this, the system crashes immediately after:
> 
> kexec_core: Starting new kernel
> 
> Cc: stable@vger.kernel.org
> Fixes: 930457057abe ("kernel/kexec_file.c: split up __kexec_load_puragory")
> Reviewed-by: Ross Zwisler <zwisler@google.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  kernel/kexec_file.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index f1a0e4e3fb5c..c7a0e51a6d87 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -901,10 +901,22 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
>  		}
>  
>  		offset = ALIGN(offset, align);
> +
> +		/*
> +		 * Check if the segment contains the entry point, if so,
> +		 * calculate the value of image->start based on it.
> +		 * If the compiler has produced more than one .text section
> +		 * (Eg: .text.hot), they are generally after the main .text
> +		 * section, and they shall not be used to calculate
> +		 * image->start. So do not re-calculate image->start if it
> +		 * is not set to the initial value, and warn the user so they
> +		 * have a chance to fix their purgatory's linker script.
> +		 */
>  		if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
>  		    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
>  		    pi->ehdr->e_entry < (sechdrs[i].sh_addr
> -					 + sechdrs[i].sh_size)) {
> +					 + sechdrs[i].sh_size) &&
> +		    !WARN_ON(kbuf->image->start != pi->ehdr->e_entry)) {
>  			kbuf->image->start -= sechdrs[i].sh_addr;
>  			kbuf->image->start += kbuf->mem + offset;
>  		}
> 

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thanks Ricardo!

-- Steve

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Steven Rostedt]

Hmm, this patch may need some more eyes. At least from the x86 maintainers.

-- Steve


On Thu, 30 Mar 2023 11:44:48 +0200
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Make sure that the .text section is not divided in multiple overlapping
> sections. This is not supported by kexec_file.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  arch/x86/purgatory/.gitignore        |  2 ++
>  arch/x86/purgatory/Makefile          | 20 +++++++++----
>  arch/x86/purgatory/kexec-purgatory.S |  2 +-
>  arch/x86/purgatory/purgatory.lds.S   | 57 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/purgatory/.gitignore b/arch/x86/purgatory/.gitignore
> index d2be1500671d..1fe71fe5945d 100644
> --- a/arch/x86/purgatory/.gitignore
> +++ b/arch/x86/purgatory/.gitignore
> @@ -1 +1,3 @@
>  purgatory.chk
> +purgatory.lds
> +purgatory
> diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
> index 17f09dc26381..4dc96d409bec 100644
> --- a/arch/x86/purgatory/Makefile
> +++ b/arch/x86/purgatory/Makefile
> @@ -16,10 +16,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
>  
>  # When linking purgatory.ro with -r unresolved symbols are not checked,
>  # also link a purgatory.chk binary without -r to check for unresolved symbols.
> -PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
> -LDFLAGS_purgatory.ro := -r $(PURGATORY_LDFLAGS)
> -LDFLAGS_purgatory.chk := $(PURGATORY_LDFLAGS)
> -targets += purgatory.ro purgatory.chk
> +PURGATORY_LDFLAGS := -nostdlib -z nodefaultlib
> +LDFLAGS_purgatory := -r $(PURGATORY_LDFLAGS) -T
> +LDFLAGS_purgatory.chk := -e purgatory_start $(PURGATORY_LDFLAGS)
> +
> +targets += purgatory.lds purgatory.ro purgatory.chk
>  
>  # Sanitizer, etc. runtimes are unavailable and cannot be linked here.
>  GCOV_PROFILE	:= n
> @@ -72,10 +73,17 @@ CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
>  AFLAGS_REMOVE_setup-x86_$(BITS).o	+= -Wa,-gdwarf-2
>  AFLAGS_REMOVE_entry64.o			+= -Wa,-gdwarf-2
>  
> -$(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
> +OBJCOPYFLAGS_purgatory.ro := -O elf64-x86-64
> +OBJCOPYFLAGS_purgatory.ro += --remove-section='*debug*'
> +OBJCOPYFLAGS_purgatory.ro += --remove-section='.comment'
> +OBJCOPYFLAGS_purgatory.ro += --remove-section='.note.*'
> +$(obj)/purgatory.ro: $(obj)/purgatory FORCE
> +		$(call if_changed,objcopy)
> +
> +$(obj)/purgatory.chk: $(obj)/purgatory FORCE
>  		$(call if_changed,ld)
>  
> -$(obj)/purgatory.chk: $(obj)/purgatory.ro FORCE
> +$(obj)/purgatory: $(obj)/purgatory.lds $(PURGATORY_OBJS) FORCE
>  		$(call if_changed,ld)
>  
>  $(obj)/kexec-purgatory.o: $(obj)/purgatory.ro $(obj)/purgatory.chk
> diff --git a/arch/x86/purgatory/kexec-purgatory.S b/arch/x86/purgatory/kexec-purgatory.S
> index 8530fe93b718..54b0d0b4dc42 100644
> --- a/arch/x86/purgatory/kexec-purgatory.S
> +++ b/arch/x86/purgatory/kexec-purgatory.S
> @@ -5,7 +5,7 @@
>  	.align	8
>  kexec_purgatory:
>  	.globl	kexec_purgatory
> -	.incbin	"arch/x86/purgatory/purgatory.ro"
> +	.incbin	"arch/x86/purgatory/purgatory"
>  .Lkexec_purgatory_end:
>  
>  	.align	8
> diff --git a/arch/x86/purgatory/purgatory.lds.S b/arch/x86/purgatory/purgatory.lds.S
> new file mode 100644
> index 000000000000..610da88aafa0
> --- /dev/null
> +++ b/arch/x86/purgatory/purgatory.lds.S
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <asm-generic/vmlinux.lds.h>
> +
> +OUTPUT_FORMAT(CONFIG_OUTPUT_FORMAT)
> +
> +#undef i386
> +
> +#include <asm/cache.h>
> +#include <asm/page_types.h>
> +
> +ENTRY(purgatory_start)
> +
> +SECTIONS
> +{
> +	. = 0;
> +	.head.text : {
> +		_head = . ;
> +		HEAD_TEXT
> +		_ehead = . ;
> +	}
> +	.rodata : {
> +		_rodata = . ;
> +		*(.rodata)	 /* read-only data */
> +		*(.rodata.*)
> +		_erodata = . ;
> +	}
> +	.text :	{
> +		_text = .; 	/* Text */
> +		*(.text)
> +		*(.text.*)
> +		*(.noinstr.text)
> +		_etext = . ;
> +	}
> +	.data :	{
> +		_data = . ;
> +		*(.data)
> +		*(.data.*)
> +		*(.bss.efistub)
> +		_edata = . ;
> +	}
> +	. = ALIGN(L1_CACHE_BYTES);
> +	.bss : {
> +		_bss = . ;
> +		*(.bss)
> +		*(.bss.*)
> +		*(COMMON)
> +		. = ALIGN(8);	/* For convenience during zeroing */
> +		_ebss = .;
> +	}
> +
> +	/* Sections to be discarded */
> +	/DISCARD/ : {
> +		*(.eh_frame)
> +		*(*__ksymtab*)
> +		*(___kcrctab*)
> +	}
> +}
> 

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Borislav Petkov]

On Thu, Mar 30, 2023 at 11:15:23AM -0400, Steven Rostedt wrote:
> > Make sure that the .text section is not divided in multiple overlapping
> > sections. This is not supported by kexec_file.

And?

What is the failure scenario? Why are you fixing it? Why do we care?

This is way too laconic.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Steven Rostedt]

On Thu, 30 Mar 2023 17:18:26 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Mar 30, 2023 at 11:15:23AM -0400, Steven Rostedt wrote:
> > > Make sure that the .text section is not divided in multiple overlapping
> > > sections. This is not supported by kexec_file.  
> 
> And?
> 
> What is the failure scenario? Why are you fixing it? Why do we care?
> 
> This is way too laconic.
> 

Yeah, I think the change log in patch 1 needs to be in this patch too,
which gives better context.

-- Steve

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Borislav Petkov]

On Thu, Mar 30, 2023 at 11:31:27AM -0400, Steven Rostedt wrote:
> On Thu, 30 Mar 2023 17:18:26 +0200
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Thu, Mar 30, 2023 at 11:15:23AM -0400, Steven Rostedt wrote:
> > > > Make sure that the .text section is not divided in multiple overlapping
> > > > sections. This is not supported by kexec_file.  
> > 
> > And?
> > 
> > What is the failure scenario? Why are you fixing it? Why do we care?
> > 
> > This is way too laconic.
> > 
> 
> Yeah, I think the change log in patch 1 needs to be in this patch too,
> which gives better context.

Just read it.

Why did it work with clang version < 16?

+ toolchains ML.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Ross Zwisler]

On Thu, Mar 30, 2023 at 3:45 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
> Make sure that the .text section is not divided in multiple overlapping
> sections. This is not supported by kexec_file.

How does this interact with patch #1 from this series, which IIUC
allows us to handle the case where the .text section is split between
.text and .text.hot?  Do we still need that patch, but only for
non-x86 platforms?  Or do we need both, and this patch will need to be
replicated for other arches?

Re: [PATCH v5 1/2] kexec: Support purgatories with .text.hot sections

[Philipp Rudo]

Hi Ricardo,

On Thu, 30 Mar 2023 11:44:47 +0200
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Clang16 links the purgatory text in two sections:
> 
>   [ 1] .text             PROGBITS         0000000000000000  00000040
>        00000000000011a1  0000000000000000  AX       0     0     16
>   [ 2] .rela.text        RELA             0000000000000000  00003498
>        0000000000000648  0000000000000018   I      24     1     8
>   ...
>   [17] .text.hot.        PROGBITS         0000000000000000  00003220
>        000000000000020b  0000000000000000  AX       0     0     1
>   [18] .rela.text.hot.   RELA             0000000000000000  00004428
>        0000000000000078  0000000000000018   I      24    17     8
> 
> And both of them have their range [sh_addr ... sh_addr+sh_size] on the
> area pointed by `e_entry`.
> 
> This causes that image->start is calculated twice, once for .text and
> another time for .text.hot. The second calculation leaves image->start
> in a random location.
> 
> Because of this, the system crashes immediately after:
> 
> kexec_core: Starting new kernel
> 
> Cc: stable@vger.kernel.org
> Fixes: 930457057abe ("kernel/kexec_file.c: split up __kexec_load_puragory")
> Reviewed-by: Ross Zwisler <zwisler@google.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  kernel/kexec_file.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index f1a0e4e3fb5c..c7a0e51a6d87 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -901,10 +901,22 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
>  		}
>  
>  		offset = ALIGN(offset, align);
> +
> +		/*
> +		 * Check if the segment contains the entry point, if so,
> +		 * calculate the value of image->start based on it.
> +		 * If the compiler has produced more than one .text section
> +		 * (Eg: .text.hot), they are generally after the main .text
> +		 * section, and they shall not be used to calculate
> +		 * image->start. So do not re-calculate image->start if it
> +		 * is not set to the initial value, and warn the user so they
> +		 * have a chance to fix their purgatory's linker script.
> +		 */
>  		if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
>  		    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
>  		    pi->ehdr->e_entry < (sechdrs[i].sh_addr
> -					 + sechdrs[i].sh_size)) {
> +					 + sechdrs[i].sh_size) &&
> +		    !WARN_ON(kbuf->image->start != pi->ehdr->e_entry)) {

Looks good to me. I'm not sure if it is better to use WARN_ON_ONCE to
avoid spamming the log when there are more than two .text sections or
when you reload the kernel. But that's only a rare corner case. So no
strong opinion from my side. Either way 

Reviewed-by: Philipp Rudo <prudo@redhat.com>

>  			kbuf->image->start -= sechdrs[i].sh_addr;
>  			kbuf->image->start += kbuf->mem + offset;
>  		}
> 

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Ricardo Ribalda]

Hi Ross

On Fri, 31 Mar 2023 at 21:14, Ross Zwisler <zwisler@kernel.org> wrote:
>
> On Thu, Mar 30, 2023 at 3:45 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
> > Make sure that the .text section is not divided in multiple overlapping
> > sections. This is not supported by kexec_file.
>
> How does this interact with patch #1 from this series, which IIUC
> allows us to handle the case where the .text section is split between
> .text and .text.hot?  Do we still need that patch, but only for
> non-x86 platforms?  Or do we need both, and this patch will need to be
> replicated for other arches?

Patch 1/2 is a must. Patch 2/2 is a nice_to_have and would be great to
have a similar patch for every arch... but I do not feel confident
enough to send it for every arch :)

If we have linker scripts for all the arches do we need 1/2?
I think so, because the user might want to load a kernel <6.4 built
with clang > 16.

Regards!


-- 
Ricardo Ribalda

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Nick Desaulniers]

Hi Ricardo,
Thanks for the patch!  Please make sure to cc our mailing list
<llvm@lists.linux.dev> for llvm specific issues.
scripts/get_maintainer.pl should recommend it, or you can find it from
clangbuiltlinux.github.io.  You can also ping me internally for
toolchain related issues.

Start of thread.
https://lore.kernel.org/lkml/20230321-kexec_clang16-v5-0-5563bf7c4173@chromium.org/

On Thu, Mar 30, 2023 at 9:00 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Mar 30, 2023 at 11:31:27AM -0400, Steven Rostedt wrote:
> > On Thu, 30 Mar 2023 17:18:26 +0200
> > Borislav Petkov <bp@alien8.de> wrote:
> >
> > > On Thu, Mar 30, 2023 at 11:15:23AM -0400, Steven Rostedt wrote:
> > > > > Make sure that the .text section is not divided in multiple overlapping
> > > > > sections. This is not supported by kexec_file.

Perhaps this is related to CrOS' use of AutoFDO creating .text.hot?
If so, it's probably more straightforward to straight up disable PGO
for kexec. See also:

commit bde971a83bbf ("KVM: arm64: nvhe: Fix build with profile optimization")

> > >
> > > And?
> > >
> > > What is the failure scenario? Why are you fixing it? Why do we care?
> > >
> > > This is way too laconic.
> > >
> >
> > Yeah, I think the change log in patch 1 needs to be in this patch too,
> > which gives better context.
>
> Just read it.
>
> Why did it work with clang version < 16?

I'll bet if we bisect llvm, we can spot what might have changed, which
may give us a clue on how to get the old behavior back; maybe without
the need for a linker script.

Ricardo, how did you verify that your fix was correct? Surely we can
check using command line utilities without needing a full blown kexec
setup? If you can share more info, I can bisect llvm quickly.  If it
requires profile data, you'll need to share it, since CrOS engineers
still have not posted public documentation on AutoFDO as I have
repeatedly asked for.

>
> + toolchains ML.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Ricardo Ribalda]

Hi Nick

On Sat, 8 Apr 2023 at 01:22, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Hi Ricardo,
> Thanks for the patch!  Please make sure to cc our mailing list
> <llvm@lists.linux.dev> for llvm specific issues.
> scripts/get_maintainer.pl should recommend it, or you can find it from
> clangbuiltlinux.github.io.  You can also ping me internally for
> toolchain related issues.
>
> Start of thread.
> https://lore.kernel.org/lkml/20230321-kexec_clang16-v5-0-5563bf7c4173@chromium.org/
>
> On Thu, Mar 30, 2023 at 9:00 AM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Thu, Mar 30, 2023 at 11:31:27AM -0400, Steven Rostedt wrote:
> > > On Thu, 30 Mar 2023 17:18:26 +0200
> > > Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > > On Thu, Mar 30, 2023 at 11:15:23AM -0400, Steven Rostedt wrote:
> > > > > > Make sure that the .text section is not divided in multiple overlapping
> > > > > > sections. This is not supported by kexec_file.
>
> Perhaps this is related to CrOS' use of AutoFDO creating .text.hot?
> If so, it's probably more straightforward to straight up disable PGO
> for kexec. See also:
>
> commit bde971a83bbf ("KVM: arm64: nvhe: Fix build with profile optimization")

It was indeed due to the AutoFDO, adding

KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=% -fprofile-use=%,
$(KBUILD_CFLAGS))

to arch/x86/purgatory/Makefile

It is definitely simpler than adding a linker script, but I am not
sure if it is the correct way to fix this... Seems like splitting
.text in multiple sections is an implementation detail of the compiler
and the only way to force it is with a linker script... Or am I
missing something?

Shall I send a new version with the KBUILD_CFLAGS ?

Thanks!

>
> > > >
> > > > And?
> > > >
> > > > What is the failure scenario? Why are you fixing it? Why do we care?
> > > >
> > > > This is way too laconic.
> > > >
> > >
> > > Yeah, I think the change log in patch 1 needs to be in this patch too,
> > > which gives better context.
> >
> > Just read it.
> >
> > Why did it work with clang version < 16?
>
> I'll bet if we bisect llvm, we can spot what might have changed, which
> may give us a clue on how to get the old behavior back; maybe without
> the need for a linker script.
>
> Ricardo, how did you verify that your fix was correct? Surely we can
> check using command line utilities without needing a full blown kexec
> setup? If you can share more info, I can bisect llvm quickly.  If it
> requires profile data, you'll need to share it, since CrOS engineers
> still have not posted public documentation on AutoFDO as I have
> repeatedly asked for.

The simplest test is to run:

$readelf -S arch/x86/purgatory/purgatory.ro | grep "] \.text"
[ 3] .text             PROGBITS         0000000000000000  000002a0

If there is only one .text section then that kernel will be load
properly via kexec_file().



>
> >
> > + toolchains ML.
> >
> > --
> > Regards/Gruss,
> >     Boris.
> >
> > https://people.kernel.org/tglx/notes-about-netiquette
>
>
>
> --
> Thanks,
> ~Nick Desaulniers



--
Ricardo Ribalda

Re: [PATCH v5 2/2] x86/purgatory: Add linker script

[Nick Desaulniers]

On Tue, Apr 11, 2023 at 2:46 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Nick
>
> On Sat, 8 Apr 2023 at 01:22, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > Hi Ricardo,
> > Thanks for the patch!  Please make sure to cc our mailing list
> > <llvm@lists.linux.dev> for llvm specific issues.
> > scripts/get_maintainer.pl should recommend it, or you can find it from
> > clangbuiltlinux.github.io.  You can also ping me internally for
> > toolchain related issues.
> >
> > Start of thread.
> > https://lore.kernel.org/lkml/20230321-kexec_clang16-v5-0-5563bf7c4173@chromium.org/
> >
> > On Thu, Mar 30, 2023 at 9:00 AM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Thu, Mar 30, 2023 at 11:31:27AM -0400, Steven Rostedt wrote:
> > > > On Thu, 30 Mar 2023 17:18:26 +0200
> > > > Borislav Petkov <bp@alien8.de> wrote:
> > > >
> > > > > On Thu, Mar 30, 2023 at 11:15:23AM -0400, Steven Rostedt wrote:
> > > > > > > Make sure that the .text section is not divided in multiple overlapping
> > > > > > > sections. This is not supported by kexec_file.
> >
> > Perhaps this is related to CrOS' use of AutoFDO creating .text.hot?
> > If so, it's probably more straightforward to straight up disable PGO
> > for kexec. See also:
> >
> > commit bde971a83bbf ("KVM: arm64: nvhe: Fix build with profile optimization")
>
> It was indeed due to the AutoFDO, adding
>
> KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=% -fprofile-use=%,
> $(KBUILD_CFLAGS))
>
> to arch/x86/purgatory/Makefile
>
> It is definitely simpler than adding a linker script, but I am not
> sure if it is the correct way to fix this... Seems like splitting
> .text in multiple sections is an implementation detail of the compiler
> and the only way to force it is with a linker script... Or am I
> missing something?

I think with the use of `unlikely` GCC will put code in .text.cold, so
it is possible to trigger this using simpler means, but...

>
> Shall I send a new version with the KBUILD_CFLAGS ?

I still think the cflags approach is way simpler.  If someone tries to
use unlikely in purgatory: "don't do that."  Same for PGO.

>
> Thanks!
>
> >
> > > > >
> > > > > And?
> > > > >
> > > > > What is the failure scenario? Why are you fixing it? Why do we care?
> > > > >
> > > > > This is way too laconic.
> > > > >
> > > >
> > > > Yeah, I think the change log in patch 1 needs to be in this patch too,
> > > > which gives better context.
> > >
> > > Just read it.
> > >
> > > Why did it work with clang version < 16?
> >
> > I'll bet if we bisect llvm, we can spot what might have changed, which
> > may give us a clue on how to get the old behavior back; maybe without
> > the need for a linker script.
> >
> > Ricardo, how did you verify that your fix was correct? Surely we can
> > check using command line utilities without needing a full blown kexec
> > setup? If you can share more info, I can bisect llvm quickly.  If it
> > requires profile data, you'll need to share it, since CrOS engineers
> > still have not posted public documentation on AutoFDO as I have
> > repeatedly asked for.
>
> The simplest test is to run:
>
> $readelf -S arch/x86/purgatory/purgatory.ro | grep "] \.text"
> [ 3] .text             PROGBITS         0000000000000000  000002a0
>
> If there is only one .text section then that kernel will be load
> properly via kexec_file().

Got it, profile data will be required to reproduce then. If you can share.
-- 
Thanks,
~Nick Desaulniers