[PATCH -tip] kcov: Make runtime functions noinstr-compatible

[PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Marco Elver]

The KCOV runtime is very minimal, only updating a field in 'current',
and none of __sanitizer_cov-functions generates reports nor calls any
other external functions.

Therefore we can make the KCOV runtime noinstr-compatible by:

  1. always-inlining internal functions and marking
     __sanitizer_cov-functions noinstr. The function write_comp_data() is
     now guaranteed to be inlined into __sanitize_cov_trace_*cmp()
     functions, which saves a call in the fast-path and reduces stack
     pressure due to the first argument being a constant.

  2. For Clang, correctly pass -fno-stack-protector via a separate
     cc-option, as -fno-conserve-stack does not exist on Clang.

The major benefit compared to adding another attribute to 'noinstr' to
not collect coverage information, is that we retain coverage visibility
in noinstr functions. We also currently lack such an attribute in both
GCC and Clang.

Signed-off-by: Marco Elver <elver@google.com>
---
Note: There are a set of KCOV patches from Andrey in -next:
https://lkml.kernel.org/r/cover.1585233617.git.andreyknvl@google.com --
Git cleanly merges this patch with those patches, and no merge conflict
is expected.
---
 kernel/Makefile |  2 +-
 kernel/kcov.c   | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/kernel/Makefile b/kernel/Makefile
index 5d935b63f812..8e282c611a72 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -35,7 +35,7 @@ KCOV_INSTRUMENT_stacktrace.o := n
 KCOV_INSTRUMENT_kcov.o := n
 KASAN_SANITIZE_kcov.o := n
 KCSAN_SANITIZE_kcov.o := n
-CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
+CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack) $(call cc-option, -fno-stack-protector)
 
 # cond_syscall is currently not LTO compatible
 CFLAGS_sys_ni.o = $(DISABLE_LTO)
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 8accc9722a81..d6e3be2d0570 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -142,7 +142,7 @@ static void kcov_remote_area_put(struct kcov_remote_area *area,
 	list_add(&area->list, &kcov_remote_areas);
 }
 
-static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
+static __always_inline bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
 {
 	unsigned int mode;
 
@@ -164,7 +164,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
 	return mode == needed_mode;
 }
 
-static notrace unsigned long canonicalize_ip(unsigned long ip)
+static __always_inline unsigned long canonicalize_ip(unsigned long ip)
 {
 #ifdef CONFIG_RANDOMIZE_BASE
 	ip -= kaslr_offset();
@@ -176,7 +176,7 @@ static notrace unsigned long canonicalize_ip(unsigned long ip)
  * Entry point from instrumented code.
  * This is called once per basic-block/edge.
  */
-void notrace __sanitizer_cov_trace_pc(void)
+void noinstr __sanitizer_cov_trace_pc(void)
 {
 	struct task_struct *t;
 	unsigned long *area;
@@ -198,7 +198,7 @@ void notrace __sanitizer_cov_trace_pc(void)
 EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
 
 #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
-static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
+static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
 {
 	struct task_struct *t;
 	u64 *area;
@@ -231,59 +231,59 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
 	}
 }
 
-void notrace __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
+void noinstr __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(0), arg1, arg2, _RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_cmp1);
 
-void notrace __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
+void noinstr __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(1), arg1, arg2, _RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_cmp2);
 
-void notrace __sanitizer_cov_trace_cmp4(u32 arg1, u32 arg2)
+void noinstr __sanitizer_cov_trace_cmp4(u32 arg1, u32 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(2), arg1, arg2, _RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_cmp4);
 
-void notrace __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
+void noinstr __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(3), arg1, arg2, _RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_cmp8);
 
-void notrace __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
+void noinstr __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(0) | KCOV_CMP_CONST, arg1, arg2,
 			_RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp1);
 
-void notrace __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
+void noinstr __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(1) | KCOV_CMP_CONST, arg1, arg2,
 			_RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp2);
 
-void notrace __sanitizer_cov_trace_const_cmp4(u32 arg1, u32 arg2)
+void noinstr __sanitizer_cov_trace_const_cmp4(u32 arg1, u32 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(2) | KCOV_CMP_CONST, arg1, arg2,
 			_RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp4);
 
-void notrace __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
+void noinstr __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
 {
 	write_comp_data(KCOV_CMP_SIZE(3) | KCOV_CMP_CONST, arg1, arg2,
 			_RET_IP_);
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp8);
 
-void notrace __sanitizer_cov_trace_switch(u64 val, u64 *cases)
+void noinstr __sanitizer_cov_trace_switch(u64 val, u64 *cases)
 {
 	u64 i;
 	u64 count = cases[0];
-- 
2.27.0.rc2.251.g90737beb825-goog

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Peter Zijlstra]

On Thu, Jun 04, 2020 at 11:50:57AM +0200, Marco Elver wrote:
> The KCOV runtime is very minimal, only updating a field in 'current',
> and none of __sanitizer_cov-functions generates reports nor calls any
> other external functions.

Not quite true; it writes to t->kcov_area, and we need to make
absolutely sure that doesn't take faults or triggers anything else
untowards.

> Therefore we can make the KCOV runtime noinstr-compatible by:
> 
>   1. always-inlining internal functions and marking
>      __sanitizer_cov-functions noinstr. The function write_comp_data() is
>      now guaranteed to be inlined into __sanitize_cov_trace_*cmp()
>      functions, which saves a call in the fast-path and reduces stack
>      pressure due to the first argument being a constant.
> 
>   2. For Clang, correctly pass -fno-stack-protector via a separate
>      cc-option, as -fno-conserve-stack does not exist on Clang.
> 
> The major benefit compared to adding another attribute to 'noinstr' to
> not collect coverage information, is that we retain coverage visibility
> in noinstr functions. We also currently lack such an attribute in both
> GCC and Clang.
> 

> -static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> +static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
>  {
>  	struct task_struct *t;
>  	u64 *area;
> @@ -231,59 +231,59 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
>  	}
>  }

This thing; that appears to be the meat of it, right?

I can't find where t->kcov_area comes from.. is that always
kcov_mmap()'s vmalloc_user() ?

That whole kcov_remote stuff confuses me.

KCOV_ENABLE() has kcov_fault_in_area(), which supposedly takes the
vmalloc faults for the current task, but who does it for the remote?

Now, luckily Joerg went and ripped out the vmalloc faults, let me check
where those patches are... w00t, they're upstream in this merge window.

So no #PF from writing to t->kcov_area then, under the assumption that
the vmalloc_user() is the only allocation site.

But then there's hardware watchpoints, if someone goes and sets a data
watchpoint in the kcov_area we're screwed. Nothing actively prevents
that from happening. Then again, the same is currently true for much of
current :/

Also, I think you need __always_inline on kaslr_offset()


And, unrelated to this patch in specific, I suppose I'm going to have to
extend objtool to look for data that is used from noinstr, to make sure
we exclude it from inspection and stuff, like that kaslr offset crud for
example.

Anyway, yes, it appears you're lucky (for having Joerg remove vmalloc
faults) and this mostly should work as is.

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Marco Elver]

On Thu, 4 Jun 2020 at 13:09, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 04, 2020 at 11:50:57AM +0200, Marco Elver wrote:
> > The KCOV runtime is very minimal, only updating a field in 'current',
> > and none of __sanitizer_cov-functions generates reports nor calls any
> > other external functions.
>
> Not quite true; it writes to t->kcov_area, and we need to make
> absolutely sure that doesn't take faults or triggers anything else
> untowards.

Ah, right.

> > Therefore we can make the KCOV runtime noinstr-compatible by:
> >
> >   1. always-inlining internal functions and marking
> >      __sanitizer_cov-functions noinstr. The function write_comp_data() is
> >      now guaranteed to be inlined into __sanitize_cov_trace_*cmp()
> >      functions, which saves a call in the fast-path and reduces stack
> >      pressure due to the first argument being a constant.
> >
> >   2. For Clang, correctly pass -fno-stack-protector via a separate
> >      cc-option, as -fno-conserve-stack does not exist on Clang.
> >
> > The major benefit compared to adding another attribute to 'noinstr' to
> > not collect coverage information, is that we retain coverage visibility
> > in noinstr functions. We also currently lack such an attribute in both
> > GCC and Clang.
> >
>
> > -static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> > +static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> >  {
> >       struct task_struct *t;
> >       u64 *area;
> > @@ -231,59 +231,59 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> >       }
> >  }
>
> This thing; that appears to be the meat of it, right?
>
> I can't find where t->kcov_area comes from.. is that always
> kcov_mmap()'s vmalloc_user() ?

Yeah, looks like it.

> That whole kcov_remote stuff confuses me.
>
> KCOV_ENABLE() has kcov_fault_in_area(), which supposedly takes the
> vmalloc faults for the current task, but who does it for the remote?
>
> Now, luckily Joerg went and ripped out the vmalloc faults, let me check
> where those patches are... w00t, they're upstream in this merge window.
>
> So no #PF from writing to t->kcov_area then, under the assumption that
> the vmalloc_user() is the only allocation site.
>
> But then there's hardware watchpoints, if someone goes and sets a data
> watchpoint in the kcov_area we're screwed. Nothing actively prevents
> that from happening. Then again, the same is currently true for much of
> current :/
>
> Also, I think you need __always_inline on kaslr_offset()
>
>
> And, unrelated to this patch in specific, I suppose I'm going to have to
> extend objtool to look for data that is used from noinstr, to make sure
> we exclude it from inspection and stuff, like that kaslr offset crud for
> example.
>
> Anyway, yes, it appears you're lucky (for having Joerg remove vmalloc
> faults) and this mostly should work as is.

Hmm, looks like this doesn't generally work then. :-/

An alternative would be to check if '__noinstr_text_start <= _RET_IP_
< __noinstr_text_end' in __sanitizer_cov-functions and return if
that's the case. This could be #ifdef'd when we get a compiler that
can do __no_sanitize_coverage. At least that way we get working KCOV
for now.

Would that work?

Thanks,
-- Marco

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Andrey Konovalov]

On Thu, Jun 4, 2020 at 1:09 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 04, 2020 at 11:50:57AM +0200, Marco Elver wrote:
> > The KCOV runtime is very minimal, only updating a field in 'current',
> > and none of __sanitizer_cov-functions generates reports nor calls any
> > other external functions.
>
> Not quite true; it writes to t->kcov_area, and we need to make
> absolutely sure that doesn't take faults or triggers anything else
> untowards.
>
> > Therefore we can make the KCOV runtime noinstr-compatible by:
> >
> >   1. always-inlining internal functions and marking
> >      __sanitizer_cov-functions noinstr. The function write_comp_data() is
> >      now guaranteed to be inlined into __sanitize_cov_trace_*cmp()
> >      functions, which saves a call in the fast-path and reduces stack
> >      pressure due to the first argument being a constant.

Maybe we could do CFLAGS_REMOVE_kcov.o = $(CC_FLAGS_FTRACE) the same
way we do it for KASAN? And drop notrace/noinstr from kcov. Would it
resolve the issue? I'm not sure which solution is better though.

> >
> >   2. For Clang, correctly pass -fno-stack-protector via a separate
> >      cc-option, as -fno-conserve-stack does not exist on Clang.
> >
> > The major benefit compared to adding another attribute to 'noinstr' to
> > not collect coverage information, is that we retain coverage visibility
> > in noinstr functions. We also currently lack such an attribute in both
> > GCC and Clang.
> >
>
> > -static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> > +static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> >  {
> >       struct task_struct *t;
> >       u64 *area;
> > @@ -231,59 +231,59 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> >       }
> >  }
>
> This thing; that appears to be the meat of it, right?
>
> I can't find where t->kcov_area comes from.. is that always
> kcov_mmap()'s vmalloc_user() ?
>
> That whole kcov_remote stuff confuses me.
>
> KCOV_ENABLE() has kcov_fault_in_area(), which supposedly takes the
> vmalloc faults for the current task, but who does it for the remote?

Hm, no one. This might be an issue, thanks for noticing!

> Now, luckily Joerg went and ripped out the vmalloc faults, let me check
> where those patches are... w00t, they're upstream in this merge window.

Could you point me to those patches?

Even though it might work fine now, we might get issues if we backport
remote kcov to older kernels.

>
> So no #PF from writing to t->kcov_area then, under the assumption that
> the vmalloc_user() is the only allocation site.
>
> But then there's hardware watchpoints, if someone goes and sets a data
> watchpoint in the kcov_area we're screwed. Nothing actively prevents
> that from happening. Then again, the same is currently true for much of
> current :/
>
> Also, I think you need __always_inline on kaslr_offset()
>
>
> And, unrelated to this patch in specific, I suppose I'm going to have to
> extend objtool to look for data that is used from noinstr, to make sure
> we exclude it from inspection and stuff, like that kaslr offset crud for
> example.
>
> Anyway, yes, it appears you're lucky (for having Joerg remove vmalloc
> faults) and this mostly should work as is.

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Marco Elver]

On Thu, 4 Jun 2020 at 16:03, Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Thu, Jun 4, 2020 at 1:09 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Thu, Jun 04, 2020 at 11:50:57AM +0200, Marco Elver wrote:
> > > The KCOV runtime is very minimal, only updating a field in 'current',
> > > and none of __sanitizer_cov-functions generates reports nor calls any
> > > other external functions.
> >
> > Not quite true; it writes to t->kcov_area, and we need to make
> > absolutely sure that doesn't take faults or triggers anything else
> > untowards.
> >
> > > Therefore we can make the KCOV runtime noinstr-compatible by:
> > >
> > >   1. always-inlining internal functions and marking
> > >      __sanitizer_cov-functions noinstr. The function write_comp_data() is
> > >      now guaranteed to be inlined into __sanitize_cov_trace_*cmp()
> > >      functions, which saves a call in the fast-path and reduces stack
> > >      pressure due to the first argument being a constant.
>
> Maybe we could do CFLAGS_REMOVE_kcov.o = $(CC_FLAGS_FTRACE) the same
> way we do it for KASAN? And drop notrace/noinstr from kcov. Would it
> resolve the issue? I'm not sure which solution is better though.

Sadly no. 'noinstr' implies 'notrace', but also places the function in
the .noinstr.text section for the purpose of objtool checking. But: we
should only mark a function 'noinstr' if it (and its callees)
satisfies the requirements that Peter outlined (are the requirements
documented somewhere?). In particular, we need to worry about vmalloc
faults.

[...]
> > > -static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> > > +static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> > >  {
> > >       struct task_struct *t;
> > >       u64 *area;
> > > @@ -231,59 +231,59 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> > >       }
> > >  }
> >
> > This thing; that appears to be the meat of it, right?
> >
> > I can't find where t->kcov_area comes from.. is that always
> > kcov_mmap()'s vmalloc_user() ?
> >
> > That whole kcov_remote stuff confuses me.
> >
> > KCOV_ENABLE() has kcov_fault_in_area(), which supposedly takes the
> > vmalloc faults for the current task, but who does it for the remote?
>
> Hm, no one. This might be an issue, thanks for noticing!
>
> > Now, luckily Joerg went and ripped out the vmalloc faults, let me check
> > where those patches are... w00t, they're upstream in this merge window.
>
> Could you point me to those patches?
>
> Even though it might work fine now, we might get issues if we backport
> remote kcov to older kernels.
>
> >
> > So no #PF from writing to t->kcov_area then, under the assumption that
> > the vmalloc_user() is the only allocation site.
> >
> > But then there's hardware watchpoints, if someone goes and sets a data
> > watchpoint in the kcov_area we're screwed. Nothing actively prevents
> > that from happening. Then again, the same is currently true for much of
> > current :/
> >
> > Also, I think you need __always_inline on kaslr_offset()
> >
> >
> > And, unrelated to this patch in specific, I suppose I'm going to have to
> > extend objtool to look for data that is used from noinstr, to make sure
> > we exclude it from inspection and stuff, like that kaslr offset crud for
> > example.
> >
> > Anyway, yes, it appears you're lucky (for having Joerg remove vmalloc
> > faults) and this mostly should work as is.

Now I am a bit worried that, even though we're lucky today, with what
Andrey said about e.g. kcov_remote faults, it'll be hard to ensure we
won't break in future. The exact set of conditions that mean we're
lucky today may change and we have no way of checking this.

I'll try to roll a v2 based on the "if (_RET_IP_ in noinstr section)
return;" and whitelist in objtool approach. Unless you see something
very wrong with that. And I do hope we'll get compiler attributes
eventually.

Thanks,
-- Marco

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Peter Zijlstra]

On Thu, Jun 04, 2020 at 04:23:38PM +0200, Marco Elver wrote:
> Sadly no. 'noinstr' implies 'notrace', but also places the function in
> the .noinstr.text section for the purpose of objtool checking.

Not only the compile time checking, but also for purpose of runtime
exclusion for things like kprobes and hw-breakpoints.

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Peter Zijlstra]

On Thu, Jun 04, 2020 at 04:02:54PM +0200, Andrey Konovalov wrote:

> > Now, luckily Joerg went and ripped out the vmalloc faults, let me check
> > where those patches are... w00t, they're upstream in this merge window.
> 
> Could you point me to those patches?

git log 7f0a002b5a21302d9f4b29ba83c96cd433ff3769...d8626138009ba58ae2c22356966c2edaa1f1c3b5

Re: [PATCH -tip] kcov: Make runtime functions noinstr-compatible

[Peter Zijlstra]

On Thu, Jun 04, 2020 at 04:02:54PM +0200, Andrey Konovalov wrote:
> On Thu, Jun 4, 2020 at 1:09 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > That whole kcov_remote stuff confuses me.
> >
> > KCOV_ENABLE() has kcov_fault_in_area(), which supposedly takes the
> > vmalloc faults for the current task, but who does it for the remote?
> 
> Hm, no one. This might be an issue, thanks for noticing!
> 
> > Now, luckily Joerg went and ripped out the vmalloc faults, let me check
> > where those patches are... w00t, they're upstream in this merge window.
> 
> Could you point me to those patches?
> 
> Even though it might work fine now, we might get issues if we backport
> remote kcov to older kernels.

Thinking more about this; you can't actually pre-fault for kernel
threads, as kernel threads will run with the mm of whatever regular
thread ran before them, and who knows if they have that vmalloc region
faulted in.

So Joerg's patches are pretty much the only way to guarantee remotes
will not his the vmalloc fault.