Messages in this thread Patch in this message | | | Date | Fri, 13 Jan 2023 00:51:53 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function |
| |
On Thu, 12 Jan 2023 18:51:14 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> Akanksha J N wrote: > > Commit 97f88a3d723162 ("powerpc/kprobes: Fix null pointer reference in > > arch_prepare_kprobe()") fixed a recent kernel oops that was caused as > > ftrace-based kprobe does not generate kprobe::ainsn::insn and it gets > > set to NULL. > > Extend multiple kprobes test to add kprobes on first 256 bytes within a > > function, to be able to test potential issues with kprobes on > > successive instructions.
What is the purpose of that test? If you intended to add a kprobe events with some offset so that it becomes ftrace-based kprobe, it should be a different test case, because
- This is a test case for checking multiple (at least 256) kprobe events can be defined and enabled.
- If you want to check the ftrace-based kprobe, it should be near the function entry, maybe within 16 bytes or so.
- Also, you don't need to enable it at once (and should not for this case).
> > The '|| true' is added with the echo statement to ignore errors that are > > caused by trying to add kprobes to non probeable lines and continue with > > the test.
Can you add another test case for that? (and send it to the MLs which Cc'd to this mail) e.g.
for i in `seq 0 16`; do echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue echo 1 > events/kprobes/testprobe/enable ( echo "forked" ) echo 0 > events/kprobes/testprobe/enable echo > kprobe_events done
BTW, after we introduce the fprobe event (https://lore.kernel.org/linux-trace-kernel/166792255429.919356.14116090269057513181.stgit@devnote3/) that test case may be update to check fprobe events.
Thank you,
> > > > Signed-off-by: Akanksha J N <akanksha@linux.ibm.com> > > --- > > .../selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++++ > > 1 file changed, 4 insertions(+) > > Thanks for adding this test! > > > > > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > > index be754f5bcf79..f005c2542baa 100644 > > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > > @@ -25,6 +25,10 @@ if [ $L -ne 256 ]; then > > exit_fail > > fi > > > > +for i in `seq 0 255`; do > > + echo p $FUNCTION_FORK+${i} >> kprobe_events || true > > +done > > + > > cat kprobe_events >> $testlog > > > > echo 1 > events/kprobes/enable > > Thinking about this more, I wonder if we should add an explicit fork > after enabling the events, similar to kprobe_args.tc: > ( echo "forked" ) > > That will ensure we hit all the probes we added. With that change: > Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > > - Naveen
-- Masami Hiramatsu (Google) <mhiramat@kernel.org>
| |