Message-ID: <20161123084522.GA20290@dhcp-128-65.nay.redhat.com>
Date:   Wed, 23 Nov 2016 16:45:22 +0800
From:   Dave Young <dyoung@...hat.com>
To:     Thiago Jung Bauermann <bauerman@...ux.vnet.ibm.com>
Cc:     kexec@...ts.infradead.org, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Eric Biederman <ebiederm@...ssion.com>,
        Vivek Goyal <vgoyal@...hat.com>, Baoquan He <bhe@...hat.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Stewart Smith <stewart@...ux.vnet.ibm.com>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>
Subject: Re: [PATCH v10 04/10] kexec_file: Add support for purgatory built as
 PIE.

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@...hat.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@...ux.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

Powered by blists - more mailing lists