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

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

[Dmitry Vyukov]

On Mon, Oct 9, 2017 at 6:47 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> ----- Forwarded message from Thomas Meyer <thomas@m3y3r.de> -----
>
> 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,

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.

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

[Thomas Meyer]

On Mon, Oct 09, 2017 at 08:10:45PM +0200, Dmitry Vyukov wrote:
> On Mon, Oct 9, 2017 at 6:47 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> > ----- Forwarded message from Thomas Meyer <thomas@m3y3r.de> -----
> >
> > 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.

greets
thomas 

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.

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

[Mark Rutland]

Hi,

On Tue, Oct 10, 2017 at 10:59:23AM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> On Mon, Oct 9, 2017 at 9:16 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
> >> > 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>

[...]

> >> 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.

Small info dump below. I *think* arm64 is mostly ok.

On arm64, our get_current() reads a system register that we setup in
early assembly with a pointer to our task struct. Our thread_info is
embedded in our task_struct.

That's setup in {primary,secondary}_switched, before we execute most C
code, including early init code like kasan_early_init and
kaslr_early_init, so it's safe to use current_thread_info() even in
those early bootstrap functions.

The only exception that I'm aware of is the EFI stub. However, that
isn't permitted to make calls to most kernel functions, and in its
makefile we (try to) enforce that it only calls into uninstrumented
position-independent functions. So any problems should be apparent at
build time.

There are a few special files (e.g. the out-of-line LL/SC atomics) which
we need to disable instrumentation for, which I intend to send patches
for at some point soon.

Thanks,
Mark.

[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

[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
>