Messages in this thread Patch in this message | | | Date | Fri, 28 Apr 2023 22:29:12 +0000 | From | Fangrui Song <> | Subject | Re: [PATCH] arm64: lds: move .got section out of .text |
| |
On 2023-04-28, Ard Biesheuvel wrote: >On Fri, 28 Apr 2023 at 22:06, Fangrui Song <maskray@google.com> wrote: >> >> On 2023-04-28, Ard Biesheuvel wrote: >> >On Fri, 28 Apr 2023 at 19:58, Fangrui Song <maskray@google.com> wrote: >> >> >> >> On 2023-04-28, Ard Biesheuvel wrote: >> >> >Hello Fangrui, >> >> >> >> Hello Ard, thank you for the rapid response. >> >> >> >> >On Fri, 28 Apr 2023 at 06:05, Fangrui Song <maskray@google.com> wrote: >> >> >> >> >> >> Currently, the .got section is placed within the output section .text. >> >> >> However, when .got is non-empty, the SHF_WRITE flag is set when linked >> >> >> by lld. GNU ld recognizes .text as a special section and ignores the >> >> >> SHF_WRITE flag. By renaming .text, we can also get the SHF_WRITE flag. >> >> >> >> >> >> Conventionally, the .got section is placed just before .got.plt (which >> >> >> should be empty and omitted in the kernel). Therefore, we move the .got >> >> >> section to a conventional location (between .text and .data) and remove >> >> >> the unneeded `. = ALIGN(16)`. >> >> >> >> >> >> Signed-off-by: Fangrui Song <maskray@google.com> >> >> >> --- >> >> >> arch/arm64/kernel/vmlinux.lds.S | 20 ++++++++++---------- >> >> >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> >> >> >> >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> >> >> index b9202c2ee18e..2bcb3b30db41 100644 >> >> >> --- a/arch/arm64/kernel/vmlinux.lds.S >> >> >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> >> >> @@ -181,18 +181,8 @@ SECTIONS >> >> >> KPROBES_TEXT >> >> >> HYPERVISOR_TEXT >> >> >> *(.gnu.warning) >> >> >> - . = ALIGN(16); >> >> >> - *(.got) /* Global offset table */ >> >> >> } >> >> >> >> >> >> - /* >> >> >> - * Make sure that the .got.plt is either completely empty or it >> >> >> - * contains only the lazy dispatch entries. >> >> >> - */ >> >> >> - .got.plt : { *(.got.plt) } >> >> >> - ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, >> >> >> - "Unexpected GOT/PLT entries detected!") >> >> >> - >> >> >> . = ALIGN(SEGMENT_ALIGN); >> >> >> _etext = .; /* End of text section */ >> >> >> >> >> >> @@ -247,6 +237,16 @@ SECTIONS >> >> >> >> >> >> . = ALIGN(SEGMENT_ALIGN); >> >> >> __inittext_end = .; >> >> >> + >> >> >> + .got : { *(.got) } >> >> > >> >> >This is the .init region, which gets freed and unmapped after boot. If >> >> >the GOT is non-empty, it needs to remain mapped, so we cannot place it >> >> >here. >> >> >> >> Thanks. I did not know the constraint. >> >> >> >> >We have the same issue with the .rodata section, which incorporates >> >> >variables marked as __ro_after_init, which are not const qualified. So >> >> >given that .rodata is already emitted as WA, and we cannot do anything >> >> >about that, let's move the GOT in there. >> >> >> >> Yes, writable .data..ro_after_init and __jump_table sections in >> >> include/asm-generic/vmlinux.lds.h (#define RO_DATA(align)) makes the >> >> output section .rodata writable. Perhaps this is very difficult to fix, >> >> and we will have writable .rodata for a long time. >> >> >> >> What do you think of moving .got/.got.plt immediately before .data? >> >> I want to place .got/.got.plt before the guaranteed-writable sections, >> >> not some sections which are "unfortunately" writable (.rodata, __modver, >> >> .hyp.rodata, .rodata.text, etc). >> >> >> >> For userspace programs, either linked with GNU ld or lld, .got/.got.plt >> >> are usually immediately before .data . >> >> >> > >> >I don't think that would be the right choice. >> > >> >We have five pseudo-segments in the kernel >> > >> >text (RX) >> >rodata (R) >> >inittext (RX) >> >initdata (RW) >> >data (RW) >> > >> >where the init ones disappear entirely when the boot completes. >> > >> >The GOT should not be modifiable, so it should not be in .data. So the >> >only appropriate 'segment' for the GOT is rodata >> > >> >Note that we don't use PIC codegen for the kernel, so all const >> >qualified data structures containing statically initialized global >> >pointer variables are emitted into .rodata as well, and relocated at >> >boot. So having the GOT in rodata too makes sense imho. >> >> arm64's vmlinux is linked with -shared -Bsymbolic and .got has many >> entries that need to be relocated. > >Actually, it is mostly empty, given that we don't use -fpic/-fpie codegen.
Yes, .got is usually small, e.g. 0x68 in my `make ARCH=arm64 LLVM=1 -j60 defconfig vmlinux` build.
>> .got is initially writable, and >> becomes read-only after relocation resolving. > >The same applies to .rodata, given that we don't use -fpic/fpie >codegen. And note that I am not referring to .data..ro_after_init here >- I mean .rodata itself, which carries all const qualified global data >structures with statically initialized pointer fields. These are >conceptually the same as GOT entries - the only difference is that >they are not created by the linker and are referenced explicitly from >the C code.
I agree that they are very similar, but there is a distinction, that's why we have both .rodata and .data.rel.ro (relro), and the latter is for sections that need relocations.
>> I think this property is significant enough to make it outside of any rodata >> segment. >> > >I disagree. > >> (In the userspace .got is typically placed in PT_GNU_RELRO by using -z relro.) >> >> If we remove inittext and initdata pseudo-segments which disappear after >> the boot, we have >> >> text (RX) >> rodata (R) >> /// where is .got (RELRO) >> data (RW) >> >> If we consider .got distinct from rodata, I think placing .got immediately >> after rodata or immediately before data is both fine? >> > >We can place it anywhere in the rodata pseudo-segment. What user space >does is not quite relevant here - what is relevant is that we already >have a pseudo-segment with the right properties. Adding a new segment >just for the GOT which is almost empty and is mapped with the same >attributes as rodata seems unnecessary to me. > >Do you have any practical concerns? Or is it just tidiness?
Unfortunately, vmlinux has many PT_LOAD program headers. I am trying to find the best position for .got that makes the most sense. When making this decision, I aim to create a layout that looks more normal and is closer to user-space programs. Additionally, I want to reduce the number of PT_LOAD program headers.
You are arguing that .got should be part of rodata. However, I disagree with this perspective from both practical and linker layout viewpoints.
If we place .got below RO_DATA(PAGE_SIZE), it will immediately follow .note or .BTF (see NOTES in include/asm-generic/vmlinux.lds.h). There isn't an option to drop SHF_WRITE from .got, and we will need a separate PT_LOAD program header for .got.
Alternatively, if we place .got below .hyp.rodata, we can eliminate one PT_LOAD because the transition from .hyp.rodata to .got does not require a separate PT_LOAD. However, having a half-writable .got before .rodata.text feels strange to me.
I feel that the region between .text/.rodata and .data is too messy. Placing .got immediately before .data makes the layout look more normal and closer to user space without causing any pessimization.
---
My linker layout viewpoint is that ideally a program's program headers should looke like
https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#layout-related
R PT_LOAD RX PT_LOAD RW PT_LOAD (overlaps with PT_GNU_RELRO) .got, .data.rel.ro (if present) RW PT_LOAD
For historical reason, I can expect swapped R and RX. Sections such as .got and .data.rel.ro are different from both .rodata and .data.
> >> >> > >> > >> > >> >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> >> index b9202c2ee18e..48bd7c25b6ab 100644 >> >> --- a/arch/arm64/kernel/vmlinux.lds.S >> >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> >> @@ -181,18 +181,8 @@ SECTIONS >> >> KPROBES_TEXT >> >> HYPERVISOR_TEXT >> >> *(.gnu.warning) >> >> - . = ALIGN(16); >> >> - *(.got) /* Global offset table */ >> >> } >> >> >> >> - /* >> >> - * Make sure that the .got.plt is either completely empty or it >> >> - * contains only the lazy dispatch entries. >> >> - */ >> >> - .got.plt : { *(.got.plt) } >> >> - ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, >> >> - "Unexpected GOT/PLT entries detected!") >> >> - >> >> . = ALIGN(SEGMENT_ALIGN); >> >> _etext = .; /* End of text section */ >> >> >> >> @@ -286,6 +276,15 @@ SECTIONS >> >> __initdata_end = .; >> >> __init_end = .; >> >> >> >> + .got : { *(.got) } >> >> + /* >> >> + * Make sure that the .got.plt is either completely empty or it >> >> + * contains only the lazy dispatch entries. >> >> + */ >> >> + .got.plt : { *(.got.plt) } >> >> + ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, >> >> + "Unexpected GOT/PLT entries detected!") >> >> + >> >> _data = .; >> >> _sdata = .; >> >> RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN) >> >> -- >> >> 2.40.1.495.gc816e09b53d-goog >> >> >> >> >> >> >> + /* >> >> >> + * Make sure that the .got.plt is either completely empty or it >> >> >> + * contains only the lazy dispatch entries. >> >> >> + */ >> >> >> + .got.plt : { *(.got.plt) } >> >> >> + ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, >> >> >> + "Unexpected GOT/PLT entries detected!") >> >> >> + >> >> >> __initdata_begin = .; >> >> >> >> >> >> init_idmap_pg_dir = .; >> >> >> -- >> >> >> 2.40.1.495.gc816e09b53d-goog >> >> >>
| |