All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML
@ 2021-01-19 11:18 Johannes Berg
  2021-01-20 16:07 ` Peter Oberparleiter
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Berg @ 2021-01-19 11:18 UTC (permalink / raw)
  To: linux-um
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu,
	Peter Oberparleiter, Johannes Berg

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

On ARCH=um, loading a module doesn't result in its constructors
getting called, which breaks module gcov since the debugfs files
are never registered. On the other hand, in-kernel constructors
have already been called by the dynamic linker, so we can't call
them again.

Get out of this conundrum by splitting CONFIG_CONSTRUCTORS into
CONFIG_CONSTRUCTORS_KERNEL and CONFIG_CONSTRUCTORS_MODULE, both
of which are enabled by default if CONFIG_CONSTRUCTORS is turned
on, but CONFIG_CONSTRUCTORS_KERNEL depends on !UML so that it's
not used on ARCH=um.

Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now,
since we really do want CONSTRUCTORS, just not kernel binary
ones.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Tested with a kernel configured with CONFIG_GCOV_KERNEL, without
the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart
from the reset file), and with it we get the files and they work.

I have no idea which tree this might go through, any suggestions?
---
 include/asm-generic/vmlinux.lds.h | 6 +++---
 include/linux/module.h            | 2 +-
 init/Kconfig                      | 9 ++++++++-
 init/main.c                       | 2 +-
 kernel/gcov/Kconfig               | 2 +-
 kernel/module.c                   | 4 ++--
 6 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b2b3d81b1535..87b300471c54 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -698,7 +698,7 @@
 		INIT_TASK_DATA(align)					\
 	}
 
-#ifdef CONFIG_CONSTRUCTORS
+#ifdef CONFIG_CONSTRUCTORS_KERNEL
 #define KERNEL_CTORS()	. = ALIGN(8);			   \
 			__ctors_start = .;		   \
 			KEEP(*(SORT(.ctors.*)))		   \
@@ -990,11 +990,11 @@
 /*
  * Clang's -fsanitize=kernel-address and -fsanitize=thread produce
  * unwanted sections (.eh_frame and .init_array.*), but
- * CONFIG_CONSTRUCTORS wants to keep any .init_array.* sections.
+ * CONFIG_CONSTRUCTORS_KERNEL wants to keep any .init_array.* sections.
  * https://bugs.llvm.org/show_bug.cgi?id=46478
  */
 #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN)
-# ifdef CONFIG_CONSTRUCTORS
+# ifdef CONFIG_CONSTRUCTORS_KERNEL
 #  define SANITIZER_DISCARDS						\
 	*(.eh_frame)
 # else
diff --git a/include/linux/module.h b/include/linux/module.h
index 7a0bcb5b1ffc..027cfdbd84bd 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -528,7 +528,7 @@ struct module {
 	atomic_t refcnt;
 #endif
 
-#ifdef CONFIG_CONSTRUCTORS
+#ifdef CONFIG_CONSTRUCTORS_MODULE
 	/* Constructor functions. */
 	ctor_fn_t *ctors;
 	unsigned int num_ctors;
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..e409de8d6c17 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -76,7 +76,14 @@ config CC_HAS_ASM_INLINE
 
 config CONSTRUCTORS
 	bool
-	depends on !UML
+
+config CONSTRUCTORS_KERNEL
+	def_bool y
+	depends on CONSTRUCTORS && !UML
+
+config CONSTRUCTORS_MODULE
+	def_bool y
+	depends on CONSTRUCTORS
 
 config IRQ_WORK
 	bool
diff --git a/init/main.c b/init/main.c
index c68d784376ca..51eb4802511c 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1066,7 +1066,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
 /* Call all constructor functions linked into the kernel. */
 static void __init do_ctors(void)
 {
-#ifdef CONFIG_CONSTRUCTORS
+#ifdef CONFIG_CONSTRUCTORS_KERNEL
 	ctor_fn_t *fn = (ctor_fn_t *) __ctors_start;
 
 	for (; fn < (ctor_fn_t *) __ctors_end; fn++)
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 3110c77230c7..f62de2dea8a3 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 if !UML
+	select CONSTRUCTORS
 	default n
 	help
 	This option enables gcov-based code profiling (e.g. for code coverage
diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..c161a360d929 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3257,7 +3257,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 					    &mod->num_unused_gpl_syms);
 	mod->unused_gpl_crcs = section_addr(info, "__kcrctab_unused_gpl");
 #endif
-#ifdef CONFIG_CONSTRUCTORS
+#ifdef CONFIG_CONSTRUCTORS_MODULE
 	mod->ctors = section_objs(info, ".ctors",
 				  sizeof(*mod->ctors), &mod->num_ctors);
 	if (!mod->ctors)
@@ -3612,7 +3612,7 @@ static bool finished_loading(const char *name)
 /* Call module constructors. */
 static void do_mod_ctors(struct module *mod)
 {
-#ifdef CONFIG_CONSTRUCTORS
+#ifdef CONFIG_CONSTRUCTORS_MODULE
 	unsigned long i;
 
 	for (i = 0; i < mod->num_ctors; i++)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML
  2021-01-19 11:18 [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML Johannes Berg
@ 2021-01-20 16:07 ` Peter Oberparleiter
  2021-01-20 16:09   ` Johannes Berg
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Oberparleiter @ 2021-01-20 16:07 UTC (permalink / raw)
  To: Johannes Berg, linux-um
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu,
	Johannes Berg, Andrew Morton

On 19.01.2021 12:18, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> On ARCH=um, loading a module doesn't result in its constructors
> getting called, which breaks module gcov since the debugfs files
> are never registered. On the other hand, in-kernel constructors
> have already been called by the dynamic linker, so we can't call
> them again.
> 
> Get out of this conundrum by splitting CONFIG_CONSTRUCTORS into
> CONFIG_CONSTRUCTORS_KERNEL and CONFIG_CONSTRUCTORS_MODULE, both
> of which are enabled by default if CONFIG_CONSTRUCTORS is turned
> on, but CONFIG_CONSTRUCTORS_KERNEL depends on !UML so that it's
> not used on ARCH=um.
> 
> Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now,
> since we really do want CONSTRUCTORS, just not kernel binary
> ones.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Do you expect other users for these new config symbols? If not it seems
to me that the goal of enabling module constructors for UML could also
be achieved without introducing new config symbols.

For example you could suppress calling any built-in kernel constructors
in case of UML by extending the ifdef in do_ctors() to depend on both
CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment).

Of course you'd still need to remove the !UML dependency in
CONFIG_GCOV_KERNEL.

> ---
> Tested with a kernel configured with CONFIG_GCOV_KERNEL, without
> the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart
> from the reset file), and with it we get the files and they work.
> 
> I have no idea which tree this might go through, any suggestions?

So far Andrew Morton was kind enough to pick up gcov-kernel related
changes, so that route might be an option.


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML
  2021-01-20 16:07 ` Peter Oberparleiter
@ 2021-01-20 16:09   ` Johannes Berg
  2021-01-20 16:20     ` Peter Oberparleiter
  2021-01-20 16:20     ` [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Johannes Berg
  0 siblings, 2 replies; 10+ messages in thread
From: Johannes Berg @ 2021-01-20 16:09 UTC (permalink / raw)
  To: Peter Oberparleiter, linux-um
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Andrew Morton

On Wed, 2021-01-20 at 17:07 +0100, Peter Oberparleiter wrote:

> Do you expect other users for these new config symbols? 

Probably not.

> If not it seems
> to me that the goal of enabling module constructors for UML could also
> be achieved without introducing new config symbols.

Yeah, true.

> For example you could suppress calling any built-in kernel constructors
> in case of UML by extending the ifdef in do_ctors() to depend on both
> CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment).
> 
> Of course you'd still need to remove the !UML dependency in
> CONFIG_GCOV_KERNEL.

Right.

I can post a separate patch and we can see which one looks nicer?

johannes



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML
  2021-01-20 16:09   ` Johannes Berg
@ 2021-01-20 16:20     ` Peter Oberparleiter
  2021-01-20 16:20     ` [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Johannes Berg
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Oberparleiter @ 2021-01-20 16:20 UTC (permalink / raw)
  To: Johannes Berg, linux-um
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Andrew Morton

On 20.01.2021 17:09, Johannes Berg wrote:
> On Wed, 2021-01-20 at 17:07 +0100, Peter Oberparleiter wrote:
> 
>> Do you expect other users for these new config symbols? 
> 
> Probably not.
> 
>> If not it seems
>> to me that the goal of enabling module constructors for UML could also
>> be achieved without introducing new config symbols.
> 
> Yeah, true.
> 
>> For example you could suppress calling any built-in kernel constructors
>> in case of UML by extending the ifdef in do_ctors() to depend on both
>> CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment).
>>
>> Of course you'd still need to remove the !UML dependency in
>> CONFIG_GCOV_KERNEL.
> 
> Right.
> 
> I can post a separate patch and we can see which one looks nicer?

Sounds good!


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov
  2021-01-20 16:09   ` Johannes Berg
  2021-01-20 16:20     ` Peter Oberparleiter
@ 2021-01-20 16:20     ` Johannes Berg
  2021-01-20 17:04       ` Peter Oberparleiter
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Berg @ 2021-01-20 16:20 UTC (permalink / raw)
  To: linux-um
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu,
	Peter Oberparleiter, Andrew Morton, Johannes Berg

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

On ARCH=um, loading a module doesn't result in its constructors
getting called, which breaks module gcov since the debugfs files
are never registered. On the other hand, in-kernel constructors
have already been called by the dynamic linker, so we can't call
them again.

Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be
selected, but avoiding the in-kernel constructor calls.

Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now,
since we really do want CONSTRUCTORS, just not kernel binary
ones.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Tested with a kernel configured with CONFIG_GCOV_KERNEL, without
the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart
from the reset file), and with it we get the files and they work.

v2:
 * special-case UML instead of splitting the CONSTRUCTORS config
---
 init/Kconfig        | 1 -
 init/main.c         | 8 +++++++-
 kernel/gcov/Kconfig | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..29ad68325028 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE
 
 config CONSTRUCTORS
 	bool
-	depends on !UML
 
 config IRQ_WORK
 	bool
diff --git a/init/main.c b/init/main.c
index c68d784376ca..a626e78dbf06 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
 /* Call all constructor functions linked into the kernel. */
 static void __init do_ctors(void)
 {
-#ifdef CONFIG_CONSTRUCTORS
+/*
+ * For UML, the constructors have already been called by the
+ * normal setup code as it's just a normal ELF binary, so we
+ * cannot do it again - but we do need CONFIG_CONSTRUCTORS
+ * even on UML for modules.
+ */
+#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML)
 	ctor_fn_t *fn = (ctor_fn_t *) __ctors_start;
 
 	for (; fn < (ctor_fn_t *) __ctors_end; fn++)
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 3110c77230c7..f62de2dea8a3 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 if !UML
+	select CONSTRUCTORS
 	default n
 	help
 	This option enables gcov-based code profiling (e.g. for code coverage
-- 
2.26.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov
  2021-01-20 16:20     ` [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Johannes Berg
@ 2021-01-20 17:04       ` Peter Oberparleiter
  2021-01-20 17:38         ` Johannes Berg
       [not found]         ` <tencent_99073B61C8137C88B76C231139F94EFB3805@qq.com>
  0 siblings, 2 replies; 10+ messages in thread
From: Peter Oberparleiter @ 2021-01-20 17:04 UTC (permalink / raw)
  To: Johannes Berg, Andrew Morton
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu,
	Johannes Berg, linux-um

On 20.01.2021 17:20, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> On ARCH=um, loading a module doesn't result in its constructors
> getting called, which breaks module gcov since the debugfs files
> are never registered. On the other hand, in-kernel constructors
> have already been called by the dynamic linker, so we can't call
> them again.
> 
> Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be
> selected, but avoiding the in-kernel constructor calls.
> 
> Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now,
> since we really do want CONSTRUCTORS, just not kernel binary
> ones.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Looks good+nicer than v1 to me!

I also tested this patch on s390 and can confirm that it doesn't break
gcov-kernel there.

Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>

Andrew, do we need additional reviews/sign-offs? Could this go in via
your tree?

> ---
> Tested with a kernel configured with CONFIG_GCOV_KERNEL, without
> the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart
> from the reset file), and with it we get the files and they work.

Just to be sure: could you confirm that this test statement for UML also
applies to v2, i.e. that it fixes the original problem you were seeing?

> 
> v2:
>  * special-case UML instead of splitting the CONSTRUCTORS config
> ---
>  init/Kconfig        | 1 -
>  init/main.c         | 8 +++++++-
>  kernel/gcov/Kconfig | 2 +-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index b77c60f8b963..29ad68325028 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE
>  
>  config CONSTRUCTORS
>  	bool
> -	depends on !UML
>  
>  config IRQ_WORK
>  	bool
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..a626e78dbf06 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>  /* Call all constructor functions linked into the kernel. */
>  static void __init do_ctors(void)
>  {
> -#ifdef CONFIG_CONSTRUCTORS
> +/*
> + * For UML, the constructors have already been called by the
> + * normal setup code as it's just a normal ELF binary, so we
> + * cannot do it again - but we do need CONFIG_CONSTRUCTORS
> + * even on UML for modules.
> + */
> +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML)
>  	ctor_fn_t *fn = (ctor_fn_t *) __ctors_start;
>  
>  	for (; fn < (ctor_fn_t *) __ctors_end; fn++)
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 3110c77230c7..f62de2dea8a3 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 if !UML
> +	select CONSTRUCTORS
>  	default n
>  	help
>  	This option enables gcov-based code profiling (e.g. for code coverage
> 


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov
  2021-01-20 17:04       ` Peter Oberparleiter
@ 2021-01-20 17:38         ` Johannes Berg
       [not found]         ` <tencent_99073B61C8137C88B76C231139F94EFB3805@qq.com>
  1 sibling, 0 replies; 10+ messages in thread
From: Johannes Berg @ 2021-01-20 17:38 UTC (permalink / raw)
  To: Peter Oberparleiter, Andrew Morton
  Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, linux-um

On Wed, 2021-01-20 at 18:04 +0100, Peter Oberparleiter wrote:
> 
> > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without
> > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart
> > from the reset file), and with it we get the files and they work.
> 
> Just to be sure: could you confirm that this test statement for UML also
> applies to v2, i.e. that it fixes the original problem you were seeing?

Yes, I tested this version too :)

johannes


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov
       [not found]         ` <tencent_99073B61C8137C88B76C231139F94EFB3805@qq.com>
@ 2021-05-10 11:37           ` Johannes Berg
  2021-05-10 13:31             ` lambertdev
  2021-05-14 13:55           ` Peter Oberparleiter
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Berg @ 2021-05-10 11:37 UTC (permalink / raw)
  To: Lambert, Peter Oberparleiter
  Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann,
	Jessica Yu, linux-um, Lambert.

Hi,

> Hi Johannes and Peter, sorry to bother but I have one question 
> on this change. The do_ctors() won’t be executed for UML 
> because  *the constructors have already been called for ELF*. 
> 
> *__ctors_start*  and  *__ctors_end* symbols. See link:
> https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676
> 
> In my environment, UML+GCC 10, I can't find __gcov_init executed 
> before kernel starts. So I did some trace and found glibc
> __libc_csu_init 
> will only execute constructors between *__init_array_start*and
> *__init_array_end*.  
> Which means if do_ctors() is not executed for UML, no elsewhere will 
> the constructors be executed.
> 
> Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed 
> some steps to make the GCOV work for UML? 

No, that doesn't seem like the right solution.

Perhaps then with that toolchain (or configuration thereof) we need to
provide __init_array_start/end labels?

Or ... maybe that actually just needs to be removed, so that the
toolchain gets to choose?

Hmm. Pretty sure it worked for me, I think also with gcc 10, but not
sure exactly where I tested.

johannes



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov
  2021-05-10 11:37           ` Johannes Berg
@ 2021-05-10 13:31             ` lambertdev
  0 siblings, 0 replies; 10+ messages in thread
From: lambertdev @ 2021-05-10 13:31 UTC (permalink / raw)
  To: Johannes Berg, Peter Oberparleiter
  Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann,
	Jessica Yu, linux-um

Hi Johannes,

My original Email address is blocked by the server of kernel.org, 
so I have to change an Email address. Please see my reply inline.

>Hi,


>



>> Hi Johannes and Peter, sorry to bother but I have one question 



>> on this change. The do_ctors() won’t be executed for UML 



>> because  *the constructors have already been called for ELF*. 



>> 



>> *__ctors_start*  and  *__ctors_end* symbols. See link:



>> https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676



>> 



>> In my environment, UML+GCC 10, I can't find __gcov_init executed 



>> before kernel starts. So I did some trace and found glibc



>> __libc_csu_init 



>> will only execute constructors between *__init_array_start*and



>> *__init_array_end*.  



>> Which means if do_ctors() is not executed for UML, no elsewhere will 



>> the constructors be executed.



>> 



>> Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed 



>> some steps to make the GCOV work for UML? 



>



>No, that doesn't seem like the right solution.


>



>Perhaps then with that toolchain (or configuration thereof) we need to



>provide __init_array_start/end labels?


Yes, that's how I worked around  in my local environment 
(change linker script to add _init_array_start/end labels).
So the __gcov_init is called before start_kernel.


>



>Or ... maybe that actually just needs to be removed, so that the



>toolchain gets to choose?



>



>Hmm. Pretty sure it worked for me, I think also with gcc 10, but not



>sure exactly where I tested.


I will make sure my environment is clean and try again. 
Could you please use gdb to check when __gcov_init is called in your setup?
e.g.
(1) gdb *your uml elf* 
(2) b __gcov_init
(3) r
(4) when the execution is breaked, use 'bt' to check the call stack
Thanks

Lambert

>



>johannes



>



>



>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov
       [not found]         ` <tencent_99073B61C8137C88B76C231139F94EFB3805@qq.com>
  2021-05-10 11:37           ` Johannes Berg
@ 2021-05-14 13:55           ` Peter Oberparleiter
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Oberparleiter @ 2021-05-14 13:55 UTC (permalink / raw)
  To: Lambert, Johannes Berg, lambertdev, Jeff Dike, richard, anton.ivanov
  Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann,
	Jessica Yu, Johannes Berg, linux-um, Lambert.

I'm adding UML maintainers for their input.

On 08.05.2021 15:50, Lambert wrote:
>> On Jan 21, 2021, at 1:04 AM, Peter Oberparleiter <oberpar@linux.ibm.com> wrote:
>> On 20.01.2021 17:20, Johannes Berg wrote:
>>> From: Johannes Berg <johannes.berg@intel.com>
>>>
>>> On ARCH=um, loading a module doesn't result in its constructors
>>> getting called, which breaks module gcov since the debugfs files
>>> are never registered. On the other hand, in-kernel constructors
>>> have already been called by the dynamic linker, so we can't call
>>> them again.
>>>
>>> Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be
>>> selected, but avoiding the in-kernel constructor calls.
>>>
>>> Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now,
>>> since we really do want CONSTRUCTORS, just not kernel binary
>>> ones.
>>>
>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>
>> Looks good+nicer than v1 to me!
>>
>> I also tested this patch on s390 and can confirm that it doesn't break
>> gcov-kernel there.
>>
>> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
>>
>> Andrew, do we need additional reviews/sign-offs? Could this go in via
>> your tree?
>>
>>> ---
>>> Tested with a kernel configured with CONFIG_GCOV_KERNEL, without
>>> the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart
>>> from the reset file), and with it we get the files and they work.
>>
>> Just to be sure: could you confirm that this test statement for UML also
>> applies to v2, i.e. that it fixes the original problem you were seeing?
>>
> 
> Hi Johannes and Peter, sorry to bother but I have one question 
> on this change. The do_ctors() won’t be executed for UML 
> because  *the constructors have already been called for ELF*. 
> 
> The init_array section for each C file is linked to *vmlinux*  between 
> *__ctors_start*  and  *__ctors_end* symbols. See link:
> https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676
> 
> In my environment, UML+GCC 10, I can't find __gcov_init executed 
> before kernel starts. So I did some trace and found glibc __libc_csu_init 
> will only execute constructors between *__init_array_start*and *__init_array_end*.  
> Which means if do_ctors() is not executed for UML, no elsewhere will 
> the constructors be executed.
> 
> Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed 
> some steps to make the GCOV work for UML?

I'm not an expert for the UML-arch, but based on this report I did some
investigations. I think that this patch might be partially in error.

Before the patch no constructors were called for UML and as a result
gcov-kernel profiling was not available, neither for built-in code nor
for module code.

With the patch CONFIG_CONSTRUCTORS was made available on UML and with it
gcov-kernel profiling. This works well for kernel modules (which I
assume is what Johannes tested), but it fails for built-in code. Note
that since UML does not specify ARCH_HAS_GCOV_PROFILE_ALL you need to
manually add GCOV_PROFILE := y to any kernel Makefile if you want to
enable profiling for that built-in code.

The subject patch is based on the assumption that constructors for
built-in kernel code is called from glibc when the vmlinux executable is
started. This does not work the way it was assumed it would, and would
be broken if it did.

It does not work because, as was already pointed out, glibc is looking
for the __init_array_start symbol to locate the list of constructor
function pointers, but the kernel uses a different symbol name
(__ctors_start). If you add the __init_array_start and _end symbols
glibc will call these functions correctly, but the kernel will crash.

The reason for the crash is that the kernel implementation of
__gcov_init() which is called from the constructors emitted by GCC's
gcov-profiling code assumes that base kernel facilities (such as printk)
have been initialized and are working as expected. Of course this is not
the case when the function is called by glibc before any kernel init
function was run.

I see multiple ways to go forward:

0. Do nothing

This patch doesn't break anything - it's just only partially effective
in that in enables gcov-kernel profiling for modules, but not for
built-in code.


1. Revert this patch

=> gcov-kernel profiling for kernel modules on UML would no longer work


2. Enable do_ctors() on UML

=> gcov-kernel profiling would work on UML for both kernel modules and
built-in code. There's a risk though that other constructors unrelated
to gcov might be called too late (I saw at least one
register_tm_clones() call while looking at the .init_array section of an
on UML executable). This risk is specific to UML code that runs before
start_kernel(). I'm not sure if there is anything there that consciously
relies on constructors.


3. Change gcov_init() to work without relying on kernel facilities and
add init_array_start and _end symbols to KERNEL_CTORS in
include/asm-generic/vmlinux.lds.h so that glibc can find and run
constructors before start_kernel(). Not sure if this is easily possible.
Certainly this is the option that results in the most amount of work.

=> gcov-kernel profiling would work on UML, any other constructors for
code run before start_kernel() would work, but this would also affect
other kernel code that relies on constructors (currently KASAN)... so
might also have further unwanted side-effects.


Any thoughts, comments or ideas for other alternatives?

> 
>>>
>>> v2:
>>> * special-case UML instead of splitting the CONSTRUCTORS config
>>> ---
>>> init/Kconfig        | 1 -
>>> init/main.c         | 8 +++++++-
>>> kernel/gcov/Kconfig | 2 +-
>>> 3 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/init/Kconfig b/init/Kconfig
>>> index b77c60f8b963..29ad68325028 100644
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE
>>>
>>> config CONSTRUCTORS
>>> 	bool
>>> -	depends on !UML
>>>
>>> config IRQ_WORK
>>> 	bool
>>> diff --git a/init/main.c b/init/main.c
>>> index c68d784376ca..a626e78dbf06 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>>> /* Call all constructor functions linked into the kernel. */
>>> static void __init do_ctors(void)
>>> {
>>> -#ifdef CONFIG_CONSTRUCTORS
>>> +/*
>>> + * For UML, the constructors have already been called by the
>>> + * normal setup code as it's just a normal ELF binary, so we
>>> + * cannot do it again - but we do need CONFIG_CONSTRUCTORS
>>> + * even on UML for modules.
>>> + */
>>> +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML)
>>> 	ctor_fn_t *fn = (ctor_fn_t *) __ctors_start;
>>>
>>> 	for (; fn < (ctor_fn_t *) __ctors_end; fn++)
>>> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
>>> index 3110c77230c7..f62de2dea8a3 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 if !UML
>>> +	select CONSTRUCTORS
>>> 	default n
>>> 	help
>>> 	This option enables gcov-based code profiling (e.g. for code coverage
>>>
>>
>>
>> -- 
>> Peter Oberparleiter
>> Linux on Z Development - IBM Germany


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-05-14 13:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 11:18 [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML Johannes Berg
2021-01-20 16:07 ` Peter Oberparleiter
2021-01-20 16:09   ` Johannes Berg
2021-01-20 16:20     ` Peter Oberparleiter
2021-01-20 16:20     ` [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Johannes Berg
2021-01-20 17:04       ` Peter Oberparleiter
2021-01-20 17:38         ` Johannes Berg
     [not found]         ` <tencent_99073B61C8137C88B76C231139F94EFB3805@qq.com>
2021-05-10 11:37           ` Johannes Berg
2021-05-10 13:31             ` lambertdev
2021-05-14 13:55           ` Peter Oberparleiter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.