Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1526162 > unrolled thread

Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as  PIE. Dave Young <dyoung@redhat.com> - 2016-11-20 03:50 +0100
    Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE. Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - 2016-11-22 00:50 +0100
      Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as  PIE. Dave Young <dyoung@redhat.com> - 2016-11-22 02:40 +0100
      Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE. Michael Ellerman <mpe@ellerman.id.au> - 2016-11-22 07:10 +0100
        Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as  PIE. Dave Young <dyoung@redhat.com> - 2016-11-22 07:20 +0100
        Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE. Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - 2016-11-22 14:50 +0100
          Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as  PIE. Dave Young <dyoung@redhat.com> - 2016-11-23 02:40 +0100
            Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE. Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - 2016-11-23 04:00 +0100
      Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as  PIE. Dave Young <dyoung@redhat.com> - 2016-11-23 10:00 +0100

#1526162 — Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE.

On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> powerpc's purgatory.ro has 12 relocation types when built as
> a relocatable object. To implement support for them requires
> arch_kexec_apply_relocations_add to duplicate a lot of code with
> module_64.c:apply_relocate_add.
> 
> When built as a Position Independent Executable there are only 4
> relocation types in purgatory.ro, so it becomes practical for the powerpc
> implementation of kexec_file to have its own relocation implementation.
> 
> Also, the purgatory is an executable and not an intermediary output from
> the compiler so it makes sense conceptually that it is easier to build
> it as a PIE than as a partially linked object.
> 
> Apart from the greatly reduced number of relocations, there are two
> differences between a relocatable object and a PIE:
> 
> 1. __kexec_load_purgatory needs to use the program headers rather than the
>    section headers to figure out how to load the binary.
> 2. Symbol values are absolute addresses instead of relative to the
>    start of the section.
> 
> This patch adds the support needed in generic code for the differences
> above and allows powerpc to load and relocate a position independent
> purgatory.
> 

[snip]

The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
not that complex. So could you look into simplify your kexec_file
implementation?

kernel/kexec_file.c kexec_apply_relocations only do limited things
and some of the logic is in arch/x86, so move general code out of arch
code, then I guess the arch code will be simpler and then we probably
do not need this PIE stuff anymore.

BTW, __kexec_really_load_purgatory looks worse than
___kexec_load_purgatory ;)

Thanks
Dave

[toc] | [next] | [standalone]

#1527132 — Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE.

Hello Dave,

Thanks for your review.

Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> > powerpc's purgatory.ro has 12 relocation types when built as
> > a relocatable object. To implement support for them requires
> > arch_kexec_apply_relocations_add to duplicate a lot of code with
> > module_64.c:apply_relocate_add.
> > 
> > When built as a Position Independent Executable there are only 4
> > relocation types in purgatory.ro, so it becomes practical for the powerpc
> > implementation of kexec_file to have its own relocation implementation.
> > 
> > Also, the purgatory is an executable and not an intermediary output from
> > the compiler so it makes sense conceptually that it is easier to build
> > it as a PIE than as a partially linked object.
> > 
> > Apart from the greatly reduced number of relocations, there are two
> > differences between a relocatable object and a PIE:
> > 
> > 1. __kexec_load_purgatory needs to use the program headers rather than the
> > 
> >    section headers to figure out how to load the binary.
> > 
> > 2. Symbol values are absolute addresses instead of relative to the
> > 
> >    start of the section.
> > 
> > This patch adds the support needed in generic code for the differences
> > above and allows powerpc to load and relocate a position independent
> > purgatory.
> 
> [snip]
> 
> The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
> not that complex. So could you look into simplify your kexec_file
> implementation?

I can try, but there is one fundamental issue here: powerpc position-dependent 
code relies more on relocations than x86 position-dependent code does, so 
there's a limit to how simple it can be made without switching to position-
independent code. And it will always be more involved than it is on x86.

BTW, building x86's purgatory as PIE results in it not having any relocation 
at all, so it's an advantage even in that architecture. Unfortunately, the 
machine locks up during reboot and I didn't have time to try to figure out 
what's going on.

> kernel/kexec_file.c kexec_apply_relocations only do limited things
> and some of the logic is in arch/x86, so move general code out of arch
> code, then I guess the arch code will be simpler

I agree that is a good idea. Is the patch below what you had in mind?

> and then we probably do not need this PIE stuff anymore.

If you are ok with the patch below I can post a new version of the series 
based on it and we can see if Michael Ellerman thinks it is enough.

> BTW, __kexec_really_load_purgatory looks worse than
> ___kexec_load_purgatory ;)

Really? I find the special handling of bss makes the section-based loader a 
bit more confusing.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center


Subject: [PATCH] kexec_file: Move generic relocation code from arch/x86 to
 kernel/kexec_file.c

The check for undefined symbols stays in arch-specific code because
powerpc needs to allow TOC symbols to be processed even though they're
undefined.

There is no functional change.

Suggested-by: Dave Young <dyoung@redhat.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 arch/x86/kernel/machine_kexec_64.c | 160 +++++++------------------------------
 include/linux/kexec.h              |   9 ++-
 kernel/kexec_file.c                | 120 +++++++++++++++++++++++++++-
 3 files changed, 154 insertions(+), 135 deletions(-)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 8c1f218926d7..f4860c408ece 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -401,143 +401,45 @@ int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel,
 }
 #endif
 
-/*
- * Apply purgatory relocations.
- *
- * ehdr: Pointer to elf headers
- * sechdrs: Pointer to section headers.
- * relsec: section index of SHT_RELA section.
- *
- * TODO: Some of the code belongs to generic code. Move that in kexec.c.
- */
-int arch_kexec_apply_relocations_add(const Elf64_Ehdr *ehdr,
-				     Elf64_Shdr *sechdrs, unsigned int relsec)
+int arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+				    unsigned int reltype, Elf_Sym *sym,
+				    const char *name, unsigned long *location,
+				    unsigned long address, unsigned long value)
 {
-	unsigned int i;
-	Elf64_Rela *rel;
-	Elf64_Sym *sym;
-	void *location;
-	Elf64_Shdr *section, *symtabsec;
-	unsigned long address, sec_base, value;
-	const char *strtab, *name, *shstrtab;
-
-	/*
-	 * ->sh_offset has been modified to keep the pointer to section
-	 * contents in memory
-	 */
-	rel = (void *)sechdrs[relsec].sh_offset;
-
-	/* Section to which relocations apply */
-	section = &sechdrs[sechdrs[relsec].sh_info];
-
-	pr_debug("Applying relocate section %u to %u\n", relsec,
-		 sechdrs[relsec].sh_info);
-
-	/* Associated symbol table */
-	symtabsec = &sechdrs[sechdrs[relsec].sh_link];
-
-	/* String table */
-	if (symtabsec->sh_link >= ehdr->e_shnum) {
-		/* Invalid strtab section number */
-		pr_err("Invalid string table section index %d\n",
-		       symtabsec->sh_link);
+	if (sym->st_shndx == SHN_UNDEF) {
+		pr_err("Undefined symbol: %s\n", name);
 		return -ENOEXEC;
 	}
 
-	strtab = (char *)sechdrs[symtabsec->sh_link].sh_offset;
-
-	/* section header string table */
-	shstrtab = (char *)sechdrs[ehdr->e_shstrndx].sh_offset;
-
-	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
-
-		/*
-		 * rel[i].r_offset contains byte offset from beginning
-		 * of section to the storage unit affected.
-		 *
-		 * This is location to update (->sh_offset). This is temporary
-		 * buffer where section is currently loaded. This will finally
-		 * be loaded to a different address later, pointed to by
-		 * ->sh_addr. kexec takes care of moving it
-		 *  (kexec_load_segment()).
-		 */
-		location = (void *)(section->sh_offset + rel[i].r_offset);
-
-		/* Final address of the location */
-		address = section->sh_addr + rel[i].r_offset;
-
-		/*
-		 * rel[i].r_info contains information about symbol table index
-		 * w.r.t which relocation must be made and type of relocation
-		 * to apply. ELF64_R_SYM() and ELF64_R_TYPE() macros get
-		 * these respectively.
-		 */
-		sym = (Elf64_Sym *)symtabsec->sh_offset +
-				ELF64_R_SYM(rel[i].r_info);
-
-		if (sym->st_name)
-			name = strtab + sym->st_name;
-		else
-			name = shstrtab + sechdrs[sym->st_shndx].sh_name;
-
-		pr_debug("Symbol: %s info: %02x shndx: %02x value=%llx size: %llx\n",
-			 name, sym->st_info, sym->st_shndx, sym->st_value,
-			 sym->st_size);
-
-		if (sym->st_shndx == SHN_UNDEF) {
-			pr_err("Undefined symbol: %s\n", name);
-			return -ENOEXEC;
-		}
-
-		if (sym->st_shndx == SHN_COMMON) {
-			pr_err("symbol '%s' in common section\n", name);
-			return -ENOEXEC;
-		}
-
-		if (sym->st_shndx == SHN_ABS)
-			sec_base = 0;
-		else if (sym->st_shndx >= ehdr->e_shnum) {
-			pr_err("Invalid section %d for symbol %s\n",
-			       sym->st_shndx, name);
-			return -ENOEXEC;
-		} else
-			sec_base = sechdrs[sym->st_shndx].sh_addr;
-
-		value = sym->st_value;
-		value += sec_base;
-		value += rel[i].r_addend;
-
-		switch (ELF64_R_TYPE(rel[i].r_info)) {
-		case R_X86_64_NONE:
-			break;
-		case R_X86_64_64:
-			*(u64 *)location = value;
-			break;
-		case R_X86_64_32:
-			*(u32 *)location = value;
-			if (value != *(u32 *)location)
-				goto overflow;
-			break;
-		case R_X86_64_32S:
-			*(s32 *)location = value;
-			if ((s64)value != *(s32 *)location)
-				goto overflow;
-			break;
-		case R_X86_64_PC32:
-			value -= (u64)address;
-			*(u32 *)location = value;
-			break;
-		default:
-			pr_err("Unknown rela relocation: %llu\n",
-			       ELF64_R_TYPE(rel[i].r_info));
-			return -ENOEXEC;
-		}
+	switch (reltype) {
+	case R_X86_64_NONE:
+		break;
+	case R_X86_64_64:
+		*(u64 *)location = value;
+		break;
+	case R_X86_64_32:
+		*(u32 *)location = value;
+		if (value != *(u32 *)location)
+			goto overflow;
+		break;
+	case R_X86_64_32S:
+		*(s32 *)location = value;
+		if ((s64)value != *(s32 *)location)
+			goto overflow;
+		break;
+	case R_X86_64_PC32:
+		value -= (u64)address;
+		*(u32 *)location = value;
+		break;
+	default:
+		pr_err("Unknown rela relocation: %u\n", reltype);
+		return -ENOEXEC;
 	}
+
 	return 0;
 
 overflow:
-	pr_err("Overflow in relocation type %d value 0x%lx\n",
-	       (int)ELF64_R_TYPE(rel[i].r_info), value);
+	pr_err("Overflow in relocation type %u value 0x%lx\n", reltype, value);
 	return -ENOEXEC;
 }
 #endif /* CONFIG_KEXEC_FILE */
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 406c33dcae13..e171a083540d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -320,8 +320,13 @@ void * __weak arch_kexec_kernel_image_load(struct kimage *image);
 int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
 int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
 					unsigned long buf_len);
-int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
-					Elf_Shdr *sechdrs, unsigned int relsec);
+int __weak arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr,
+					   Elf_Shdr *sechdrs,
+					   unsigned int reltype, Elf_Sym *sym,
+					   const char *name,
+					   unsigned long *location,
+					   unsigned long address,
+					   unsigned long value);
 int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 					unsigned int relsec);
 void arch_kexec_protect_crashkres(void);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 037c321c5618..1517f977cc25 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -61,8 +61,10 @@ int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
 
 /* Apply relocations of type RELA */
 int __weak
-arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
-				 unsigned int relsec)
+arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+				unsigned int reltype, Elf_Sym *sym,
+				const char *name, unsigned long *location,
+				unsigned long address, unsigned long value)
 {
 	pr_err("RELA relocation unsupported.\n");
 	return -ENOEXEC;
@@ -793,6 +795,117 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	return ret;
 }
 
+/**
+ * kexec_apply_relocations_add - apply purgatory relocations
+ * @ehdr:	Pointer to elf headers
+ * @sechdrs:	Pointer to section headers.
+ * @relsec:	Section index of SHT_RELA section.
+ */
+static int kexec_apply_relocations_add(const Elf64_Ehdr *ehdr,
+				       Elf64_Shdr *sechdrs, unsigned int relsec)
+{
+	int ret;
+	unsigned int i;
+	Elf64_Rela *rel;
+	Elf64_Sym *sym;
+	void *location;
+	Elf64_Shdr *section, *symtabsec;
+	unsigned long address, sec_base, value;
+	const char *strtab, *name, *shstrtab;
+
+	/*
+	 * ->sh_offset has been modified to keep the pointer to section
+	 * contents in memory
+	 */
+	rel = (void *)sechdrs[relsec].sh_offset;
+
+	/* Section to which relocations apply */
+	section = &sechdrs[sechdrs[relsec].sh_info];
+
+	pr_debug("Applying relocate section %u to %u\n", relsec,
+		 sechdrs[relsec].sh_info);
+
+	/* Associated symbol table */
+	symtabsec = &sechdrs[sechdrs[relsec].sh_link];
+
+	/* String table */
+	if (symtabsec->sh_link >= ehdr->e_shnum) {
+		/* Invalid strtab section number */
+		pr_err("Invalid string table section index %d\n",
+		       symtabsec->sh_link);
+		return -ENOEXEC;
+	}
+
+	/* String table for the associated symbol table. */
+	strtab = (char *)sechdrs[symtabsec->sh_link].sh_offset;
+
+	/* section header string table */
+	shstrtab = (char *)sechdrs[ehdr->e_shstrndx].sh_offset;
+
+	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
+
+		/*
+		 * rel[i].r_offset contains byte offset from beginning
+		 * of section to the storage unit affected.
+		 *
+		 * This is location to update (->sh_offset). This is temporary
+		 * buffer where section is currently loaded. This will finally
+		 * be loaded to a different address later, pointed to by
+		 * ->sh_addr. kexec takes care of moving it
+		 *  (kexec_load_segment()).
+		 */
+		location = (void *)(section->sh_offset + rel[i].r_offset);
+
+		/* Final address of the location */
+		address = section->sh_addr + rel[i].r_offset;
+
+		/*
+		 * rel[i].r_info contains information about symbol table index
+		 * w.r.t which relocation must be made and type of relocation
+		 * to apply. ELF64_R_SYM() and ELF64_R_TYPE() macros get
+		 * these respectively.
+		 */
+		sym = (Elf64_Sym *)symtabsec->sh_offset +
+				ELF64_R_SYM(rel[i].r_info);
+
+		if (sym->st_name)
+			name = strtab + sym->st_name;
+		else
+			name = shstrtab + sechdrs[sym->st_shndx].sh_name;
+
+		pr_debug("Symbol: %s info: %02x shndx: %02x value=%llx size: %llx\n",
+			 name, sym->st_info, sym->st_shndx, sym->st_value,
+			 sym->st_size);
+
+		if (sym->st_shndx == SHN_COMMON) {
+			pr_err("symbol '%s' in common section\n", name);
+			return -ENOEXEC;
+		}
+
+		if (sym->st_shndx == SHN_ABS)
+			sec_base = 0;
+		else if (sym->st_shndx >= ehdr->e_shnum) {
+			pr_err("Invalid section %d for symbol %s\n",
+			       sym->st_shndx, name);
+			return -ENOEXEC;
+		} else
+			sec_base = sechdrs[sym->st_shndx].sh_addr;
+
+		value = sym->st_value;
+		value += sec_base;
+		value += rel[i].r_addend;
+
+		ret = arch_kexec_apply_relocation_add(ehdr, sechdrs,
+						      ELF64_R_TYPE(rel[i].r_info),
+						      sym, name, location,
+						      address, value);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int kexec_apply_relocations(struct kimage *image)
 {
 	int i, ret;
@@ -836,8 +949,7 @@ static int kexec_apply_relocations(struct kimage *image)
 		 * relocations of type SHT_RELA/SHT_REL.
 		 */
 		if (sechdrs[i].sh_type == SHT_RELA)
-			ret = arch_kexec_apply_relocations_add(pi->ehdr,
-							       sechdrs, i);
+			ret = kexec_apply_relocations_add(pi->ehdr, sechdrs, i);
 		else if (sechdrs[i].sh_type == SHT_REL)
 			ret = arch_kexec_apply_relocations(pi->ehdr,
 							   sechdrs, i);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1527181

On 11/21/16 at 09:49pm, Thiago Jung Bauermann wrote:
> Hello Dave,
> 
> Thanks for your review.
> 
> Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> > On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> > > powerpc's purgatory.ro has 12 relocation types when built as
> > > a relocatable object. To implement support for them requires
> > > arch_kexec_apply_relocations_add to duplicate a lot of code with
> > > module_64.c:apply_relocate_add.
> > > 
> > > When built as a Position Independent Executable there are only 4
> > > relocation types in purgatory.ro, so it becomes practical for the powerpc
> > > implementation of kexec_file to have its own relocation implementation.
> > > 
> > > Also, the purgatory is an executable and not an intermediary output from
> > > the compiler so it makes sense conceptually that it is easier to build
> > > it as a PIE than as a partially linked object.
> > > 
> > > Apart from the greatly reduced number of relocations, there are two
> > > differences between a relocatable object and a PIE:
> > > 
> > > 1. __kexec_load_purgatory needs to use the program headers rather than the
> > > 
> > >    section headers to figure out how to load the binary.
> > > 
> > > 2. Symbol values are absolute addresses instead of relative to the
> > > 
> > >    start of the section.
> > > 
> > > This patch adds the support needed in generic code for the differences
> > > above and allows powerpc to load and relocate a position independent
> > > purgatory.
> > 
> > [snip]
> > 
> > The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
> > not that complex. So could you look into simplify your kexec_file
> > implementation?
> 
> I can try, but there is one fundamental issue here: powerpc position-dependent 
> code relies more on relocations than x86 position-dependent code does, so 
> there's a limit to how simple it can be made without switching to position-
> independent code. And it will always be more involved than it is on x86.
> 
> BTW, building x86's purgatory as PIE results in it not having any relocation 
> at all, so it's an advantage even in that architecture. Unfortunately, the 
> machine locks up during reboot and I didn't have time to try to figure out 
> what's going on.
> 
> > kernel/kexec_file.c kexec_apply_relocations only do limited things
> > and some of the logic is in arch/x86, so move general code out of arch
> > code, then I guess the arch code will be simpler
> 
> I agree that is a good idea. Is the patch below what you had in mind?
> 
> > and then we probably do not need this PIE stuff anymore.
> 
> If you are ok with the patch below I can post a new version of the series 
> based on it and we can see if Michael Ellerman thinks it is enough.
> 

Will review it and do a test. Thanks. I believe this will benefit for
other arches if they want a kexec_file in the future.

> > BTW, __kexec_really_load_purgatory looks worse than
> > ___kexec_load_purgatory ;)
> 
> Really? I find the special handling of bss makes the section-based loader a 
> bit more confusing.

I'm not sure I understand above about "*bss*", personally I like
___kexec_load_purgatory more. But anyway if we move arch code as general
code then it will be not necessary anymore..

> 
> -- 
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 
> 
> Subject: [PATCH] kexec_file: Move generic relocation code from arch/x86 to
>  kernel/kexec_file.c
> 
> The check for undefined symbols stays in arch-specific code because
> powerpc needs to allow TOC symbols to be processed even though they're
> undefined.
> 
> There is no functional change.
> 
> Suggested-by: Dave Young <dyoung@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  arch/x86/kernel/machine_kexec_64.c | 160 +++++++------------------------------
>  include/linux/kexec.h              |   9 ++-
>  kernel/kexec_file.c                | 120 +++++++++++++++++++++++++++-
>  3 files changed, 154 insertions(+), 135 deletions(-)
> 
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 8c1f218926d7..f4860c408ece 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -401,143 +401,45 @@ int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel,
>  }
>  #endif
>  
> -/*
> - * Apply purgatory relocations.
> - *
> - * ehdr: Pointer to elf headers
> - * sechdrs: Pointer to section headers.
> - * relsec: section index of SHT_RELA section.
> - *
> - * TODO: Some of the code belongs to generic code. Move that in kexec.c.
> - */
> -int arch_kexec_apply_relocations_add(const Elf64_Ehdr *ehdr,
> -				     Elf64_Shdr *sechdrs, unsigned int relsec)
> +int arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> +				    unsigned int reltype, Elf_Sym *sym,
> +				    const char *name, unsigned long *location,
> +				    unsigned long address, unsigned long value)
>  {
> -	unsigned int i;
> -	Elf64_Rela *rel;
> -	Elf64_Sym *sym;
> -	void *location;
> -	Elf64_Shdr *section, *symtabsec;
> -	unsigned long address, sec_base, value;
> -	const char *strtab, *name, *shstrtab;
> -
> -	/*
> -	 * ->sh_offset has been modified to keep the pointer to section
> -	 * contents in memory
> -	 */
> -	rel = (void *)sechdrs[relsec].sh_offset;
> -
> -	/* Section to which relocations apply */
> -	section = &sechdrs[sechdrs[relsec].sh_info];
> -
> -	pr_debug("Applying relocate section %u to %u\n", relsec,
> -		 sechdrs[relsec].sh_info);
> -
> -	/* Associated symbol table */
> -	symtabsec = &sechdrs[sechdrs[relsec].sh_link];
> -
> -	/* String table */
> -	if (symtabsec->sh_link >= ehdr->e_shnum) {
> -		/* Invalid strtab section number */
> -		pr_err("Invalid string table section index %d\n",
> -		       symtabsec->sh_link);
> +	if (sym->st_shndx == SHN_UNDEF) {
> +		pr_err("Undefined symbol: %s\n", name);
>  		return -ENOEXEC;
>  	}
>  
> -	strtab = (char *)sechdrs[symtabsec->sh_link].sh_offset;
> -
> -	/* section header string table */
> -	shstrtab = (char *)sechdrs[ehdr->e_shstrndx].sh_offset;
> -
> -	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
> -
> -		/*
> -		 * rel[i].r_offset contains byte offset from beginning
> -		 * of section to the storage unit affected.
> -		 *
> -		 * This is location to update (->sh_offset). This is temporary
> -		 * buffer where section is currently loaded. This will finally
> -		 * be loaded to a different address later, pointed to by
> -		 * ->sh_addr. kexec takes care of moving it
> -		 *  (kexec_load_segment()).
> -		 */
> -		location = (void *)(section->sh_offset + rel[i].r_offset);
> -
> -		/* Final address of the location */
> -		address = section->sh_addr + rel[i].r_offset;
> -
> -		/*
> -		 * rel[i].r_info contains information about symbol table index
> -		 * w.r.t which relocation must be made and type of relocation
> -		 * to apply. ELF64_R_SYM() and ELF64_R_TYPE() macros get
> -		 * these respectively.
> -		 */
> -		sym = (Elf64_Sym *)symtabsec->sh_offset +
> -				ELF64_R_SYM(rel[i].r_info);
> -
> -		if (sym->st_name)
> -			name = strtab + sym->st_name;
> -		else
> -			name = shstrtab + sechdrs[sym->st_shndx].sh_name;
> -
> -		pr_debug("Symbol: %s info: %02x shndx: %02x value=%llx size: %llx\n",
> -			 name, sym->st_info, sym->st_shndx, sym->st_value,
> -			 sym->st_size);
> -
> -		if (sym->st_shndx == SHN_UNDEF) {
> -			pr_err("Undefined symbol: %s\n", name);
> -			return -ENOEXEC;
> -		}
> -
> -		if (sym->st_shndx == SHN_COMMON) {
> -			pr_err("symbol '%s' in common section\n", name);
> -			return -ENOEXEC;
> -		}
> -
> -		if (sym->st_shndx == SHN_ABS)
> -			sec_base = 0;
> -		else if (sym->st_shndx >= ehdr->e_shnum) {
> -			pr_err("Invalid section %d for symbol %s\n",
> -			       sym->st_shndx, name);
> -			return -ENOEXEC;
> -		} else
> -			sec_base = sechdrs[sym->st_shndx].sh_addr;
> -
> -		value = sym->st_value;
> -		value += sec_base;
> -		value += rel[i].r_addend;
> -
> -		switch (ELF64_R_TYPE(rel[i].r_info)) {
> -		case R_X86_64_NONE:
> -			break;
> -		case R_X86_64_64:
> -			*(u64 *)location = value;
> -			break;
> -		case R_X86_64_32:
> -			*(u32 *)location = value;
> -			if (value != *(u32 *)location)
> -				goto overflow;
> -			break;
> -		case R_X86_64_32S:
> -			*(s32 *)location = value;
> -			if ((s64)value != *(s32 *)location)
> -				goto overflow;
> -			break;
> -		case R_X86_64_PC32:
> -			value -= (u64)address;
> -			*(u32 *)location = value;
> -			break;
> -		default:
> -			pr_err("Unknown rela relocation: %llu\n",
> -			       ELF64_R_TYPE(rel[i].r_info));
> -			return -ENOEXEC;
> -		}
> +	switch (reltype) {
> +	case R_X86_64_NONE:
> +		break;
> +	case R_X86_64_64:
> +		*(u64 *)location = value;
> +		break;
> +	case R_X86_64_32:
> +		*(u32 *)location = value;
> +		if (value != *(u32 *)location)
> +			goto overflow;
> +		break;
> +	case R_X86_64_32S:
> +		*(s32 *)location = value;
> +		if ((s64)value != *(s32 *)location)
> +			goto overflow;
> +		break;
> +	case R_X86_64_PC32:
> +		value -= (u64)address;
> +		*(u32 *)location = value;
> +		break;
> +	default:
> +		pr_err("Unknown rela relocation: %u\n", reltype);
> +		return -ENOEXEC;
>  	}
> +
>  	return 0;
>  
>  overflow:
> -	pr_err("Overflow in relocation type %d value 0x%lx\n",
> -	       (int)ELF64_R_TYPE(rel[i].r_info), value);
> +	pr_err("Overflow in relocation type %u value 0x%lx\n", reltype, value);
>  	return -ENOEXEC;
>  }
>  #endif /* CONFIG_KEXEC_FILE */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 406c33dcae13..e171a083540d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -320,8 +320,13 @@ void * __weak arch_kexec_kernel_image_load(struct kimage *image);
>  int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
>  int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
>  					unsigned long buf_len);
> -int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
> -					Elf_Shdr *sechdrs, unsigned int relsec);
> +int __weak arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr,
> +					   Elf_Shdr *sechdrs,
> +					   unsigned int reltype, Elf_Sym *sym,
> +					   const char *name,
> +					   unsigned long *location,
> +					   unsigned long address,
> +					   unsigned long value);
>  int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
>  					unsigned int relsec);
>  void arch_kexec_protect_crashkres(void);
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 037c321c5618..1517f977cc25 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -61,8 +61,10 @@ int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
>  
>  /* Apply relocations of type RELA */
>  int __weak
> -arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> -				 unsigned int relsec)
> +arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> +				unsigned int reltype, Elf_Sym *sym,
> +				const char *name, unsigned long *location,
> +				unsigned long address, unsigned long value)
>  {
>  	pr_err("RELA relocation unsupported.\n");
>  	return -ENOEXEC;
> @@ -793,6 +795,117 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	return ret;
>  }
>  
> +/**
> + * kexec_apply_relocations_add - apply purgatory relocations
> + * @ehdr:	Pointer to elf headers
> + * @sechdrs:	Pointer to section headers.
> + * @relsec:	Section index of SHT_RELA section.
> + */
> +static int kexec_apply_relocations_add(const Elf64_Ehdr *ehdr,
> +				       Elf64_Shdr *sechdrs, unsigned int relsec)
> +{
> +	int ret;
> +	unsigned int i;
> +	Elf64_Rela *rel;
> +	Elf64_Sym *sym;
> +	void *location;
> +	Elf64_Shdr *section, *symtabsec;
> +	unsigned long address, sec_base, value;
> +	const char *strtab, *name, *shstrtab;
> +
> +	/*
> +	 * ->sh_offset has been modified to keep the pointer to section
> +	 * contents in memory
> +	 */
> +	rel = (void *)sechdrs[relsec].sh_offset;
> +
> +	/* Section to which relocations apply */
> +	section = &sechdrs[sechdrs[relsec].sh_info];
> +
> +	pr_debug("Applying relocate section %u to %u\n", relsec,
> +		 sechdrs[relsec].sh_info);
> +
> +	/* Associated symbol table */
> +	symtabsec = &sechdrs[sechdrs[relsec].sh_link];
> +
> +	/* String table */
> +	if (symtabsec->sh_link >= ehdr->e_shnum) {
> +		/* Invalid strtab section number */
> +		pr_err("Invalid string table section index %d\n",
> +		       symtabsec->sh_link);
> +		return -ENOEXEC;
> +	}
> +
> +	/* String table for the associated symbol table. */
> +	strtab = (char *)sechdrs[symtabsec->sh_link].sh_offset;
> +
> +	/* section header string table */
> +	shstrtab = (char *)sechdrs[ehdr->e_shstrndx].sh_offset;
> +
> +	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
> +
> +		/*
> +		 * rel[i].r_offset contains byte offset from beginning
> +		 * of section to the storage unit affected.
> +		 *
> +		 * This is location to update (->sh_offset). This is temporary
> +		 * buffer where section is currently loaded. This will finally
> +		 * be loaded to a different address later, pointed to by
> +		 * ->sh_addr. kexec takes care of moving it
> +		 *  (kexec_load_segment()).
> +		 */
> +		location = (void *)(section->sh_offset + rel[i].r_offset);
> +
> +		/* Final address of the location */
> +		address = section->sh_addr + rel[i].r_offset;
> +
> +		/*
> +		 * rel[i].r_info contains information about symbol table index
> +		 * w.r.t which relocation must be made and type of relocation
> +		 * to apply. ELF64_R_SYM() and ELF64_R_TYPE() macros get
> +		 * these respectively.
> +		 */
> +		sym = (Elf64_Sym *)symtabsec->sh_offset +
> +				ELF64_R_SYM(rel[i].r_info);
> +
> +		if (sym->st_name)
> +			name = strtab + sym->st_name;
> +		else
> +			name = shstrtab + sechdrs[sym->st_shndx].sh_name;
> +
> +		pr_debug("Symbol: %s info: %02x shndx: %02x value=%llx size: %llx\n",
> +			 name, sym->st_info, sym->st_shndx, sym->st_value,
> +			 sym->st_size);
> +
> +		if (sym->st_shndx == SHN_COMMON) {
> +			pr_err("symbol '%s' in common section\n", name);
> +			return -ENOEXEC;
> +		}
> +
> +		if (sym->st_shndx == SHN_ABS)
> +			sec_base = 0;
> +		else if (sym->st_shndx >= ehdr->e_shnum) {
> +			pr_err("Invalid section %d for symbol %s\n",
> +			       sym->st_shndx, name);
> +			return -ENOEXEC;
> +		} else
> +			sec_base = sechdrs[sym->st_shndx].sh_addr;
> +
> +		value = sym->st_value;
> +		value += sec_base;
> +		value += rel[i].r_addend;
> +
> +		ret = arch_kexec_apply_relocation_add(ehdr, sechdrs,
> +						      ELF64_R_TYPE(rel[i].r_info),
> +						      sym, name, location,
> +						      address, value);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int kexec_apply_relocations(struct kimage *image)
>  {
>  	int i, ret;
> @@ -836,8 +949,7 @@ static int kexec_apply_relocations(struct kimage *image)
>  		 * relocations of type SHT_RELA/SHT_REL.
>  		 */
>  		if (sechdrs[i].sh_type == SHT_RELA)
> -			ret = arch_kexec_apply_relocations_add(pi->ehdr,
> -							       sechdrs, i);
> +			ret = kexec_apply_relocations_add(pi->ehdr, sechdrs, i);
>  		else if (sechdrs[i].sh_type == SHT_REL)
>  			ret = arch_kexec_apply_relocations(pi->ehdr,
>  							   sechdrs, i);
> -- 
> 2.7.4
> 
> 

[toc] | [prev] | [next] | [standalone]

#1527245 — Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE.

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
>> On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
>> > powerpc's purgatory.ro has 12 relocation types when built as
>> > a relocatable object. To implement support for them requires
>> > arch_kexec_apply_relocations_add to duplicate a lot of code with
>> > module_64.c:apply_relocate_add.
>> > 
>> > When built as a Position Independent Executable there are only 4
>> > relocation types in purgatory.ro, so it becomes practical for the powerpc
>> > implementation of kexec_file to have its own relocation implementation.
>> > 
>> > Also, the purgatory is an executable and not an intermediary output from
>> > the compiler so it makes sense conceptually that it is easier to build
>> > it as a PIE than as a partially linked object.
>> > 
>> > Apart from the greatly reduced number of relocations, there are two
>> > differences between a relocatable object and a PIE:
>> > 
>> > 1. __kexec_load_purgatory needs to use the program headers rather than the
>> > 
>> >    section headers to figure out how to load the binary.
>> > 
>> > 2. Symbol values are absolute addresses instead of relative to the
>> > 
>> >    start of the section.
>> > 
>> > This patch adds the support needed in generic code for the differences
>> > above and allows powerpc to load and relocate a position independent
>> > purgatory.
>> 
>> [snip]
>> 
>> The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
>> not that complex. So could you look into simplify your kexec_file
>> implementation?
>
> I can try, but there is one fundamental issue here: powerpc position-dependent 
> code relies more on relocations than x86 position-dependent code does, so 
> there's a limit to how simple it can be made without switching to position-
> independent code. And it will always be more involved than it is on x86.

I think we need to go back to the drawing board on this one.

My hope was that building purgatory as PIE would reduce the amount of
complexity, but instead it's just added more. Sorry for sending you in
that direction.


In general I dislike the level of complexity of the kexec-tools
purgatory, and in particular I'm not comfortable with things like:

diff --git a/arch/powerpc/purgatory/sha256.c b/arch/powerpc/purgatory/sha256.c
new file mode 100644
index 000000000000..6abee1877d56
--- /dev/null
+++ b/arch/powerpc/purgatory/sha256.c
@@ -0,0 +1,6 @@
+#include "../boot/string.h"
+
+/* Avoid including x86's boot/string.h in sha256.c. */
+#define BOOT_STRING_H
+
+#include "../../x86/purgatory/sha256.c"


I think the best way to get this over the line would be to take the
kexec-lite purgatory implementation and use that to begin with. I know
it doesn't have all the features of the kexec-tools version, but it
should work, and we can look at adding the extra features later.

I'll try and get that working tonight.

cheers

[toc] | [prev] | [next] | [standalone]

#1527247

Hi Michael
On 11/22/16 at 05:01pm, Michael Ellerman wrote:
> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> >> On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> >> > powerpc's purgatory.ro has 12 relocation types when built as
> >> > a relocatable object. To implement support for them requires
> >> > arch_kexec_apply_relocations_add to duplicate a lot of code with
> >> > module_64.c:apply_relocate_add.
> >> > 
> >> > When built as a Position Independent Executable there are only 4
> >> > relocation types in purgatory.ro, so it becomes practical for the powerpc
> >> > implementation of kexec_file to have its own relocation implementation.
> >> > 
> >> > Also, the purgatory is an executable and not an intermediary output from
> >> > the compiler so it makes sense conceptually that it is easier to build
> >> > it as a PIE than as a partially linked object.
> >> > 
> >> > Apart from the greatly reduced number of relocations, there are two
> >> > differences between a relocatable object and a PIE:
> >> > 
> >> > 1. __kexec_load_purgatory needs to use the program headers rather than the
> >> > 
> >> >    section headers to figure out how to load the binary.
> >> > 
> >> > 2. Symbol values are absolute addresses instead of relative to the
> >> > 
> >> >    start of the section.
> >> > 
> >> > This patch adds the support needed in generic code for the differences
> >> > above and allows powerpc to load and relocate a position independent
> >> > purgatory.
> >> 
> >> [snip]
> >> 
> >> The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
> >> not that complex. So could you look into simplify your kexec_file
> >> implementation?
> >
> > I can try, but there is one fundamental issue here: powerpc position-dependent 
> > code relies more on relocations than x86 position-dependent code does, so 
> > there's a limit to how simple it can be made without switching to position-
> > independent code. And it will always be more involved than it is on x86.
> 
> I think we need to go back to the drawing board on this one.
> 
> My hope was that building purgatory as PIE would reduce the amount of
> complexity, but instead it's just added more. Sorry for sending you in
> that direction.
> 
> 
> In general I dislike the level of complexity of the kexec-tools
> purgatory, and in particular I'm not comfortable with things like:
> 
> diff --git a/arch/powerpc/purgatory/sha256.c b/arch/powerpc/purgatory/sha256.c
> new file mode 100644
> index 000000000000..6abee1877d56
> --- /dev/null
> +++ b/arch/powerpc/purgatory/sha256.c
> @@ -0,0 +1,6 @@
> +#include "../boot/string.h"
> +
> +/* Avoid including x86's boot/string.h in sha256.c. */
> +#define BOOT_STRING_H
> +
> +#include "../../x86/purgatory/sha256.c"
> 

Agreed, include x86 code in powerpc looks bad

> 
> I think the best way to get this over the line would be to take the
> kexec-lite purgatory implementation and use that to begin with. I know
> it doesn't have all the features of the kexec-tools version, but it
> should work, and we can look at adding the extra features later.

Instead of adding other implementation, moving the purgatory sha256 code
out of x86 sounds better so that we can reuse them cleanly..

> 
> I'll try and get that working tonight.
> 
> cheers

Thanks
Dave

[toc] | [prev] | [next] | [standalone]

#1527509 — Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE.

Am Dienstag, 22. November 2016, 17:01:10 BRST schrieb Michael Ellerman:
> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> >> On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> >> > powerpc's purgatory.ro has 12 relocation types when built as
> >> > a relocatable object. To implement support for them requires
> >> > arch_kexec_apply_relocations_add to duplicate a lot of code with
> >> > module_64.c:apply_relocate_add.
> >> > 
> >> > When built as a Position Independent Executable there are only 4
> >> > relocation types in purgatory.ro, so it becomes practical for the
> >> > powerpc
> >> > implementation of kexec_file to have its own relocation implementation.
> >> > 
> >> > Also, the purgatory is an executable and not an intermediary output
> >> > from
> >> > the compiler so it makes sense conceptually that it is easier to build
> >> > it as a PIE than as a partially linked object.
> >> > 
> >> > Apart from the greatly reduced number of relocations, there are two
> >> > differences between a relocatable object and a PIE:
> >> > 
> >> > 1. __kexec_load_purgatory needs to use the program headers rather than
> >> > the
> >> > 
> >> >    section headers to figure out how to load the binary.
> >> > 
> >> > 2. Symbol values are absolute addresses instead of relative to the
> >> > 
> >> >    start of the section.
> >> > 
> >> > This patch adds the support needed in generic code for the differences
> >> > above and allows powerpc to load and relocate a position independent
> >> > purgatory.
> >> 
> >> [snip]
> >> 
> >> The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
> >> not that complex. So could you look into simplify your kexec_file
> >> implementation?
> > 
> > I can try, but there is one fundamental issue here: powerpc
> > position-dependent code relies more on relocations than x86
> > position-dependent code does, so there's a limit to how simple it can be
> > made without switching to position- independent code. And it will always
> > be more involved than it is on x86.
> I think we need to go back to the drawing board on this one.
> 
> My hope was that building purgatory as PIE would reduce the amount of
> complexity, but instead it's just added more. Sorry for sending you in
> that direction.

It added complexity because in my series powerpc was using a PIE purgatory but 
x86 kept using a partially-linked object (because of the problem I mentioned I 
had when trying out a PIE x86 purgatory), so generic code needed two purgatory 
loaders.

I'll see if I can make the PIE x86 purgatory to work so that generic code can 
have only one loader implementation. Then it will indeed be simpler.


Am Dienstag, 22. November 2016, 14:16:22 BRST schrieb Dave Young:
> Hi Michael
> 
> On 11/22/16 at 05:01pm, Michael Ellerman wrote:
> > In general I dislike the level of complexity of the kexec-tools
> > purgatory, and in particular I'm not comfortable with things like:
> > 
> > diff --git a/arch/powerpc/purgatory/sha256.c
> > b/arch/powerpc/purgatory/sha256.c new file mode 100644
> > index 000000000000..6abee1877d56
> > --- /dev/null
> > +++ b/arch/powerpc/purgatory/sha256.c
> > @@ -0,0 +1,6 @@
> > +#include "../boot/string.h"
> > +
> > +/* Avoid including x86's boot/string.h in sha256.c. */
> > +#define BOOT_STRING_H
> > +
> > +#include "../../x86/purgatory/sha256.c"
> 
> Agreed, include x86 code in powerpc looks bad
> 
> > I think the best way to get this over the line would be to take the
> > kexec-lite purgatory implementation and use that to begin with. I know
> > it doesn't have all the features of the kexec-tools version, but it
> > should work, and we can look at adding the extra features later.
> 
> Instead of adding other implementation, moving the purgatory sha256 code
> out of x86 sounds better so that we can reuse them cleanly..

Do you have a suggestion of where that code can live so that it can be shared 
between purgatories for different arches?

Do we need a purgatory with generic and arch-specific code like in kexec-
tools?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

[toc] | [prev] | [next] | [standalone]

#1528072

On 11/22/16 at 11:44am, Thiago Jung Bauermann wrote:
> Am Dienstag, 22. November 2016, 17:01:10 BRST schrieb Michael Ellerman:
> > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > > Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> > >> On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> > >> > powerpc's purgatory.ro has 12 relocation types when built as
> > >> > a relocatable object. To implement support for them requires
> > >> > arch_kexec_apply_relocations_add to duplicate a lot of code with
> > >> > module_64.c:apply_relocate_add.
> > >> > 
> > >> > When built as a Position Independent Executable there are only 4
> > >> > relocation types in purgatory.ro, so it becomes practical for the
> > >> > powerpc
> > >> > implementation of kexec_file to have its own relocation implementation.
> > >> > 
> > >> > Also, the purgatory is an executable and not an intermediary output
> > >> > from
> > >> > the compiler so it makes sense conceptually that it is easier to build
> > >> > it as a PIE than as a partially linked object.
> > >> > 
> > >> > Apart from the greatly reduced number of relocations, there are two
> > >> > differences between a relocatable object and a PIE:
> > >> > 
> > >> > 1. __kexec_load_purgatory needs to use the program headers rather than
> > >> > the
> > >> > 
> > >> >    section headers to figure out how to load the binary.
> > >> > 
> > >> > 2. Symbol values are absolute addresses instead of relative to the
> > >> > 
> > >> >    start of the section.
> > >> > 
> > >> > This patch adds the support needed in generic code for the differences
> > >> > above and allows powerpc to load and relocate a position independent
> > >> > purgatory.
> > >> 
> > >> [snip]
> > >> 
> > >> The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
> > >> not that complex. So could you look into simplify your kexec_file
> > >> implementation?
> > > 
> > > I can try, but there is one fundamental issue here: powerpc
> > > position-dependent code relies more on relocations than x86
> > > position-dependent code does, so there's a limit to how simple it can be
> > > made without switching to position- independent code. And it will always
> > > be more involved than it is on x86.
> > I think we need to go back to the drawing board on this one.
> > 
> > My hope was that building purgatory as PIE would reduce the amount of
> > complexity, but instead it's just added more. Sorry for sending you in
> > that direction.
> 
> It added complexity because in my series powerpc was using a PIE purgatory but 
> x86 kept using a partially-linked object (because of the problem I mentioned I 
> had when trying out a PIE x86 purgatory), so generic code needed two purgatory 
> loaders.
> 
> I'll see if I can make the PIE x86 purgatory to work so that generic code can 
> have only one loader implementation. Then it will indeed be simpler.

Do we really need the PIE purgatory, after moving generic code out of
x86, there will be no much benefit, no? Anyway, the first step should be
making the purgatory code more generic so that it can be easier for
other arches to support kexec_file in the future. 

> 
> 
> Am Dienstag, 22. November 2016, 14:16:22 BRST schrieb Dave Young:
> > Hi Michael
> > 
> > On 11/22/16 at 05:01pm, Michael Ellerman wrote:
> > > In general I dislike the level of complexity of the kexec-tools
> > > purgatory, and in particular I'm not comfortable with things like:
> > > 
> > > diff --git a/arch/powerpc/purgatory/sha256.c
> > > b/arch/powerpc/purgatory/sha256.c new file mode 100644
> > > index 000000000000..6abee1877d56
> > > --- /dev/null
> > > +++ b/arch/powerpc/purgatory/sha256.c
> > > @@ -0,0 +1,6 @@
> > > +#include "../boot/string.h"
> > > +
> > > +/* Avoid including x86's boot/string.h in sha256.c. */
> > > +#define BOOT_STRING_H
> > > +
> > > +#include "../../x86/purgatory/sha256.c"
> > 
> > Agreed, include x86 code in powerpc looks bad
> > 
> > > I think the best way to get this over the line would be to take the
> > > kexec-lite purgatory implementation and use that to begin with. I know
> > > it doesn't have all the features of the kexec-tools version, but it
> > > should work, and we can look at adding the extra features later.
> > 
> > Instead of adding other implementation, moving the purgatory sha256 code
> > out of x86 sounds better so that we can reuse them cleanly..
> 
> Do you have a suggestion of where that code can live so that it can be shared 
> between purgatories for different arches?

Maybe it is better to stay in lib/purgatory/

> 
> Do we need a purgatory with generic and arch-specific code like in kexec-
> tools?

Yes, if we have more arches to add kexec_file, this should be
necessary..

> 
> -- 
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 

Thanks
Dave

[toc] | [prev] | [next] | [standalone]

#1528093 — Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as PIE.

Am Mittwoch, 23. November 2016, 09:32:58 BRST schrieb Dave Young:
> On 11/22/16 at 11:44am, Thiago Jung Bauermann wrote:
> > Am Dienstag, 22. November 2016, 17:01:10 BRST schrieb Michael Ellerman:
> > > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > > > Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> > > >> On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> > > >> > powerpc's purgatory.ro has 12 relocation types when built as
> > > >> > a relocatable object. To implement support for them requires
> > > >> > arch_kexec_apply_relocations_add to duplicate a lot of code with
> > > >> > module_64.c:apply_relocate_add.
> > > >> > 
> > > >> > When built as a Position Independent Executable there are only 4
> > > >> > relocation types in purgatory.ro, so it becomes practical for the
> > > >> > powerpc
> > > >> > implementation of kexec_file to have its own relocation
> > > >> > implementation.
> > > >> > 
> > > >> > Also, the purgatory is an executable and not an intermediary output
> > > >> > from
> > > >> > the compiler so it makes sense conceptually that it is easier to
> > > >> > build
> > > >> > it as a PIE than as a partially linked object.
> > > >> > 
> > > >> > Apart from the greatly reduced number of relocations, there are two
> > > >> > differences between a relocatable object and a PIE:
> > > >> > 
> > > >> > 1. __kexec_load_purgatory needs to use the program headers rather
> > > >> > than
> > > >> > the
> > > >> > 
> > > >> >    section headers to figure out how to load the binary.
> > > >> > 
> > > >> > 2. Symbol values are absolute addresses instead of relative to the
> > > >> > 
> > > >> >    start of the section.
> > > >> > 
> > > >> > This patch adds the support needed in generic code for the
> > > >> > differences
> > > >> > above and allows powerpc to load and relocate a position
> > > >> > independent
> > > >> > purgatory.
> > > >> 
> > > >> [snip]
> > > >> 
> > > >> The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it
> > > >> is
> > > >> not that complex. So could you look into simplify your kexec_file
> > > >> implementation?
> > > > 
> > > > I can try, but there is one fundamental issue here: powerpc
> > > > position-dependent code relies more on relocations than x86
> > > > position-dependent code does, so there's a limit to how simple it can
> > > > be
> > > > made without switching to position- independent code. And it will
> > > > always
> > > > be more involved than it is on x86.
> > > 
> > > I think we need to go back to the drawing board on this one.
> > > 
> > > My hope was that building purgatory as PIE would reduce the amount of
> > > complexity, but instead it's just added more. Sorry for sending you in
> > > that direction.
> > 
> > It added complexity because in my series powerpc was using a PIE purgatory
> > but x86 kept using a partially-linked object (because of the problem I
> > mentioned I had when trying out a PIE x86 purgatory), so generic code
> > needed two purgatory loaders.
> > 
> > I'll see if I can make the PIE x86 purgatory to work so that generic code
> > can have only one loader implementation. Then it will indeed be simpler.
> Do we really need the PIE purgatory, after moving generic code out of
> x86, there will be no much benefit, no?

It still makes a big difference on powerpc, even after moving out the generic 
code. I just got the PIE purgatory working on x86 and it also simplifies the 
code there, so it's a win for both architectures.

I'll clean up the code and post tomorrow so that you can see what you think.

> Anyway, the first step should be
> making the purgatory code more generic so that it can be easier for
> other arches to support kexec_file in the future.

I'll try putting sha256.c in lib/purgatory/ as you suggested.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

[toc] | [prev] | [next] | [standalone]

#1528211

On 11/21/16 at 09:49pm, Thiago Jung Bauermann wrote:
> Hello Dave,
> 
> Thanks for your review.
> 
> Am Sonntag, 20. November 2016, 10:45:46 BRST schrieb Dave Young:
> > On 11/10/16 at 01:27am, Thiago Jung Bauermann wrote:
> > > powerpc's purgatory.ro has 12 relocation types when built as
> > > a relocatable object. To implement support for them requires
> > > arch_kexec_apply_relocations_add to duplicate a lot of code with
> > > module_64.c:apply_relocate_add.
> > > 
> > > When built as a Position Independent Executable there are only 4
> > > relocation types in purgatory.ro, so it becomes practical for the powerpc
> > > implementation of kexec_file to have its own relocation implementation.
> > > 
> > > Also, the purgatory is an executable and not an intermediary output from
> > > the compiler so it makes sense conceptually that it is easier to build
> > > it as a PIE than as a partially linked object.
> > > 
> > > Apart from the greatly reduced number of relocations, there are two
> > > differences between a relocatable object and a PIE:
> > > 
> > > 1. __kexec_load_purgatory needs to use the program headers rather than the
> > > 
> > >    section headers to figure out how to load the binary.
> > > 
> > > 2. Symbol values are absolute addresses instead of relative to the
> > > 
> > >    start of the section.
> > > 
> > > This patch adds the support needed in generic code for the differences
> > > above and allows powerpc to load and relocate a position independent
> > > purgatory.
> > 
> > [snip]
> > 
> > The kexec-tools machine_apply_elf_rel is pretty simple for ppc64, it is
> > not that complex. So could you look into simplify your kexec_file
> > implementation?
> 
> I can try, but there is one fundamental issue here: powerpc position-dependent 
> code relies more on relocations than x86 position-dependent code does, so 
> there's a limit to how simple it can be made without switching to position-
> independent code. And it will always be more involved than it is on x86.
> 
> BTW, building x86's purgatory as PIE results in it not having any relocation 
> at all, so it's an advantage even in that architecture. Unfortunately, the 
> machine locks up during reboot and I didn't have time to try to figure out 
> what's going on.
> 
> > kernel/kexec_file.c kexec_apply_relocations only do limited things
> > and some of the logic is in arch/x86, so move general code out of arch
> > code, then I guess the arch code will be simpler
> 
> I agree that is a good idea. Is the patch below what you had in mind?
> 
> > and then we probably do not need this PIE stuff anymore.
> 
> If you are ok with the patch below I can post a new version of the series 
> based on it and we can see if Michael Ellerman thinks it is enough.
> 
> > BTW, __kexec_really_load_purgatory looks worse than
> > ___kexec_load_purgatory ;)
> 
> Really? I find the special handling of bss makes the section-based loader a 
> bit more confusing.
> 
> -- 
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 
> 
> Subject: [PATCH] kexec_file: Move generic relocation code from arch/x86 to
>  kernel/kexec_file.c
> 
> The check for undefined symbols stays in arch-specific code because
> powerpc needs to allow TOC symbols to be processed even though they're
> undefined.
> 
> There is no functional change.
> 
> Suggested-by: Dave Young <dyoung@redhat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  arch/x86/kernel/machine_kexec_64.c | 160 +++++++------------------------------
>  include/linux/kexec.h              |   9 ++-
>  kernel/kexec_file.c                | 120 +++++++++++++++++++++++++++-
>  3 files changed, 154 insertions(+), 135 deletions(-)
> 
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 8c1f218926d7..f4860c408ece 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -401,143 +401,45 @@ int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel,
>  }
>  #endif
>  
> -/*
> - * Apply purgatory relocations.
> - *
> - * ehdr: Pointer to elf headers
> - * sechdrs: Pointer to section headers.
> - * relsec: section index of SHT_RELA section.
> - *
> - * TODO: Some of the code belongs to generic code. Move that in kexec.c.
> - */
> -int arch_kexec_apply_relocations_add(const Elf64_Ehdr *ehdr,
> -				     Elf64_Shdr *sechdrs, unsigned int relsec)
> +int arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> +				    unsigned int reltype, Elf_Sym *sym,
> +				    const char *name, unsigned long *location,
> +				    unsigned long address, unsigned long value)
>  {
> -	unsigned int i;
> -	Elf64_Rela *rel;
> -	Elf64_Sym *sym;
> -	void *location;
> -	Elf64_Shdr *section, *symtabsec;
> -	unsigned long address, sec_base, value;
> -	const char *strtab, *name, *shstrtab;
> -
> -	/*
> -	 * ->sh_offset has been modified to keep the pointer to section
> -	 * contents in memory
> -	 */
> -	rel = (void *)sechdrs[relsec].sh_offset;
> -
> -	/* Section to which relocations apply */
> -	section = &sechdrs[sechdrs[relsec].sh_info];
> -
> -	pr_debug("Applying relocate section %u to %u\n", relsec,
> -		 sechdrs[relsec].sh_info);
> -
> -	/* Associated symbol table */
> -	symtabsec = &sechdrs[sechdrs[relsec].sh_link];
> -
> -	/* String table */
> -	if (symtabsec->sh_link >= ehdr->e_shnum) {
> -		/* Invalid strtab section number */
> -		pr_err("Invalid string table section index %d\n",
> -		       symtabsec->sh_link);
> +	if (sym->st_shndx == SHN_UNDEF) {
> +		pr_err("Undefined symbol: %s\n", name);
>  		return -ENOEXEC;
>  	}
>  
> -	strtab = (char *)sechdrs[symtabsec->sh_link].sh_offset;
> -
> -	/* section header string table */
> -	shstrtab = (char *)sechdrs[ehdr->e_shstrndx].sh_offset;
> -
> -	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
> -
> -		/*
> -		 * rel[i].r_offset contains byte offset from beginning
> -		 * of section to the storage unit affected.
> -		 *
> -		 * This is location to update (->sh_offset). This is temporary
> -		 * buffer where section is currently loaded. This will finally
> -		 * be loaded to a different address later, pointed to by
> -		 * ->sh_addr. kexec takes care of moving it
> -		 *  (kexec_load_segment()).
> -		 */
> -		location = (void *)(section->sh_offset + rel[i].r_offset);
> -
> -		/* Final address of the location */
> -		address = section->sh_addr + rel[i].r_offset;
> -
> -		/*
> -		 * rel[i].r_info contains information about symbol table index
> -		 * w.r.t which relocation must be made and type of relocation
> -		 * to apply. ELF64_R_SYM() and ELF64_R_TYPE() macros get
> -		 * these respectively.
> -		 */
> -		sym = (Elf64_Sym *)symtabsec->sh_offset +
> -				ELF64_R_SYM(rel[i].r_info);
> -
> -		if (sym->st_name)
> -			name = strtab + sym->st_name;
> -		else
> -			name = shstrtab + sechdrs[sym->st_shndx].sh_name;
> -
> -		pr_debug("Symbol: %s info: %02x shndx: %02x value=%llx size: %llx\n",
> -			 name, sym->st_info, sym->st_shndx, sym->st_value,
> -			 sym->st_size);
> -
> -		if (sym->st_shndx == SHN_UNDEF) {
> -			pr_err("Undefined symbol: %s\n", name);
> -			return -ENOEXEC;
> -		}
> -
> -		if (sym->st_shndx == SHN_COMMON) {
> -			pr_err("symbol '%s' in common section\n", name);
> -			return -ENOEXEC;
> -		}
> -
> -		if (sym->st_shndx == SHN_ABS)
> -			sec_base = 0;
> -		else if (sym->st_shndx >= ehdr->e_shnum) {
> -			pr_err("Invalid section %d for symbol %s\n",
> -			       sym->st_shndx, name);
> -			return -ENOEXEC;
> -		} else
> -			sec_base = sechdrs[sym->st_shndx].sh_addr;
> -
> -		value = sym->st_value;
> -		value += sec_base;
> -		value += rel[i].r_addend;
> -
> -		switch (ELF64_R_TYPE(rel[i].r_info)) {
> -		case R_X86_64_NONE:
> -			break;
> -		case R_X86_64_64:
> -			*(u64 *)location = value;
> -			break;
> -		case R_X86_64_32:
> -			*(u32 *)location = value;
> -			if (value != *(u32 *)location)
> -				goto overflow;
> -			break;
> -		case R_X86_64_32S:
> -			*(s32 *)location = value;
> -			if ((s64)value != *(s32 *)location)
> -				goto overflow;
> -			break;
> -		case R_X86_64_PC32:
> -			value -= (u64)address;
> -			*(u32 *)location = value;
> -			break;
> -		default:
> -			pr_err("Unknown rela relocation: %llu\n",
> -			       ELF64_R_TYPE(rel[i].r_info));
> -			return -ENOEXEC;
> -		}
> +	switch (reltype) {
> +	case R_X86_64_NONE:
> +		break;
> +	case R_X86_64_64:
> +		*(u64 *)location = value;
> +		break;
> +	case R_X86_64_32:
> +		*(u32 *)location = value;
> +		if (value != *(u32 *)location)
> +			goto overflow;
> +		break;
> +	case R_X86_64_32S:
> +		*(s32 *)location = value;
> +		if ((s64)value != *(s32 *)location)
> +			goto overflow;
> +		break;
> +	case R_X86_64_PC32:
> +		value -= (u64)address;
> +		*(u32 *)location = value;
> +		break;
> +	default:
> +		pr_err("Unknown rela relocation: %u\n", reltype);
> +		return -ENOEXEC;
>  	}
> +
>  	return 0;
>  
>  overflow:
> -	pr_err("Overflow in relocation type %d value 0x%lx\n",
> -	       (int)ELF64_R_TYPE(rel[i].r_info), value);
> +	pr_err("Overflow in relocation type %u value 0x%lx\n", reltype, value);
>  	return -ENOEXEC;
>  }
>  #endif /* CONFIG_KEXEC_FILE */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 406c33dcae13..e171a083540d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -320,8 +320,13 @@ void * __weak arch_kexec_kernel_image_load(struct kimage *image);
>  int __weak arch_kimage_file_post_load_cleanup(struct kimage *image);
>  int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
>  					unsigned long buf_len);
> -int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
> -					Elf_Shdr *sechdrs, unsigned int relsec);
> +int __weak arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr,
> +					   Elf_Shdr *sechdrs,
> +					   unsigned int reltype, Elf_Sym *sym,
> +					   const char *name,
> +					   unsigned long *location,
> +					   unsigned long address,
> +					   unsigned long value);
>  int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
>  					unsigned int relsec);
>  void arch_kexec_protect_crashkres(void);
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 037c321c5618..1517f977cc25 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -61,8 +61,10 @@ int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
>  
>  /* Apply relocations of type RELA */
>  int __weak
> -arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> -				 unsigned int relsec)
> +arch_kexec_apply_relocation_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> +				unsigned int reltype, Elf_Sym *sym,
> +				const char *name, unsigned long *location,
> +				unsigned long address, unsigned long value)
>  {
>  	pr_err("RELA relocation unsupported.\n");
>  	return -ENOEXEC;
> @@ -793,6 +795,117 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	return ret;
>  }
>  
> +/**
> + * kexec_apply_relocations_add - apply purgatory relocations
> + * @ehdr:	Pointer to elf headers
> + * @sechdrs:	Pointer to section headers.
> + * @relsec:	Section index of SHT_RELA section.
> + */
> +static int kexec_apply_relocations_add(const Elf64_Ehdr *ehdr,
> +				       Elf64_Shdr *sechdrs, unsigned int relsec)
> +{
> +	int ret;
> +	unsigned int i;
> +	Elf64_Rela *rel;
> +	Elf64_Sym *sym;
> +	void *location;
> +	Elf64_Shdr *section, *symtabsec;
> +	unsigned long address, sec_base, value;
> +	const char *strtab, *name, *shstrtab;
> +
> +	/*
> +	 * ->sh_offset has been modified to keep the pointer to section
> +	 * contents in memory
> +	 */
> +	rel = (void *)sechdrs[relsec].sh_offset;
> +
> +	/* Section to which relocations apply */
> +	section = &sechdrs[sechdrs[relsec].sh_info];
> +
> +	pr_debug("Applying relocate section %u to %u\n", relsec,
> +		 sechdrs[relsec].sh_info);
> +
> +	/* Associated symbol table */
> +	symtabsec = &sechdrs[sechdrs[relsec].sh_link];
> +
> +	/* String table */
> +	if (symtabsec->sh_link >= ehdr->e_shnum) {
> +		/* Invalid strtab section number */
> +		pr_err("Invalid string table section index %d\n",
> +		       symtabsec->sh_link);
> +		return -ENOEXEC;
> +	}
> +
> +	/* String table for the associated symbol table. */
> +	strtab = (char *)sechdrs[symtabsec->sh_link].sh_offset;
> +
> +	/* section header string table */
> +	shstrtab = (char *)sechdrs[ehdr->e_shstrndx].sh_offset;
> +
> +	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
> +
> +		/*
> +		 * rel[i].r_offset contains byte offset from beginning
> +		 * of section to the storage unit affected.
> +		 *
> +		 * This is location to update (->sh_offset). This is temporary
> +		 * buffer where section is currently loaded. This will finally
> +		 * be loaded to a different address later, pointed to by
> +		 * ->sh_addr. kexec takes care of moving it
> +		 *  (kexec_load_segment()).
> +		 */
> +		location = (void *)(section->sh_offset + rel[i].r_offset);
> +
> +		/* Final address of the location */
> +		address = section->sh_addr + rel[i].r_offset;
> +
> +		/*
> +		 * rel[i].r_info contains information about symbol table index
> +		 * w.r.t which relocation must be made and type of relocation
> +		 * to apply. ELF64_R_SYM() and ELF64_R_TYPE() macros get
> +		 * these respectively.
> +		 */
> +		sym = (Elf64_Sym *)symtabsec->sh_offset +
> +				ELF64_R_SYM(rel[i].r_info);
> +
> +		if (sym->st_name)
> +			name = strtab + sym->st_name;
> +		else
> +			name = shstrtab + sechdrs[sym->st_shndx].sh_name;
> +
> +		pr_debug("Symbol: %s info: %02x shndx: %02x value=%llx size: %llx\n",
> +			 name, sym->st_info, sym->st_shndx, sym->st_value,
> +			 sym->st_size);
> +
> +		if (sym->st_shndx == SHN_COMMON) {
> +			pr_err("symbol '%s' in common section\n", name);
> +			return -ENOEXEC;
> +		}
> +
> +		if (sym->st_shndx == SHN_ABS)
> +			sec_base = 0;
> +		else if (sym->st_shndx >= ehdr->e_shnum) {
> +			pr_err("Invalid section %d for symbol %s\n",
> +			       sym->st_shndx, name);
> +			return -ENOEXEC;
> +		} else
> +			sec_base = sechdrs[sym->st_shndx].sh_addr;
> +
> +		value = sym->st_value;
> +		value += sec_base;
> +		value += rel[i].r_addend;
> +
> +		ret = arch_kexec_apply_relocation_add(ehdr, sechdrs,
> +						      ELF64_R_TYPE(rel[i].r_info),
> +						      sym, name, location,
> +						      address, value);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int kexec_apply_relocations(struct kimage *image)
>  {
>  	int i, ret;
> @@ -836,8 +949,7 @@ static int kexec_apply_relocations(struct kimage *image)
>  		 * relocations of type SHT_RELA/SHT_REL.
>  		 */
>  		if (sechdrs[i].sh_type == SHT_RELA)
> -			ret = arch_kexec_apply_relocations_add(pi->ehdr,
> -							       sechdrs, i);
> +			ret = kexec_apply_relocations_add(pi->ehdr, sechdrs, i);
>  		else if (sechdrs[i].sh_type == SHT_REL)
>  			ret = arch_kexec_apply_relocations(pi->ehdr,
>  							   sechdrs, i);

Hi,

As we are here, two weak functions looks not necessary, let arch code to
determin if SHT_REL can be handled is reasonable though I'm not sure why
there is not such things in kexec-tools.

Could you just use arch_kexec_apply_relocations for these two types
instead of adding another?

Thanks
Dave

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web