Revert "Additional icache paranoia: non-PLT relocations can modify the text segment."

[jrtc27]

Summary

reloc_nonplt_object, and thus reloc_non_plt, only ever handles data
relocations, so this paranoia is completely unfounded and only has the
effect of significantly slowing down program startup for binaries with
large amounts of code, like Clang.

If this breaks any systems, that would likely be due to insufficient
flushing in the pmap implementation for executable mappings, as this
existing rtld behaviour would mask any such bugs.

This reverts commit 4b51c69976fd84e93ec7695858375c8150c4fe61.

Test Plan

Entirely untested, but came out of an IRC discussion with ivy.

[kib]

in principle, non-plt relocations can modify any kind of segments. There are DT_TEXTREL binries, as well as nothing in ELF spec prevents existence of PF_W|PF_X segment with relocs.
IMO the commits that added the icache sync were done due to some actual problems seen.

I think a better approach is to lookup the segment for each relocation, and mark the segment for sync if it is executable. I believe that this should be not a problem due to very limited number of segments. In worst case it could be optimized with the helper hash table.

[jrtc27]

in principle, non-plt relocations can modify any kind of segments. There are DT_TEXTREL binries, as well as nothing in ELF spec prevents existence of PF_W|PF_X segment with relocs.
IMO the commits that added the icache sync were done due to some actual problems seen.

Yes, but none of the relocations that reloc_nonplt_object implements are ones that relocate instructions, they're all "relocate this data word to be this piece of data". This isn't x86, it's a RISC architecture, so there are specific relocations to modify instruction fields, rather than just relocating a word that's part of some bigger long instruction encoding.

[jrtc27]

IMO the commits that added the icache sync were done due to some actual problems seen.

Probably due to a broken pmap (at least at the time) then.

[kib]

In D54221#1238704, @jrtc27 wrote:

IMO the commits that added the icache sync were done due to some actual problems seen.

Probably due to a broken pmap (at least at the time) then.

You mean, flushing i- or d-cache when doing mprotect? The MI VM only calls into pmap_protect() when removing protection. For cases of upgrading access rights, it is left to pmap_enter(). IOW, it is rather complicated for pmap, and I think that explicit isync calls from userspace are better.

That said, due to locality of relocations sections, I think that for the efficient implementation of my proposal, the hash is even not needed. It is enough to memoize the last lookup of the segment, and verify that current relocation still fits into it. If not, then icache can be flushed if the segment was executable.

[jrtc27]

In D54221#1238719, @kib wrote:

In D54221#1238704, @jrtc27 wrote:

IMO the commits that added the icache sync were done due to some actual problems seen.

Probably due to a broken pmap (at least at the time) then.

You mean, flushing i- or d-cache when doing mprotect? The MI VM only calls into pmap_protect() when removing protection. For cases of upgrading access rights, it is left to pmap_enter(). IOW, it is rather complicated for pmap, and I think that explicit isync calls from userspace are better.

Or just mmap'ing them, because you might pick up another process's icache entries. This is how all the pmaps are meant to work today, you can go read the arm64 and riscv code for example to see them dealing with case. Otherwise userspace has to know that, if it mmap's some code with PROT_EXEC, it needs to cache invalidate it, which would be (a) an ABI break (b) different to Linux.

That said, due to locality of relocations sections, I think that for the efficient implementation of my proposal, the hash is even not needed. It is enough to memoize the last lookup of the segment, and verify that current relocation still fits into it. If not, then icache can be flushed if the segment was executable.

Sure. You could. But we know that no relocation will ever be for instructions, because we don't handle any of the relocations that can be used for instructions. So this would be pointless complexity. It's exactly the same story on arm64 and riscv. There's no corresponding icache flushing there at all, despite the memory models being all approximately the same (close enough at least for the purposes of this discussion). We just do not support text relocations.

Or if you don't believe me, you can go read the glibc code. There they decide whether to cache flush based on the relocation type seen (and do it per relocation, but you could instead batch them up and just flush the whole object if any such relocation is found). And if you compare the list of relocations that it regards as needing a cache flush with the set of relocations we handle, you will find that the intersection is empty.

[tpearson_raptorengineering.com]

I'm leaning toward @jrtc27's side of things here, but I would like to know more about the issues that were seen that prompted adding this code in the first place. Worst case is that the code being reverted "fixed" the original issue by simply modifying timing / avoiding a race condition, especially given that a.) we tend to have a lot more cores than the other two weak memory ordering architectures and b.) they don't have this kind of flushing behavior yet apparently function normally.

[kib]

Ok, if somebody from the powerpc crowd confirm that the isync is redundant there, I am fine with that.
If not, I put a sketch for my proposal at D54246.

[ivy]

i've added this patch to my local ppc64le build and haven't noticed any problems so far, but i'm not really qualified to say if it's actually correct or not. (that said, i tend to agree that if we actually need to cache flush here, this seems like an ABI issue that needs to be fixed elsewhere.)

[jhibbits]

I can't speak from authority on this, the original change almost predates my joining. @nwhitehorn made the change, and judging by the timing it was during his powerpc64 bringup, so likely related to some G5 or POWER4 issues encountered during his work. It's possible the real problem has been ironed out in the past 14 years.

[ivy]

after my last comment, i ran into an unexpected make(1) crash while running this patch. i need to do some more testing to see if this is actually the cause, but i suggest holding off on landing it for now.