[PATCH] D111181: [llvm-objcopy][ELF] Don't assume RELA sections are dynamic if they carry the SHF_ALLOC flag

[Ard Biesheuvel via Phabricator via llvm-commits]

ardb added a comment.

In D111181#3046870 <https://reviews.llvm.org/D111181#3046870>, @MaskRay wrote:

> In D111181#3046669 <https://reviews.llvm.org/D111181#3046669>, @ardb wrote:
>
>> In D111181#3046461 <https://reviews.llvm.org/D111181#3046461>, @MaskRay wrote:
>>
>>> In D111181#3046373 <https://reviews.llvm.org/D111181#3046373>, @ardb wrote:
>>>
>>>> In D111181#3045770 <https://reviews.llvm.org/D111181#3045770>, @MaskRay wrote:
>>>>
>>>>>> While this occurs rarely for normal user programs, the Linux kernel uses partially linked ET_REL objects as its loadable module format, and there, we sometimes rely on SHF_ALLOC to indicate to the module loader that the static relocations (which the Linux kernel fixes up at module load time) need to be preserved in memory after loading of the module completes.
>>>>>>
>>>>>> So let's extend the existing heuristic with a check against the file type: for ET_REL type files, which are only partiallly linked and cannot carry dynamic relocations, we assume that the RELA sections are static. For ET_DYN/ET_EXEC type files, we stick to the old behavior, which is to use the SHF_ALLOC flag as a hint to decide whether relocation sections are static or dynamic.
>>>>>
>>>>> If this is a Linux kernel requirement, can you link to the related discussion?
>>>>> From my experience dealing with ELF files, I don't think the Linux kernel usage is correct.
>>>>>
>>>>> SHT_REL[A] SHF_ALLOC sections are dynamic relocation sections.
>>>>
>>>> Where is this specified?
>>>
>>> This area is not specified in the generic ABI. This is a convention used as an (unspecified) protocol between the linker and the loader.
>>
>> Which linker and which loader? Does llvm-objcopy only target ELF files created with LLD for use in user programs? And which libc/libdl implementations is it limited to?
>
> I have considered many linkers (GNU ld, gold, ld.lld) and many loaders (glibc ld.so, musl ld.so, FreeBSD rtld, NetBSD libexec-elf).
>
>> Please try to understand that ELF may be used in different ways in different contexts, and unspecified protocols and conventions that only cover user executables and shared libraries may not work for complex bare metal programs such as the Linux kernel.
>
> Sure. But SHF_ALLOC SHT_REL[A] in an ET_REL file is a quite unusual thing.
> How did you get it in the first place?
> I don't think any linker's -r mode can synthesize such relocation sections.

$ llvm-objcopy --set-section-flags=.rela.text=alloc,readonly fips140.ko

could be used, but in this case, I am using my own C program.

> The justification for its usage is exactly the thing I requested.
> I requested a kernel discussion in my initial reply, but you did not provide one until this comment.

True, my apologies.

>>> I think the convention started when SunOS contributed dynamic linking to System V release 4 which was then inherited to all ELF operating systems.
>>
>> ELF user space programs, right? What about other contexts where ELF is used?
>>
>>>>> Since ET_REL files should not contain dynamic linking information, it doesn't make sense to use SHT_REL[A] SHF_ALLOC sections.
>>>>> At the very least SHF_ALLOC `.rela.text` linking to non-SHF_ALLOC `.symtab` seems wrong.
>>>>
>>>> The use case in question does not require the symtab section or strtab section, but I agree that this is an inconsistency.
>>>
>>> This is the exact point I question the Linux kernel's use case.
>>> So I asked for a link about the usage.
>>
>> There is some background here:
>> https://android-review.googlesource.com/c/kernel/common/+/1842462/3

...

>>>> The reason we want this is to be able to track how/where the text and rodata were modified after they were loaded into memory and relocated in-place.
>>>>
>>>> Are you claiming that this arrangement violates the ELF spec? If it does, it would be helpful if you could point out why this is the case. I agree that this is an unusual use of ELF sections, but that by itself should not be a justification for disregarding this change.
>>>
>>> The ELF specification is loosely specified in some areas. Not violating the ELF spec doesn't mean we should add some code (complexity).
>>> We need to place house rules.
>>
>> House rules are fine if you communicate them in some way. Dismissing something because it violates unspecified protocols or unspecified house rules seems arbitrary at the very least.
>
> See my previous paragraph. I am not sure "dismissing something" is on my side. I requested a link in my first reply, but you only did just now.
> OK, let's just calm down.

Apologies if the tone of my reply suggested otherwise, but I can assure you that I am perfectly calm.

>> I understand that you may dislike the use of SHF_ALLOC SHT_RELA sections in this manner, but that does not make it wrong, even if it is in poor taste.
>
> I follow Occam's razor for such unusual things.
> If your patch decreased code complexity I would probably not object.
> But your patch increases complexity, so I request justification, especially given that it quite unusual (SHF_ALLOC SHT_REL[A] in an ET_REL file; a SHF_ALLOC section's sh_link field links to a non-SHF_ALLOC section).
>
> "does not make it wrong" is not a good justification. Why can't such unusual usage be avoided in the first place?

I guess you are saying that llvm-objcopy only targets the subset of well-formed, compliant ELF files that are likely to be emitted by tools created for building user space programs.

Assuming that a SHT_RELA section with SHF_ALLOC set in an ET_REL file is a dynamic relocation section is an odd thing to do regardless of the context, given that such files cannot be used for dynamic relocation in the first place, so I would argue that this is a code improvement in itself.

In any case, thanks for the discussion, Please disregard this patch, we will find another solution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111181/new/

https://reviews.llvm.org/D111181