[PATCH] um: Fix kcov crash before kernel is started.

[PATCH] um: Fix kcov crash before kernel is started.

[Thomas Meyer]

UMLs current_thread_info() unconditionally assumes that the top of the stack
contains the thread_info structure. But on UML the __sanitizer_cov_trace_pc
function is called for *all* functions! This results in an early crash:

Prevent kcov from using invalid curent_thread_info() data by checking
the system_state.

Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
---
 kernel/kcov.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 3f693a0f6f3e..d601c0e956f6 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -56,6 +56,12 @@ void notrace __sanitizer_cov_trace_pc(void)
 	struct task_struct *t;
 	enum kcov_mode mode;
 
+#ifdef CONFIG_UML
+	if(!(system_state == SYSTEM_SCHEDULING ||
+	     system_state == SYSTEM_RUNNING))
+		return;
+#endif
+
 	t = current;
 	/*
 	 * We are interested in code coverage as a function of a syscall inputs,
-- 
2.11.0

Re: [PATCH] um: Fix kcov crash before kernel is started.

[Richard Weinberger]

Am Sonntag, 8. Oktober 2017, 12:31:58 CEST schrieb Thomas Meyer:
> UMLs current_thread_info() unconditionally assumes that the top of the stack
> contains the thread_info structure. But on UML the __sanitizer_cov_trace_pc
> function is called for *all* functions! This results in an early crash:
> 
> Prevent kcov from using invalid curent_thread_info() data by checking
> the system_state.
> 
> Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
> ---
>  kernel/kcov.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 3f693a0f6f3e..d601c0e956f6 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -56,6 +56,12 @@ void notrace __sanitizer_cov_trace_pc(void)
>  	struct task_struct *t;
>  	enum kcov_mode mode;
> 
> +#ifdef CONFIG_UML
> +	if(!(system_state == SYSTEM_SCHEDULING ||
> +	     system_state == SYSTEM_RUNNING))
> +		return;
> +#endif

Hmm, and why does it work on all other archs then?

Thanks,
//richard

Re: [PATCH] um: Fix kcov crash before kernel is started.

[Thomas Meyer]

On Sun, Oct 08, 2017 at 12:44:12PM +0200, Richard Weinberger wrote:
> Am Sonntag, 8. Oktober 2017, 12:31:58 CEST schrieb Thomas Meyer:
> > UMLs current_thread_info() unconditionally assumes that the top of the stack
> > contains the thread_info structure. But on UML the __sanitizer_cov_trace_pc
> > function is called for *all* functions! This results in an early crash:
> > 
> > Prevent kcov from using invalid curent_thread_info() data by checking
> > the system_state.
> > 
> > Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
> > ---
> >  kernel/kcov.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index 3f693a0f6f3e..d601c0e956f6 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -56,6 +56,12 @@ void notrace __sanitizer_cov_trace_pc(void)
> >  	struct task_struct *t;
> >  	enum kcov_mode mode;
> > 
> > +#ifdef CONFIG_UML
> > +	if(!(system_state == SYSTEM_SCHEDULING ||
> > +	     system_state == SYSTEM_RUNNING))
> > +		return;
> > +#endif
> 
> Hmm, and why does it work on all other archs then?

Hi,

I guess UML is different then other archs! But to be honest I'm not sure
why. I assume that __sanitizer_cov_trace_pc on other archs isn't called
that early, or that curent_thread_info returns NULL on other archs when
the first task isn't running yet.

But as I fail to use/setup the qemu gdb attachment to debug early x86_64 code
I can't say exactly why.

Maybe someone how knows the inner workings of x86_64 and/or kcov can
answer this question!

> 
> Thanks,
> //richard
> 

Re: [thomas@m3y3r.de: Re: [PATCH] um: Fix kcov crash before kernel is started.]

[Dmitry Vyukov]

On Mon, Oct 9, 2017 at 9:16 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
>> > Hi,
>> >
>> > are you able to shed light on this topic?
>> > Any help is greatly appreciated!
>> >
>> > With kind regards
>> > thomas
>> >
>> > Date: Sun, 8 Oct 2017 13:18:24 +0200
>> > From: Thomas Meyer <thomas@m3y3r.de>
>> > To: Richard Weinberger <richard@nod.at>
>> > Cc: user-mode-linux-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
>> > Subject: Re: [PATCH] um: Fix kcov crash before kernel is started.
>> > User-Agent: NeoMutt/20170113 (1.7.2)
>> >
>> > On Sun, Oct 08, 2017 at 12:44:12PM +0200, Richard Weinberger wrote:
>> >> Am Sonntag, 8. Oktober 2017, 12:31:58 CEST schrieb Thomas Meyer:
>> >> > UMLs current_thread_info() unconditionally assumes that the top of the stack
>> >> > contains the thread_info structure. But on UML the __sanitizer_cov_trace_pc
>> >> > function is called for *all* functions! This results in an early crash:
>> >> >
>> >> > Prevent kcov from using invalid curent_thread_info() data by checking
>> >> > the system_state.
>> >> >
>> >> > Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
>> >> > ---
>> >> >  kernel/kcov.c | 6 ++++++
>> >> >  1 file changed, 6 insertions(+)
>> >> >
>> >> > diff --git a/kernel/kcov.c b/kernel/kcov.c
>> >> > index 3f693a0f6f3e..d601c0e956f6 100644
>> >> > --- a/kernel/kcov.c
>> >> > +++ b/kernel/kcov.c
>> >> > @@ -56,6 +56,12 @@ void notrace __sanitizer_cov_trace_pc(void)
>> >> >     struct task_struct *t;
>> >> >     enum kcov_mode mode;
>> >> >
>> >> > +#ifdef CONFIG_UML
>> >> > +   if(!(system_state == SYSTEM_SCHEDULING ||
>> >> > +        system_state == SYSTEM_RUNNING))
>> >> > +           return;
>> >> > +#endif
>> >>
>> >> Hmm, and why does it work on all other archs then?
>> >
>> > Hi,
>> >
>> > I guess UML is different then other archs! But to be honest I'm not sure
>> > why. I assume that __sanitizer_cov_trace_pc on other archs isn't called
>> > that early, or that curent_thread_info returns NULL on other archs when
>> > the first task isn't running yet.
>> >
>> > But as I fail to use/setup the qemu gdb attachment to debug early x86_64 code
>> > I can't say exactly why.
>> >
>> > Maybe someone how knows the inner workings of x86_64 and/or kcov can
>> > answer this question!
>>
>>
>> Hi,
>
> Hi,
>
>> Yes, kcov can have some issues with early bootstrap code, because it
>> accesses current and it can also conflict with say, per-cpu setup code
>> (at least it was the case for x86). For x86 and arm64 we just bulk
>> blacklist instrumentation of arch code involved in early bootstrap.
>> See e.g. KCOV_INSTRUMENT in arch/x86/boot/Makefile. I think you need
>> to do the same for um. Start with bulk ignoring as much as possible
>> until you get it booting and then bisect back from there.
>
> oh, arch/um/* already contains the Makefile exception settings!
> I guess CONFIG_KCOV_INSTRUMENT_ALL overrides the the Makefile settings?
> Or doesn't it? I looked at scripts/Makefile.lib but failed to understand
> what config options has precedens in that case.


Then, I guess, boot code calls into some common instrumented code,
which gets into kcov and crashes.

This check helps, right?

+#ifdef CONFIG_UML
+   if(!(system_state == SYSTEM_SCHEDULING ||
+        system_state == SYSTEM_RUNNING))
+           return;
+#endif

Which means we somehow get here during boot. Is it possible to get a
stack trace for the return statement?

There is no common recipe. I think x86/arm64 are somewhat fragile in
this aspect as well, but somehow work. First of all we need to
understand how we get into the instrumentation callback during boot.

[PATCH] um: Fix kcov crash before kernel is started.

[Thomas Meyer]

UMLs current_thread_info() unconditionally assumes that the top of the stack
contains the thread_info structure.
Prevent kcov from using invalid curent_thread_info() data by disable
instrumentation of early startup code.

Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
---
 arch/um/kernel/skas/Makefile | 2 ++
 lib/Makefile                 | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/arch/um/kernel/skas/Makefile b/arch/um/kernel/skas/Makefile
index 0b76d8869c94..df3aedb974a2 100644
--- a/arch/um/kernel/skas/Makefile
+++ b/arch/um/kernel/skas/Makefile
@@ -3,6 +3,8 @@
 # Licensed under the GPL
 #
 
+KCOV_INSTRUMENT                := n
+
 obj-y := clone.o mmu.o process.o syscall.o uaccess.o
 
 # clone.o is in the stub, so it can't be built with profiling
diff --git a/lib/Makefile b/lib/Makefile
index dafa79613fb4..18319ad5daab 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -16,6 +16,10 @@ KCOV_INSTRUMENT_list_debug.o := n
 KCOV_INSTRUMENT_debugobjects.o := n
 KCOV_INSTRUMENT_dynamic_debug.o := n
 
+ifdef CONFIG_UML
+KCOV_INSTRUMENT_cmdline.o := n
+endif
+
 lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 rbtree.o radix-tree.o dump_stack.o timerqueue.o\
 	 idr.o int_sqrt.o extable.o \
-- 
2.11.0

Re: [PATCH] um: Fix kcov crash before kernel is started.

[Richard Weinberger]

Am Samstag, 14. Oktober 2017, 00:00:25 CEST schrieb Thomas Meyer:
> UMLs current_thread_info() unconditionally assumes that the top of the stack
> contains the thread_info structure.
> Prevent kcov from using invalid curent_thread_info() data by disable
> instrumentation of early startup code.
> 
> Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
> ---
>  arch/um/kernel/skas/Makefile | 2 ++
>  lib/Makefile                 | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/um/kernel/skas/Makefile b/arch/um/kernel/skas/Makefile
> index 0b76d8869c94..df3aedb974a2 100644
> --- a/arch/um/kernel/skas/Makefile
> +++ b/arch/um/kernel/skas/Makefile
> @@ -3,6 +3,8 @@
>  # Licensed under the GPL
>  #
> 
> +KCOV_INSTRUMENT                := n

So, you disable kconv for the whole SKAS code?
That's a bit broad. ;-\

>  obj-y := clone.o mmu.o process.o syscall.o uaccess.o
> 
>  # clone.o is in the stub, so it can't be built with profiling
> diff --git a/lib/Makefile b/lib/Makefile
> index dafa79613fb4..18319ad5daab 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -16,6 +16,10 @@ KCOV_INSTRUMENT_list_debug.o := n
>  KCOV_INSTRUMENT_debugobjects.o := n
>  KCOV_INSTRUMENT_dynamic_debug.o := n
> 
> +ifdef CONFIG_UML
> +KCOV_INSTRUMENT_cmdline.o := n
> +endif
> +

huh? Why do we need an exception for UML here?

Thanks,
//richard