[PATCH] x86/build: Simplify patterns for unwanted section

[PATCH] x86/build: Simplify patterns for unwanted section

[Fangrui Song]

A s390 patch modeling its --orphan-handling= after x86 [1] sparked my
motivation to simplify patterns. Commit 5354e84598f2 ("x86/build: Add
asserts for unwanted sections") added asserts that certain input
sections must be absent or empty. The patterns can be simplified.

For dynamic relocations,

*(.rela.*) is sufficient to match all dynamic relocations synthesized by
GNU ld and LLD. .rela_* is unnecessary. --emit-relocs may create .rela_*
sections for section names prefixed with _, but they are not matched by
linker scripts.

.plt instead of .plt.* is sufficient to match synthesized PLT entries.

.igot and .igot.plt are for non-preemptible STT_GNU_IFUNC in GNU ld (LLD
just uses .got), which the kernel does not use. In addition, if .igot or
.igot.plt is ever non-empty, there will be .rela.* dynamic relocations
leading to an assert failure anyway.

[1]: https://lore.kernel.org/all/20240207-s390-lld-and-orphan-warn-v1-6-8a665b3346ab@kernel.org/

Signed-off-by: Fangrui Song <maskray@google.com>
---
 arch/x86/boot/compressed/vmlinux.lds.S | 6 +++---
 arch/x86/kernel/vmlinux.lds.S          | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
index 083ec6d7722a..9f288f67972a 100644
--- a/arch/x86/boot/compressed/vmlinux.lds.S
+++ b/arch/x86/boot/compressed/vmlinux.lds.S
@@ -104,17 +104,17 @@ SECTIONS
 	ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
 
 	.plt : {
-		*(.plt) *(.plt.*)
+		*(.plt)
 	}
 	ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
 
 	.rel.dyn : {
-		*(.rel.*) *(.rel_*)
+		*(.rel.*)
 	}
 	ASSERT(SIZEOF(.rel.dyn) == 0, "Unexpected run-time relocations (.rel) detected!")
 
 	.rela.dyn : {
-		*(.rela.*) *(.rela_*)
+		*(.rela.*)
 	}
 	ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
 }
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index a349dbfc6d5a..b3da7b81d2b3 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -463,22 +463,22 @@ SECTIONS
 	 * explicitly check instead of blindly discarding.
 	 */
 	.got : {
-		*(.got) *(.igot.*)
+		*(.got)
 	}
 	ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
 
 	.plt : {
-		*(.plt) *(.plt.*) *(.iplt)
+		*(.plt)
 	}
 	ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
 
 	.rel.dyn : {
-		*(.rel.*) *(.rel_*)
+		*(.rel.*)
 	}
 	ASSERT(SIZEOF(.rel.dyn) == 0, "Unexpected run-time relocations (.rel) detected!")
 
 	.rela.dyn : {
-		*(.rela.*) *(.rela_*)
+		*(.rela.*)
 	}
 	ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
 }
-- 
2.43.0.687.g38aa6559b0-goog

Re: [PATCH] x86/build: Simplify patterns for unwanted section

[Kees Cook]

On Wed, Feb 14, 2024 at 01:29:29PM -0800, Fangrui Song wrote:
> A s390 patch modeling its --orphan-handling= after x86 [1] sparked my
> motivation to simplify patterns. Commit 5354e84598f2 ("x86/build: Add
> asserts for unwanted sections") added asserts that certain input
> sections must be absent or empty. The patterns can be simplified.
> 
> For dynamic relocations,
> 
> *(.rela.*) is sufficient to match all dynamic relocations synthesized by
> GNU ld and LLD. .rela_* is unnecessary. --emit-relocs may create .rela_*
> sections for section names prefixed with _, but they are not matched by
> linker scripts.
> 
> .plt instead of .plt.* is sufficient to match synthesized PLT entries.

Do you mean ".plt.foo" matches ".plt" ?

> .igot and .igot.plt are for non-preemptible STT_GNU_IFUNC in GNU ld (LLD
> just uses .got), which the kernel does not use. In addition, if .igot or
> .igot.plt is ever non-empty, there will be .rela.* dynamic relocations
> leading to an assert failure anyway.

I think at the time I was dealing with avoid multiple warnings out of
the linker, as I was getting orphan warnings in addition to the
non-empty warnings.

> 
> [1]: https://lore.kernel.org/all/20240207-s390-lld-and-orphan-warn-v1-6-8a665b3346ab@kernel.org/
> 
> Signed-off-by: Fangrui Song <maskray@google.com>

Is anything harmed by leaving all of this as-is?

-Kees

> ---
>  arch/x86/boot/compressed/vmlinux.lds.S | 6 +++---
>  arch/x86/kernel/vmlinux.lds.S          | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> index 083ec6d7722a..9f288f67972a 100644
> --- a/arch/x86/boot/compressed/vmlinux.lds.S
> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> @@ -104,17 +104,17 @@ SECTIONS
>  	ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
>  
>  	.plt : {
> -		*(.plt) *(.plt.*)
> +		*(.plt)
>  	}
>  	ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
>  
>  	.rel.dyn : {
> -		*(.rel.*) *(.rel_*)
> +		*(.rel.*)
>  	}
>  	ASSERT(SIZEOF(.rel.dyn) == 0, "Unexpected run-time relocations (.rel) detected!")
>  
>  	.rela.dyn : {
> -		*(.rela.*) *(.rela_*)
> +		*(.rela.*)
>  	}
>  	ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
>  }
> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index a349dbfc6d5a..b3da7b81d2b3 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -463,22 +463,22 @@ SECTIONS
>  	 * explicitly check instead of blindly discarding.
>  	 */
>  	.got : {
> -		*(.got) *(.igot.*)
> +		*(.got)
>  	}
>  	ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
>  
>  	.plt : {
> -		*(.plt) *(.plt.*) *(.iplt)
> +		*(.plt)
>  	}
>  	ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
>  
>  	.rel.dyn : {
> -		*(.rel.*) *(.rel_*)
> +		*(.rel.*)
>  	}
>  	ASSERT(SIZEOF(.rel.dyn) == 0, "Unexpected run-time relocations (.rel) detected!")
>  
>  	.rela.dyn : {
> -		*(.rela.*) *(.rela_*)
> +		*(.rela.*)
>  	}
>  	ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
>  }
> -- 
> 2.43.0.687.g38aa6559b0-goog
> 

-- 
Kees Cook

Re: [PATCH] x86/build: Simplify patterns for unwanted section

[Fangrui Song]

On Wed, Feb 14, 2024 at 2:07 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Feb 14, 2024 at 01:29:29PM -0800, Fangrui Song wrote:
> > A s390 patch modeling its --orphan-handling= after x86 [1] sparked my
> > motivation to simplify patterns. Commit 5354e84598f2 ("x86/build: Add
> > asserts for unwanted sections") added asserts that certain input
> > sections must be absent or empty. The patterns can be simplified.
> >
> > For dynamic relocations,
> >
> > *(.rela.*) is sufficient to match all dynamic relocations synthesized by
> > GNU ld and LLD. .rela_* is unnecessary. --emit-relocs may create .rela_*
> > sections for section names prefixed with _, but they are not matched by
> > linker scripts.
> >
> > .plt instead of .plt.* is sufficient to match synthesized PLT entries.
>
> Do you mean ".plt.foo" matches ".plt" ?

I mean we just need .plt : { *(.plt) } , not .plt : { *(.plt) *(.plt.*) }.

The linker synthesized section for PLT entries is .plt, not suffixed.

> > .igot and .igot.plt are for non-preemptible STT_GNU_IFUNC in GNU ld (LLD
> > just uses .got), which the kernel does not use. In addition, if .igot or
> > .igot.plt is ever non-empty, there will be .rela.* dynamic relocations
> > leading to an assert failure anyway.
>
> I think at the time I was dealing with avoid multiple warnings out of
> the linker, as I was getting orphan warnings in addition to the
> non-empty warnings.
>
> >
> > [1]: https://lore.kernel.org/all/20240207-s390-lld-and-orphan-warn-v1-6-8a665b3346ab@kernel.org/
> >
> > Signed-off-by: Fangrui Song <maskray@google.com>
>
> Is anything harmed by leaving all of this as-is?
>
> -Kees

No harm. But ports adopting --orphan-handling= (like s390) may copy
the unneeded .rela_* .
When people read .rela_*, they might think whether the kernel does
anything special that
.rela_* needs to be matched.

> > ---
> >  arch/x86/boot/compressed/vmlinux.lds.S | 6 +++---
> >  arch/x86/kernel/vmlinux.lds.S          | 8 ++++----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> > index 083ec6d7722a..9f288f67972a 100644
> > --- a/arch/x86/boot/compressed/vmlinux.lds.S
> > +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> > @@ -104,17 +104,17 @@ SECTIONS
> >       ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
> >
> >       .plt : {
> > -             *(.plt) *(.plt.*)
> > +             *(.plt)
> >       }
> >       ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
> >
> >       .rel.dyn : {
> > -             *(.rel.*) *(.rel_*)
> > +             *(.rel.*)
> >       }
> >       ASSERT(SIZEOF(.rel.dyn) == 0, "Unexpected run-time relocations (.rel) detected!")
> >
> >       .rela.dyn : {
> > -             *(.rela.*) *(.rela_*)
> > +             *(.rela.*)
> >       }
> >       ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
> >  }
> > diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> > index a349dbfc6d5a..b3da7b81d2b3 100644
> > --- a/arch/x86/kernel/vmlinux.lds.S
> > +++ b/arch/x86/kernel/vmlinux.lds.S
> > @@ -463,22 +463,22 @@ SECTIONS
> >        * explicitly check instead of blindly discarding.
> >        */
> >       .got : {
> > -             *(.got) *(.igot.*)
> > +             *(.got)
> >       }
> >       ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
> >
> >       .plt : {
> > -             *(.plt) *(.plt.*) *(.iplt)
> > +             *(.plt)
> >       }
> >       ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure linkages detected!")
> >
> >       .rel.dyn : {
> > -             *(.rel.*) *(.rel_*)
> > +             *(.rel.*)
> >       }
> >       ASSERT(SIZEOF(.rel.dyn) == 0, "Unexpected run-time relocations (.rel) detected!")
> >
> >       .rela.dyn : {
> > -             *(.rela.*) *(.rela_*)
> > +             *(.rela.*)
> >       }
> >       ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!")
> >  }
> > --
> > 2.43.0.687.g38aa6559b0-goog
> >
>
> --
> Kees Cook



-- 
宋方睿

Re: [PATCH] x86/build: Simplify patterns for unwanted section

[Kees Cook]

On Wed, Feb 14, 2024 at 02:13:01PM -0800, Fangrui Song wrote:
> On Wed, Feb 14, 2024 at 2:07 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Feb 14, 2024 at 01:29:29PM -0800, Fangrui Song wrote:
> > > A s390 patch modeling its --orphan-handling= after x86 [1] sparked my
> > > motivation to simplify patterns. Commit 5354e84598f2 ("x86/build: Add
> > > asserts for unwanted sections") added asserts that certain input
> > > sections must be absent or empty. The patterns can be simplified.
> > >
> > > For dynamic relocations,
> > >
> > > *(.rela.*) is sufficient to match all dynamic relocations synthesized by
> > > GNU ld and LLD. .rela_* is unnecessary. --emit-relocs may create .rela_*
> > > sections for section names prefixed with _, but they are not matched by
> > > linker scripts.
> > >
> > > .plt instead of .plt.* is sufficient to match synthesized PLT entries.
> >
> > Do you mean ".plt.foo" matches ".plt" ?
> 
> I mean we just need .plt : { *(.plt) } , not .plt : { *(.plt) *(.plt.*) }.

But then, for example, if it gets generated, .plt.got ends up being
reported as an orphan...

> 
> The linker synthesized section for PLT entries is .plt, not suffixed.
> 
> > > .igot and .igot.plt are for non-preemptible STT_GNU_IFUNC in GNU ld (LLD
> > > just uses .got), which the kernel does not use. In addition, if .igot or

Right, the issue has been getting totally weird sections emitted by the
linker. If you're saying you'd rather those get reported as orphan
sections instead of being validated for being zero sized, and that works
for all the architectures, then okay.

> > > .igot.plt is ever non-empty, there will be .rela.* dynamic relocations
> > > leading to an assert failure anyway.
> >
> > I think at the time I was dealing with avoid multiple warnings out of
> > the linker, as I was getting orphan warnings in addition to the
> > non-empty warnings.
> >
> > >
> > > [1]: https://lore.kernel.org/all/20240207-s390-lld-and-orphan-warn-v1-6-8a665b3346ab@kernel.org/
> > >
> > > Signed-off-by: Fangrui Song <maskray@google.com>
> >
> > Is anything harmed by leaving all of this as-is?
> >
> > -Kees
> 
> No harm. But ports adopting --orphan-handling= (like s390) may copy
> the unneeded .rela_* .
> When people read .rela_*, they might think whether the kernel does
> anything special that
> .rela_* needs to be matched.

I added these because the were being generated. See commit d1c0272bc1c0
("x86/boot/compressed: Remove, discard, or assert for unwanted sections")

I don't want to suddenly start generating warnings for older/broken
linkers. (i.e. a change like this needs really careful testing, and that
needs to be detailed in the commit log.)

-Kees

-- 
Kees Cook

Re: [PATCH] x86/build: Simplify patterns for unwanted section

[Fangrui Song]

On Wed, Feb 14, 2024 at 2:40 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Feb 14, 2024 at 02:13:01PM -0800, Fangrui Song wrote:
> > On Wed, Feb 14, 2024 at 2:07 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Feb 14, 2024 at 01:29:29PM -0800, Fangrui Song wrote:
> > > > A s390 patch modeling its --orphan-handling= after x86 [1] sparked my
> > > > motivation to simplify patterns. Commit 5354e84598f2 ("x86/build: Add
> > > > asserts for unwanted sections") added asserts that certain input
> > > > sections must be absent or empty. The patterns can be simplified.
> > > >
> > > > For dynamic relocations,
> > > >
> > > > *(.rela.*) is sufficient to match all dynamic relocations synthesized by
> > > > GNU ld and LLD. .rela_* is unnecessary. --emit-relocs may create .rela_*
> > > > sections for section names prefixed with _, but they are not matched by
> > > > linker scripts.
> > > >
> > > > .plt instead of .plt.* is sufficient to match synthesized PLT entries.
> > >
> > > Do you mean ".plt.foo" matches ".plt" ?
> >
> > I mean we just need .plt : { *(.plt) } , not .plt : { *(.plt) *(.plt.*) }.
>
> But then, for example, if it gets generated, .plt.got ends up being
> reported as an orphan...
>
> >
> > The linker synthesized section for PLT entries is .plt, not suffixed.
> >
> > > > .igot and .igot.plt are for non-preemptible STT_GNU_IFUNC in GNU ld (LLD
> > > > just uses .got), which the kernel does not use. In addition, if .igot or
>
> Right, the issue has been getting totally weird sections emitted by the
> linker. If you're saying you'd rather those get reported as orphan
> sections instead of being validated for being zero sized, and that works
> for all the architectures, then okay.

Thanks. I trust my judgement here:)

> > > > .igot.plt is ever non-empty, there will be .rela.* dynamic relocations
> > > > leading to an assert failure anyway.
> > >
> > > I think at the time I was dealing with avoid multiple warnings out of
> > > the linker, as I was getting orphan warnings in addition to the
> > > non-empty warnings.
> > >
> > > >
> > > > [1]: https://lore.kernel.org/all/20240207-s390-lld-and-orphan-warn-v1-6-8a665b3346ab@kernel.org/
> > > >
> > > > Signed-off-by: Fangrui Song <maskray@google.com>
> > >
> > > Is anything harmed by leaving all of this as-is?
> > >
> > > -Kees
> >
> > No harm. But ports adopting --orphan-handling= (like s390) may copy
> > the unneeded .rela_* .
> > When people read .rela_*, they might think whether the kernel does
> > anything special that
> > .rela_* needs to be matched.
>
> I added these because the were being generated. See commit d1c0272bc1c0
> ("x86/boot/compressed: Remove, discard, or assert for unwanted sections")
>
> I don't want to suddenly start generating warnings for older/broken
> linkers. (i.e. a change like this needs really careful testing, and that
> needs to be detailed in the commit log.)
>
> -Kees

I saw this commit and still believe .rela_*  is unnecessary for all
supported binutils versions.
GNU ld 2.25 does not support --orphan-handling=error (2015-09
feature), but it manages to link vmlinux without any warning.

/tmp/binutils-2.25/out/release/ld/ld-new -m elf_x86_64 -z noexecstack
--emit-relocs --discard-none -z max-page-size=0x200000 --build-id=sha1
--script=./arch/x86/kernel/vmlinux.lds -o vmlinux --whole-archive
vmlinux.o .vmlinux.export.o init/version-timestamp.o
--no-whole-archive --start-group --end-group .tmp_vmlinux.kallsyms2.o


> --
> Kees Cook



-- 
宋方睿