* [PATCH] vmlinux.lds.S: Explicitly set _stext for mips
@ 2024-11-12 20:08 Rong Xu
2024-11-13 1:39 ` Masahiro Yamada
0 siblings, 1 reply; 3+ messages in thread
From: Rong Xu @ 2024-11-12 20:08 UTC (permalink / raw)
To: Thomas Bogendoerfer, Masahiro Yamada, Nick Desaulniers,
Klara Modin, Rong Xu, linux-mips, linux-kernel
With commit 622240ea8d71 ("vmlinux.lds.h: Adjust symbol ordering in
text output section"), symobls in sections .text.unlikely will be
placed before symbols in .text. This can lead to the misplacement
of _stext, which resides in the .text section, and result in a boot
hang.
Explicitly set _stext to the beginning of text output section.
Signed-off-by: Rong Xu <xur@google.com>
Reported-by: Klara Modin <klarasmodin@gmail.com>
Tested-by: Klara Modin <klarasmodin@gmail.com>
---
arch/mips/kernel/vmlinux.lds.S | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
index 9ff55cb80a64..62c3db869a18 100644
--- a/arch/mips/kernel/vmlinux.lds.S
+++ b/arch/mips/kernel/vmlinux.lds.S
@@ -61,6 +61,11 @@ SECTIONS
/* read-only */
_text = .; /* Text and read-only data */
.text : {
+ /* Explicitly set _stext to the beginning of text output
+ section. _stext is in text section and might be matched
+ after symbols in sections with a specific prefix (like
+ .text.unlikely). */
+ _stext = .;
TEXT_TEXT
SCHED_TEXT
LOCK_TEXT
base-commit: 06513ddaf77b8f49ef8540c92d92c9ef0ad49426
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] vmlinux.lds.S: Explicitly set _stext for mips
2024-11-12 20:08 [PATCH] vmlinux.lds.S: Explicitly set _stext for mips Rong Xu
@ 2024-11-13 1:39 ` Masahiro Yamada
2024-11-13 6:39 ` Rong Xu
0 siblings, 1 reply; 3+ messages in thread
From: Masahiro Yamada @ 2024-11-13 1:39 UTC (permalink / raw)
To: Rong Xu
Cc: Thomas Bogendoerfer, Nick Desaulniers, Klara Modin, linux-mips,
linux-kernel
Please use "MIPS:" for the subject prefix
because this patch only affects MIPS.
On Wed, Nov 13, 2024 at 5:08 AM Rong Xu <xur@google.com> wrote:
>
> With commit 622240ea8d71 ("vmlinux.lds.h: Adjust symbol ordering in
> text output section"), symobls in sections .text.unlikely will be
> placed before symbols in .text. This can lead to the misplacement
> of _stext, which resides in the .text section, and result in a boot
> hang.
>
> Explicitly set _stext to the beginning of text output section.
I will insert this patch before 622240ea8d71 to avoid breakage.
Please rephrase the commit description without referring to
622240ea8d71.
For example, you can say as follows:
------------->-----------
MIPS: move _stext definition to vmlinux.lds.S
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.
Move the _stext definition to the linker script to enforce an explicit
ordering.
------------->-----------
Please feel free to update the description, but this must be
fixed regardless of your patch set.
Even without your patch set, the .text section
comes after .text.hot. So, it is broken.
#define TEXT_TEXT \
ALIGN_FUNCTION(); \
*(.text.hot .text.hot.*) \
*(TEXT_MAIN .text.fixup) \
> Signed-off-by: Rong Xu <xur@google.com>
> Reported-by: Klara Modin <klarasmodin@gmail.com>
> Tested-by: Klara Modin <klarasmodin@gmail.com>
> ---
> arch/mips/kernel/vmlinux.lds.S | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> index 9ff55cb80a64..62c3db869a18 100644
> --- a/arch/mips/kernel/vmlinux.lds.S
> +++ b/arch/mips/kernel/vmlinux.lds.S
> @@ -61,6 +61,11 @@ SECTIONS
> /* read-only */
> _text = .; /* Text and read-only data */
> .text : {
> + /* Explicitly set _stext to the beginning of text output
> + section. _stext is in text section and might be matched
> + after symbols in sections with a specific prefix (like
> + .text.unlikely). */
I do not think this comment is necessary
because this is a common pattern.
The typical linker script example is documented.
https://github.com/torvalds/linux/blob/v6.11-rc7/include/asm-generic/vmlinux.lds.h#L21
Many architectures define _stext in vmlinux.lds.S
without such verbose comments.
$ find . -name vmlinux.lds.S | xargs grep "_stext ="
./arch/mips/kernel/vmlinux.lds.S: _stext = . ;
./arch/openrisc/kernel/vmlinux.lds.S: _stext = .;
./arch/xtensa/kernel/vmlinux.lds.S: _stext = .;
./arch/s390/kernel/vmlinux.lds.S: _stext = .; /* Start of text section */
./arch/nios2/kernel/vmlinux.lds.S: _stext = .;
./arch/loongarch/kernel/vmlinux.lds.S: _stext = .;
./arch/x86/kernel/vmlinux.lds.S: _stext = .;
./arch/riscv/kernel/vmlinux.lds.S: _stext = .;
./arch/parisc/kernel/vmlinux.lds.S: _stext = .;
./arch/csky/kernel/vmlinux.lds.S: _stext = .;
./arch/arc/kernel/vmlinux.lds.S: _stext = .;
./arch/arm64/kernel/vmlinux.lds.S: _stext = .; /* Text and read-only data */
./arch/arm/kernel/vmlinux.lds.S: _stext = .; /* Text and read-only data */
./arch/hexagon/kernel/vmlinux.lds.S: _stext = .;
./arch/powerpc/kernel/vmlinux.lds.S: _stext = .;
./arch/microblaze/kernel/vmlinux.lds.S: _stext = . ;
> + _stext = .;
_etext is defined outside the .text {} block.
If you want "_stext" and "_etext" to look symmetrical,
perhaps you might want to change like this?
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
index 9ff55cb80a64..d575f945d422 100644
--- a/arch/mips/kernel/vmlinux.lds.S
+++ b/arch/mips/kernel/vmlinux.lds.S
@@ -60,6 +60,7 @@ SECTIONS
. = LINKER_LOAD_ADDRESS;
/* read-only */
_text = .; /* Text and read-only data */
+ _stext = .;
.text : {
TEXT_TEXT
SCHED_TEXT
If you move the _stext definition to the linker script,
you can remove EXPORT(_stext) from
arch/mips/kernel/head.S
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] vmlinux.lds.S: Explicitly set _stext for mips
2024-11-13 1:39 ` Masahiro Yamada
@ 2024-11-13 6:39 ` Rong Xu
0 siblings, 0 replies; 3+ messages in thread
From: Rong Xu @ 2024-11-13 6:39 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Thomas Bogendoerfer, Nick Desaulniers, Klara Modin, linux-mips,
linux-kernel
Hi Masahiro,
Thank you so much for the suggestions. I'll send the updated patch shortly.
Best,
-Rong
On Tue, Nov 12, 2024 at 5:40 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Please use "MIPS:" for the subject prefix
> because this patch only affects MIPS.
>
>
>
> On Wed, Nov 13, 2024 at 5:08 AM Rong Xu <xur@google.com> wrote:
> >
> > With commit 622240ea8d71 ("vmlinux.lds.h: Adjust symbol ordering in
> > text output section"), symobls in sections .text.unlikely will be
> > placed before symbols in .text. This can lead to the misplacement
> > of _stext, which resides in the .text section, and result in a boot
> > hang.
> >
> > Explicitly set _stext to the beginning of text output section.
>
>
> I will insert this patch before 622240ea8d71 to avoid breakage.
>
> Please rephrase the commit description without referring to
> 622240ea8d71.
>
>
> For example, you can say as follows:
>
> ------------->-----------
> MIPS: move _stext definition to vmlinux.lds.S
>
> 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.
>
> Move the _stext definition to the linker script to enforce an explicit
> ordering.
> ------------->-----------
>
>
> Please feel free to update the description, but this must be
> fixed regardless of your patch set.
>
> Even without your patch set, the .text section
> comes after .text.hot. So, it is broken.
>
> #define TEXT_TEXT \
> ALIGN_FUNCTION(); \
> *(.text.hot .text.hot.*) \
> *(TEXT_MAIN .text.fixup) \
>
>
>
>
> > Signed-off-by: Rong Xu <xur@google.com>
> > Reported-by: Klara Modin <klarasmodin@gmail.com>
> > Tested-by: Klara Modin <klarasmodin@gmail.com>
> > ---
> > arch/mips/kernel/vmlinux.lds.S | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> > index 9ff55cb80a64..62c3db869a18 100644
> > --- a/arch/mips/kernel/vmlinux.lds.S
> > +++ b/arch/mips/kernel/vmlinux.lds.S
> > @@ -61,6 +61,11 @@ SECTIONS
> > /* read-only */
> > _text = .; /* Text and read-only data */
> > .text : {
> > + /* Explicitly set _stext to the beginning of text output
> > + section. _stext is in text section and might be matched
> > + after symbols in sections with a specific prefix (like
> > + .text.unlikely). */
>
> I do not think this comment is necessary
> because this is a common pattern.
>
> The typical linker script example is documented.
> https://github.com/torvalds/linux/blob/v6.11-rc7/include/asm-generic/vmlinux.lds.h#L21
>
>
>
>
> Many architectures define _stext in vmlinux.lds.S
> without such verbose comments.
>
> $ find . -name vmlinux.lds.S | xargs grep "_stext ="
> ./arch/mips/kernel/vmlinux.lds.S: _stext = . ;
> ./arch/openrisc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/xtensa/kernel/vmlinux.lds.S: _stext = .;
> ./arch/s390/kernel/vmlinux.lds.S: _stext = .; /* Start of text section */
> ./arch/nios2/kernel/vmlinux.lds.S: _stext = .;
> ./arch/loongarch/kernel/vmlinux.lds.S: _stext = .;
> ./arch/x86/kernel/vmlinux.lds.S: _stext = .;
> ./arch/riscv/kernel/vmlinux.lds.S: _stext = .;
> ./arch/parisc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/csky/kernel/vmlinux.lds.S: _stext = .;
> ./arch/arc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/arm64/kernel/vmlinux.lds.S: _stext = .; /* Text and read-only data */
> ./arch/arm/kernel/vmlinux.lds.S: _stext = .; /* Text and read-only data */
> ./arch/hexagon/kernel/vmlinux.lds.S: _stext = .;
> ./arch/powerpc/kernel/vmlinux.lds.S: _stext = .;
> ./arch/microblaze/kernel/vmlinux.lds.S: _stext = . ;
>
>
>
>
> > + _stext = .;
>
> _etext is defined outside the .text {} block.
>
> If you want "_stext" and "_etext" to look symmetrical,
> perhaps you might want to change like this?
>
>
> diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> index 9ff55cb80a64..d575f945d422 100644
> --- a/arch/mips/kernel/vmlinux.lds.S
> +++ b/arch/mips/kernel/vmlinux.lds.S
> @@ -60,6 +60,7 @@ SECTIONS
> . = LINKER_LOAD_ADDRESS;
> /* read-only */
> _text = .; /* Text and read-only data */
> + _stext = .;
> .text : {
> TEXT_TEXT
> SCHED_TEXT
>
>
>
>
> If you move the _stext definition to the linker script,
> you can remove EXPORT(_stext) from
> arch/mips/kernel/head.S
>
>
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-13 6:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-12 20:08 [PATCH] vmlinux.lds.S: Explicitly set _stext for mips Rong Xu
2024-11-13 1:39 ` Masahiro Yamada
2024-11-13 6:39 ` Rong Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®