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

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

[Hans de Goede]

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

Changes to the sha256 code has caused the purgatory in 5.4-rc1 to have
a missing symbol on memzero_explicit, yet things still happily build.

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

This causes a build of 5.4-rc1 with this patch added to fail as it should:

  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

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Add a .gitignore file with purgatory.chk listed in it

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/x86/purgatory/.gitignore |  1 +
 arch/x86/purgatory/Makefile   | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/purgatory/.gitignore

diff --git a/arch/x86/purgatory/.gitignore b/arch/x86/purgatory/.gitignore
new file mode 100644
index 000000000000..d2be1500671d
--- /dev/null
+++ b/arch/x86/purgatory/.gitignore
@@ -0,0 +1 @@
+purgatory.chk
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index fb4ee5444379..5bb58247699d 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -14,8 +14,12 @@ $(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
+# Since we link purgatory.ro with -r unresolved symbols are not checked, so we
+# also link a purgatory.chk binary without -r to check for unresolved symbols.
+PURGATORY_LDFLAGS := -e purgatory_start -nostdlib -z nodefaultlib
+LDFLAGS_purgatory.ro := -r $(PURGATORY_LDFLAGS)
+LDFLAGS_purgatory.chk := $(PURGATORY_LDFLAGS)
+targets += purgatory.ro purgatory.chk
 
 KASAN_SANITIZE	:= n
 KCOV_INSTRUMENT := n
@@ -58,12 +62,15 @@ CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)
 
+$(obj)/purgatory.chk: $(obj)/purgatory.ro FORCE
+		$(call if_changed,ld)
+
 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.ro $(obj)/purgatory.chk FORCE
 	$(call if_changed,bin2c)
 
 obj-$(CONFIG_KEXEC_FILE)	+= kexec-purgatory.o
-- 
2.25.0

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on v5.6-rc3 next-20200228]
[cannot apply to tip/auto-latest]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/x86-purgatory-Make-sure-we-fail-the-build-if-purgatory-ro-has-missing-symbols/20200223-040035
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 248ed51048c40d36728e70914e38bffd7821da57
config: x86_64-randconfig-s1-20200229 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: arch/x86/purgatory/purgatory.ro: in function `kstrtoull':
>> (.text+0x2b0e): undefined reference to `ftrace_likely_update'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40199 bytes --]

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

[Hans de Goede]

Hi,

On 2/29/20 6:16 PM, kbuild test robot wrote:
> Hi Hans,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on tip/x86/core]
> [also build test ERROR on v5.6-rc3 next-20200228]
> [cannot apply to tip/auto-latest]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/x86-purgatory-Make-sure-we-fail-the-build-if-purgatory-ro-has-missing-symbols/20200223-040035
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 248ed51048c40d36728e70914e38bffd7821da57
> config: x86_64-randconfig-s1-20200229 (attached as .config)
> compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
> reproduce:
>          # save the attached .config to linux build tree
>          make ARCH=x86_64
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     ld: arch/x86/purgatory/purgatory.ro: in function `kstrtoull':
>>> (.text+0x2b0e): undefined reference to `ftrace_likely_update'

AFAICT this is the patch working as intended and pointing out an actual issue
with the purgatory code. Which shows that we really should get this
patch merged...

Regards,

Hans

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

[Borislav Petkov]

On Sat, Feb 29, 2020 at 07:12:57PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/29/20 6:16 PM, kbuild test robot wrote:
> > Hi Hans,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on tip/x86/core]
> > [also build test ERROR on v5.6-rc3 next-20200228]
> > [cannot apply to tip/auto-latest]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/x86-purgatory-Make-sure-we-fail-the-build-if-purgatory-ro-has-missing-symbols/20200223-040035
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 248ed51048c40d36728e70914e38bffd7821da57
> > config: x86_64-randconfig-s1-20200229 (attached as .config)
> > compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
> > reproduce:
> >          # save the attached .config to linux build tree
> >          make ARCH=x86_64
> > 
> > If you fix the issue, kindly add following tag
> > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> >     ld: arch/x86/purgatory/purgatory.ro: in function `kstrtoull':
> > > > (.text+0x2b0e): undefined reference to `ftrace_likely_update'
> 
> AFAICT this is the patch working as intended and pointing out an actual issue
> with the purgatory code. Which shows that we really should get this
> patch merged...

... and break the build? I don't think so.

I know, it would fail silently now but I don't recall anyone complaining
about it. So it was a don't care so far.

IOW, first this ftrace_likely_update thing needs to be sorted out and
then this merged.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Arvind Sankar]

On Wed, Mar 11, 2020 at 09:49:54PM +0100, Borislav Petkov wrote:
> On Sat, Feb 29, 2020 at 07:12:57PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 2/29/20 6:16 PM, kbuild test robot wrote:
> > > Hi Hans,
> > > 
> > > I love your patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on tip/x86/core]
> > > [also build test ERROR on v5.6-rc3 next-20200228]
> > > [cannot apply to tip/auto-latest]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help
> > > improve the system. BTW, we also suggest to use '--base' option to specify the
> > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > > 
> > > url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/x86-purgatory-Make-sure-we-fail-the-build-if-purgatory-ro-has-missing-symbols/20200223-040035
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 248ed51048c40d36728e70914e38bffd7821da57
> > > config: x86_64-randconfig-s1-20200229 (attached as .config)
> > > compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
> > > reproduce:
> > >          # save the attached .config to linux build tree
> > >          make ARCH=x86_64
> > > 
> > > If you fix the issue, kindly add following tag
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > >     ld: arch/x86/purgatory/purgatory.ro: in function `kstrtoull':
> > > > > (.text+0x2b0e): undefined reference to `ftrace_likely_update'
> > 
> > AFAICT this is the patch working as intended and pointing out an actual issue
> > with the purgatory code. Which shows that we really should get this
> > patch merged...
> 
> ... and break the build? I don't think so.
> 
> I know, it would fail silently now but I don't recall anyone complaining
> about it. So it was a don't care so far.
> 
> IOW, first this ftrace_likely_update thing needs to be sorted out and
> then this merged.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Hans, I think adding -DDISABLE_BRANCH_PROFILING to PURGATORY_CFLAGS
might fix this.

Thanks.

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

[Hans de Goede]

HI,

On 3/11/20 10:20 PM, Arvind Sankar wrote:
> On Wed, Mar 11, 2020 at 09:49:54PM +0100, Borislav Petkov wrote:
>> On Sat, Feb 29, 2020 at 07:12:57PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 2/29/20 6:16 PM, kbuild test robot wrote:
>>>> Hi Hans,
>>>>
>>>> I love your patch! Yet something to improve:
>>>>
>>>> [auto build test ERROR on tip/x86/core]
>>>> [also build test ERROR on v5.6-rc3 next-20200228]
>>>> [cannot apply to tip/auto-latest]
>>>> [if your patch is applied to the wrong git tree, please drop us a note to help
>>>> improve the system. BTW, we also suggest to use '--base' option to specify the
>>>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>>>>
>>>> url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/x86-purgatory-Make-sure-we-fail-the-build-if-purgatory-ro-has-missing-symbols/20200223-040035
>>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 248ed51048c40d36728e70914e38bffd7821da57
>>>> config: x86_64-randconfig-s1-20200229 (attached as .config)
>>>> compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
>>>> reproduce:
>>>>           # save the attached .config to linux build tree
>>>>           make ARCH=x86_64
>>>>
>>>> If you fix the issue, kindly add following tag
>>>> Reported-by: kbuild test robot <lkp@intel.com>
>>>>
>>>> All errors (new ones prefixed by >>):
>>>>
>>>>      ld: arch/x86/purgatory/purgatory.ro: in function `kstrtoull':
>>>>>> (.text+0x2b0e): undefined reference to `ftrace_likely_update'
>>>
>>> AFAICT this is the patch working as intended and pointing out an actual issue
>>> with the purgatory code. Which shows that we really should get this
>>> patch merged...
>>
>> ... and break the build? I don't think so.
>>
>> I know, it would fail silently now but I don't recall anyone complaining
>> about it. So it was a don't care so far.
>>
>> IOW, first this ftrace_likely_update thing needs to be sorted out and
>> then this merged.
>>
>> Thx.
>>
>> -- 
>> Regards/Gruss,
>>      Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
> 
> Hans, I think adding -DDISABLE_BRANCH_PROFILING to PURGATORY_CFLAGS
> might fix this.

Yes I was just looking into doing that myself, I agree that that
should fix this. I will mail out a new patch-series with that as
preparation patch soon.

Regards,

Hans

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

[Hans de Goede]

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

Changes to the sha256 code has caused the purgatory in 5.4-rc1 to have
a missing symbol on memzero_explicit, yet things still happily build.

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

This causes a build of 5.4-rc1 with this patch added to fail as it should:

  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

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Add a .gitignore file with purgatory.chk listed in it

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/x86/purgatory/.gitignore |  1 +
 arch/x86/purgatory/Makefile   | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/purgatory/.gitignore

diff --git a/arch/x86/purgatory/.gitignore b/arch/x86/purgatory/.gitignore
new file mode 100644
index 000000000000..d2be1500671d
--- /dev/null
+++ b/arch/x86/purgatory/.gitignore
@@ -0,0 +1 @@
+purgatory.chk
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index fb4ee5444379..5bb58247699d 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -14,8 +14,12 @@ $(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
+# Since we link purgatory.ro with -r unresolved symbols are not checked, so we
+# also link a purgatory.chk binary without -r to check for unresolved symbols.
+PURGATORY_LDFLAGS := -e purgatory_start -nostdlib -z nodefaultlib
+LDFLAGS_purgatory.ro := -r $(PURGATORY_LDFLAGS)
+LDFLAGS_purgatory.chk := $(PURGATORY_LDFLAGS)
+targets += purgatory.ro purgatory.chk
 
 KASAN_SANITIZE	:= n
 KCOV_INSTRUMENT := n
@@ -58,12 +62,15 @@ CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)
 
+$(obj)/purgatory.chk: $(obj)/purgatory.ro FORCE
+		$(call if_changed,ld)
+
 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.ro $(obj)/purgatory.chk FORCE
 	$(call if_changed,bin2c)
 
 obj-$(CONFIG_KEXEC_FILE)	+= kexec-purgatory.o
-- 
2.23.0