Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200222164419.GB3326744@rani.riverdale.lan> <20200222171859.3594058-1-nivedita@alum.mit.edu>
 <20200222181413.GA22627@ubuntu-m2-xlarge-x86> <20200222185806.ywnqhfqmy67akfsa@google.com>
 <20200222201715.GA3674682@rani.riverdale.lan> <20200222210101.diqw4zt6lz42ekgx@google.com>
In-Reply-To: <20200222210101.diqw4zt6lz42ekgx@google.com>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Sat, 22 Feb 2020 15:33:20 -0800
Message-ID: <CAKwvOdn2pmRqJ+Rs+dhAPJy3hOb4VNn70XB40jcVgTeM8XmeFQ@mail.gmail.com>
Subject: Re: [PATCH] x86/boot/compressed: Fix compressed kernel linking with lld
To:     Arvind Sankar <nivedita@alum.mit.edu>,
        Borislav Petkov <bp@alien8.de>
Cc:     Nathan Chancellor <natechancellor@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>,
        Michael Matz <matz@suse.de>, Fangrui Song <maskray@google.com>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Feb 22, 2020 at 1:01 PM 'Fangrui Song' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> On 2020-02-22, Arvind Sankar wrote:
> >On Sat, Feb 22, 2020 at 10:58:06AM -0800, Fangrui Song wrote:
> >> On 2020-02-22, Nathan Chancellor wrote:
> >> >On Sat, Feb 22, 2020 at 12:18:59PM -0500, Arvind Sankar wrote:
> >> >> Commit TBD ("x86/boot/compressed: Remove unnecessary sections from
> >> >> bzImage") discarded unnecessary sections with *(*). While this works
> >> >> fine with the bfd linker, lld tries to also discard essential sections
> >> >> like .shstrtab, .symtab and .strtab, which results in the link failing
> >> >> since .shstrtab is required by the ELF specification. .symtab and
> >> >> .strtab are also necessary to generate the zoffset.h file for the
> >> >> bzImage header.
> >> >>
> >> >> Since the only sizeable section that can be discarded is .eh_frame,
> >> >> restrict the discard to only .eh_frame to be safe.
> >> >>
> >> >> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> >> >> ---
> >> >> Sending as a fix on top of tip/x86/boot.
> >> >>
> >> >>  arch/x86/boot/compressed/vmlinux.lds.S | 4 ++--
> >> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> >> >> index 12a20603d92e..469dcf800a2c 100644
> >> >> --- a/arch/x86/boot/compressed/vmlinux.lds.S
> >> >> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> >> >> @@ -74,8 +74,8 @@ SECTIONS
> >> >>   . = ALIGN(PAGE_SIZE);   /* keep ZO size page aligned */
> >> >>   _end = .;
> >> >>
> >> >> - /* Discard all remaining sections */
> >> >> + /* Discard .eh_frame to save some space */
> >> >>   /DISCARD/ : {
> >> >> -         *(*)
> >> >> +         *(.eh_frame)
> >> >>   }
> >> >>  }
> >> >> --
> >> >> 2.24.1
> >> >>
> >> >
> >> >FWIW:
> >> >
> >> >Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> >>
> >> I am puzzled. Doesn't -fno-asynchronous-unwind-tables suppress
> >> .eh_frame in the object files? Why are there still .eh_frame?
> >>
> >> Though, there is prior art: arch/s390/boot/compressed/vmlinux.lds.S also discards .eh_frame
> >
> >The compressed kernel doesn't use the regular flags and it seems it
> >doesn't have that option. Maybe we should add it in to avoid generating
> >those in the first place.

Ah, yikes.  For reference, please see my commit:

commit b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than
reset KBUILD_CFLAGS")

I'm of the conviction that reassigning KBUILD_CFLAGS via `:=`, as
opposed to strictly filtering flags out of it or appending to it, is
an antipattern.  We very very carefully construct KBUILD_CFLAGS in top
level and arch/ Makefiles, and it's very easy to miss a flag or to
when you "reset" KBUILD_CFLAGS.

*Boom* Case in point.

I meant to audit the rest of the places we do this in the kernel, but
haven't had the time to revisit arch/x86/boot/compressed/Makefile.

For now, I suggest:
1. revert `Commit TBD ("x86/boot/compressed: Remove unnecessary
sections from bzImage")` as it runs afoul differences in `*` for
`DISCARD` sections between linkers, as the intent was to remove
.eh_frame, of which it's less work to not generate them in the first
place via compiler flag, rather than generate then discard via linker.
2. simply add `KBUILD_CFLAGS += -fno-asynchronous-unwind-tables` to
arch/x86/boot/compressed/Makefile with Fangrui's Sugguested-by tag.
3. Remind me to revisit my proposed cleanup of
arch/x86/boot/compressed/Makefile (which eventually will undo #2). ;)
4. tglx to remind me that my compiler is broken and that I should fix it. :P

> >
> >The .eh_frame discard in arch/x86/kernel/vmlinux.lds.S does seem
> >superfluous though.
>
> Yes, please do that. I recommend suppressting unneeded sections at
> compile time, instead of discarding them at link time.
>
> https://github.com/torvalds/linux/commit/83a092cf95f28696ddc36c8add0cf03ac034897f
> added -Wl,--orphan-handling=warn to arch/powerpc/Makefile .
> x86 can follow if that is appropriate.

Kees has patches for that; I recommend waiting on his series.

> I don't recommend -Wl,--orphan-handling=error, which can unnecessarily
> break the builds.

Agreed. Until we have CI testing with LLD which is a WIP (action item on me).
-- 
Thanks,
~Nick Desaulniers