On Thu, 22 Aug 2019 15:52:05 +0530
"Naveen N. Rao" [off-list ref] wrote:

Jisheng Zhang wrote:

quoted

Hi,

On Thu, 22 Aug 2019 12:23:58 +0530
"Naveen N. Rao" [off-list ref] wrote:  

quoted

Jisheng Zhang wrote:  

...

quoted

quoted

quoted

+/* Ftrace callback handler for kprobes -- called under preepmt
disabed */
+void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
+                        struct ftrace_ops *ops, struct pt_regs *regs)
+{
+     struct kprobe *p;
+     struct kprobe_ctlblk *kcb;
+
+     /* Preempt is disabled by ftrace */
+     p = get_kprobe((kprobe_opcode_t *)ip);
+     if (unlikely(!p) || kprobe_disabled(p))
+             return;
+
+     kcb = get_kprobe_ctlblk();
+     if (kprobe_running()) {
+             kprobes_inc_nmissed_count(p);
+     } else {
+             unsigned long orig_ip = instruction_pointer(regs);
+             /* Kprobe handler expects regs->pc = pc + 4 as breakpoint hit */
+             instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));  

Just want to make sure that you've confirmed that this is what happens
with a regular trap/brk based kprobe on ARM64. The reason for setting
the instruction pointer here is to ensure that it is set to the same
value as would be set if there was a trap/brk instruction at the ftrace
location. This ensures that the kprobe pre handler sees the same value
regardless.  

Due to the arm64's DYNAMIC_FTRACE_WITH_REGS implementation, the code itself
is correct. But this doesn't look like "there was a trap instruction at
the ftrace location".

W/O KPROBE_ON_FTRACE:

foo:
00    insA
04    insB
08    insC

kprobe's pre_handler() will see pc points to 00.  

In this case, the probe will be placed at foo+0x00, so pre_handler()
seeing that address in pt_regs is correct behavior - as long as arm64
'brk' instruction causes an exception with the instruction pointer set

Yep, confirmed with regular trap/brk based kprobes, I do see PC set to
the "brk" instruction.

*to* the 'brk' instruction. This is similar to how powerpc 'trap' works.
However, x86 'int3' causes an exception *after* execution of the
instruction.

Got it. I understand where's the comment "expects regs->pc = pc + 1" from.

quoted

W/ KPROBE_ON_FTRACE:

foo:
00    lr saver
04    nop     // will be modified to ftrace call ins when KPROBE is armed
08    insA
0c    insB  

In this case, if user asks for a probe to be placed at 'foo', we will
choose foo+0x04 and from that point on, the behavior should reflect that
a kprobe was placed at foo+0x04. In particular, the pre_handler() should
see foo+0x04 in pt_regs. The post_handler() would then see foo+0x08.

quoted

later, kprobe_ftrace_handler() will see pc points to 04, so pc + 4 will
point to 08 the same as the one w/o KPROBE_ON_FTRACE.  

I didn't mean to compare regular trap/brk based kprobes with
KPROBES_ON_FTRACE. The only important aspect is that the handlers see
consistent pt_regs in both cases, depending on where the kprobe was
placed. Choosing a different address/offset to place a kprobe during its
registration is an orthogonal aspect.

Indeed, previously, I want to let the PC point to the same instruction, it
seems I misunderstood the "consistent" meaning.

quoted

It seems I need to fix the comment.  

Given your explanation above, I think you can simply drop the first
adjustment to the instruction pointer before the pre handler invocation.
The rest of the code looks fine.

Yep, thanks a lot. Will send out a new version soon.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel