| Message ID | 20230207135147.5ce618d6@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing/selftests: Ignore __pfx_ symbols in kprobe test | expand |
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
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.
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
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.
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 >
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,
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/
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
On Mon, 13 Feb 2023 20:46:43 -0500 Steven Rostedt <rostedt@goodmis.org> 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 <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
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
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
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
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.