Re: [PATCH v2 03/21] elf: add dynamic relocation support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 1/6/26 1:53 PM, Sascha Hauer wrote:
> Add support for applying dynamic relocations to ELF binaries. This allows
> loading ET_DYN (position-independent) binaries and ET_EXEC binaries at
> custom load addresses.
> 
> Key changes:
> - Add elf_image.reloc_offset to track offset between vaddr and load address
> - Implement elf_compute_load_offset() to calculate relocation offset
> - Add elf_set_load_address() API to specify custom load address
> - Implement elf_find_dynamic_segment() to locate PT_DYNAMIC
> - Add elf_relocate() to apply relocations
> - Provide weak default elf_apply_relocations() stub for unsupported architectures
> - Add ELF dynamic section accessors
> 
> The relocation offset type is unsigned long to properly handle pointer
> arithmetic and avoid casting issues.
> 
> Architecture-specific implementations should override the weak
> elf_apply_relocations() function to handle their relocation types.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Wrong way round ^

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

But some nitpicks and a question below.

> +static void *elf_get_dest(struct elf_image *elf, void *phdr)

How about elf_phdr_relocated_paddr as a more descriptive name?

> +	if (elf->load_address) {
> +		elf->base_load_addr = elf->load_address;
> +	} else if (elf->type == ET_EXEC) {
> +		elf->base_load_addr = NULL;
> +	} else {
> +		elf->base_load_addr = (void *)(phys_addr_t)min_paddr;
> +	}

Curly braces can be removed.

> +	if (elf->type == ET_EXEC && !elf->load_address) {
> +		elf->reloc_offset = 0;
> +	} else {
> +		elf->reloc_offset = ((unsigned long)elf->base_load_addr - min_vaddr);
> +	}

Ditto.


> +void elf_set_load_address(struct elf_image *elf, void *addr)
> +{
> +	elf->load_address = addr;
> +}
> +
> +static void *elf_find_dynamic_segment(struct elf_image *elf)
> +{
> +	void *buf = elf->hdr_buf;
> +	void *phdr = buf + elf_hdr_e_phoff(elf, buf);
> +	int i;
> +
> +	for (i = 0; i < elf_hdr_e_phnum(elf, buf); i++) {
> +		if (elf_phdr_p_type(elf, phdr) == PT_DYNAMIC) {
> +			u64 offset = elf_phdr_p_offset(elf, phdr);
> +
> +			/* If loaded from file, PT_DYNAMIC might not be in hdr_buf */
> +			if (elf->filename)
> +				return elf_get_dest(elf, phdr);
> +			else
> +				/* Binary in memory, use offset */
> +				return elf->hdr_buf + offset;

Does elf_get_dest(elf, phdr) not compute the same address that we would
get here?

> +	/* Check that we found exactly one relocation type */
> +	if (rel && rela) {
> +		pr_err("ELF has both REL and RELA relocations\n");
> +		return -EINVAL;
> +	}

Future work could be checking these things at compile time and skipping
the checks in PBL.

> diff --git a/lib/Makefile b/lib/Makefile
> index 6d259dd94e163336d7fdab38c7b74b301aabc5c5..da2c8ffe1dbf512a901295c89494e0837f31a0d9 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -24,6 +24,7 @@ obj-y			+= readkey.o
>  obj-y			+= kfifo.o
>  obj-y			+= libbb.o
>  obj-y			+= libgen.o
> +obj-y			+= elf_reloc.o

Why is this not obj-pbl-y?

Cheers,
Ahmad

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |