diff mbox series

vmlinux.lds.S: Explicitly set _stext for mips

Message ID 20241112200827.2594291-1-xur@google.com (mailing list archive)
State Superseded
Headers show
Series vmlinux.lds.S: Explicitly set _stext for mips | expand

Commit Message

Rong Xu Nov. 12, 2024, 8:08 p.m. UTC 
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(+)


base-commit: 06513ddaf77b8f49ef8540c92d92c9ef0ad49426

Comments

Masahiro Yamada Nov. 13, 2024, 1:39 a.m. UTC | #1 
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
Rong Xu Nov. 13, 2024, 6:39 a.m. UTC | #2 
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
diff mbox series

Patch

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