diff mbox series

[v2,1/3] kprobes/x86: use instruction_pointer and instruction_pointer_set

Message ID 20190820114109.4624d56b@xhacker.debian (mailing list archive)
State New, archived
Headers show
Series arm64: KPROBES_ON_FTRACE | expand

Commit Message

Jisheng Zhang Aug. 20, 2019, 3:52 a.m. UTC 
This is to make the x86 kprobe_ftrace_handler() more common so that
the code could be reused in future.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/ftrace.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Thomas Gleixner Aug. 20, 2019, 8:53 a.m. UTC | #1 
On Tue, 20 Aug 2019, Jisheng Zhang wrote:

> This is to make the x86 kprobe_ftrace_handler() more common so that
> the code could be reused in future.

While I agree with the change in general, I can't find anything which
reuses that code. So the change log is pretty useless and I have no idea
how this is related to the rest of the series.

Thanks,

	tglx
Jisheng Zhang Aug. 20, 2019, 9:02 a.m. UTC | #2 
Hi Thomas,

On Tue, 20 Aug 2019 10:53:58 +0200 (CEST) Thomas Gleixner wrote:

> 
> 
> On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> 
> > This is to make the x86 kprobe_ftrace_handler() more common so that
> > the code could be reused in future.  
> 
> While I agree with the change in general, I can't find anything which
> reuses that code. So the change log is pretty useless and I have no idea
> how this is related to the rest of the series.

In v1, this code is moved from x86 to common kprobes.c [1]
But I agree with Masami, consolidation could be done when arm64 kprobes
on ftrace is stable.

In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
as x86's, the only difference is comment, e.g

/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */

while in arm64

/* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */


W/ above, any suggestion about the suitable change log?

Thanks

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674417.html
Jisheng Zhang Aug. 20, 2019, 9:20 a.m. UTC | #3 
On Tue, 20 Aug 2019 09:02:59 +0000 Jisheng Zhang wrote:


> 
> 
> Hi Thomas,
> 
> On Tue, 20 Aug 2019 10:53:58 +0200 (CEST) Thomas Gleixner wrote:
> 
> >
> >
> > On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> >  
> > > This is to make the x86 kprobe_ftrace_handler() more common so that
> > > the code could be reused in future.  
> >
> > While I agree with the change in general, I can't find anything which
> > reuses that code. So the change log is pretty useless and I have no idea
> > how this is related to the rest of the series.  

Indeed, this isn't related to the rest of the series. So will update the
change log and resend it alone.

> 
> In v1, this code is moved from x86 to common kprobes.c [1]
> But I agree with Masami, consolidation could be done when arm64 kprobes
> on ftrace is stable.
> 
> In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> as x86's, the only difference is comment, e.g
> 
> /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
> 
> while in arm64
> 
> /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */
> 
> 
> W/ above, any suggestion about the suitable change log?
> 
> Thanks
>
Peter Zijlstra Aug. 20, 2019, 1:21 p.m. UTC | #4 
On Tue, Aug 20, 2019 at 09:02:59AM +0000, Jisheng Zhang wrote:
> In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> as x86's, the only difference is comment, e.g
> 
> /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
> 
> while in arm64
> 
> /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */

What's weird; I thought ARM has fixed sized instructions and they are
all 4 bytes? So how does a single byte offset make sense for ARM?
Masami Hiramatsu (Google) Aug. 21, 2019, 1:52 a.m. UTC | #5 
Hi Jisheng,

On Tue, 20 Aug 2019 09:02:59 +0000
Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:

> Hi Thomas,
> 
> On Tue, 20 Aug 2019 10:53:58 +0200 (CEST) Thomas Gleixner wrote:
> 
> > 
> > 
> > On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> > 
> > > This is to make the x86 kprobe_ftrace_handler() more common so that
> > > the code could be reused in future.  
> > 
> > While I agree with the change in general, I can't find anything which
> > reuses that code. So the change log is pretty useless and I have no idea
> > how this is related to the rest of the series.
> 
> In v1, this code is moved from x86 to common kprobes.c [1]
> But I agree with Masami, consolidation could be done when arm64 kprobes
> on ftrace is stable.

We'll revisit to consolidate the code after we got 3rd or 4th clones.

> 
> In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> as x86's, the only difference is comment, e.g
> 
> /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
> 
> while in arm64
> 
> /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */

As Peter pointed, on arm64, is that really 1 or 4 bytes?
This part is heavily depends on the processor software-breakpoint
implementation.

> 
> 
> W/ above, any suggestion about the suitable change log?

I think you just need to keep the first half of the description.
Since this patch itself is not related to the series, could you update
the description and resend it as a single cleanup patch out of the series?

Thank you!

> 
> Thanks
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674417.html
Jisheng Zhang Aug. 21, 2019, 2:02 a.m. UTC | #6 
Hi Peter,

On Tue, 20 Aug 2019 15:21:10 +0200 Peter Zijlstra wrote:

> 
> 
> On Tue, Aug 20, 2019 at 09:02:59AM +0000, Jisheng Zhang wrote:
> > In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> > as x86's, the only difference is comment, e.g
> >
> > /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
> >
> > while in arm64
> >
> > /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */  
> 
> What's weird; I thought ARM has fixed sized instructions and they are
> all 4 bytes? So how does a single byte offset make sense for ARM?

I believe the "+1" here means + one kprobe_opcode_t.

Thanks
Jisheng Zhang Aug. 21, 2019, 2:09 a.m. UTC | #7 
Hi,

On Wed, 21 Aug 2019 10:52:47 +0900 Masami Hiramatsu wrote:
> 
> 
> Hi Jisheng,
> 
> On Tue, 20 Aug 2019 09:02:59 +0000
> Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> 
> > Hi Thomas,
> >
> > On Tue, 20 Aug 2019 10:53:58 +0200 (CEST) Thomas Gleixner wrote:
> >  
> > >
> > >
> > > On Tue, 20 Aug 2019, Jisheng Zhang wrote:
> > >  
> > > > This is to make the x86 kprobe_ftrace_handler() more common so that
> > > > the code could be reused in future.  
> > >
> > > While I agree with the change in general, I can't find anything which
> > > reuses that code. So the change log is pretty useless and I have no idea
> > > how this is related to the rest of the series.  
> >
> > In v1, this code is moved from x86 to common kprobes.c [1]
> > But I agree with Masami, consolidation could be done when arm64 kprobes
> > on ftrace is stable.  
> 
> We'll revisit to consolidate the code after we got 3rd or 4th clones.
> 
> >
> > In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> > as x86's, the only difference is comment, e.g
> >
> > /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
> >
> > while in arm64
> >
> > /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */  
> 
> As Peter pointed, on arm64, is that really 1 or 4 bytes?
> This part is heavily depends on the processor software-breakpoint
> implementation.

Per my understanding, the "+1" here means "+ one kprobe_opcode_t".

> 
> >
> >
> > W/ above, any suggestion about the suitable change log?  
> 
> I think you just need to keep the first half of the description.
> Since this patch itself is not related to the series, could you update
> the description and resend it as a single cleanup patch out of the series?
> 

Got it. Will do today.

Thanks a lot
Masami Hiramatsu (Google) Aug. 23, 2019, 2:51 p.m. UTC | #8 
Hi Jisheng,

On Wed, 21 Aug 2019 02:09:10 +0000
Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:

> > > In v2, actually, the arm64 version's kprobe_ftrace_handler() is the same
> > > as x86's, the only difference is comment, e.g
> > >
> > > /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
> > >
> > > while in arm64
> > >
> > > /* Kprobe handler expects regs->pc = ip + 1 as breakpoint hit */  
> > 
> > As Peter pointed, on arm64, is that really 1 or 4 bytes?
> > This part is heavily depends on the processor software-breakpoint
> > implementation.
> 
> Per my understanding, the "+1" here means "+ one kprobe_opcode_t".

No, that is the size of INT3. It just emulates the software trap on x86.

Thank you,
diff mbox series

Patch

diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 681a4b36e9bb..c2ad0b9259ca 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -28,9 +28,9 @@  void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(p);
 	} else {
-		unsigned long orig_ip = regs->ip;
+		unsigned long orig_ip = instruction_pointer(regs);
 		/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
-		regs->ip = ip + sizeof(kprobe_opcode_t);
+		instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));
 
 		__this_cpu_write(current_kprobe, p);
 		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
@@ -39,12 +39,13 @@  void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 			 * Emulate singlestep (and also recover regs->ip)
 			 * as if there is a 5byte nop
 			 */
-			regs->ip = (unsigned long)p->addr + MCOUNT_INSN_SIZE;
+			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);
 			}
-			regs->ip = orig_ip;
+			instruction_pointer_set(regs, orig_ip);
 		}
 		/*
 		 * If pre_handler returns !0, it changes regs->ip. We have to