KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Tested on berlin arm64 platform.
~ # mount -t debugfs debugfs /sys/kernel/debug/
~ # cd /sys/kernel/debug/
/sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
before the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009fe28 k _do_fork+0x0 [DISABLED]
after the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
Signed-off-by: Jisheng Zhang <[email protected]>
---
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Applied after arm64 FTRACE_WITH_REGS:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674404.html
Changes since v3:
- move kprobe_lookup_name() and arch_kprobe_on_func_entry to ftrace.c since
we only want to choose the ftrace entry for KPROBES_ON_FTRACE.
- only choose ftrace entry if (addr && !offset)
Changes since v2:
- remove patch1, make it a single cleanup patch
- remove "This patch" in the change log
- implement arm64's kprobe_lookup_name() and arch_kprobe_on_func_entry instead
of patching the common kprobes code
Changes since v1:
- make the kprobes/x86: use instruction_pointer and instruction_pointer_set
as patch1
- add Masami's ACK to patch1
- add some description about KPROBES_ON_FTRACE and why we need it on
arm64
- correct the log before the patch
- remove the consolidation patch, make it as TODO
- only adjust kprobe's addr when KPROBE_FLAG_FTRACE is set
- if KPROBES_ON_FTRACE, ftrace_call_adjust() the kprobe's addr before
calling ftrace_location()
- update the kprobes-on-ftrace/arch-support.txt in doc
.../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/probes/Makefile | 1 +
arch/arm64/kernel/probes/ftrace.c | 84 +++++++++++++++++++
4 files changed, 87 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/probes/ftrace.c
diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
index 68f266944d5f..e8358a38981c 100644
--- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
+++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
@@ -9,7 +9,7 @@
| alpha: | TODO |
| arc: | TODO |
| arm: | TODO |
- | arm64: | TODO |
+ | arm64: | ok |
| c6x: | TODO |
| csky: | TODO |
| h8300: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 663392d1eae2..928700f15e23 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -167,6 +167,7 @@ config ARM64
select HAVE_STACKPROTECTOR
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_KPROBES
+ select HAVE_KPROBES_ON_FTRACE
select HAVE_KRETPROBES
select HAVE_GENERIC_VDSO
select IOMMU_DMA if IOMMU_SUPPORT
diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
index 8e4be92e25b1..4020cfc66564 100644
--- a/arch/arm64/kernel/probes/Makefile
+++ b/arch/arm64/kernel/probes/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \
simulate-insn.o
obj-$(CONFIG_UPROBES) += uprobes.o decode-insn.o \
simulate-insn.o
+obj-$(CONFIG_KPROBES_ON_FTRACE) += ftrace.o
diff --git a/arch/arm64/kernel/probes/ftrace.c b/arch/arm64/kernel/probes/ftrace.c
new file mode 100644
index 000000000000..5989c57660f3
--- /dev/null
+++ b/arch/arm64/kernel/probes/ftrace.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dynamic Ftrace based Kprobes Optimization
+ *
+ * Copyright (C) Hitachi Ltd., 2012
+ * Copyright (C) 2019 Jisheng Zhang <[email protected]>
+ * Synaptics Incorporated
+ */
+
+#include <linux/kprobes.h>
+
+/* 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));
+
+ __this_cpu_write(current_kprobe, p);
+ kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+ if (!p->pre_handler || !p->pre_handler(p, regs)) {
+ /*
+ * Emulate singlestep (and also recover regs->pc)
+ * as if there is a nop
+ */
+ instruction_pointer_set(regs,
+ (unsigned long)p->addr + MCOUNT_INSN_SIZE);
+ if (unlikely(p->post_handler)) {
+ kcb->kprobe_status = KPROBE_HIT_SSDONE;
+ p->post_handler(p, regs, 0);
+ }
+ instruction_pointer_set(regs, orig_ip);
+ }
+ /*
+ * If pre_handler returns !0, it changes regs->pc. We have to
+ * skip emulating post_handler.
+ */
+ __this_cpu_write(current_kprobe, NULL);
+ }
+}
+NOKPROBE_SYMBOL(kprobe_ftrace_handler);
+
+kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
+{
+ unsigned long addr = kallsyms_lookup_name(name);
+
+ if (addr && !offset) {
+ unsigned long faddr;
+ /*
+ * with -fpatchable-function-entry=2, the first 4 bytes is the
+ * LR saver, then the actual call insn. So ftrace location is
+ * always on the first 4 bytes offset.
+ */
+ faddr = ftrace_location_range(addr,
+ addr + AARCH64_INSN_SIZE);
+ if (faddr)
+ return (kprobe_opcode_t *)faddr;
+ }
+ return (kprobe_opcode_t *)addr;
+}
+
+bool arch_kprobe_on_func_entry(unsigned long offset)
+{
+ return offset <= AARCH64_INSN_SIZE;
+}
+
+int arch_prepare_kprobe_ftrace(struct kprobe *p)
+{
+ p->ainsn.api.insn = NULL;
+ return 0;
+}
--
2.23.0.rc1
Jisheng Zhang wrote:
> KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
> eliminates the need for a trap, as well as the need to emulate or
> single-step instructions.
>
> Tested on berlin arm64 platform.
>
> ~ # mount -t debugfs debugfs /sys/kernel/debug/
> ~ # cd /sys/kernel/debug/
> /sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
>
> before the patch:
>
> /sys/kernel/debug # cat kprobes/list
> ffffff801009fe28 k _do_fork+0x0 [DISABLED]
>
> after the patch:
>
> /sys/kernel/debug # cat kprobes/list
> ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
>
> Signed-off-by: Jisheng Zhang <[email protected]>
This looks good to me. Except for a small confirmation below:
Reviewed-by: Naveen N. Rao <[email protected]>
> ---
> KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
> eliminates the need for a trap, as well as the need to emulate or
> single-step instructions.
>
> Applied after arm64 FTRACE_WITH_REGS:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674404.html
>
> Changes since v3:
> - move kprobe_lookup_name() and arch_kprobe_on_func_entry to ftrace.c since
> we only want to choose the ftrace entry for KPROBES_ON_FTRACE.
> - only choose ftrace entry if (addr && !offset)
>
> Changes since v2:
> - remove patch1, make it a single cleanup patch
> - remove "This patch" in the change log
> - implement arm64's kprobe_lookup_name() and arch_kprobe_on_func_entry instead
> of patching the common kprobes code
>
> Changes since v1:
> - make the kprobes/x86: use instruction_pointer and instruction_pointer_set
> as patch1
> - add Masami's ACK to patch1
> - add some description about KPROBES_ON_FTRACE and why we need it on
> arm64
> - correct the log before the patch
> - remove the consolidation patch, make it as TODO
> - only adjust kprobe's addr when KPROBE_FLAG_FTRACE is set
> - if KPROBES_ON_FTRACE, ftrace_call_adjust() the kprobe's addr before
> calling ftrace_location()
> - update the kprobes-on-ftrace/arch-support.txt in doc
>
>
> .../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/probes/Makefile | 1 +
> arch/arm64/kernel/probes/ftrace.c | 84 +++++++++++++++++++
> 4 files changed, 87 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/kernel/probes/ftrace.c
>
> diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> index 68f266944d5f..e8358a38981c 100644
> --- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> +++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> @@ -9,7 +9,7 @@
> | alpha: | TODO |
> | arc: | TODO |
> | arm: | TODO |
> - | arm64: | TODO |
> + | arm64: | ok |
> | c6x: | TODO |
> | csky: | TODO |
> | h8300: | TODO |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 663392d1eae2..928700f15e23 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -167,6 +167,7 @@ config ARM64
> select HAVE_STACKPROTECTOR
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_KPROBES
> + select HAVE_KPROBES_ON_FTRACE
> select HAVE_KRETPROBES
> select HAVE_GENERIC_VDSO
> select IOMMU_DMA if IOMMU_SUPPORT
> diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
> index 8e4be92e25b1..4020cfc66564 100644
> --- a/arch/arm64/kernel/probes/Makefile
> +++ b/arch/arm64/kernel/probes/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \
> simulate-insn.o
> obj-$(CONFIG_UPROBES) += uprobes.o decode-insn.o \
> simulate-insn.o
> +obj-$(CONFIG_KPROBES_ON_FTRACE) += ftrace.o
> diff --git a/arch/arm64/kernel/probes/ftrace.c b/arch/arm64/kernel/probes/ftrace.c
> new file mode 100644
> index 000000000000..5989c57660f3
> --- /dev/null
> +++ b/arch/arm64/kernel/probes/ftrace.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Dynamic Ftrace based Kprobes Optimization
> + *
> + * Copyright (C) Hitachi Ltd., 2012
> + * Copyright (C) 2019 Jisheng Zhang <[email protected]>
> + * Synaptics Incorporated
> + */
> +
> +#include <linux/kprobes.h>
> +
> +/* 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.
Further changes to the instruction pointer are to achieve the same
effect for kprobe post handlers.
- Naveen
Jisheng Zhang wrote:
> Hi,
>
> On Thu, 22 Aug 2019 12:23:58 +0530
> "Naveen N. Rao" <[email protected]> wrote:
>> Jisheng Zhang wrote:
...
>> > +/* 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
*to* the 'brk' instruction. This is similar to how powerpc 'trap' works.
However, x86 'int3' causes an exception *after* execution of the
instruction.
>
> 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.
>
> 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.
>
> 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.
- Naveen
On Thu, 22 Aug 2019 15:52:05 +0530
"Naveen N. Rao" <[email protected]> wrote:
>
>
> Jisheng Zhang wrote:
> > Hi,
> >
> > On Thu, 22 Aug 2019 12:23:58 +0530
> > "Naveen N. Rao" <[email protected]> wrote:
> >> Jisheng Zhang wrote:
> ...
> >> > +/* 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.
>
> >
> > 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.
>
> >
> > 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.
>
> >
> > 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.
Hi,
On Thu, 22 Aug 2019 12:23:58 +0530
"Naveen N. Rao" <[email protected]> wrote:
>
>
> Jisheng Zhang wrote:
> > KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
> > eliminates the need for a trap, as well as the need to emulate or
> > single-step instructions.
> >
> > Tested on berlin arm64 platform.
> >
> > ~ # mount -t debugfs debugfs /sys/kernel/debug/
> > ~ # cd /sys/kernel/debug/
> > /sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
> >
> > before the patch:
> >
> > /sys/kernel/debug # cat kprobes/list
> > ffffff801009fe28 k _do_fork+0x0 [DISABLED]
> >
> > after the patch:
> >
> > /sys/kernel/debug # cat kprobes/list
> > ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
>
> This looks good to me. Except for a small confirmation below:
> Reviewed-by: Naveen N. Rao <[email protected]>
>
>
<...>
> > +/* 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.
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
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.
It seems I need to fix the comment.
>
> Further changes to the instruction pointer are to achieve the same
> effect for kprobe post handlers.
>
>
Hi,
On Thu, 22 Aug 2019 18:32:54 +0800
Jisheng Zhang <[email protected]> wrote:
> On Thu, 22 Aug 2019 15:52:05 +0530
> "Naveen N. Rao" <[email protected]> wrote:
>
> >
> >
> > Jisheng Zhang wrote:
> > > Hi,
> > >
> > > On Thu, 22 Aug 2019 12:23:58 +0530
> > > "Naveen N. Rao" <[email protected]> wrote:
> > >> Jisheng Zhang wrote:
> > ...
> > >> > +/* 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.
>
> >
> > >
> > > 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.
> >
> > >
> > > 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.
>
> >
> > >
> > > 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.
Just send out v5. But the first adjustment is modified as
instruction_pointer_set(regs, ip);
Because in entry of kprobe_ftrace_handler() pc/ip(the first parameter) points
to foo+0x4, while regs->pc points to foo+0x8. Based on your previous
explanation, I think we should instruction_pointer_set(regs, ip) to let the
pre_handler see foo+0x4
Thanks a lot for your help