x86: kprobes change kprobe_handler flow
PATCH 1/1 x86: kprobes change kprobe_handler flow
Make the control flow of kprobe_handler more obvious.
Collapse the separate if blocks/gotos with if/else blocks this unifies the duplication of the check for a breakpoint instruction race with another cpu.
Create two jump targets:
preempt_out: re-enables preemption before returning ret
out: only returns ret
---
Masami, noticed a small bug in the previous version in the !p case when the breakpoint was the kernel's. Please review this version.
arch/x86/kernel/kprobes.c | 60 +++++++++++++++++++++------------------------ 1 files changed, 28 insertions(+), 32 deletions(-)
arch/x86/kernel/kprobes.c +28 -32
@@ -480,32 +480,28 @@ static int __kprobes kprobe_handler(struct pt_regs *regs)preempt_disable (); kcb =get_kprobe_ctlblk (); -/* Check we're not actually recursing */ -if (kprobe_running ()) { - p =get_kprobe (addr); -if (p) { + p =get_kprobe (addr); +if (p) { +/* Check we're not actually recursing */ +if (kprobe_running ()) { ret =reenter_kprobe (p, regs, kcb);if (kcb->kprobe_status ==KPROBE_REENTER ) -return 1 ; + { + ret =1 ; + goto out; + } + goto preempt_out; }else { -if (*addr !=BREAKPOINT_INSTRUCTION ) { -/* The breakpoint instruction was removed by -* another cpu right after we hit, no further -* handling of this interrupt is appropriate -*/ - regs->ip = (unsigned long )addr; +set_current_kprobe (p, regs, kcb); + kcb->kprobe_status =KPROBE_HIT_ACTIVE ; +if (p->pre_handler && p->pre_handler (p, regs)) + { +/* handler set things up, skip ss setup */ ret =1 ; - goto no_kprobe; + goto out; } - p =__get_cpu_var (current_kprobe); -if (p->break_handler && p->break_handler (p, regs)) - goto ss_probe; } - goto no_kprobe; - } - - p =get_kprobe (addr); -if (!p) { + }else {if (*addr !=BREAKPOINT_INSTRUCTION ) {/* * The breakpoint instruction was removed right
@@ -518,34 +514,34 @@ static int __kprobes kprobe_handler(struct pt_regs *regs)*/ regs->ip = (unsigned long )addr; ret =1 ; + goto preempt_out; + } +if (kprobe_running ()) { + p =__get_cpu_var (current_kprobe); +if (p->break_handler && p->break_handler (p, regs)) + goto ss_probe; }/* Not one of ours: let kernel handle it */ - goto no_kprobe; + goto preempt_out; } -set_current_kprobe (p, regs, kcb); - kcb->kprobe_status =KPROBE_HIT_ACTIVE ; - -if (p->pre_handler && p->pre_handler (p, regs)) -/* handler has already set things up, so skip ss setup */ -return 1 ; - ss_probe: + ret =1 ;#if !defined(CONFIG_PREEMPT ) || defined(CONFIG_PM )if (p->ainsn.boostable ==1 && !p->post_handler) {/* Boost up -- we can execute copied instructions directly */ reset_current_kprobe (); regs->ip = (unsigned long )p->ainsn.insn; -preempt_enable_no_resched (); -return 1 ; + goto preempt_out; }#endif prepare_singlestep (p, regs); kcb->kprobe_status =KPROBE_HIT_SS ; -return 1 ; + goto out; -no_kprobe: +preempt_out:preempt_enable_no_resched (); +out:return ret; }
Signature · 1 line
1.5.4.rc2.1097.gb6e0d
Hello Harvey,
Thank you for your great works!
Quoted 5 lines · Harvey Harrison
Make the control flow of
kprobe_handlermore obvious.Collapse the separate if blocks/gotos with if/else blocks this unifies the duplication of the check for a breakpoint instruction race with another cpu.
I agree it is good to unify the duplications of breakpoint checking and get_kprobe() calling.
Quoted 4 lines
Create two jump targets:
preempt_out: re-enables preemption before returning ret
out: only returns ret
However, I'm not sure we should change "no_kprobe". That label is commonly used in arch/*/kernel/kprobes.c.
And also, I prefer "return 1" to "{ret = 1; goto out;}" for simplicity.
Or, how about initializing "ret" as 1 instead of 0?
Ananth, Jim,
I'd like to hear your comments on it.
Quoted 21 lines
Signed-off-by Harvey Harrison <[email protected]>---
Masami, noticed a small bug in the previous version in the !p case when the breakpoint was the kernel's. Please review this version.arch/x86/kernel/kprobes.c | 60 +++++++++++++++++++++------------------------ 1 files changed, 28 insertions(+), 32 deletions(-)arch/x86/kernel/kprobes.c +0
-4@@ -480,32 +480,28 @@ static int __kprobes kprobe_handler(struct pt_regs *regs)preempt_disable (); kcb =get_kprobe_ctlblk (); -/* Check we're not actually recursing */ -if (kprobe_running ()) { - p =get_kprobe (addr); -if (p) {
arch/x86/kernel/kprobes.c static int __kprobes kprobe_handler(struct pt_regs *regs) · line 491
Quoted 10 lines
arch/x86/kernel/kprobes.c +7
-1+ p =get_kprobe (addr); +if (p) { +/* Check we're not actually recursing */ +if (kprobe_running ()) { ret =reenter_kprobe (p, regs, kcb);if (kcb->kprobe_status ==KPROBE_REENTER ) -return 1 ; + { + ret =1 ; + goto out;
I think "return 1" is better.
Quoted 7 lines
arch/x86/kernel/kprobes.c +2
-4+ } + goto preempt_out; }else { -if (*addr !=BREAKPOINT_INSTRUCTION ) { -/* The breakpoint instruction was removed by -* another cpu right after we hit, no further -* handling of this interrupt is appropriate
arch/x86/kernel/kprobes.c static int __kprobes kprobe_handler(struct pt_regs *regs) · line 501
Quoted 10 lines
arch/x86/kernel/kprobes.c +6
-3-*/ - regs->ip = (unsigned long )addr; +set_current_kprobe (p, regs, kcb); + kcb->kprobe_status =KPROBE_HIT_ACTIVE ; +if (p->pre_handler && p->pre_handler (p, regs)) + { +/* handler set things up, skip ss setup */ ret =1 ; - goto no_kprobe; + goto out;
Ditto.
arch/x86/kernel/kprobes.c static int __kprobes kprobe_handler(struct pt_regs *regs) · line 504
Quoted 11 lines
arch/x86/kernel/kprobes.c +1
-8} - p =__get_cpu_var (current_kprobe); -if (p->break_handler && p->break_handler (p, regs)) - goto ss_probe; } - goto no_kprobe; - } - - p =get_kprobe (addr); -if (!p) { + }else {
I think you'd better move "!p" block forward, because
this block means "relatively rare" cases. (sure, I know jprobe uses this block.)
Quoted 32 lines
arch/x86/kernel/kprobes.c +8
-8if (*addr !=BREAKPOINT_INSTRUCTION ) {/* * The breakpoint instruction was removed right @@ -518,34 +514,34 @@ static int __kprobes kprobe_handler(struct pt_regs *regs)*/ regs->ip = (unsigned long )addr; ret =1 ; + goto preempt_out; + } +if (kprobe_running ()) { + p =__get_cpu_var (current_kprobe); +if (p->break_handler && p->break_handler (p, regs)) + goto ss_probe; }/* Not one of ours: let kernel handle it */ - goto no_kprobe; + goto preempt_out; } -set_current_kprobe (p, regs, kcb); - kcb->kprobe_status =KPROBE_HIT_ACTIVE ; - -if (p->pre_handler && p->pre_handler (p, regs)) -/* handler has already set things up, so skip ss setup */ -return 1 ; - ss_probe: + ret =1 ;#if !defined(CONFIG_PREEMPT ) || defined(CONFIG_PM )if (p->ainsn.boostable ==1 && !p->post_handler) {/* Boost up -- we can execute copied instructions directly */ reset_current_kprobe ();
arch/x86/kernel/kprobes.c static int __kprobes kprobe_handler(struct pt_regs *regs) · line 540
Quoted 10 lines
arch/x86/kernel/kprobes.c +2
-3regs->ip = (unsigned long )p->ainsn.insn; -preempt_enable_no_resched (); - return 1; + goto preempt_out; }#endif prepare_singlestep (p, regs); kcb->kprobe_status =KPROBE_HIT_SS ; -return 1 ; + goto out;
I think "return 1" is better.
Quoted 8 lines
arch/x86/kernel/kprobes.c +2
-1-no_kprobe: +preempt_out:preempt_enable_no_resched (); +out:return ret; }
Signature · 7 lines
Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [email protected], [email protected]
Quoted 2 lines · Masami Hiramatsu
And also, I prefer "return 1" to "{ret = 1; goto out;}" for simplicity.
it seems that the code already uses 'ret', and sets it to 1 in certain cases - in that light it's a tiny bit cleaner to just have a single return code flow.
Ingo