| Message ID | 20161128014226.7656-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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.
Nicholas Piggin <npiggin@gmail.com> 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? > 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. cheers
On Mon, 28 Nov 2016 22:07:39 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote: > Nicholas Piggin <npiggin@gmail.com> 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? 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. > > > 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, >>> 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... >>> +# 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 <nickc@redhat.com> wrote: > Hi Nicholas, > > >>> 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... > > >>> +# 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, >> ... 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 <nickc@redhat.com> wrote: > Hi Nicholas, > > >> ... 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. > > > 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. Okay. It sounds like we should use --no-dynamic-linker whether or not your patch is changed. Thanks, Nick
Nicholas Piggin <npiggin@gmail.com> writes: > On Mon, 28 Nov 2016 22:07:39 +1100 > Michael Ellerman <mpe@ellerman.id.au> wrote: >> Nicholas Piggin <npiggin@gmail.com> writes: >> > 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 <npiggin@gmail.com> writes: > > On Mon, 28 Nov 2016 22:07:39 +1100 > > Michael Ellerman <mpe@ellerman.id.au> wrote: > >> Nicholas Piggin <npiggin@gmail.com> writes: > >>> 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 <anton@samba.org> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/ff45000fcb56b5b0f1a14a865d3541 cheers
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
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 <anton@samba.org> 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(-)