From: "Steven Rostedt (Google)" <[email protected]>
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) <[email protected]>
---
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
On Tue, 7 Feb 2023 13:51:47 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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) <[email protected]>
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
On Tue, Feb 07, 2023 at 01:54:02PM -0500, Steven Rostedt wrote:
> On Tue, 7 Feb 2023 13:51:47 -0500
> Steven Rostedt <[email protected]> wrote:
>
> > From: "Steven Rostedt (Google)" <[email protected]>
> >
> > 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) <[email protected]>
>
> 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.
On Wed, 8 Feb 2023 19:05:08 +0100
Peter Zijlstra <[email protected]> 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
On Wed, Feb 08, 2023 at 05:03:04PM -0500, Steven Rostedt wrote:
> On Wed, 8 Feb 2023 19:05:08 +0100
> Peter Zijlstra <[email protected]> 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.
On Tue, 7 Feb 2023 13:51:47 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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) <[email protected]>
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) <[email protected]>
On Wed, 8 Feb 2023 19:05:08 +0100
Peter Zijlstra <[email protected]> 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 <[email protected]> wrote:
> >
> > > From: "Steven Rostedt (Google)" <[email protected]>
> > >
> > > 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) <[email protected]>
> >
> > 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) <[email protected]>
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/[email protected]/
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/[email protected]/
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/[email protected]/
On Tue, 7 Feb 2023 13:51:47 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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) <[email protected]>
Shuah,
Can you pick this patch up?
Thanks,
-- Steve
On Mon, 13 Feb 2023 20:46:43 -0500
Steven Rostedt <[email protected]> wrote:
> Shuah,
>
> Can you pick this patch up?
ping?
-- Steve
On 3/18/23 12:42, Steven Rostedt wrote:
> On Mon, 13 Feb 2023 20:46:43 -0500
> Steven Rostedt <[email protected]> wrote:
>
>> Shuah,
>>
>> Can you pick this patch up?
>
> ping?
>
> -- Steve
l I will queue this up. Sorry for the delay.
thanks,
-- Shuah
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 <[email protected]> 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
On Mon, 20 Mar 2023 10:30:44 -0600
Shuah Khan <[email protected]> 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
On Mon, 20 Mar 2023 10:30:44 -0600
Shuah Khan <[email protected]> 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