x86/boot/compressed/64: Define __force_order only when CONFIG_RANDOMIZE_BASE is unset

Message ID 20191221151813.1573450-1-raj.khem@gmail.com
State New
Headers show
Series
  • x86/boot/compressed/64: Define __force_order only when CONFIG_RANDOMIZE_BASE is unset
Related show

Commit Message

Khem Raj Dec. 21, 2019, 3:18 p.m.
kaslr_64.c also defines the same variable, however when both files are
included into final link, linker complains about multiple definition of
`__force_order' which is coming from kaslr_64.o and pgtable_64.o, its
possible that kaslr_64.o is disabled via CONFIG_RANDOMIZE_BASE config
option, therefore define it conditionally only when
CONFIG_RANDOMIZE_BASE is not set

Since arch/x86/boot/compressed/Makefile overrides global CFLAGS it loses
-fno-common option which would have caught this

Signed-off-by: Khem Raj <raj.khem@gmail.com>

Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86-ml <x86@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/boot/compressed/pgtable_64.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.24.1

Comments

Kirill A. Shutemov Dec. 23, 2019, 5:10 p.m. | #1
On Sat, Dec 21, 2019 at 07:18:13AM -0800, Khem Raj wrote:
> Since arch/x86/boot/compressed/Makefile overrides global CFLAGS it loses

> -fno-common option which would have caught this


If this doesn't cause any visible problems, why bother?

Hopefully, we will be able to drop it altogether once we ditch GCC 4.X
support.

-- 
 Kirill A. Shutemov
Khem Raj Dec. 23, 2019, 10:25 p.m. | #2
On Mon, Dec 23, 2019 at 9:10 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>

> On Sat, Dec 21, 2019 at 07:18:13AM -0800, Khem Raj wrote:

> > Since arch/x86/boot/compressed/Makefile overrides global CFLAGS it loses

> > -fno-common option which would have caught this

>

> If this doesn't cause any visible problems, why bother?

>


it does break builds with gcc trunk as of now e.g.

> Hopefully, we will be able to drop it altogether once we ditch GCC 4.X

> support.

>


gcc10 is switching defaults to -fno-common so we need to solve this one way or
other, I am not sure if gcc 4.x will be dropped before gcc10 release
which would be
in mid of 2020

> --

>  Kirill A. Shutemov
Kirill A. Shutemov Dec. 23, 2019, 11:08 p.m. | #3
On Mon, Dec 23, 2019 at 02:25:02PM -0800, Khem Raj wrote:
> On Mon, Dec 23, 2019 at 9:10 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:

> >

> > On Sat, Dec 21, 2019 at 07:18:13AM -0800, Khem Raj wrote:

> > > Since arch/x86/boot/compressed/Makefile overrides global CFLAGS it loses

> > > -fno-common option which would have caught this

> >

> > If this doesn't cause any visible problems, why bother?

> >

> 

> it does break builds with gcc trunk as of now e.g.

> 

> > Hopefully, we will be able to drop it altogether once we ditch GCC 4.X

> > support.

> >

> 

> gcc10 is switching defaults to -fno-common so we need to solve this one way or

> other, I am not sure if gcc 4.x will be dropped before gcc10 release

> which would be

> in mid of 2020


Okay, it makes sense then. Please include this info into the commit
message.

Also, I wounder if it would be cleaner to define both of them as __weak?

-- 
 Kirill A. Shutemov
Arnd Bergmann Dec. 30, 2019, 12:12 p.m. | #4
On Tue, Dec 24, 2019 at 12:08 AM Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Mon, Dec 23, 2019 at 02:25:02PM -0800, Khem Raj wrote:

> > On Mon, Dec 23, 2019 at 9:10 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:

> > >

> > > On Sat, Dec 21, 2019 at 07:18:13AM -0800, Khem Raj wrote:

> > > > Since arch/x86/boot/compressed/Makefile overrides global CFLAGS it loses

> > > > -fno-common option which would have caught this

> > >

> > > If this doesn't cause any visible problems, why bother?

> > >

> >

> > it does break builds with gcc trunk as of now e.g.

> >

> > > Hopefully, we will be able to drop it altogether once we ditch GCC 4.X

> > > support.

> > >

> >

> > gcc10 is switching defaults to -fno-common so we need to solve this one way or

> > other, I am not sure if gcc 4.x will be dropped before gcc10 release

> > which would be

> > in mid of 2020

>

> Okay, it makes sense then. Please include this info into the commit

> message.

>

> Also, I wounder if it would be cleaner to define both of them as __weak?


Or maybe make the #ifdef check for gcc < 5 instead of checking for
CONFIG_RANDOMIZE_BASE? That way it will be found by whoever
cleans up the code when we increase the minimum compiler
version to one that doesn't require the hack.

        Arnd
Kees Cook Dec. 30, 2019, 6:34 p.m. | #5
On Mon, Dec 30, 2019 at 01:12:31PM +0100, Arnd Bergmann wrote:
> On Tue, Dec 24, 2019 at 12:08 AM Kirill A. Shutemov

> <kirill@shutemov.name> wrote:

> > On Mon, Dec 23, 2019 at 02:25:02PM -0800, Khem Raj wrote:

> > > On Mon, Dec 23, 2019 at 9:10 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:

> > > >

> > > > On Sat, Dec 21, 2019 at 07:18:13AM -0800, Khem Raj wrote:

> > > > > Since arch/x86/boot/compressed/Makefile overrides global CFLAGS it loses

> > > > > -fno-common option which would have caught this

> > > >

> > > > If this doesn't cause any visible problems, why bother?

> > > >

> > >

> > > it does break builds with gcc trunk as of now e.g.

> > >

> > > > Hopefully, we will be able to drop it altogether once we ditch GCC 4.X

> > > > support.

> > > >

> > >

> > > gcc10 is switching defaults to -fno-common so we need to solve this one way or

> > > other, I am not sure if gcc 4.x will be dropped before gcc10 release

> > > which would be

> > > in mid of 2020

> >

> > Okay, it makes sense then. Please include this info into the commit

> > message.

> >

> > Also, I wounder if it would be cleaner to define both of them as __weak?

> 

> Or maybe make the #ifdef check for gcc < 5 instead of checking for

> CONFIG_RANDOMIZE_BASE? That way it will be found by whoever

> cleans up the code when we increase the minimum compiler

> version to one that doesn't require the hack.


I agree: that seems a better way to do this.

-- 
Kees Cook

Patch

diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index c8862696a47b..077d19268b0b 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -12,7 +12,9 @@ 
  * It is not referenced from the code, but GCC < 5 with -fPIE would fail
  * due to an undefined symbol. Define it to make these ancient GCCs work.
  */
+#ifndef CONFIG_RANDOMIZE_BASE
 unsigned long __force_order;
+#endif
 
 #define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
 #define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */