Skip to contentSkip to conversation

powerpc/boot: request no dynamic linker for boot wrapper

PATCH Applied Nicholas Piggin 6 participants 11 messages Nov 28, 2016 – Dec 5, 2016 PowerPC

lore
Share & tools
lore

Nicholas Piggin

author reviewer
#0 lore raw

PATCH 1/1 powerpc/boot: request no dynamic linker for boot wrapper

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

A recent binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=1a9ccd70f9a75dc6b48d340059f28ef3550c107b
has broken kernel builds:

ld: arch/powerpc/boot/zImage.pseries: Not enough room for program headers, try linking with -N
ld: final link failed: Bad value

Reported-by Anton Blanchard <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>

---

Hi,

I think this should do the trick (zImage.pseries compiles and boots mambo here, haven't tested zImage.epapr). Cc'ing Alan just in case :)

Thanks,
Nick

 arch/powerpc/boot/wrapper | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/powerpc/boot/wrapper +8 -1
index 404b3aa..cd941a8 100755
@@ -181,6 +181,13 @@ case "$elfformat" in
     elf32-powerpc)	format=elf32ppc	;;
 esac
 
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+	nodl="--no-dynamic-linker"
+fi
 
 platformo=$object/"$platform".o
 lds=$object/zImage.lds

@@ -446,7 +453,7 @@ if [ "$platform" != "miboot" ]; then
         text_start="-Ttext $link_address"
     fi
 #link everything
-    ${CROSS}ld -m $format -T $lds $text_start $pie -o "$ofile" \
+    ${CROSS}ld -m $format -T $lds $text_start $pie $nodl -o "$ofile" \
 	$platformo $tmp $object/wrapper.a
     rm $tmp
 fi
Signature · 1 line
2.10.2
#1 lore raw
Quoted 2 lines · Nicholas Piggin

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

OK, so the kernel change is quite reasonable in isolation, but see below.

Quoted 7 lines

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

A recent binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=1a9ccd70f9a75dc6b48d340059f28ef3550c107b
has broken kernel builds:

So this change added space for another header, it seems. I suspect that was accidental, particularly since there was no mention of get_program_header_size in the ChangeLog entry.

Signature · 2 lines
Alan Modra
Australia Development Lab, IBM
reviewer
#2 lore raw
In reply to Nicholas Piggin
Quoted 6 lines · Nicholas Piggin

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

2.28 is not released yet though is it?

So can we just declare versions with that behaviour broken?

Quoted 5 lines
arch/powerpc/boot/wrapper +0 -0
index 404b3aa..cd941a8 100755

arch/powerpc/boot/wrapper case "$elfformat" in · line 190

Quoted 10 lines
arch/powerpc/boot/wrapper +7 -0
@@ -181,6 +181,13 @@ case "$elfformat" in
     elf32-powerpc)	format=elf32ppc	;;
 esac
 
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+	nodl="--no-dynamic-linker"
+fi

Some distros (RHEL at least?), ship the wrapper as a standalone script. So I don't think we can call things in $srctree. Or at least I don't know how that's supposed to work when it's shipped standalone.

We're also basically reinventing ld-option, which is a PITA.

cheers

author reviewer
#3 lore raw
Quoted 5 lines · On Mon, 28 Nov 2016 22:07:39 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 3 lines · Nicholas Piggin

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

Quoted 7 lines
Quoted 3 lines

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

2.28 is not released yet though is it?

So can we just declare versions with that behaviour broken?

No it's not released yet, but I don't know if it's due entirely to binutils bug. Let's see what Nick thinks.

Quoted 9 lines
Quoted 9 lines
arch/powerpc/boot/wrapper +0 -0
index 404b3aa..cd941a8 100755
@@ -181,6 +181,13 @@ case "$elfformat" in
     elf32-powerpc)	format=elf32ppc	;;
 esac
Quoted 12 lines

arch/powerpc/boot/wrapper case "$elfformat" in · line 190

Quoted 7 lines
arch/powerpc/boot/wrapper +7 -0
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+	nodl="--no-dynamic-linker"
+fi

Some distros (RHEL at least?), ship the wrapper as a standalone script. So I don't think we can call things in $srctree. Or at least I don't know how that's supposed to work when it's shipped standalone. We're also basically reinventing ld-option, which is a PITA.

Okay I didn't realize that. It's already using mkuboot.sh, but only for uboot targets... I don't know, I don't have any good ideas at the moment.

Maybe go back to using the linker script if possible? (I hadn't been able to quite get it right yesterday, and this option seemed cleaner, but it might be possible)

Thanks,
Nick

#4 lore raw

Hi Nicholas,

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

Quoted 6 lines
Quoted 3 lines

2.28 is not released yet though is it?

So can we just declare versions with that behaviour broken?

No it's not released yet, but I don't know if it's due entirely to binutils bug. Let's see what Nick thinks.

Well the patch that caused this problem was an attempt to fix the linker so that it would enforce the ELF standard. Prior to the patch the linker would silently create binaries that violated the standard and which, at least for the people reporting the problem in binutils PR 20815, failed to execute.

It now appears however that some programs, including the boot wrapper and the Linux kernel, may actually rely upon non-standard ELF binaries being created. Before I revert the patch however, I would like to ask...

Quoted 7 lines
Quoted 7 lines

arch/powerpc/boot/wrapper case "$elfformat" in · line 190

Quoted 7 lines
arch/powerpc/boot/wrapper +7 -0
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+	nodl="--no-dynamic-linker"
+fi

... this actually seems like a better fix to me. If you do not want the PT_INTERP segment, then telling this linker this is a good idea. So wouldn't a patch like this be a better solution to the problem ?

Cheers
Nick

author reviewer
#5 lore raw
Quoted 25 lines · On Mon, 28 Nov 2016 12:39:38 +0000 Nick Clifton <[email protected]> wrote:

Hi Nicholas,

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

Quoted 6 lines
Quoted 3 lines

2.28 is not released yet though is it?

So can we just declare versions with that behaviour broken?

No it's not released yet, but I don't know if it's due entirely to binutils bug. Let's see what Nick thinks.

Well the patch that caused this problem was an attempt to fix the linker so that it would enforce the ELF standard. Prior to the patch the linker would silently create binaries that violated the standard and which, at least for the people reporting the problem in binutils PR 20815, failed to execute.

It now appears however that some programs, including the boot wrapper and the Linux kernel, may actually rely upon non-standard ELF binaries being created. Before I revert the patch however, I would like to ask...

Quoted 11 lines
Quoted 7 lines
Quoted 7 lines

arch/powerpc/boot/wrapper case "$elfformat" in · line 190

Quoted 7 lines
arch/powerpc/boot/wrapper +7 -0
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+	nodl="--no-dynamic-linker"
+fi

... this actually seems like a better fix to me. If you do not want the PT_INTERP segment, then telling this linker this is a good idea. So wouldn't a patch like this be a better solution to the problem ?

Yes, I wasn't asking for the binutils change to be reverted. We're generally happy to adapt to toolchain improvements. I don't think the boot wrapper is relying on this non-standard form. If we go with --no-dynamic-linker then I'm assuming we get a standard ELF binary? That seems desirable.

I was just checking whether this is the best think for the kernel to do. Is it possible to get a similar behaviour using the linker script instead (so it's compatible with older binutils)?

Thanks,
Nick

#6 lore raw

Hi Nicholas,

Quoted 5 lines
Quoted 3 lines

... this actually seems like a better fix to me. If you do not want the PT_INTERP segment, then telling this linker this is a good idea. So wouldn't a patch like this be a better solution to the problem ?

Yes, I wasn't asking for the binutils change to be reverted.

Oh right. Actually it looks like at least part of the patch is going to have to be reverted, (the part that sorts the PT_LOAD segments into ascending order), as currently it breaks building Linux kernels. *sigh*

Quoted 4 lines

I don't think the
boot wrapper is relying on this non-standard form. If we go with --no-dynamic-linker then I'm assuming we get a standard ELF binary? That seems desirable.

Yes, you definitely should get a proper ELF binary.

Quoted 3 lines

I was just checking whether this is the best think for the kernel to do. Is it possible to get a similar behaviour using the linker script instead (so it's compatible with older binutils)?

Actually probably not. :-( Several backends, including the PPC, will now attempt to automatically create and install the PT_INTERP segment unless you explicitly tell them not too. Even if you are using a custom linker script.

Cheers
Nick

author reviewer
#7 lore raw
Quoted 11 lines · On Mon, 28 Nov 2016 14:25:31 +0000 Nick Clifton <[email protected]> wrote:

Hi Nicholas,

Quoted 5 lines
Quoted 3 lines

... this actually seems like a better fix to me. If you do not want the PT_INTERP segment, then telling this linker this is a good idea. So wouldn't a patch like this be a better solution to the problem ?

Yes, I wasn't asking for the binutils change to be reverted.

Oh right. Actually it looks like at least part of the patch is going to have to be reverted, (the part that sorts the PT_LOAD segments into ascending order), as currently it breaks building Linux kernels. *sigh*

If the kernel has been doing the wrong thing, we can accept the breakage. It's a matter for binutils policy in the end I suppose.

Quoted 8 lines
Quoted 5 lines

I don't think the
boot wrapper is relying on this non-standard form. If we go with --no-dynamic-linker then I'm assuming we get a standard ELF binary? That seems desirable.

Yes, you definitely should get a proper ELF binary.

Quoted 7 lines
Quoted 3 lines

I was just checking whether this is the best think for the kernel to do. Is it possible to get a similar behaviour using the linker script instead (so it's compatible with older binutils)?

Actually probably not. :-( Several backends, including the PPC, will now attempt to automatically create and install the PT_INTERP segment unless you explicitly tell them not too. Even if you are using a custom linker script.

Okay. It sounds like we should use --no-dynamic-linker whether or not your patch is changed.

Thanks,
Nick

reviewer
#8 lore raw
In reply to Nicholas Piggin
Quoted 24 lines · Nicholas Piggin
Quoted 9 lines · On Mon, 28 Nov 2016 22:07:39 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 8 lines · Nicholas Piggin
arch/powerpc/boot/wrapper +0 -0
index 404b3aa..cd941a8 100755
@@ -181,6 +181,13 @@ case "$elfformat" in
     elf32-powerpc)	format=elf32ppc	;;
 esac
Quoted 12 lines

arch/powerpc/boot/wrapper case "$elfformat" in · line 190

Quoted 7 lines
arch/powerpc/boot/wrapper +7 -0
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+	nodl="--no-dynamic-linker"
+fi

Some distros (RHEL at least?), ship the wrapper as a standalone script. So I don't think we can call things in $srctree. Or at least I don't know how that's supposed to work when it's shipped standalone. We're also basically reinventing ld-option, which is a PITA.

Quoted 3 lines

Okay I didn't realize that. It's already using mkuboot.sh, but only for uboot targets... I don't know, I don't have any good ideas at the moment.

That looks like a bug, but I guess no one runs Fedora/RHEL on those machines? Previously it just called mkimage using $PATH.

The Fedora spec file does:

make DESTDIR=$RPM_BUILD_ROOT bootwrapper_install WRAPPER_OBJDIR=%{_libdir}/kernel-wrapper WRAPPER_DTSDIR=%{_libdir}/kernel-wrapper/dts

bootwrapper_install installs a bunch of files, and also calls:

quiet_cmd_install_wrapper = INSTALL $(patsubst $(DESTDIR)$(WRAPPER_BINDIR)/%,%,$@)

      cmd_install_wrapper = $(INSTALL)  -m0755 $(patsubst $(DESTDIR)$(WRAPPER_BINDIR)/%,$(srctree)/$(obj)/%,$@) $@ ;\
				sed -i $@ -e 's%^object=.*%object=$(WRAPPER_OBJDIR)%' \
					  -e 's%^objbin=.*%objbin=$(WRAPPER_BINDIR)%' \

ie. it seds the script. So we could probably just install ld-version.sh into $(DESTDIR)$(WRAPPER_OBJDIR) and then sed $srctree maybe?

But it's old code and I'm not that across how it gets used in the wild. CC'ing Gustavo who is our Fedora/RHEL/Power guy.

cheers

#9 lore raw
Quoted 43 lines · Michael Ellerman
Quoted 24 lines · Nicholas Piggin
Quoted 9 lines · On Mon, 28 Nov 2016 22:07:39 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 8 lines · Nicholas Piggin
arch/powerpc/boot/wrapper +0 -0
index 404b3aa..cd941a8 100755
@@ -181,6 +181,13 @@ case "$elfformat" in
     elf32-powerpc)    format=elf32ppc    ;;
 esac
Quoted 12 lines

arch/powerpc/boot/wrapper case "$elfformat" in · line 190

Quoted 7 lines
arch/powerpc/boot/wrapper +7 -0
+# Do not include PT_INTERP segment when linking pie. Non-pie linking
+# just ignores this option.
+LD_VERSION=$(${CROSS}ld --version | $srctree/scripts/ld-version.sh)
+LD_NO_DL_MIN_VERSION=$(echo 2.26 | $srctree/scripts/ld-version.sh)
+if [ "$LD_VERSION" -ge "$LD_NO_DL_MIN_VERSION" ] ; then
+    nodl="--no-dynamic-linker"
+fi

Some distros (RHEL at least?), ship the wrapper as a standalone script. So I don't think we can call things in $srctree. Or at least I don't know how that's supposed to work when it's shipped standalone. We're also basically reinventing ld-option, which is a PITA.

Quoted 3 lines

Okay I didn't realize that. It's already using mkuboot.sh, but only for uboot targets... I don't know, I don't have any good ideas at the moment.

That looks like a bug, but I guess no one runs Fedora/RHEL on those machines? Previously it just called mkimage using $PATH.

The Fedora spec file does:

make DESTDIR=$RPM_BUILD_ROOT bootwrapper_install WRAPPER_OBJDIR=%{_libdir}/kernel-wrapper WRAPPER_DTSDIR=%{_libdir}/kernel-wrapper/dts

bootwrapper_install installs a bunch of files, and also calls:

quiet_cmd_install_wrapper = INSTALL $(patsubst $(DESTDIR)$(WRAPPER_BINDIR)/%,%,$@)

      cmd_install_wrapper = $(INSTALL)  -m0755 $(patsubst $(DESTDIR)$(WRAPPER_BINDIR)/%,$(srctree)/$(obj)/%,$@) $@ ;\
                sed -i $@ -e 's%^object=.*%object=$(WRAPPER_OBJDIR)%' \
                      -e 's%^objbin=.*%objbin=$(WRAPPER_BINDIR)%' \
Quoted 5 lines

ie. it seds the script. So we could probably just install ld-version.sh into $(DESTDIR)$(WRAPPER_OBJDIR) and then sed $srctree maybe?

But it's old code and I'm not that across how it gets used in the wild. CC'ing Gustavo who is our Fedora/RHEL/Power guy.

I don't know either how (or if) boot wrapper is used in Fedora as stand-alone. I think Michael's suggestion to use sed to replace $srctree should be fine.

[]'s
Gustavo

Quoted 3 lines

cheers

Applied
#10 lore raw
In reply to Nicholas Piggin
Quoted 11 lines · Nicholas Piggin

The boot wrapper performs its own relocations and does not require PT_INTERP segment.

Without this option, binutils 2.28 and newer tries to create a program header segment due to PT_INTERP, and the link fails because there is no space for it.

A recent binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=1a9ccd70f9a75dc6b48d340059f28ef3550c107b
has broken kernel builds:

Quoted 5 lines

ld: arch/powerpc/boot/zImage.pseries: Not enough room for program headers, try linking with -N
ld: final link failed: Bad value

Reported-by Anton Blanchard <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/ff45000fcb56b5b0f1a14a865d3541

cheers