Re: [PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()

[Christophe Leroy]

Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
patch_instruction() can fail in some scenarios. Add appropriate error
checking so that such failures are caught and logged, and suitable error
code is returned.

Fixes: d07df82c43be8 ("powerpc/kprobes: Move kprobes over to 
patch_instruction()")
Fixes: f3eca95638931 ("powerpc/kprobes/optprobes: Use patch_instruction()")
Signed-off-by: Naveen N. Rao <naveen.n....@linux.vnet.ibm.com>
---
  arch/powerpc/kernel/kprobes.c   | 10 +++-
  arch/powerpc/kernel/optprobes.c | 99 ++++++++++++++++++++++++++-------
  2 files changed, 87 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 81efb605113e..4a297ae2bd87 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -138,13 +138,19 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
  
  void arch_arm_kprobe(struct kprobe *p)
  {
-       patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+       int rc = patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+
+       if (rc)
+               WARN("Failed to patch trap at 0x%pK: %d\n", (void *)p->addr, 
rc);
  }
  NOKPROBE_SYMBOL(arch_arm_kprobe);
  
  void arch_disarm_kprobe(struct kprobe *p)
  {
-       patch_instruction(p->addr, p->opcode);
+       int rc = patch_instruction(p->addr, p->opcode);
+
+       if (rc)
+               WARN("Failed to remove trap at 0x%pK: %d\n", (void *)p->addr, 
rc);
  }
  NOKPROBE_SYMBOL(arch_disarm_kprobe);
  
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..046485bb0a52 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe 
*op)
        }
  }
  
+#define PATCH_INSN(addr, instr)						     \
+do {                                                                        \
+       int rc = patch_instruction((unsigned int *)(addr), instr);           \
+       if (rc) {                                                            \
+               pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", 
\
+                               __func__, __LINE__,                          \
+                               (void *)(addr), (void *)(addr), rc);         \
+               return rc;                                                   \
+       }                                                                    \
+} while (0)
+

I hate this kind of macro which hides the "return".

What about keeping the return action in the caller ?

Otherwise, what about implementing something based on the use of goto, 
on the same model as unsafe_put_user() for instance ?


Christophe