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

[Christophe Leroy <christophe.leroy@c-s.fr>]

On 04/24/2020 06:26 PM, Naveen N. Rao wrote:
> Steven Rostedt wrote:
>> On Thu, 23 Apr 2020 17:41:52 +0200
>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>> > 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 ?
> 
> Thanks for the review.
> 
> I noticed this as a warning from checkpatch.pl, but this looked compact 
> and correct for use in the two following functions. You'll notice that I 
> added it just before the two functions this is used in.
> 
> I suppose 'goto err' is usable too, but the ftrace code (patch 2) will 
> end up with more changes. I'm also struggling to see how a 'goto' is 
> less offensive. I think Steve's suggestion below would be the better way 
> to go, to make things explicit.
> 

Sure it's be more explicit, but then more lines also. 3 lines for only 
one really usefull.

With goto, I would look like:

diff --git a/arch/powerpc/kernel/optprobes.c 
b/arch/powerpc/kernel/optprobes.c
index 046485bb0a52..938208f824da 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,14 +139,14 @@ void arch_remove_optimized_kprobe(struct 
optimized_kprobe *op)
  	}
  }

-#define PATCH_INSN(addr, instr)						     \
+#define PATCH_INSN(addr, instr, label)						     \
  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;						     \
+		goto label;						     \
  	}								     \
  } while (0)

@@ -159,14 +159,17 @@ static int patch_imm32_load_insns(unsigned int 
val, kprobe_opcode_t *addr)
  {
  	/* addis r4,0,(insn)@h */
  	PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
-			  ((val >> 16) & 0xffff));
+			  ((val >> 16) & 0xffff), failed);
  	addr++;

  	/* ori r4,r4,(insn)@l */
  	PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(4) |
-			  ___PPC_RS(4) | (val & 0xffff));
+			  ___PPC_RS(4) | (val & 0xffff), failed);

  	return 0;
+
+failed:
+	return -EFAULT;
  }

  /*
@@ -177,29 +180,32 @@ static int patch_imm64_load_insns(unsigned long 
val, kprobe_opcode_t *addr)
  {
  	/* lis r3,(op)@highest */
  	PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
-			  ((val >> 48) & 0xffff));
+			  ((val >> 48) & 0xffff), failed);
  	addr++;

  	/* ori r3,r3,(op)@higher */
  	PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
-			  ___PPC_RS(3) | ((val >> 32) & 0xffff));
+			  ___PPC_RS(3) | ((val >> 32) & 0xffff), failed);
  	addr++;

  	/* rldicr r3,r3,32,31 */
  	PATCH_INSN(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
-			  ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31));
+			  ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31), failed);
  	addr++;

  	/* oris r3,r3,(op)@h */
  	PATCH_INSN(addr, PPC_INST_ORIS | ___PPC_RA(3) |
-			  ___PPC_RS(3) | ((val >> 16) & 0xffff));
+			  ___PPC_RS(3) | ((val >> 16) & 0xffff), failed);
  	addr++;

  	/* ori r3,r3,(op)@l */
  	PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
-			  ___PPC_RS(3) | (val & 0xffff));
+			  ___PPC_RS(3) | (val & 0xffff), failed);

  	return 0;
+
+failed:
+	return -EFAULT;
  }

  int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct 
kprobe *p)
@@ -291,23 +297,8 @@ int arch_prepare_optimized_kprobe(struct 
optimized_kprobe *op, struct kprobe *p)
  		goto error;
  	}

-	rc = patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
-	if (rc) {
-		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
-				__func__, __LINE__,
-				(void *)(buff + TMPL_CALL_HDLR_IDX), rc);
-		rc = -EFAULT;
-		goto error;
-	}
-
-	rc = patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
-	if (rc) {
-		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
-				__func__, __LINE__,
-				(void *)(buff + TMPL_EMULATE_IDX), rc);
-		rc = -EFAULT;
-		goto error;
-	}
+	PATCH_INSN(buff + TMPL_CALL_HDLR_IDX, branch_op_callback, efault);
+	PATCH_INSN(buff + TMPL_EMULATE_IDX, branch_emulate_step, efault);

  	/*
  	 * 3. load instruction to be emulated into relevant register, and
@@ -336,6 +327,8 @@ int arch_prepare_optimized_kprobe(struct 
optimized_kprobe *op, struct kprobe *p)

  	return 0;

+efault:
+	rc = -EFAULT;
  error:
  	free_ppc_optinsn_slot(buff, 0);
  	return rc;


Christophe