[PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The kprobe probepoint.tc test started failing because of the added __pfx_
symbols that were added because of -fpatchable-function-entry=X,Y causing
unwinders to see them as part of the previous functions. But kprobes can
not be added on top of them. The selftest looks for tracefs_create_dir and
picks it and the previous and following functions to add at their address.
This caused it to include __pfx_tracefs_create_dir which is invalid to
attach a kprobe to and caused the test to fail.

Fixes: 9f2899fe36a62 ("objtool: Add option to generate prefix symbols")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc
index 624269c8d534..e1b7506c1b11 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc
@@ -21,7 +21,7 @@ set_offs() { # prev target next
 
 # We have to decode symbol addresses to get correct offsets.
 # If the offset is not an instruction boundary, it cause -EILSEQ.
-set_offs `grep -A1 -B1 ${TARGET_FUNC} /proc/kallsyms | cut -f 1 -d " " | xargs`
+set_offs `grep -v __pfx_ /proc/kallsyms | grep -A1 -B1 ${TARGET_FUNC} |  cut -f 1 -d " " | xargs`
 
 UINT_TEST=no
 # printf "%x" -1 returns (unsigned long)-1.
-- 
2.39.0

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

On Tue, 7 Feb 2023 13:51:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The kprobe probepoint.tc test started failing because of the added __pfx_
> symbols that were added because of -fpatchable-function-entry=X,Y causing
> unwinders to see them as part of the previous functions. But kprobes can
> not be added on top of them. The selftest looks for tracefs_create_dir and
> picks it and the previous and following functions to add at their address.
> This caused it to include __pfx_tracefs_create_dir which is invalid to
> attach a kprobe to and caused the test to fail.
> 
> Fixes: 9f2899fe36a62 ("objtool: Add option to generate prefix symbols")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

This is assuming that kprobes can not be added on top of these. But another
solution could be to have kprobes just pick the function the __pfx_ is for.
Would that be a better solution?

-- Steve

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Peter Zijlstra]

On Tue, Feb 07, 2023 at 01:54:02PM -0500, Steven Rostedt wrote:
> On Tue, 7 Feb 2023 13:51:47 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > The kprobe probepoint.tc test started failing because of the added __pfx_
> > symbols that were added because of -fpatchable-function-entry=X,Y causing
> > unwinders to see them as part of the previous functions. But kprobes can
> > not be added on top of them. The selftest looks for tracefs_create_dir and
> > picks it and the previous and following functions to add at their address.
> > This caused it to include __pfx_tracefs_create_dir which is invalid to
> > attach a kprobe to and caused the test to fail.
> > 
> > Fixes: 9f2899fe36a62 ("objtool: Add option to generate prefix symbols")
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> 
> This is assuming that kprobes can not be added on top of these. But another
> solution could be to have kprobes just pick the function the __pfx_ is for.
> Would that be a better solution?

Simply refusing them is simplest. I don't see a compelling reason to
make this complicated.

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

On Wed, 8 Feb 2023 19:05:08 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> > This is assuming that kprobes can not be added on top of these. But another
> > solution could be to have kprobes just pick the function the __pfx_ is for.
> > Would that be a better solution?  
> 
> Simply refusing them is simplest. I don't see a compelling reason to
> make this complicated.

OK, so you are good with the patch as is then?

-- Steve

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Peter Zijlstra]

On Wed, Feb 08, 2023 at 05:03:04PM -0500, Steven Rostedt wrote:
> On Wed, 8 Feb 2023 19:05:08 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > This is assuming that kprobes can not be added on top of these. But another
> > > solution could be to have kprobes just pick the function the __pfx_ is for.
> > > Would that be a better solution?  
> > 
> > Simply refusing them is simplest. I don't see a compelling reason to
> > make this complicated.
> 
> OK, so you are good with the patch as is then?

Yeah, but given I've no idea about the whole test thing or .tc files I
didn't ack.

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Masami Hiramatsu]

On Tue, 7 Feb 2023 13:51:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The kprobe probepoint.tc test started failing because of the added __pfx_
> symbols that were added because of -fpatchable-function-entry=X,Y causing
> unwinders to see them as part of the previous functions. But kprobes can
> not be added on top of them. The selftest looks for tracefs_create_dir and
> picks it and the previous and following functions to add at their address.
> This caused it to include __pfx_tracefs_create_dir which is invalid to
> attach a kprobe to and caused the test to fail.
> 
> Fixes: 9f2899fe36a62 ("objtool: Add option to generate prefix symbols")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

This looks good to me. 

Acked-by: 

Thanks,

> ---
>  tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc
> index 624269c8d534..e1b7506c1b11 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc
> @@ -21,7 +21,7 @@ set_offs() { # prev target next
>  
>  # We have to decode symbol addresses to get correct offsets.
>  # If the offset is not an instruction boundary, it cause -EILSEQ.
> -set_offs `grep -A1 -B1 ${TARGET_FUNC} /proc/kallsyms | cut -f 1 -d " " | xargs`
> +set_offs `grep -v __pfx_ /proc/kallsyms | grep -A1 -B1 ${TARGET_FUNC} |  cut -f 1 -d " " | xargs`
>  
>  UINT_TEST=no
>  # printf "%x" -1 returns (unsigned long)-1.
> -- 
> 2.39.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Masami Hiramatsu]

On Wed, 8 Feb 2023 19:05:08 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Feb 07, 2023 at 01:54:02PM -0500, Steven Rostedt wrote:
> > On Tue, 7 Feb 2023 13:51:47 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > > 
> > > The kprobe probepoint.tc test started failing because of the added __pfx_
> > > symbols that were added because of -fpatchable-function-entry=X,Y causing
> > > unwinders to see them as part of the previous functions. But kprobes can
> > > not be added on top of them. The selftest looks for tracefs_create_dir and
> > > picks it and the previous and following functions to add at their address.
> > > This caused it to include __pfx_tracefs_create_dir which is invalid to
> > > attach a kprobe to and caused the test to fail.
> > > 
> > > Fixes: 9f2899fe36a62 ("objtool: Add option to generate prefix symbols")
> > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > 
> > This is assuming that kprobes can not be added on top of these. But another
> > solution could be to have kprobes just pick the function the __pfx_ is for.
> > Would that be a better solution?
> 
> Simply refusing them is simplest. I don't see a compelling reason to
> make this complicated.

Yeah, and __pfx_ symbols has some "range", that means it is hard to translate
the probe address if user specify __pfx_*+offset.

BTW, currently kprobe event rejects this __pfx_ symbols because it is notrace
symbols, thus we can trace it if CONFIG_KPROBE_EVENTS_ON_NOTRACE=y.
But I guess it should not probe that place always because it should never
executed right?

Thank you,


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Peter Zijlstra]

On Thu, Feb 09, 2023 at 11:23:05PM +0900, Masami Hiramatsu wrote:

> BTW, currently kprobe event rejects this __pfx_ symbols because it is notrace
> symbols, thus we can trace it if CONFIG_KPROBE_EVENTS_ON_NOTRACE=y.
> But I guess it should not probe that place always because it should never
> executed right?

Execution can take place in those ranges when X86_FEATURE_CALL_DEPTH is
enabled or when CONFIG_KCFI && X86_FEATURE_IBT.

In the first of those cases the prefix bytes are filled with call
accounting instructions and every direct call to $sym is patched to
point to __pfx_$sym+6 (aka $sym-10).

  https://lore.kernel.org/all/20220915111039.092790446@infradead.org/

In the second case (FineIBT) it is probably easiest if you look at the
comment in arch/x86/kernel/alternative.c near CONFIG_FINEIBT.

  https://lore.kernel.org/all/20221027092812.185993858@infradead.org/

The __pfx_ and __cfi_ symbols are the same (in fact, when
CONFIG_CFI_CLANG=y the compiler generates them and objtool no longer
emits the __pfx_ symbols).

  https://lore.kernel.org/all/20221028194453.592512209@infradead.org/

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

On Tue, 7 Feb 2023 13:51:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The kprobe probepoint.tc test started failing because of the added __pfx_
> symbols that were added because of -fpatchable-function-entry=X,Y causing
> unwinders to see them as part of the previous functions. But kprobes can
> not be added on top of them. The selftest looks for tracefs_create_dir and
> picks it and the previous and following functions to add at their address.
> This caused it to include __pfx_tracefs_create_dir which is invalid to
> attach a kprobe to and caused the test to fail.
> 
> Fixes: 9f2899fe36a62 ("objtool: Add option to generate prefix symbols")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Shuah,

Can you pick this patch up?

Thanks,

-- Steve

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

On Mon, 13 Feb 2023 20:46:43 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> Shuah,
> 
> Can you pick this patch up?

ping?

-- Steve

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Shuah Khan]

On 3/18/23 12:42, Steven Rostedt wrote:
> On Mon, 13 Feb 2023 20:46:43 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> Shuah,
>>
>> Can you pick this patch up?
> 
> ping?
> 
> -- Steve

l I will queue this up. Sorry for the delay.

thanks,
-- Shuah

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Shuah Khan]

On 3/20/23 06:56, Shuah Khan wrote:
> On 3/18/23 12:42, Steven Rostedt wrote:
>> On Mon, 13 Feb 2023 20:46:43 -0500
>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>>> Shuah,
>>>
>>> Can you pick this patch up?
>>
>> ping?
>>
>> -- Steve
> 
> l I will queue this up. Sorry for the delay.
> 

Steve,

The patch doesn't apply to linux-kselftest fixes branch.
Please rebase and resend with cc to linux-kselftest.
Makes sense why it got buried in my regular Inbox.

This one didn't show up in kselftest list. Fixes tag SHA
is 13 char long. I fixed it and tried to apply. When you
resend, please fix the Fixes tag as well.

thanks,
-- Shuah

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

On Mon, 20 Mar 2023 10:30:44 -0600
Shuah Khan <skhan@linuxfoundation.org> wrote:
> 
> Steve,
> 
> The patch doesn't apply to linux-kselftest fixes branch.
> Please rebase and resend with cc to linux-kselftest.
> Makes sense why it got buried in my regular Inbox.
> 
> This one didn't show up in kselftest list. Fixes tag SHA
> is 13 char long. I fixed it and tried to apply. When you
> resend, please fix the Fixes tag as well.

Will do. Thanks Shuah!

-- Steve

Re: [PATCH] tracing/selftests: Ignore __pfx_ symbols in kprobe test

[Steven Rostedt]

On Mon, 20 Mar 2023 10:30:44 -0600
Shuah Khan <skhan@linuxfoundation.org> wrote:

> The patch doesn't apply to linux-kselftest fixes branch.
> Please rebase and resend with cc to linux-kselftest.
> Makes sense why it got buried in my regular Inbox.

And it doesn't apply because Masami fixed it already.

I'll just drop it. Thanks!

-- Steve