Page MenuHomePhabricator

[ELF] Relocations: amends c8d9d0000b2f72b50729bfdc7bbb9dd3e9ecd6a0
Needs RevisionPublic

Authored by adalava on Sep 12 2022, 8:04 PM.

Details

Reviewers
nemanjai
MaskRay
Summary

It's expected to set set hasDirectReloc only if not ifunc.

This fixes the issue described in https://reviews.freebsd.org/D36234

Diff Detail

Event Timeline

adalava created this revision.Sep 12 2022, 8:04 PM
adalava published this revision for review.Sep 12 2022, 8:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2022, 8:04 PM
MaskRay requested changes to this revision.Sep 12 2022, 8:32 PM

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

This revision now requires changes to proceed.Sep 12 2022, 8:32 PM

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

@MaskRay, I don't understand, the commit c8d9d0000b2f72b50729bfdc7bbb9dd3e9ecd6a0 message says that it will "set hasDirectReloc only if not ifunc", but the change seems to do the opposite, that's why I'm proposing the amend change (and because it fixes the FreeBSD copyin/copyout symbol).

The issue I'm pursuing started after https://reviews.freebsd.org/rG47a57144af25a7bd768b29272d50a36fdf2874ba, where the IFUNC copyin/copyout is called thought a struct of function pointers. In LLD I found that the symbols get HAS_DIRECT_RELOC set and the type is changed from STT_GNU_IFUNC to STT_FUNC, then kernel modules crash when try to call them. It can be workarounded by creating a local wrapper function that just calls the function intended (code in https://reviews.freebsd.org/D36234). Do you have an idea of what else is missing on LLD?

I had no success creating an small code that reproduces it, but I can share the lld's reproducer for the kernel. Thanks!

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

@MaskRay, I don't understand, the commit c8d9d0000b2f72b50729bfdc7bbb9dd3e9ecd6a0 message says that it will "set hasDirectReloc only if not ifunc", but the change seems to do the opposite, that's why I'm proposing the amend change (and because it fixes the FreeBSD copyin/copyout symbol).

I made a mistake in the commit message. It should be "if ifunc"... You can verify that hasDirectReloc (now HAS_DIRECT_RELOC) is only functional when isGnuIfunc() is true.

The issue I'm pursuing started after https://reviews.freebsd.org/rG47a57144af25a7bd768b29272d50a36fdf2874ba, where the IFUNC copyin/copyout is called thought a struct of function pointers. In LLD I found that the symbols get HAS_DIRECT_RELOC set and the type is changed from STT_GNU_IFUNC to STT_FUNC, then kernel modules crash when try to call them. It can be workarounded by creating a local wrapper function that just calls the function intended (code in https://reviews.freebsd.org/D36234). Do you have an idea of what else is missing on LLD?

I had no success creating an small code that reproduces it, but I can share the lld's reproducer for the kernel. Thanks!

I need a reproduce tarball to analyze. shared STT_GNU_IFUNC becomes STT_FUNC in Symbols.h:436 and an ifunc with HAS_DIRECT_RELOC is converted to STT_FUNC, too.
The changes are necessary for correct semantics.

I made a mistake in the commit message. It should be "if ifunc"... You can verify that hasDirectReloc (now HAS_DIRECT_RELOC) is only functional when isGnuIfunc() is true.

oh, np!

I need a reproduce tarball to analyze. shared STT_GNU_IFUNC becomes STT_FUNC in Symbols.h:436 and an ifunc with HAS_DIRECT_RELOC is converted to STT_FUNC, too.
The changes are necessary for correct semantics.

Here it is, with more details: https://github.com/llvm/llvm-project/issues/57722

Thanks!