At the time being we have something like
if (something) {
p = get();
if (p) {
if (something_wrong)
goto out;
...
return;
} else if (a != b) {
if (some_error)
goto out;
...
}
goto out;
}
p = get();
if (!p) {
if (a != b) {
if (some_error)
goto out;
...
}
goto out;
}
This is similar to
p = get();
if (something) {
if (p) {
if (something_wrong)
goto out;
...
return;
}
}
if (!p) {
if (a != b) {
if (some_error)
goto out;
...
}
goto out;
}
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/kprobes.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index f8b848aa65bd..7a925eb76ec0 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -276,8 +276,8 @@ int kprobe_handler(struct pt_regs *regs)
kcb = get_kprobe_ctlblk();
/* Check we're not actually recursing */
+ p = get_kprobe(addr);
if (kprobe_running()) {
- p = get_kprobe(addr);
if (p) {
kprobe_opcode_t insn = *p->ainsn.insn;
if (kcb->kprobe_status == KPROBE_HIT_SS &&
@@ -308,22 +308,9 @@ int kprobe_handler(struct pt_regs *regs)
}
prepare_singlestep(p, regs);
return 1;
- } else if (*addr != BREAKPOINT_INSTRUCTION) {
- /* If trap variant, then it belongs not to us */
- kprobe_opcode_t cur_insn = *addr;
-
- if (is_trap(cur_insn))
- goto no_kprobe;
- /* The breakpoint instruction was removed by
- * another cpu right after we hit, no further
- * handling of this interrupt is appropriate
- */
- ret = 1;
}
- goto no_kprobe;
}
- p = get_kprobe(addr);
if (!p) {
if (*addr != BREAKPOINT_INSTRUCTION) {
/*
--
2.25.0
if (a) {
if (b)
do_something();
}
Is equivalent to
if (a & b)
do_something();
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/kprobes.c | 58 +++++++++++++++++------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 7a925eb76ec0..d7c80a078c1e 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -277,38 +277,36 @@ int kprobe_handler(struct pt_regs *regs)
/* Check we're not actually recursing */
p = get_kprobe(addr);
- if (kprobe_running()) {
- if (p) {
- kprobe_opcode_t insn = *p->ainsn.insn;
- if (kcb->kprobe_status == KPROBE_HIT_SS &&
- is_trap(insn)) {
- /* Turn off 'trace' bits */
- regs->msr &= ~MSR_SINGLESTEP;
- regs->msr |= kcb->kprobe_saved_msr;
- goto no_kprobe;
- }
- /* We have reentered the kprobe_handler(), since
- * another probe was hit while within the handler.
- * We here save the original kprobes variables and
- * just single step on the instruction of the new probe
- * without calling any user handlers.
- */
- save_previous_kprobe(kcb);
- set_current_kprobe(p, regs, kcb);
- kprobes_inc_nmissed_count(p);
- kcb->kprobe_status = KPROBE_REENTER;
- if (p->ainsn.boostable >= 0) {
- ret = try_to_emulate(p, regs);
-
- if (ret > 0) {
- restore_previous_kprobe(kcb);
- preempt_enable_no_resched();
- return 1;
- }
+ if (kprobe_running() && p) {
+ kprobe_opcode_t insn = *p->ainsn.insn;
+
+ if (kcb->kprobe_status == KPROBE_HIT_SS && is_trap(insn)) {
+ /* Turn off 'trace' bits */
+ regs->msr &= ~MSR_SINGLESTEP;
+ regs->msr |= kcb->kprobe_saved_msr;
+ goto no_kprobe;
+ }
+ /* We have reentered the kprobe_handler(), since
+ * another probe was hit while within the handler.
+ * We here save the original kprobes variables and
+ * just single step on the instruction of the new probe
+ * without calling any user handlers.
+ */
+ save_previous_kprobe(kcb);
+ set_current_kprobe(p, regs, kcb);
+ kprobes_inc_nmissed_count(p);
+ kcb->kprobe_status = KPROBE_REENTER;
+ if (p->ainsn.boostable >= 0) {
+ ret = try_to_emulate(p, regs);
+
+ if (ret > 0) {
+ restore_previous_kprobe(kcb);
+ preempt_enable_no_resched();
+ return 1;
}
- prepare_singlestep(p, regs);
- return 1;
}
+ prepare_singlestep(p, regs);
+ return 1;
}
if (!p) {
--
2.25.0
Christophe Leroy wrote:
> At the time being we have something like
>
> if (something) {
> p = get();
> if (p) {
> if (something_wrong)
> goto out;
> ...
> return;
> } else if (a != b) {
> if (some_error)
> goto out;
> ...
> }
> goto out;
> }
> p = get();
> if (!p) {
> if (a != b) {
> if (some_error)
> goto out;
> ...
> }
> goto out;
> }
>
> This is similar to
>
> p = get();
> if (something) {
> if (p) {
> if (something_wrong)
> goto out;
> ...
> return;
> }
> }
> if (!p) {
> if (a != b) {
> if (some_error)
> goto out;
> ...
> }
> goto out;
> }
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/kernel/kprobes.c | 15 +--------------
> 1 file changed, 1 insertion(+), 14 deletions(-)
Good cleanup, thanks.
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index f8b848aa65bd..7a925eb76ec0 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -276,8 +276,8 @@ int kprobe_handler(struct pt_regs *regs)
> kcb = get_kprobe_ctlblk();
>
> /* Check we're not actually recursing */
> + p = get_kprobe(addr);
> if (kprobe_running()) {
> - p = get_kprobe(addr);
> if (p) {
> kprobe_opcode_t insn = *p->ainsn.insn;
> if (kcb->kprobe_status == KPROBE_HIT_SS &&
> @@ -308,22 +308,9 @@ int kprobe_handler(struct pt_regs *regs)
> }
> prepare_singlestep(p, regs);
> return 1;
> - } else if (*addr != BREAKPOINT_INSTRUCTION) {
> - /* If trap variant, then it belongs not to us */
> - kprobe_opcode_t cur_insn = *addr;
> -
> - if (is_trap(cur_insn))
> - goto no_kprobe;
> - /* The breakpoint instruction was removed by
> - * another cpu right after we hit, no further
> - * handling of this interrupt is appropriate
> - */
> - ret = 1;
> }
> - goto no_kprobe;
A minot nit -- removing the above goto makes a slight change to the
logic. But, see my comments for the next patch.
- Naveen
> }
>
> - p = get_kprobe(addr);
> if (!p) {
> if (*addr != BREAKPOINT_INSTRUCTION) {
> /*
> --
> 2.25.0
>
>
Christophe Leroy wrote:
> if (a) {
> if (b)
> do_something();
> }
>
> Is equivalent to
>
> if (a & b)
> do_something();
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/kernel/kprobes.c | 58 +++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 7a925eb76ec0..d7c80a078c1e 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -277,38 +277,36 @@ int kprobe_handler(struct pt_regs *regs)
>
> /* Check we're not actually recursing */
> p = get_kprobe(addr);
> - if (kprobe_running()) {
> - if (p) {
> - kprobe_opcode_t insn = *p->ainsn.insn;
> - if (kcb->kprobe_status == KPROBE_HIT_SS &&
> - is_trap(insn)) {
> - /* Turn off 'trace' bits */
> - regs->msr &= ~MSR_SINGLESTEP;
> - regs->msr |= kcb->kprobe_saved_msr;
> - goto no_kprobe;
> - }
> - /* We have reentered the kprobe_handler(), since
> - * another probe was hit while within the handler.
> - * We here save the original kprobes variables and
> - * just single step on the instruction of the new probe
> - * without calling any user handlers.
> - */
> - save_previous_kprobe(kcb);
> - set_current_kprobe(p, regs, kcb);
> - kprobes_inc_nmissed_count(p);
> - kcb->kprobe_status = KPROBE_REENTER;
> - if (p->ainsn.boostable >= 0) {
> - ret = try_to_emulate(p, regs);
> -
> - if (ret > 0) {
> - restore_previous_kprobe(kcb);
> - preempt_enable_no_resched();
> - return 1;
> - }
> + if (kprobe_running() && p) {
> + kprobe_opcode_t insn = *p->ainsn.insn;
> +
> + if (kcb->kprobe_status == KPROBE_HIT_SS && is_trap(insn)) {
> + /* Turn off 'trace' bits */
> + regs->msr &= ~MSR_SINGLESTEP;
> + regs->msr |= kcb->kprobe_saved_msr;
> + goto no_kprobe;
> + }
> + /* We have reentered the kprobe_handler(), since
> + * another probe was hit while within the handler.
> + * We here save the original kprobes variables and
> + * just single step on the instruction of the new probe
> + * without calling any user handlers.
> + */
> + save_previous_kprobe(kcb);
> + set_current_kprobe(p, regs, kcb);
> + kprobes_inc_nmissed_count(p);
> + kcb->kprobe_status = KPROBE_REENTER;
> + if (p->ainsn.boostable >= 0) {
> + ret = try_to_emulate(p, regs);
> +
> + if (ret > 0) {
> + restore_previous_kprobe(kcb);
> + preempt_enable_no_resched();
> + return 1;
> }
> - prepare_singlestep(p, regs);
> - return 1;
> }
> + prepare_singlestep(p, regs);
> + return 1;
> }
>
If we move the below !p case before the check for kprobe_running() right
after get_kprobe(), we won't need to check for (p) above and we won't
have any change in logic from Patch 1.
> if (!p) {
- Naveen
Le 18/02/2020 à 15:39, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> At the time being we have something like
>>
>> if (something) {
>> p = get();
>> if (p) {
>> if (something_wrong)
>> goto out;
>> ...
>> return;
>> } else if (a != b) {
>> if (some_error)
>> goto out;
>> ...
>> }
>> goto out;
>> }
>> p = get();
>> if (!p) {
>> if (a != b) {
>> if (some_error)
>> goto out;
>> ...
>> }
>> goto out;
>> }
>>
>> This is similar to
>>
>> p = get();
>> if (something) {
>> if (p) {
>> if (something_wrong)
>> goto out;
>> ...
>> return;
>> }
>> }
>> if (!p) {
>> if (a != b) {
>> if (some_error)
>> goto out;
>> ...
>> }
>> goto out;
>> }
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> arch/powerpc/kernel/kprobes.c | 15 +--------------
>> 1 file changed, 1 insertion(+), 14 deletions(-)
>
> Good cleanup, thanks.
>
>>
>> diff --git a/arch/powerpc/kernel/kprobes.c
>> b/arch/powerpc/kernel/kprobes.c
>> index f8b848aa65bd..7a925eb76ec0 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -276,8 +276,8 @@ int kprobe_handler(struct pt_regs *regs)
>> kcb = get_kprobe_ctlblk();
>>
>> /* Check we're not actually recursing */
>> + p = get_kprobe(addr);
>> if (kprobe_running()) {
>> - p = get_kprobe(addr);
>> if (p) {
>> kprobe_opcode_t insn = *p->ainsn.insn;
>> if (kcb->kprobe_status == KPROBE_HIT_SS &&
>> @@ -308,22 +308,9 @@ int kprobe_handler(struct pt_regs *regs)
>> }
>> prepare_singlestep(p, regs);
>> return 1;
>> - } else if (*addr != BREAKPOINT_INSTRUCTION) {
>> - /* If trap variant, then it belongs not to us */
>> - kprobe_opcode_t cur_insn = *addr;
>> -
>> - if (is_trap(cur_insn))
>> - goto no_kprobe;
>> - /* The breakpoint instruction was removed by
>> - * another cpu right after we hit, no further
>> - * handling of this interrupt is appropriate
>> - */
>> - ret = 1;
>> }
>> - goto no_kprobe;
>
> A minot nit -- removing the above goto makes a slight change to the
> logic. But, see my comments for the next patch.
All legs of the (p) case are have either a return or a goto, so that
goto no_kprobe is limited to the !p case, we have to fall_through now.
Christophe