On Thu, 12 Jan 2023 18:51:14 +0530
"Naveen N. Rao" [off-list ref] wrote:

Akanksha J N wrote:

quoted

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).

quoted

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/ (local)) that test case may be
update to check fprobe events.

Thank you,

quoted

Signed-off-by: Akanksha J N <redacted>
---
 .../selftests/ftrace/test.d/kprobe/multiple_kprobes.tc        | 4 ++++
 1 file changed, 4 insertions(+)

Thanks for adding this test!

quoted

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 <redacted>


- Naveen

-- 
Masami Hiramatsu (Google) [off-list ref]