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

["Fāng-ruì Sòng" <maskray@google.com>]

On Fri, Oct 15, 2021 at 6:00 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> On Fri, 15 Oct 2021 05:21:23 -0700
> "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
> > On Fri, Oct 15, 2021 at 5:09 AM Szabolcs Nagy via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> > >
> > > The 10/15/2021 09:54, Lukasz Majewski wrote:
> > > > This change is a partial revert of commit
> > > > bca0f5cbc9257c13322b99e55235c4f21ba0bd82
> > > > "arm: Simplify elf_machine_{load_address,dynamic}" 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 (very early)
> > > > _dl_start() entry point for the program. It shall return the load
> > > > address of the dynamic linker program.
> > > >
> > > > 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 is pre-set by binutils.
> > > >
> > > > The problem starts when one runs 'prelink' on the rootfs created
> > > > with for example OE/Yocto.
> > > > Then the _ehdr_start stays as 0x0, but the ELF header's sections
> > > > have different addresses - for example 0x41000000 instead of the
> > > > originally set 0x0.
> > > >
> > > > This is crucial when /sbin/init is executed. Value set in
> > > > __ehdr_start symbol is not updated. This causes the program to
> > > > crash very early when ld-linux-armhf.so.3's _dl_start is
> > > > executed, as calculated offset for loader relocation is going to
> > > > hit the kernel space (0xf7xxyyyy).
> > > >
> > > > It looks like the correct way to obtain the _dl_start offset on
> > > > ARM is to use assembler instruction 'adr' at execution time (so
> > > > the prelink assigned offset is taken into consideration) instead
> > > > of __ehdr_start.
> > > >
> > > > With this patch we only modify the elf_machine_load_address()
> > > > function, as it is called very early, before the
> > > > ld-linux-armhf.so.3 is performing relocation (also its own one).
> > >
> > > i'd use an explanation like:
> > >
> > > __ehdr_start is a linker created symbol that points to the elf
> > > header. The elf header is at the beginning of the elf file and
> > > normally its virtual address is 0 in a shared library.  This means
> > > the runtime address of __ehdr_start is the load address of the
> > > module.  However if prelinking is applied to ld.so then all virtual
> > > addresses are moved by an offset so the runtime address of the elf
> > > header becomes the load address + prelink offset.  The kernel does
> > > not treat prelinked ld.so specially so the load address is not 0,
> > > it still has to be computed, but simply using __ehdr_start no
> > > longer gives a correct value for that.
> > >
> > > This issue affects all targets with prelinking support, but so far
> > > we only got reports from OE/Yocto builds for arm that has prelinked
> > > ld.so.
> > >
> > > but i think a better fix is possible than revert:
> >
> > I think either prelink should be fixed not to prelink ld.so or Yocto
> > should be fixed not to prelink ld.so.
> >
>
> Could you explain why?
>
> Was the relocation of ld.so (I guess that ld.so = ld-linux-arm.so) a
> bug from the very beginning and it was apparent just now?

Prelinking improves application relocation performance but prelinking
ld.so itself doesn't provide any saving.
It is very likely that the prelink program doesn't intend to prelink
ld.so. It just doesn't provide a diagnostic.
If we look at the problem from this angle, prelinking ld.so is a pilot
error: OE/Yocto used an unsupported thing which happened to work in
the past.
Now, the unsupported (well, it can be supported if prelink correctly
prelinks ld.so) thing fails.
I sent the original commit trying to untangle the messy arm code.
Although Szabolcs's version is still short, I'd prefer we don't work
around glibc for error/prelink errors.

> From my point of view - the original change to use __ehdr_start broke
> working setups, so it is a regression and shall be fixed in glibc.
>
> Anyway, it would be beneficial to have input from other glibc
> developers how to proceed with this issue.
>
>
> 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