[PATCH] Revert "um: Enable CONFIG_CONSTRUCTORS"

[PATCH] Revert "um: Enable CONFIG_CONSTRUCTORS"

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This reverts commit 786b2384bf1c ("um: Enable CONFIG_CONSTRUCTORS").

There are two issues with this commit, uncovered by Anton in tests
on some (Debian) systems:

1) I completely forgot to call any constructors if CONFIG_CONSTRUCTORS
   isn't set. Don't recall now if it just wasn't needed on my system, or
   if I never tested this case.

2) With that fixed, it works - with CONFIG_CONSTRUCTORS *unset*. If I
   set CONFIG_CONSTRUCTORS, it fails again, which isn't totally
   unexpected since whatever wanted to run is likely to have to run
   before the kernel init etc. that calls the constructors in this case.

Basically, some constructors that gcc emits (libc has?) need to run
very early during init; the failure mode otherwise was that the ptrace
fork test already failed:

----------------------
$ ./linux mem=512M
Core dump limits :
	soft - 0
	hard - NONE
Checking that ptrace can change system call numbers...check_ptrace : child exited with exitcode 6, while expecting 0; status 0x67f
Aborted
----------------------

Thinking more about this, it's clear that we simply cannot support
CONFIG_CONSTRUCTORS in UML. All the cases we need now (gcov, kasan)
involve not use of the __attribute__((constructor)), but instead
some constructor code/entry generated by gcc. Therefore, we cannot
distinguish between kernel constructors and system constructors.

Thus, revert this commit.

Cc: stable@vger.kernel.org [5.4+]
Fixes: 786b2384bf1c ("um: Enable CONFIG_CONSTRUCTORS")
Reported-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/um/include/asm/common.lds.S | 2 +-
 arch/um/kernel/dyn.lds.S         | 1 +
 init/Kconfig                     | 1 +
 kernel/gcov/Kconfig              | 2 +-
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index d7086b985f27..4049f2c46387 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -83,8 +83,8 @@
 	__preinit_array_end = .;
   }
   .init_array : {
-        /* dummy - we call this ourselves */
 	__init_array_start = .;
+	*(.init_array)
 	__init_array_end = .;
   }
   .fini_array : {
diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
index c69d69ee96be..f5001481010c 100644
--- a/arch/um/kernel/dyn.lds.S
+++ b/arch/um/kernel/dyn.lds.S
@@ -103,6 +103,7 @@ SECTIONS
      be empty, which isn't pretty.  */
   . = ALIGN(32 / 8);
   .preinit_array     : { *(.preinit_array) }
+  .init_array     : { *(.init_array) }
   .fini_array     : { *(.fini_array) }
   .data           : {
     INIT_TASK_DATA(KERNEL_STACK_SIZE)
diff --git a/init/Kconfig b/init/Kconfig
index b4daad2bac23..0328b53d09ad 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -54,6 +54,7 @@ config CC_DISABLE_WARN_MAYBE_UNINITIALIZED
 
 config CONSTRUCTORS
 	bool
+	depends on !UML
 
 config IRQ_WORK
 	bool
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 060e8e726755..3941a9c48f83 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling"
 config GCOV_KERNEL
 	bool "Enable gcov-based kernel profiling"
 	depends on DEBUG_FS
-	select CONSTRUCTORS
+	select CONSTRUCTORS if !UML
 	default n
 	---help---
 	This option enables gcov-based code profiling (e.g. for code coverage
-- 
2.23.0

Re: [PATCH] Revert "um: Enable CONFIG_CONSTRUCTORS"

[Anton Ivanov]

On 04/12/2019 16:43, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This reverts commit 786b2384bf1c ("um: Enable CONFIG_CONSTRUCTORS").
> 
> There are two issues with this commit, uncovered by Anton in tests
> on some (Debian) systems:
> 
> 1) I completely forgot to call any constructors if CONFIG_CONSTRUCTORS
>     isn't set. Don't recall now if it just wasn't needed on my system, or
>     if I never tested this case.
> 
> 2) With that fixed, it works - with CONFIG_CONSTRUCTORS *unset*. If I
>     set CONFIG_CONSTRUCTORS, it fails again, which isn't totally
>     unexpected since whatever wanted to run is likely to have to run
>     before the kernel init etc. that calls the constructors in this case.
> 
> Basically, some constructors that gcc emits (libc has?) need to run
> very early during init; the failure mode otherwise was that the ptrace
> fork test already failed:
> 
> ----------------------
> $ ./linux mem=512M
> Core dump limits :
> 	soft - 0
> 	hard - NONE
> Checking that ptrace can change system call numbers...check_ptrace : child exited with exitcode 6, while expecting 0; status 0x67f
> Aborted
> ----------------------
> 
> Thinking more about this, it's clear that we simply cannot support
> CONFIG_CONSTRUCTORS in UML. All the cases we need now (gcov, kasan)
> involve not use of the __attribute__((constructor)), but instead
> some constructor code/entry generated by gcc. Therefore, we cannot
> distinguish between kernel constructors and system constructors.
> 
> Thus, revert this commit.
> 
> Cc: stable@vger.kernel.org [5.4+]
> Fixes: 786b2384bf1c ("um: Enable CONFIG_CONSTRUCTORS")
> Reported-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   arch/um/include/asm/common.lds.S | 2 +-
>   arch/um/kernel/dyn.lds.S         | 1 +
>   init/Kconfig                     | 1 +
>   kernel/gcov/Kconfig              | 2 +-
>   4 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
> index d7086b985f27..4049f2c46387 100644
> --- a/arch/um/include/asm/common.lds.S
> +++ b/arch/um/include/asm/common.lds.S
> @@ -83,8 +83,8 @@
>   	__preinit_array_end = .;
>     }
>     .init_array : {
> -        /* dummy - we call this ourselves */
>   	__init_array_start = .;
> +	*(.init_array)
>   	__init_array_end = .;
>     }
>     .fini_array : {
> diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
> index c69d69ee96be..f5001481010c 100644
> --- a/arch/um/kernel/dyn.lds.S
> +++ b/arch/um/kernel/dyn.lds.S
> @@ -103,6 +103,7 @@ SECTIONS
>        be empty, which isn't pretty.  */
>     . = ALIGN(32 / 8);
>     .preinit_array     : { *(.preinit_array) }
> +  .init_array     : { *(.init_array) }
>     .fini_array     : { *(.fini_array) }
>     .data           : {
>       INIT_TASK_DATA(KERNEL_STACK_SIZE)
> diff --git a/init/Kconfig b/init/Kconfig
> index b4daad2bac23..0328b53d09ad 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -54,6 +54,7 @@ config CC_DISABLE_WARN_MAYBE_UNINITIALIZED
>   
>   config CONSTRUCTORS
>   	bool
> +	depends on !UML
>   
>   config IRQ_WORK
>   	bool
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 060e8e726755..3941a9c48f83 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling"
>   config GCOV_KERNEL
>   	bool "Enable gcov-based kernel profiling"
>   	depends on DEBUG_FS
> -	select CONSTRUCTORS
> +	select CONSTRUCTORS if !UML
>   	default n
>   	---help---
>   	This option enables gcov-based code profiling (e.g. for code coverage
> 

Acked-by: Anton Ivanov <anton.ivanov@cambridgegreys.co.uk>

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

Re: [PATCH] Revert "um: Enable CONFIG_CONSTRUCTORS"

[Richard Weinberger]

On Wed, Dec 4, 2019 at 7:35 PM Anton Ivanov
<anton.ivanov@cambridgegreys.com> wrote:
>
>
>
> On 04/12/2019 16:43, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > This reverts commit 786b2384bf1c ("um: Enable CONFIG_CONSTRUCTORS").
> >
> > There are two issues with this commit, uncovered by Anton in tests
> > on some (Debian) systems:
> >
> > 1) I completely forgot to call any constructors if CONFIG_CONSTRUCTORS
> >     isn't set. Don't recall now if it just wasn't needed on my system, or
> >     if I never tested this case.
> >
> > 2) With that fixed, it works - with CONFIG_CONSTRUCTORS *unset*. If I
> >     set CONFIG_CONSTRUCTORS, it fails again, which isn't totally
> >     unexpected since whatever wanted to run is likely to have to run
> >     before the kernel init etc. that calls the constructors in this case.
> >
> > Basically, some constructors that gcc emits (libc has?) need to run
> > very early during init; the failure mode otherwise was that the ptrace
> > fork test already failed:
> >
> > ----------------------
> > $ ./linux mem=512M
> > Core dump limits :
> >       soft - 0
> >       hard - NONE
> > Checking that ptrace can change system call numbers...check_ptrace : child exited with exitcode 6, while expecting 0; status 0x67f
> > Aborted
> > ----------------------
> >
> > Thinking more about this, it's clear that we simply cannot support
> > CONFIG_CONSTRUCTORS in UML. All the cases we need now (gcov, kasan)
> > involve not use of the __attribute__((constructor)), but instead
> > some constructor code/entry generated by gcc. Therefore, we cannot
> > distinguish between kernel constructors and system constructors.
> >
> > Thus, revert this commit.
> >
> > Cc: stable@vger.kernel.org [5.4+]
> > Fixes: 786b2384bf1c ("um: Enable CONFIG_CONSTRUCTORS")
> > Reported-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > ---
> >   arch/um/include/asm/common.lds.S | 2 +-
> >   arch/um/kernel/dyn.lds.S         | 1 +
> >   init/Kconfig                     | 1 +
> >   kernel/gcov/Kconfig              | 2 +-
> >   4 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
> > index d7086b985f27..4049f2c46387 100644
> > --- a/arch/um/include/asm/common.lds.S
> > +++ b/arch/um/include/asm/common.lds.S
> > @@ -83,8 +83,8 @@
> >       __preinit_array_end = .;
> >     }
> >     .init_array : {
> > -        /* dummy - we call this ourselves */
> >       __init_array_start = .;
> > +     *(.init_array)
> >       __init_array_end = .;
> >     }
> >     .fini_array : {
> > diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
> > index c69d69ee96be..f5001481010c 100644
> > --- a/arch/um/kernel/dyn.lds.S
> > +++ b/arch/um/kernel/dyn.lds.S
> > @@ -103,6 +103,7 @@ SECTIONS
> >        be empty, which isn't pretty.  */
> >     . = ALIGN(32 / 8);
> >     .preinit_array     : { *(.preinit_array) }
> > +  .init_array     : { *(.init_array) }
> >     .fini_array     : { *(.fini_array) }
> >     .data           : {
> >       INIT_TASK_DATA(KERNEL_STACK_SIZE)
> > diff --git a/init/Kconfig b/init/Kconfig
> > index b4daad2bac23..0328b53d09ad 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -54,6 +54,7 @@ config CC_DISABLE_WARN_MAYBE_UNINITIALIZED
> >
> >   config CONSTRUCTORS
> >       bool
> > +     depends on !UML
> >
> >   config IRQ_WORK
> >       bool
> > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> > index 060e8e726755..3941a9c48f83 100644
> > --- a/kernel/gcov/Kconfig
> > +++ b/kernel/gcov/Kconfig
> > @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling"
> >   config GCOV_KERNEL
> >       bool "Enable gcov-based kernel profiling"
> >       depends on DEBUG_FS
> > -     select CONSTRUCTORS
> > +     select CONSTRUCTORS if !UML
> >       default n
> >       ---help---
> >       This option enables gcov-based code profiling (e.g. for code coverage
> >
>
> Acked-by: Anton Ivanov <anton.ivanov@cambridgegreys.co.uk>

Applied. Thanks!

-- 
Thanks,
//richard