[BUGFIX PATCH] tracing/kprobes: Fix to check notrace function with correct range

[BUGFIX PATCH] tracing/kprobes: Fix to check notrace function with correct range

[Masami Hiramatsu]

Fix within_notrace_func() to check notrace function correctly.

Since the ftrace_location_range(start, end) function checks
the range inclusively (start <= ftrace-loc <= end), the end
address must not include the entry address of next function.

However, within_notrace_func() uses kallsyms_lookup_size_offset()
to get the function size and calculate the end address from
adding the size to the entry address. This means the end address
is the entry address of the next function.

In the result, within_notrace_func() fails to find notrace
function if the next function of the target function is
ftraced.

Let's subtract 1 from the end address so that ftrace_location_range()
can check it correctly.

Fixes: commit 45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reported-by: Michael Rodin <michael@rodin.online>
---
 kernel/trace/trace_kprobe.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 65a4157af851..ad384b31fe01 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -513,7 +513,14 @@ static bool within_notrace_func(struct trace_kprobe *tk)
 	if (!addr || !kallsyms_lookup_size_offset(addr, &size, &offset))
 		return false;
 
-	return !ftrace_location_range(addr - offset, addr - offset + size);
+	/* Get the entry address of the target function */
+	addr -= offset;
+
+	/*
+	 * Since ftrace_location_range() does inclusive range check, we need
+	 * to subtract 1 byte from the end address.
+	 */
+	return !ftrace_location_range(addr, addr + size - 1);
 }
 #else
 #define within_notrace_func(tk)	(false)

Re: [BUGFIX PATCH] tracing/kprobes: Fix to check notrace function with correct range

[Steven Rostedt]

On Tue, 21 Aug 2018 22:04:57 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix within_notrace_func() to check notrace function correctly.
> 
> Since the ftrace_location_range(start, end) function checks
> the range inclusively (start <= ftrace-loc <= end), the end
> address must not include the entry address of next function.
> 
> However, within_notrace_func() uses kallsyms_lookup_size_offset()
> to get the function size and calculate the end address from
> adding the size to the entry address. This means the end address
> is the entry address of the next function.
> 
> In the result, within_notrace_func() fails to find notrace
> function if the next function of the target function is
> ftraced.
> 
> Let's subtract 1 from the end address so that ftrace_location_range()
> can check it correctly.
> 
> Fixes: commit 45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Michael Rodin <michael@rodin.online>
> ---
>

Applied. Thanks Masami! I'll start testing this and send it upstream
when it's finished.

-- Steve

Re: [BUGFIX PATCH] tracing/kprobes: Fix to check notrace function with correct range

[Steven Rostedt]

On Tue, 21 Aug 2018 09:42:49 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 21 Aug 2018 22:04:57 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Fix within_notrace_func() to check notrace function correctly.
> > 
> > Since the ftrace_location_range(start, end) function checks
> > the range inclusively (start <= ftrace-loc <= end), the end
> > address must not include the entry address of next function.
> > 
> > However, within_notrace_func() uses kallsyms_lookup_size_offset()
> > to get the function size and calculate the end address from
> > adding the size to the entry address. This means the end address
> > is the entry address of the next function.
> > 
> > In the result, within_notrace_func() fails to find notrace
> > function if the next function of the target function is
> > ftraced.
> > 
> > Let's subtract 1 from the end address so that ftrace_location_range()
> > can check it correctly.
> > 
> > Fixes: commit 45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Reported-by: Michael Rodin <michael@rodin.online>
> > ---
> >  
> 
> Applied. Thanks Masami! I'll start testing this and send it upstream
> when it's finished.
> 

Hmm, this fix shows the extent of not tracing functions with notrace
attched much deeper. For one thing, we can't hook kprobes to the
__schedule() function (which is now what all the main schedule
functions call). One of my tests used this function to test kprobes and
it failed.

I'll push this to Linus, but I'm wondering if we want to perhaps add a
white list of functions marked "notrace" but still kprobes can trace?

-- Steve

Re: [BUGFIX PATCH] tracing/kprobes: Fix to check notrace function with correct range

[Masami Hiramatsu]

On Wed, 22 Aug 2018 08:58:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 21 Aug 2018 09:42:49 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 21 Aug 2018 22:04:57 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Fix within_notrace_func() to check notrace function correctly.
> > > 
> > > Since the ftrace_location_range(start, end) function checks
> > > the range inclusively (start <= ftrace-loc <= end), the end
> > > address must not include the entry address of next function.
> > > 
> > > However, within_notrace_func() uses kallsyms_lookup_size_offset()
> > > to get the function size and calculate the end address from
> > > adding the size to the entry address. This means the end address
> > > is the entry address of the next function.
> > > 
> > > In the result, within_notrace_func() fails to find notrace
> > > function if the next function of the target function is
> > > ftraced.
> > > 
> > > Let's subtract 1 from the end address so that ftrace_location_range()
> > > can check it correctly.
> > > 
> > > Fixes: commit 45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Reported-by: Michael Rodin <michael@rodin.online>
> > > ---
> > >  
> > 
> > Applied. Thanks Masami! I'll start testing this and send it upstream
> > when it's finished.
> > 
> 
> Hmm, this fix shows the extent of not tracing functions with notrace
> attched much deeper. For one thing, we can't hook kprobes to the
> __schedule() function (which is now what all the main schedule
> functions call). One of my tests used this function to test kprobes and
> it failed.
> 
> I'll push this to Linus, but I'm wondering if we want to perhaps add a
> white list of functions marked "notrace" but still kprobes can trace?

Yes, that is what I'm thinking about. Maybe most of notrace function is
safe for kprobes (like __schedule()). And if it is really critical, those
functions must be marked by NOKPROBE_SYMBOL().

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH] tracing/kprobes: Fix to check notrace function with correct range

[Steven Rostedt]

On Thu, 23 Aug 2018 10:18:31 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 22 Aug 2018 08:58:09 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 21 Aug 2018 09:42:49 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >   
> > > On Tue, 21 Aug 2018 22:04:57 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >   
> > > > Fix within_notrace_func() to check notrace function correctly.
> > > > 
> > > > Since the ftrace_location_range(start, end) function checks
> > > > the range inclusively (start <= ftrace-loc <= end), the end
> > > > address must not include the entry address of next function.
> > > > 
> > > > However, within_notrace_func() uses kallsyms_lookup_size_offset()
> > > > to get the function size and calculate the end address from
> > > > adding the size to the entry address. This means the end address
> > > > is the entry address of the next function.
> > > > 
> > > > In the result, within_notrace_func() fails to find notrace
> > > > function if the next function of the target function is
> > > > ftraced.
> > > > 
> > > > Let's subtract 1 from the end address so that ftrace_location_range()
> > > > can check it correctly.
> > > > 
> > > > Fixes: commit 45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Reported-by: Michael Rodin <michael@rodin.online>
> > > > ---
> > > >    
> > > 
> > > Applied. Thanks Masami! I'll start testing this and send it upstream
> > > when it's finished.
> > >   
> > 
> > Hmm, this fix shows the extent of not tracing functions with notrace
> > attched much deeper. For one thing, we can't hook kprobes to the
> > __schedule() function (which is now what all the main schedule
> > functions call). One of my tests used this function to test kprobes and
> > it failed.
> > 
> > I'll push this to Linus, but I'm wondering if we want to perhaps add a
> > white list of functions marked "notrace" but still kprobes can trace?  
> 
> Yes, that is what I'm thinking about. Maybe most of notrace function is
> safe for kprobes (like __schedule()). And if it is really critical, those
> functions must be marked by NOKPROBE_SYMBOL().
> 

Correct. This notrace was added because we want to not trace any
function call between preempt_schedule() and where NEED_RESCHED is
cleared. Because the function tracer does a preempt_enable_notrace()
which triggers the preempt_schedule() call again, and we can end up in
a recursive loop. But that is fixed, but by tracing __schedule() we do
a song and dance of calling preempt_schedule_notrace and makes the
trace output weird.

See commit 499d79559ffe4.

-- Steve