Require the SHF_ALLOC flag for program sections from kernel object modules.

[jhb]

Summary

ELF object files can contain program sections which are not supposed
to be loaded into memory (e.g. .comment). Normally the static linker
uses these flags to decide which sections are allocated to loadable
program segments in ELF binaries and shared objects (including kernels
on all architectures and kernel modules on architectures other than
amd64).

Mapping ELF object files (such as amd64 kernel modules) into memory
directly is a bit of a grey area. ELF object files are intended to be used as
inputs to the static linker. As a result, there is not a standardized definition
for what the memory layout of an ELF object should be (none of the section
headers have valid virtual memory addresses for example).

The kernel and loader were not checking the SHF_ALLOC flag but loading
any program sections with certain types such as SHT_PROGBITS. As a result,
the kernel and loader would load into RAM some sections that weren't marked
with SHF_ALLOC such as .comment that are not loaded into RAM for kernel
modules on other architectures (which are implemented as ELF shared objects).
Aside from possibly requiring slightly more RAM to hold a kernel module this
does not affect runtime correctness as the kernel relocates symbols based on
the layout it uses.

Debuggers such as gdb and lldb do not extract symbol tables from a running process
or kernel. Instead, they replicate the memory layout of ELF executables and
shared objects and use that to construct their own symbol tables. For executables
and shared objects this works fine. For ELF objects the current logic in kgdb (and
probably lldb based on a simple reading) assumes that only sections with SHF_ALLOC
are memory resident when constructing a memory layout. If the debugger constructs
a different memory layout than the kernel, then it will compute different addresses for
symbols causing symbols in the debugger to appear to have the wrong values (though
the kernel itself is working fine). The current port of mdb does not check SHF_ALLOC
as it replicates the kernel's logic in its existing kernel support.

The bfd linker sorts the sections in ELF object files such that all of the allocated sections
(sections with SHF_ALLOCATED) are placed first followed by unallocated sections. As
a result, when kgdb composed a memory layout using only the allocated sections, this
layout happened to match the layout used by the kernel and loader. The lld linker does
not sort the sections in ELF object files and mixed allocated and unallocated sections.
This resulted in kgdb composing a different memory layout than the kernel and loader.

We could either patch kgdb (and possibly in the future lldb) to use custom handling when
generating memory layouts for kernel modules that are ELF objects, or we could change
the kernel and loader to check SHF_ALLOCATED. I chose the latter as I feel we shouldn't
be loading things into RAM that the module won't use. This should mostly be a NOP when
linking with bfd but will allow the existing kgdb to work with amd64 kernel modules linked
with lld.

Note that we only require SHF_ALLOC for "program" sections for types
like SHT_PROGBITS and SHT_NOBITS. Other section types such as symbol
tables, string tables, and relocations must also be loaded and are not
marked with SHF_ALLOC.

Test Plan

• use kgdb to examine a global variable in a kernel module ('t4_list' from if_cxgbe.ko) before and after the change with the module loaded at both boot time and runtime.

[emaste]

I think this is fine. Do you want to handle the case in ​link_elf_link_preload too?

[jhb]

In D13926#292027, @emaste wrote:

I think this is fine. Do you want to handle the case in ​link_elf_link_preload too?

I do think I need to adjust that for completeness, but perhaps a bit differently. In particular, we should take whatever the loader gives us probably by skipping entries with 'sh_addr == 0' rather than checking SHF_ALLOC directly which we already do for part of link_elf_link_preload. I think it is only the final loop that populates progtab[] that should be adjusted (and it probably doesn't hurt that we aren't skipping the sh_addr == 0 sections currently).

[jhb]

• Don't setup progtab entries for sections not loaded by the loader.

[emaste]

sys/kern/link_elf_obj.c

265 ↗

(On Diff #38055)

Maybe a comment here since it's not immediately clear from local context why sh_addr would be 0?

[kib]

I think that this change does not affect correctness of the loader and kernel linkers code. I mean that, in my opinion, it is fine to either load or not load the non-allocatable sections. More, it is fine to introduce arbitrary inter-section gaps between the sections, because compiler or assembler cannot know the final linking layout of the resulting executable or shared object, so they must provide enough relocations to make the linking result correct. From this PoV, presence of the non-allocatable sections between progbits is just a gap.

I suspect the problem is that gdb does not handle .o at all, it expects that the section table describes the result of the final link. This would explain the assumptions about the layout, perhaps ?

To make it clear: I do not object against committing this change if (or since) it provides the immediate relief for the kgdb functionality.

[jhb]

In D13926#292526, @kib wrote:

I think that this change does not affect correctness of the loader and kernel linkers code. I mean that, in my opinion, it is fine to either load or not load the non-allocatable sections. More, it is fine to introduce arbitrary inter-section gaps between the sections, because compiler or assembler cannot know the final linking layout of the resulting executable or shared object, so they must provide enough relocations to make the linking result correct. From this PoV, presence of the non-allocatable sections between progbits is just a gap.

It does potentially waste RAM (not so much for .comment, but possibly for CTF for which we load a duplicate copy). I think markj@ also ran into this with dtrace and had to employ some kind of workaround actually. At the time I wanted to make use of the CTF we had already loaded, but I think I'd rather that if we want to do that, we mark the CTF as SHF_ALLOC so that both DSOs and object files will always load it.

I suspect the problem is that gdb does not handle .o at all, it expects that the section table describes the result of the final link. This would explain the assumptions about the layout, perhaps ?

GDB just assumes that sections without SHF_ALLOC will not allocate space in virtual memory, so it walks the section table constructing a layout that assumes the memory-resident sections are packed together with their requested alignment, etc. Namely, the routine build_section_table() used for both executables and shared libraries (and kgdb treats modules as shared libraries in effect) uses this helper function:

https://github.com/bsdjhb/gdb/blob/master/gdb/exec.c#L445

It skips sections without SEC_ALLOC set (which on ELF is mapped to SHF_ALLOC). lldb also seems to honor this (see ObjectFileELF::SetLoadAddress() which assumes it skip sections without this flag when computing a trial layout of an ELF object).

To make it clear: I do not object against committing this change if (or since) it provides the immediate relief for the kgdb functionality.

I could perhaps add a special case in kgdb, but we'd have to also replicate that special case in lldb, etc. Fighting against what ELF intends doesn't really seem useful. I will add markj@ to get his thoughts though (perhaps he recalls the previous issue with dtrace IIRC about this)

[kib]

In D13926#292633, @jhb wrote:

Fighting against what ELF intends doesn't really seem useful.

ELF intent was that .o are only used as an input to the static linker. If gdb or lldb accept .o as the image, they should also accept that there is no layout of sections yet.

[jhb]

In D13926#292681, @kib wrote:

In D13926#292633, @jhb wrote:

Fighting against what ELF intends doesn't really seem useful.

ELF intent was that .o are only used as an input to the static linker. If gdb or lldb accept .o as the image, they should also accept that there is no layout of sections yet.

Using .o's as input to the kernel instead of the static linker violates the ELF intent first. We could at least try to violate it less badly. I think honoring SHF_ALLOC is less bad and gives a layout closer to what a DSO generated by the static linker would produce (if it did not have PLT stubs). In theory we should be able to free the relocation tables once we have finished relocating in which case we would only need to keep the symbol and string tables around outside of SHF_ALLOC sections. I think that not checking SHF_ALLOC here was just an oversight from the original work rather than an intentional design decision. Arguably we should build the layout by loading all the SHF_ALLOC sections in order and then loading the special sections like relocations and symbol tables on the side (which we basically do already, and checking SHF_ALLOC would not require MD #ifdef's like SHT_X86_64_UNWIND).

[markj]

In D13926#292633, @jhb wrote:

In D13926#292526, @kib wrote:

I think that this change does not affect correctness of the loader and kernel linkers code. I mean that, in my opinion, it is fine to either load or not load the non-allocatable sections. More, it is fine to introduce arbitrary inter-section gaps between the sections, because compiler or assembler cannot know the final linking layout of the resulting executable or shared object, so they must provide enough relocations to make the linking result correct. From this PoV, presence of the non-allocatable sections between progbits is just a gap.

It does potentially waste RAM (not so much for .comment, but possibly for CTF for which we load a duplicate copy). I think markj@ also ran into this with dtrace and had to employ some kind of workaround actually.

I'm having trouble remembering what you're referring to. Perhaps the dm_sec_offsets array that we maintain for .o modules in libdtrace?

At the time I wanted to make use of the CTF we had already loaded, but I think I'd rather that if we want to do that, we mark the CTF as SHF_ALLOC so that both DSOs and object files will always load it.

For the kernel that would make sense, but I'm not so sure about userland binaries. For that case, I'd like to treat it as debug info and strip it into a separate .gnu_debuglink file.

[jhb]

I attempted to update the comment a bit to not claim that there is only one correct way, but that mapping ELF objects directly is a grey area. I still prefer the approach in the patch, but hopefully the message is now more accurate?