| Message ID | 20191204174346.78dfd358bd15.I19e7eb2601fbdc0270fb1e1b647a75301e9e4503@changeid |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Revert "um: Enable CONFIG_CONSTRUCTORS" | expand |
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>
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!
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