Re: [PATCH v4 2/2] x86/purgatory: Make sure we fail the build if purgatory.ro has missing symbols

From: Hans de Goede <hdegoede@redhat.com>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] x86/purgatory: Make sure we fail the build if purgatory.ro has missing symbols
Date: Thu, 12 Mar 2020 12:31:39 +0100	[thread overview]
Message-ID: <3d58e77d-41e5-7927-fe84-4c058015e469@redhat.com> (raw)
In-Reply-To: <20200312001006.GA170175@rani.riverdale.lan>

Hi,

On 3/12/20 1:10 AM, Arvind Sankar wrote:
> On Wed, Mar 11, 2020 at 10:46:01PM +0100, Hans de Goede wrote:
>> Since we link purgatory.ro with -r aka we enable "incremental linking"
>> no checks for unresolved symbols is done while linking purgatory.ro.
>>
> 
> Do we actually need to link purgatory with -r? We could use
> --emit-relocs to get the relocation sections generated the way the main
> x86 kernel does, no?
> 
> Eg like the below? This would avoid the double-link creating
> purgatory.chk.

Ok, I've just given that a try and it does indeed catch missing symbols
I'm not sure if it still generates a working purgatory though (I did
not try kexec from a kernel with it).

Since this all is somewhat black magic to me, my goal was to not change
how we build the purgatory while still adding a step which checks for
missing symbols, as my changes from a while back to unify all the SHA256
implementations in the kernel had accidentally caused a missing symbol
which went unnoticed for a while.

Also the same patch, using the 2 step approach has already been merged
for the s390x purgatory code, so doing it your way would lead to 2
different approaches in the kernel.

I do agree that your way does seem to be more elegant though...

I also see that the kbuild test robot has managed to come up with
yet another set of Kconfig options triggering missing symbols.

IMHO this shows that we really need some patch to detect these,
because clearly there are a lot of config-s with missing symbols
in the purgatory out there with no-one noticing.

I will send out a v5 of my patch-set changing the first patch to
also fix the new issue the kbuild test robot has found. I'm going
to leave this patch as is. If you prefer replacing the second patch
in the set (this patch) with your solution then that is fine with me.

Regards,

Hans

> diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
> index fb4ee5444379..5332f95ca1d3 100644
> --- a/arch/x86/purgatory/Makefile
> +++ b/arch/x86/purgatory/Makefile
> @@ -14,8 +14,8 @@ $(obj)/sha256.o: $(srctree)/lib/crypto/sha256.c FORCE
>   
>   CFLAGS_sha256.o := -D__DISABLE_EXPORTS
>   
> -LDFLAGS_purgatory.ro := -e purgatory_start -r --no-undefined -nostdlib -z nodefaultlib
> -targets += purgatory.ro
> +LDFLAGS_purgatory := -e purgatory_start --emit-relocs -nostdlib -z nodefaultlib
> +targets += purgatory
>   
>   KASAN_SANITIZE	:= n
>   KCOV_INSTRUMENT := n
> @@ -55,7 +55,7 @@ CFLAGS_sha256.o			+= $(PURGATORY_CFLAGS)
>   CFLAGS_REMOVE_string.o		+= $(PURGATORY_CFLAGS_REMOVE)
>   CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
>   
> -$(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
> +$(obj)/purgatory: $(PURGATORY_OBJS) FORCE
>   		$(call if_changed,ld)
>   
>   targets += kexec-purgatory.c
> @@ -63,7 +63,7 @@ targets += kexec-purgatory.c
>   quiet_cmd_bin2c = BIN2C   $@
>         cmd_bin2c = $(objtree)/scripts/bin2c kexec_purgatory < $< > $@
>   
> -$(obj)/kexec-purgatory.c: $(obj)/purgatory.ro FORCE
> +$(obj)/kexec-purgatory.c: $(obj)/purgatory FORCE
>   	$(call if_changed,bin2c)
>   
>   obj-$(CONFIG_KEXEC_FILE)	+= kexec-purgatory.o
> 