[RFC v2 0/1] s390/purgatory: Make sure we fail the build if purgatory has missing symbols

[RFC v2 0/1] s390/purgatory: Make sure we fail the build if purgatory has missing symbols

[Hans de Goede]

Hi s390 maintainers,

Here is a second RFC version of my patch for $subject, mirroring the
changes in v2 of the x86 patch.

As last time this patch is completely UNTESTED.

Changes in v2:
- Using 2 if_changed lines under a single rule does not work, then
  1 of the 2 will always execute each build.
  Instead add a new (unused) purgatory.chk intermediate which gets
  linked from purgatory.ro without -r to do the missing symbols check
- This also fixes the check generating an a.out file (oops)

Relevant part of the cover letter from v1:

In 5.4-rc1 the 2 different sha256 implementations for the purgatory resp.
for crypto/sha256_generic.c have been consolidated into 1 single shared
implementation under lib/crypto/sha256.c .

At least on x86 this was causing silent corruption of the purgatory due
to a missing memzero_explicit symbol in the purgatory string.c/.o file.

With the x86 equivalent of this patch applied a x86 build of 5.4-rc1 now
correctly fails:

  CHK     arch/x86/purgatory/purgatory.ro
ld: arch/x86/purgatory/purgatory.ro: in function `sha256_transform':
sha256.c:(.text+0x1c0c): undefined reference to `memzero_explicit'
make[2]: *** [arch/x86/purgatory/Makefile:72:
    arch/x86/purgatory/kexec-purgatory.c] Error 1
make[1]: *** [scripts/Makefile.build:509: arch/x86/purgatory] Error 2
make: *** [Makefile:1650: arch/x86] Error 2

It would be great if the s390 maintainers can test this equivalent patch
on s390.

As for fixing the missing memzero_explicit symbol, we are currently
discussing making memzero_explicit a static inline wrapper of memset
in string.h, so that we do not need to implement it in multiple places.

This discussion is Cc-ed to the generic linux-kernel@vger.kernel.org list,
it is happening in the
"[PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit" thread.

Regards,

Hans

[RFC v2] s390/purgatory: Make sure we fail the build if purgatory has missing symbols

[Hans de Goede]

Since we link purgatory with -r aka we enable "incremental linking"
no checks for unresolved symbols are done while linking the purgatory.

This commit adds an extra check for unresolved symbols by calling ld
without -r before running objcopy to generate purgatory.ro.

This will help us catch missing symbols in the purgatory sooner.

Note this commit also removes --no-undefined from LDFLAGS_purgatory
as that has no effect.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Using 2 if_changed lines under a single rule does not work, then
  1 of the 2 will always execute each build.
  Instead add a new (unused) purgatory.chk intermediate which gets
  linked from purgatory.ro without -r to do the missing symbols check
- This also fixes the check generating an a.out file (oops)
---
 arch/s390/purgatory/.gitignore |  1 +
 arch/s390/purgatory/Makefile   | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/s390/purgatory/.gitignore b/arch/s390/purgatory/.gitignore
index 04a03433c720..c82157f46b18 100644
--- a/arch/s390/purgatory/.gitignore
+++ b/arch/s390/purgatory/.gitignore
@@ -1,3 +1,4 @@
 purgatory
+purgatory.chk
 purgatory.lds
 purgatory.ro
diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
index bc0d7a0d0394..13e9a5dc0a07 100644
--- a/arch/s390/purgatory/Makefile
+++ b/arch/s390/purgatory/Makefile
@@ -4,7 +4,7 @@ OBJECT_FILES_NON_STANDARD := y
 
 purgatory-y := head.o purgatory.o string.o sha256.o mem.o
 
-targets += $(purgatory-y) purgatory.lds purgatory purgatory.ro
+targets += $(purgatory-y) purgatory.lds purgatory purgatory.chk purgatory.ro
 PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y))
 
 $(obj)/sha256.o: $(srctree)/lib/crypto/sha256.c FORCE
@@ -26,15 +26,22 @@ KBUILD_CFLAGS += $(CLANG_FLAGS)
 KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
 KBUILD_AFLAGS := $(filter-out -DCC_USING_EXPOLINE,$(KBUILD_AFLAGS))
 
-LDFLAGS_purgatory := -r --no-undefined -nostdlib -z nodefaultlib -T
+# Since we link purgatory with -r unresolved symbols are not checked, so we
+# also link a purgatory.chk binary without -r to check for unresolved symbols.
+PURGATORY_LDFLAGS := -nostdlib -z nodefaultlib
+LDFLAGS_purgatory := -r $(PURGATORY_LDFLAGS) -T
+LDFLAGS_purgatory.chk := -e purgatory_start $(PURGATORY_LDFLAGS)
 $(obj)/purgatory: $(obj)/purgatory.lds $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)
 
+$(obj)/purgatory.chk: $(obj)/purgatory FORCE
+		$(call if_changed,ld)
+
 OBJCOPYFLAGS_purgatory.ro := -O elf64-s390
 OBJCOPYFLAGS_purgatory.ro += --remove-section='*debug*'
 OBJCOPYFLAGS_purgatory.ro += --remove-section='.comment'
 OBJCOPYFLAGS_purgatory.ro += --remove-section='.note.*'
-$(obj)/purgatory.ro: $(obj)/purgatory FORCE
+$(obj)/purgatory.ro: $(obj)/purgatory $(obj)/purgatory.chk FORCE
 		$(call if_changed,objcopy)
 
 $(obj)/kexec-purgatory.o: $(obj)/kexec-purgatory.S $(obj)/purgatory.ro FORCE
-- 
2.23.0

Re: [RFC v2 0/1] s390/purgatory: Make sure we fail the build if purgatory has missing symbols

[Philipp Rudo]

Hi Hans,

also adding Ingo on Cc.

I tested you patch on s390 and it does what it's supposed to do. The build now
fails with 

  LD      arch/s390/purgatory/purgatory.chk
arch/s390/purgatory/purgatory: In function `sha256_update':
(.text+0x3bc2): undefined reference to `memzero_explicit'
/home/prudo/git/linux/linux/arch/s390/purgatory/Makefile:38: recipe for target 'arch/s390/purgatory/purgatory.chk' failed
make[3]: *** [arch/s390/purgatory/purgatory.chk] Error 1

After applying Arvid's memzero_explizit fix ("[PATCH] lib/string: make
memzero_explicit inline instead of external") as well the build works again.

My only problem is how to uptream your patch. Just adding it to our branch
would cause a (intentional) build breakage until Ingo's branch is merged.

@Vasliy & Ingo: Can you please find a solution for this.

Thanks
Philipp

On Tue,  8 Oct 2019 10:54:20 +0200[PATCH] lib/string: make memzero_explicit
inline instead of external Hans de Goede <hdegoede@redhat.com> wrote:

> Hi s390 maintainers,
> 
> Here is a second RFC version of my patch for $subject, mirroring the
> changes in v2 of the x86 patch.
> 
> As last time this patch is completely UNTESTED.
> 
> Changes in v2:
> - Using 2 if_changed lines under a single rule does not work, then
>   1 of the 2 will always execute each build.
>   Instead add a new (unused) purgatory.chk intermediate which gets
>   linked from purgatory.ro without -r to do the missing symbols check
> - This also fixes the check generating an a.out file (oops)
> 
> Relevant part of the cover letter from v1:
> 
> In 5.4-rc1 the 2 different sha256 implementations for the purgatory resp.
> for crypto/sha256_generic.c have been consolidated into 1 single shared
> implementation under lib/crypto/sha256.c .
> 
> At least on x86 this was causing silent corruption of the purgatory due
> to a missing memzero_explicit symbol in the purgatory string.c/.o file.
> 
> With the x86 equivalent of this patch applied a x86 build of 5.4-rc1 now
> correctly fails:
> 
>   CHK     arch/x86/purgatory/purgatory.ro
> ld: arch/x86/purgatory/purgatory.ro: in function `sha256_transform':
> sha256.c:(.text+0x1c0c): undefined reference to `memzero_explicit'
> make[2]: *** [arch/x86/purgatory/Makefile:72:
>     arch/x86/purgatory/kexec-purgatory.c] Error 1
> make[1]: *** [scripts/Makefile.build:509: arch/x86/purgatory] Error 2
> make: *** [Makefile:1650: arch/x86] Error 2
> 
> It would be great if the s390 maintainers can test this equivalent patch
> on s390.
> 
> As for fixing the missing memzero_explicit symbol, we are currently
> discussing making memzero_explicit a static inline wrapper of memset
> in string.h, so that we do not need to implement it in multiple places.
> 
> This discussion is Cc-ed to the generic linux-kernel@vger.kernel.org list,
> it is happening in the
> "[PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit" thread.
> 
> Regards,
> 
> Hans
> 
> 
> 

Re: [RFC v2 0/1] s390/purgatory: Make sure we fail the build if purgatory has missing symbols

[Christian Borntraeger]

On 09.10.19 11:39, Philipp Rudo wrote:
> Hi Hans,
> 
> also adding Ingo on Cc.
> 
> I tested you patch on s390 and it does what it's supposed to do. The build now
> fails with 
> 
>   LD      arch/s390/purgatory/purgatory.chk
> arch/s390/purgatory/purgatory: In function `sha256_update':
> (.text+0x3bc2): undefined reference to `memzero_explicit'
> /home/prudo/git/linux/linux/arch/s390/purgatory/Makefile:38: recipe for target 'arch/s390/purgatory/purgatory.chk' failed
> make[3]: *** [arch/s390/purgatory/purgatory.chk] Error 1
> 
> After applying Arvid's memzero_explizit fix ("[PATCH] lib/string: make
> memzero_explicit inline instead of external") as well the build works again.
> 
> My only problem is how to uptream your patch. Just adding it to our branch
> would cause a (intentional) build breakage until Ingo's branch is merged.
> 
> @Vasliy & Ingo: Can you please find a solution for this.


I talked quickly to Vasily. The best solution is likely to carry that
patch in the tree that contains the fix. Ingo, can you carry the s390
patch in your tip tree as well?

With my s390 co-maintainer hat on:

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Please also consider Philipps mail as "Tested-by:"