Re: [PATCH 1/2] powerpc/kprobes: Remove redundant code

Le 18/02/2020 à 15:39, Naveen N. Rao a écrit :

Christophe Leroy wrote:

quoted

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 <redacted>
---
 arch/powerpc/kernel/kprobes.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

Good cleanup, thanks.

quoted

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