Re: [PATCH] dl: Use "adr" assembler command to get proper load address

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 5845 bytes --]

Hi Fangrui,

> On 2021-09-07, Lukasz Majewski wrote:
> >This change is a partial revert of commit
> >bca0f5cbc9257c13322b99e55235c4f21ba0bd82 which imposed usage of
> >__ehdr_start linker variable to get the address of loaded program.
> >
> >The elf_machine_load_address() function is declared in the
> >sysdeps/arm/dl-machine.h header. It is called from _dl_start() entry
> >point for the program. It shall return the load address of the
> >dynamic linker program.  
> 
> Yes.
> 
> >With this revert the 'adr' assembler instruction is used instead of a
> >place holder:
> >
> >arm-poky-linux-gnueabi-objdump -t ld-linux-armhf.so.3 | grep ehdr
> >00000000 l       .note.gnu.build-id     00000000      __ehdr_start
> >
> >which shall be pre-set by binutils.  
> 
> I don't understand this. The sh_addr field of the binutils defined
> __ehdr_start is 0.
> 
> Declararing __ehdr_start as hidden and accessing it with PC-relative
> addressing generates computes the runtime address of __ehdr_start,
> which equals the load base.

Maybe I don't get it - but shouldn't this be filled in by binutils
(linker ?) when the program is assembled before run?

Or is the __ehdr_start used just as a relative offset to get the proper
position of ld-linux-armh.so when called?

> 
> What's wrong with it? The objdump -t line doesn't tell why this change
> is good.

I'm also puzzled why this causes the QEMU versalite board to break.
(/sbin/init dies with SIGSEGFAULT = 0xb [*])

It looks like the &__ehdr_start is not equal in the result to 'adr'
assembler code in QEMU.

I also thought that the issue is caused by fs/binfmt_elf.c changes in
the Linux kernel (as mine is 5.1), so I've tested it with 5.10 and
5.14. No change.

The QEMU is pretty recent - it is 6.0.0 version for ARM.

> Can you compare objdump -dr output and show the incorrect
> code sequences with C assessing __ehdr_start?

Ok. I will explicitly compare _dl_start function with and without this
patch.

> 
> >This is crucial in the QEMU ARM environment for which (when
> >/sbin/init is executed) values set in __ehdr_start symbol are wrong.
> >This causes the program to crash very early - when the
> >/lib/ld-linux-armhf.so.3 is executed as a prerequisite to /sbin/init
> >execution.  
> 
> >The kernel's fs/binfmt_elf.c is though responsible for setting
> >up execution environment, not binutils.  
> 
> Whether the symbol entry __ehdr_start exists doesn't matter.
> The hidden definition does not have any associated relocation.

I'm also wondering if similar issue is visible with "normal" - i.e. not
QEMU ARM run init.

Maybe the "rough" environment in which /sbin/init is run is the culprit?

> 
> >It looks like the only robust way to obtain the _dl_start offset is
> >to use assembler instruction - not rely on values provided by
> >binutils.
> >
> >HW:
> >Hardware name: ARM-Versatile Express (Run with QEMU)
> >Tested (affected) kernels v5.1.12, v5.10.62 and v5.14.1
> >
> >When the /sbin/init is setup for run from Linux kernel's very small
> >environment with LD_DEBUG=all the __ehdr_start is not shown at all.
> >
> >Fixes: BZ #28293
> >---
> > sysdeps/arm/dl-machine.h | 28 +++++++++++++++++++++++++---
> > 1 file changed, 25 insertions(+), 3 deletions(-)
> >
> >diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
> >index eb13cb8b57..58ebef6ecd 100644
> >--- a/sysdeps/arm/dl-machine.h
> >+++ b/sysdeps/arm/dl-machine.h
> >@@ -38,11 +38,33 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr)
> > }
> >
> > /* Return the run-time load address of the shared object.  */
> >-static inline ElfW(Addr) __attribute__ ((unused))
> >+static inline Elf32_Addr __attribute__ ((unused))
> > elf_machine_load_address (void)
> > {
> >-  extern const ElfW(Ehdr) __ehdr_start attribute_hidden;
> >-  return (ElfW(Addr)) &__ehdr_start;
> >+  Elf32_Addr pcrel_addr;
> >+#ifdef SHARED
> >+  extern Elf32_Addr __dl_start (void *) asm ("_dl_start");
> >+  Elf32_Addr got_addr = (Elf32_Addr) &__dl_start;
> >+  asm ("adr %0, _dl_start" : "=r" (pcrel_addr));
> >+#else
> >+  extern Elf32_Addr __dl_relocate_static_pie (void *)
> >+    asm ("_dl_relocate_static_pie") attribute_hidden;
> >+  Elf32_Addr got_addr = (Elf32_Addr) &__dl_relocate_static_pie;
> >+  asm ("adr %0, _dl_relocate_static_pie" : "=r" (pcrel_addr));
> >+#endif
> >+#ifdef __thumb__
> >+  /* Clear the low bit of the function address.
> >+
> >+     NOTE: got_addr is from GOT table whose lsb is always set by
> >linker if it's
> >+     Thumb function address.  PCREL_ADDR comes from PC-relative
> >calculation
> >+     which will finish during assembling.  GAS assembler before the
> >fix for
> >+     PR gas/21458 was not setting the lsb but does after that.
> >Always do the
> >+     strip for both, so the code works with various combinations of
> >glibc and
> >+     Binutils.  */
> >+  got_addr &= ~(Elf32_Addr) 1;
> >+  pcrel_addr &= ~(Elf32_Addr) 1;
> >+#endif
> >+  return pcrel_addr - got_addr;
> > }
> >
> > /* Return the link-time address of _DYNAMIC.  */
> >-- 
> >2.20.1
> >  

Note:

[*] - the QEMU debugging is pretty difficult with /sbin/init. Qemu has
good support for kernel debugging (-s -S switches). Then for user space
program one could use the "on-board" gdb or gdbserver.

However, /sbin/init needs to be debugged from linux and scheduler
context switches needs to be taken into account to review how the code
is executed. And such debugging scenario has issue with QEMU-arm.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]