linux-kernel - Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.2411140107290.9262@angie.orcam.me.uk>
Date: Thu, 14 Nov 2024 01:29:57 +0000 (GMT)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: Masahiro Yamada <masahiroy@...nel.org>
cc: Rong Xu <xur@...gle.com>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>, 
    Nick Desaulniers <ndesaulniers@...gle.com>, 
    Klara Modin <klarasmodin@...il.com>, linux-mips@...r.kernel.org, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] MIPS: move _stext definition to vmlinux.lds.S

On Thu, 14 Nov 2024, Masahiro Yamada wrote:

> > > The _stext symbol is intended to reference the start of the text section.
> > > However, it currently relies on a fragile link order because the existing
> > > EXPORT(_stext) resides within the .text section, which is not guaranteed
> > > to be placed first.
> >
> >  Umm, arch/mips/kernel/head.S does mean to be linked first.  We rely on it
> > for environments where there's no entry point is available and execution
> > starts from the beginning of the image.  See the comment right below your
> > change.
> >
> > > Move the _stext definition to the linker script to enforce an explicit
> > > ordering.
> >
> >  So if you say that the link order is fragile (which it may well be), then
> > that problem has to be fixed instead, likely with the linker script too,
> > and then perhaps an ASSERT placed there to verify that it has worked and
> > `_stext' refers to the beginning, taking into account what follows too.
> 
> 
> arch/mips/kernel/head.S is always passed as the first object
> in the link command because it is listed in scripts/head-object-list.txt
> 
> What you missed to understand is, the .text section of the first object
> is NOT guaranteed to be placed at the start of the image.

 I know how linker scripts work, thank you very much.  However there was 
nothing to understand from the commit description as hardly any has been 
given.

> Assume, we pass 3 objects, head.o, foo.o, bar.o to the linker
> in this order.
> 
> - head.o  contains a .text section
> - foo.o contains .text and .text.hot sections
> - bar.o contains .text and .text.hot sections
> 
> 
> The output will contain the sections in this order:
>    foo.o#.text.hot
>    bar.o#.text.hot
>    head.o#.text
>    foo.o#.text
>    bar.o#.text
> 
> 
> This result comes from the fact that TEXT_MAIN
> is not necessarily placed first.
> 
> See the macro in include/asm-generic/vmlinux.lds.h
> 
> #define TEXT_TEXT                                                       \
>                 ALIGN_FUNCTION();                                       \
>                 *(.text.hot .text.hot.*)                                \
>                 *(TEXT_MAIN .text.fixup)                                \
>                 *(.text.unlikely .text.unlikely.*)                      \
>                 *(.text.unknown .text.unknown.*)                        \

 It corresponds to what I suggested in the second paragraph of my previous 
reply, which I retained quoted above for your convenience.  The reviewer 
is not required to give the submitter a complete solution, but rather some 
guidance for the submitter to find the correct solution themselves.

> BTW, "head.o must be passed to the linker as the first object"
> is a bad convention in old days.
> If you expect the entry point at the beginning of the kernel image,
> it must be marked as __HEAD, which is placed in the .head.text section.
> 
> See commit ce697ccee1a8

 I do not disagree, however such details need to be given in the change 
description.  The purpose of the change description is not to repeat in 
the texual form what the patch does, because everyone can see it, but to 
give rationale and any reasoning beyond the change being made, especially 
when fiddling with something that has worked for 30 years now.

 It's not that the old is always good, but at least it has proved in the 
field, so you need to convince the reviewer why the new is more suitable.

> Well-maintained architectures got rid of
> stupid "head.o must be passed first" requirement:
> 
>  - 2348e6bf4421
>  - 994b7ac1697b
>  - 5353fff29e42
> 
> If MIPS migrates to the cleaner __HEAD solution,
> it will be appreciated, but this is another story.

 Patches are welcome.

  Maciej