[PATCH] 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 <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

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(-)

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 404b3aa..cd941a8 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper

@@ -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

-- 
2.10.2

On Mon, Nov 28, 2016 at 12:42:26PM +1100, Nicholas Piggin wrote:

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.

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.

-- 
Alan Modra
Australia Development Lab, IBM

Nicholas Piggin [off-list ref] writes:

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?

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

On Mon, 28 Nov 2016 22:07:39 +1100
Michael Ellerman [off-list ref] wrote:

Nicholas Piggin [off-list ref] writes:

quoted

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?

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

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 404b3aa..cd941a8 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper

@@ -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.

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

Hi Nicholas,

quoted

quoted

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?

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

quoted

+# 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

On Mon, 28 Nov 2016 12:39:38 +0000
Nick Clifton [off-list ref] wrote:

Hi Nicholas,

quoted

quoted

quoted

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?  

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

quoted

quoted

+# 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

Hi Nicholas,

quoted

... 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*

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.

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

On Mon, 28 Nov 2016 14:25:31 +0000
Nick Clifton [off-list ref] wrote:

Hi Nicholas,

quoted

quoted

... 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

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

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

Nicholas Piggin [off-list ref] writes:

On Mon, 28 Nov 2016 22:07:39 +1100
Michael Ellerman [off-list ref] wrote:

quoted

Nicholas Piggin [off-list ref] writes:

quoted

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 404b3aa..cd941a8 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper

@@ -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.

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

On 11/29/2016 01:42 AM, Michael Ellerman wrote:

Nicholas Piggin [off-list ref] writes:

quoted

On Mon, 28 Nov 2016 22:07:39 +1100
Michael Ellerman [off-list ref] wrote:

quoted

Nicholas Piggin [off-list ref] writes:

quoted

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 404b3aa..cd941a8 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper

@@ -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.

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.

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

cheers

On Mon, 2016-11-28 at 01:42:26 UTC, Nicholas Piggin wrote:

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 <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

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

cheers