[llvm-branch-commits] [lld] ELF: Only rewrite non-preemptible IFUNCs to IPLT functions if a non-IRELATIVE relocation is needed. (PR #133531)

[Peter Collingbourne via llvm-branch-commits]

pcc wrote:

> I’m worried about the added complexity of ifunc and the extra parameter in isStaticLinkTimeConstant, a key component of relocation scanning where performance is critical. (Our scanning process is already noticeably slower than mold’s, and these factors are contributing to the gap.)

Fair enough, I will check the performance after this patch.

> If the security hardening efforts rely on ifunc, I’d strongly suggest reconsidering this approach.

It's not so much "rely" but that the alternatives have downsides that IMO end up making them worse than the ifunc approach.

> What’s the next PR in line?

Here is the list: https://github.com/orgs/llvm/projects/38

> If the goal of increasing ifunc adoption is to avoid updating rtld/libc, I don’t see that as a practical solution, and I’d strongly push back against linker changes. We should focus on improving rtld/libc instead. See: https://sourceware.org/bugzilla/show_bug.cgi?id=31959"

On the contrary, I see it as more practical than the alternatives.

If avoiding upgrades were the only reason for this change, I agree with you that we should be looking at a static-pie based approach instead. We may even consider dropping #133532 as it may be possible to develop another solution that uses the static-pie approach.

The main reason for supporting this use case for ifuncs is to decouple the pointer encoding scheme (which is a compiler implementation detail) from the dynamic loader. For pointer field protection we have these requirements:
- Be able to use deactivation symbols to deactivate pointer encoding in other object files.
- Be able to use arbitrary pointer encoding schemes as part of the non-arm64 support.

The following alternatives were considered:
- Have the compiler generate code in .init_array.0 to initialize the fields. This has the downside of making parts of .data.rel.ro writable which goes against the overall goal of improving security. Doing things this way *might* work if we unprotect relro, apply the pseudo-relocations and protect again, but this would need to be done once for the whole binary to avoid substantial startup overhead, and there may be restrictions such as mseal that prevent this from working. This is probably what I'll do if I get too much pushback from this approach, it looks like [glibc will support opting out of mseal](https://sourceware.org/pipermail/glibc-cvs/2024q3/086012.html).
- Introduce a new relocation format that would allow specifying a deactivation symbol (two operands to a relocation). This would introduce a substantial burden on linkers due to the new relocation encoding with an additional operand and would still not give us a flexible pointer encoding.
- Introduce new relocation types for every new encoding that may be used (combined with the new relocation format). This would substantially reduce our velocity as the new encodings would need to be agreed on by every supported non-arm64 psabi, as well as introducing complexity to linkers as they would need to know about every new relocation type used for encodings (this would be basically like introducing a new R_AARCH64_AUTH_ABS64/R_AARCH64_AUTH_RELATIVE pair every time).

https://github.com/llvm/llvm-project/pull/133531