Page MenuHomePhabricator

[ELF] Move R_*_IRELATIVE from .rel[a].plt to .rel[a].dyn unless --pack-dyn-relocs=android[+relr]
ClosedPublic

Authored by MaskRay on Aug 2 2019, 6:17 AM.

Details

Summary

An R_*_IRELATIVE represents the address of a STT_GNU_IFUNC symbol
(redirected at runtime) which is non-preemptable and is not associated
with a canonical PLT (associated with a symbol with a section index of
SHN_UNDEF but a non-zero st_value).

.rel[a].plt [DT_JMPREL, DT_JMPREL+DT_JMPRELSZ) contains relocations that
can be lazily resolved. R_*_IRELATIVE are always eagerly resolved, so
conceptually they do not belong to .rela.plt. "iplt" is mostly a misnomer.

glibc powerpc and powerpc64 do not resolve R_*_IRELATIVE if they are in .rela.plt.

// a.o - synthesized PLT call stub has an R_*_IRELATIVE
void ifunc(); int main() { ifunc(); }
// b.o
static void real() {}
asm (".type ifunc, %gnu_indirect_function");
void *ifunc() { return ℜ }

The lld-linked executable crashes. ld.bfd places R_*_IRELATIVE in
.rela.dyn and the executable works.

glibc i386, x86_64, and aarch64 have logic
(glibc/sysdeps/*/dl-machine.h:elf_machine_lazy_rel) to eagerly resolve
R_*_IRELATIVE in .rel[a].plt so the lld-linked executable works.

Move R_*_IRELATIVE from .rel[a].plt to .rel[a].dyn to fix the crashes on
glibc powerpc/powerpc64. This also helps simplifying ifunc
implementation in FreeBSD rtld-elf powerpc64.

If --pack-dyn-relocs=android[+relr] is specified, the Android packed
dynamic relocation format is used for .rela.dyn. We cannot name
in.relaIplt ".rela.dyn" because the output section will have mixed
formats. This can be improved in the future.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Aug 2 2019, 6:17 AM
MaskRay updated this revision to Diff 213054.Aug 2 2019, 8:32 AM
MaskRay retitled this revision from [ELF] Move R_*_IRELATIVE from .rel[a].plt to .rel[a].dyn unless --pack-dyn-relocs=android to [ELF] Move R_*_IRELATIVE from .rel[a].plt to .rel[a].dyn unless --pack-dyn-relocs=android[+relr].
MaskRay edited the summary of this revision. (Show Details)
MaskRay added reviewers: grimar, luporl, peter.smith, ruiu.
MaskRay removed a subscriber: wuzish.

Ready

MaskRay added a reviewer: pcc.Aug 2 2019, 8:33 AM
MaskRay edited the summary of this revision. (Show Details)Aug 2 2019, 8:36 AM
pcc added a comment.Aug 2 2019, 9:32 AM

With this dynamic loaders will now need to be able to understand IRELATIVE relocations in .rela.dyn. Have you checked whether we can reasonably expect this to be universal or does this need to be done on a per-architecture basis?

ELF/Writer.cpp
507 ↗(On Diff #213054)

Update comment

1071 ↗(On Diff #213054)

Update comment

MaskRay added a comment.EditedAug 2 2019, 10:07 AM

Have you checked whether we can reasonably expect this to be universal or does this need to be done on a per-architecture basis?

On non-Android, it is universal to place R_*_IRELATIVE in .rel[a].dyn :) __glibc_unlikely suggests it is not a good idea to have R_*_IRELATIVE in .rel[a].plt:

sysdeps/aarch64/dl-machine.h
444:  else if (__glibc_unlikely (r_type == AARCH64_R(IRELATIVE)))

sysdeps/x86_64/dl-machine.h
574:  else if (__glibc_unlikely (r_type == R_X86_64_IRELATIVE))

On Android (--pack-dyn-relocs=android[+relr]), because there is already one ".rela.dyn" used for the packed format. We cannot create another ".rela.dyn". So I mentioned in the description this should be improved. Either synthetic section should be renamed to not have an output section with mixed formats.

Once this is accepted, I'll rename in.relaIplt to in.relaIdyn.

MaskRay updated this revision to Diff 213074.Aug 2 2019, 10:07 AM

Update comments

pcc added a comment.Aug 2 2019, 10:33 AM

Have you checked whether we can reasonably expect this to be universal or does this need to be done on a per-architecture basis?

On non-Android, it is universal to place R_*_IRELATIVE in .rel[a].dyn :)

Are you saying that that's the behaviour of an existing linker? If so, this change is fine with me. Otherwise we need to be careful because if there is a dynamic loader out there that only supports R_*_IRELATIVE in .rel[a].plt and not in .rel[a].dyn on some architecture (basically the inverse of the situation that you discovered), your change would break that dynamic loader on that architecture.

MaskRay added a comment.EditedAug 2 2019, 6:15 PM
In D65651#1612576, @pcc wrote:

Have you checked whether we can reasonably expect this to be universal or does this need to be done on a per-architecture basis?

On non-Android, it is universal to place R_*_IRELATIVE in .rel[a].dyn :)

Are you saying that that's the behaviour of an existing linker? If so, this change is fine with me. Otherwise we need to be careful because if there is a dynamic loader out there that only supports R_*_IRELATIVE in .rel[a].plt and not in .rel[a].dyn on some architecture (basically the inverse of the situation that you discovered), your change would break that dynamic loader on that architecture.

  • arm (non-Android and Android): no-op
  • i386/x86_64/aarch64: .rela.plt -> .rela.dyn (either works). R_*_IRELATIVE in .rel[a].plt is discouraged by __glibc_unlikely... A glibc aarch64 maintainer says on aarch64, R_*_IRELATIVE can appear in both .rela.plt and .rela.dyn, but he wants to *fix the problem* by using .rela.dyn exclusively.
  • powerpc: .rela.plt -> .rela.dyn (fixes a crash)
  • powerpc64:.rela.plt -> .rela.dyn (fixes a crash)
  • mips: binutils/glibc don't support ifunc

Chatted with @luporl. They are still bringing up FreeBSD powerpc64. Moving from .rela.plt to .rela.dyn will save their rtld-elf trouble..

(On glibc powerpc/powerpc64, without the change, we can use -z now or LD_BIND_NOW=1 to fix the crash.)

Does the summary look good?

MaskRay updated this revision to Diff 213163.Aug 2 2019, 6:21 PM
MaskRay edited the summary of this revision. (Show Details)

Update summary. This doesn't fix PR42759 (FreeBSD rtld-elf powerpc64). It still helps their implementation, though.

pcc accepted this revision.Aug 2 2019, 6:31 PM
In D65651#1612576, @pcc wrote:

Have you checked whether we can reasonably expect this to be universal or does this need to be done on a per-architecture basis?

On non-Android, it is universal to place R_*_IRELATIVE in .rel[a].dyn :)

Are you saying that that's the behaviour of an existing linker? If so, this change is fine with me. Otherwise we need to be careful because if there is a dynamic loader out there that only supports R_*_IRELATIVE in .rel[a].plt and not in .rel[a].dyn on some architecture (basically the inverse of the situation that you discovered), your change would break that dynamic loader on that architecture.

  • arm (non-Android and Android): no-op
  • i386/x86_64/aarch64: .rela.plt -> .rela.dyn (either works). R_*_IRELATIVE in .rel[a].plt is discouraged by __glibc_unlikely... A glibc aarch64 maintainer says on aarch64, R_*_IRELATIVE can appear in both .rela.plt and .rela.dyn, but he wants to *fix the problem* by using .rela.dyn exclusively.
  • powerpc: .rela.plt -> .rela.dyn (fixes a crash)
  • powerpc64:.rela.plt -> .rela.dyn (fixes a crash)
  • mips: binutils/glibc don't support ifunc

    Chatted with @luporl. They are still bringing up FreeBSD powerpc64. Moving from .rela.plt to .rela.dyn will save their rtld-elf trouble..

    (On glibc powerpc/powerpc64, without the change, we can use -z now or LD_BIND_NOW=1 to fix the crash.)

    Does the summary look good?

Okay, thanks for checking. LGTM

This revision is now accepted and ready to land.Aug 2 2019, 6:31 PM
This revision was automatically updated to reflect the committed changes.