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

[Anton Ivanov <anton.ivanov@cambridgegreys.com>]

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/