Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Apr 2023 22:29:12 +0000
From:   Fangrui Song <maskray@google.com>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Bill Wendling <morbo@google.com>,
        Nick Desaulniers <ndesaulniers@google.com>
Subject: Re: [PATCH] arm64: lds: move .got section out of .text
Message-ID: <20230428222912.yanrwnl5hwykv6ve@google.com>
References: <20230428050442.180913-1-maskray@google.com>
 <CAMj1kXGpbKgS8mNxVuAyPvT-vW0LWZOXgqsy5TvKhzJRs_rHkA@mail.gmail.com>
 <20230428185814.mmnb3jafp7fnwdrh@google.com>
 <CAMj1kXFgv6GHm5PvR9Qq9-VwL+5NXwpChkB59c-0-tEJPNYdVQ@mail.gmail.com>
 <20230428210611.pxgmj2tja3y2c3lr@google.com>
 <CAMj1kXGKPyFOtPE30eanbD+5StvXGgX7Bi9iziTK9iqFg9=9wg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <CAMj1kXGKPyFOtPE30eanbD+5StvXGgX7Bi9iziTK9iqFg9=9wg@mail.gmail.com>
Precedence: bulk

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
>> >> >>