[PATCH 0/3] powerpc: Enhance error handling with patch_instruction()

[PATCH 0/3] powerpc: Enhance error handling with patch_instruction()

[Naveen N. Rao]

This patchset updates error handling with patch_instruction(). The first 
patch fixes an issue with do_patch_instruction() with STRICT_KERNEL_RWX 
wherein errors were not being returned back. The second and third 
patches update users of patch_instruction() in ftrace and kprobes code 
to properly validate return value from patch_instruction() and to notify 
errors.

- Naveen

Naveen N. Rao (3):
  powerpc: Properly return error code from do_patch_instruction()
  powerpc/ftrace: Simplify error checking when patching instructions
  powerpc/kprobes: Check return value of patch_instruction()

 arch/powerpc/kernel/kprobes.c      | 10 ++-
 arch/powerpc/kernel/optprobes.c    | 99 ++++++++++++++++++++++++------
 arch/powerpc/kernel/trace/ftrace.c | 69 ++++++++++-----------
 arch/powerpc/lib/code-patching.c   |  6 +-
 4 files changed, 123 insertions(+), 61 deletions(-)


base-commit: 8299da600ad05b8aa0f15ec0f5f03bd40e37d6f0
-- 
2.25.1

[PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Naveen N. Rao]

With STRICT_KERNEL_RWX, we are currently ignoring return value from
__patch_instruction() in do_patch_instruction(), resulting in the error
not being propagated back. Fix the same.

Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/lib/code-patching.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 3345f039a876..5c713a6c0bd8 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
 
 static int do_patch_instruction(unsigned int *addr, unsigned int instr)
 {
-	int err;
+	int err, rc = 0;
 	unsigned int *patch_addr = NULL;
 	unsigned long flags;
 	unsigned long text_poke_addr;
@@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
 	patch_addr = (unsigned int *)(text_poke_addr) +
 			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
 
-	__patch_instruction(addr, instr, patch_addr);
+	rc = __patch_instruction(addr, instr, patch_addr);
 
 	err = unmap_patch_area(text_poke_addr);
 	if (err)
@@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
 out:
 	local_irq_restore(flags);
 
-	return err;
+	return rc ? rc : err;
 }
 #else /* !CONFIG_STRICT_KERNEL_RWX */
 
-- 
2.25.1

[PATCH 2/3] powerpc/ftrace: Simplify error checking when patching instructions

[Naveen N. Rao]

Introduce a macro PATCH_INSN() to simplify instruction patching, and to
make the error messages more uniform and useful:
- print an error message that includes the original return value
- print the function name and line numbers, so that the offending
  location is clear
- always return -EPERM, which ftrace_bug() expects for proper error
  handling

Also eliminate use of patch_branch() since most such uses already call
create_branch() for error checking before patching. Instead, use the
return value from create_branch() with PATCH_INSN().

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/trace/ftrace.c | 69 ++++++++++++++----------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 7ea0ca044b65..5cf84c0c64cb 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -31,6 +31,17 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
+#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 -EPERM;						     \
+	}								     \
+} while (0)
+
 /*
  * We generally only have a single long_branch tramp and at most 2 or 3 plt
  * tramps generated. But, we don't use the plt tramps currently. We also allot
@@ -78,8 +89,7 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
 	}
 
 	/* replace the text with the new text */
-	if (patch_instruction((unsigned int *)ip, new))
-		return -EPERM;
+	PATCH_INSN(ip, new);
 
 	return 0;
 }
@@ -204,10 +214,7 @@ __ftrace_make_nop(struct module *mod,
 	}
 #endif /* CONFIG_MPROFILE_KERNEL */
 
-	if (patch_instruction((unsigned int *)ip, pop)) {
-		pr_err("Patching NOP failed.\n");
-		return -EPERM;
-	}
+	PATCH_INSN(ip, pop);
 
 	return 0;
 }
@@ -276,8 +283,7 @@ __ftrace_make_nop(struct module *mod,
 
 	op = PPC_INST_NOP;
 
-	if (patch_instruction((unsigned int *)ip, op))
-		return -EPERM;
+	PATCH_INSN(ip, op);
 
 	return 0;
 }
@@ -322,7 +328,7 @@ static int add_ftrace_tramp(unsigned long tramp)
  */
 static int setup_mcount_compiler_tramp(unsigned long tramp)
 {
-	int i, op;
+	unsigned int i, op;
 	unsigned long ptr;
 	static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];
 
@@ -366,16 +372,14 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
 #else
 	ptr = ppc_global_function_entry((void *)ftrace_caller);
 #endif
-	if (!create_branch((void *)tramp, ptr, 0)) {
+	op = create_branch((void *)tramp, ptr, 0);
+	if (!op) {
 		pr_debug("%ps is not reachable from existing mcount tramp\n",
 				(void *)ptr);
 		return -1;
 	}
 
-	if (patch_branch((unsigned int *)tramp, ptr, 0)) {
-		pr_debug("REL24 out of range!\n");
-		return -1;
-	}
+	PATCH_INSN(tramp, op);
 
 	if (add_ftrace_tramp(tramp)) {
 		pr_debug("No tramp locations left\n");
@@ -416,10 +420,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
 		}
 	}
 
-	if (patch_instruction((unsigned int *)ip, PPC_INST_NOP)) {
-		pr_err("Patching NOP failed.\n");
-		return -EPERM;
-	}
+	PATCH_INSN(ip, PPC_INST_NOP);
 
 	return 0;
 }
@@ -557,15 +558,13 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	}
 
 	/* Ensure branch is within 24 bits */
-	if (!create_branch(ip, tramp, BRANCH_SET_LINK)) {
+	op[0] = create_branch(ip, tramp, BRANCH_SET_LINK);
+	if (!op[0]) {
 		pr_err("Branch out of range\n");
 		return -EINVAL;
 	}
 
-	if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
-		pr_err("REL24 out of range!\n");
-		return -EINVAL;
-	}
+	PATCH_INSN(ip, op[0]);
 
 	return 0;
 }
@@ -603,8 +602,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 	pr_devel("write to %lx\n", rec->ip);
 
-	if (patch_instruction((unsigned int *)ip, op))
-		return -EPERM;
+	PATCH_INSN(ip, op);
 
 	return 0;
 }
@@ -650,11 +648,14 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
 		return -EINVAL;
 	}
 
-	if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
-		pr_err("Error patching branch to ftrace tramp!\n");
+	op = create_branch(ip, tramp, BRANCH_SET_LINK);
+	if (!op) {
+		pr_err("Branch out of range\n");
 		return -EINVAL;
 	}
 
+	PATCH_INSN(ip, op);
+
 	return 0;
 }
 
@@ -748,10 +749,8 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 	/* The new target may be within range */
 	if (test_24bit_addr(ip, addr)) {
 		/* within range */
-		if (patch_branch((unsigned int *)ip, addr, BRANCH_SET_LINK)) {
-			pr_err("REL24 out of range!\n");
-			return -EINVAL;
-		}
+		op = create_branch((unsigned int *)ip, addr, BRANCH_SET_LINK);
+		PATCH_INSN(ip, op);
 
 		return 0;
 	}
@@ -776,15 +775,13 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 	}
 
 	/* Ensure branch is within 24 bits */
-	if (!create_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK)) {
+	op = create_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK);
+	if (!op) {
 		pr_err("Branch out of range\n");
 		return -EINVAL;
 	}
 
-	if (patch_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK)) {
-		pr_err("REL24 out of range!\n");
-		return -EINVAL;
-	}
+	PATCH_INSN(ip, op);
 
 	return 0;
 }
-- 
2.25.1

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

[Naveen N. Rao]

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.rao@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)
+
 /*
  * emulate_step() requires insn to be emulated as
  * second parameter. Load register 'r4' with the
  * instruction.
  */
-void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
+static int patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
 {
 	/* addis r4,0,(insn)@h */
-	patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
+	PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
 			  ((val >> 16) & 0xffff));
 	addr++;
 
 	/* ori r4,r4,(insn)@l */
-	patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) |
+	PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(4) |
 			  ___PPC_RS(4) | (val & 0xffff));
+
+	return 0;
 }
 
 /*
  * Generate instructions to load provided immediate 64-bit value
  * to register 'r3' and patch these instructions at 'addr'.
  */
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+static int patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
 {
 	/* lis r3,(op)@highest */
-	patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
+	PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
 			  ((val >> 48) & 0xffff));
 	addr++;
 
 	/* ori r3,r3,(op)@higher */
-	patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(3) |
+	PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
 			  ___PPC_RS(3) | ((val >> 32) & 0xffff));
 	addr++;
 
 	/* rldicr r3,r3,32,31 */
-	patch_instruction(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
+	PATCH_INSN(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
 			  ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31));
 	addr++;
 
 	/* oris r3,r3,(op)@h */
-	patch_instruction(addr, PPC_INST_ORIS | ___PPC_RA(3) |
+	PATCH_INSN(addr, PPC_INST_ORIS | ___PPC_RA(3) |
 			  ___PPC_RS(3) | ((val >> 16) & 0xffff));
 	addr++;
 
 	/* ori r3,r3,(op)@l */
-	patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(3) |
+	PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
 			  ___PPC_RS(3) | (val & 0xffff));
+
+	return 0;
 }
 
 int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
@@ -216,14 +231,18 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	 * be within 32MB on either side of the current instruction.
 	 */
 	b_offset = (unsigned long)buff - (unsigned long)p->addr;
-	if (!is_offset_in_branch_range(b_offset))
+	if (!is_offset_in_branch_range(b_offset)) {
+		rc = -ERANGE;
 		goto error;
+	}
 
 	/* Check if the return address is also within 32MB range */
 	b_offset = (unsigned long)(buff + TMPL_RET_IDX) -
 			(unsigned long)nip;
-	if (!is_offset_in_branch_range(b_offset))
+	if (!is_offset_in_branch_range(b_offset)) {
+		rc = -ERANGE;
 		goto error;
+	}
 
 	/* Setup template */
 	/* We can optimize this via patch_instruction_window later */
@@ -231,15 +250,22 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	pr_devel("Copying template to %p, size %lu\n", buff, size);
 	for (i = 0; i < size; i++) {
 		rc = patch_instruction(buff + i, *(optprobe_template_entry + i));
-		if (rc < 0)
+		if (rc) {
+			pr_err("%s: Error copying optprobe template to 0x%pK: %d\n",
+					__func__, (void *)(buff + i), rc);
+			rc = -EFAULT;
 			goto error;
+		}
 	}
 
 	/*
 	 * Fixup the template with instructions to:
 	 * 1. load the address of the actual probepoint
 	 */
-	patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX);
+	if (patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX)) {
+		rc = -EFAULT;
+		goto error;
+	}
 
 	/*
 	 * 2. branch to optimized_callback() and emulate_step()
@@ -248,6 +274,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	emulate_step_addr = (kprobe_opcode_t *)ppc_kallsyms_lookup_name("emulate_step");
 	if (!op_callback_addr || !emulate_step_addr) {
 		WARN(1, "Unable to lookup optimized_callback()/emulate_step()\n");
+		rc = -ERANGE;
 		goto error;
 	}
 
@@ -259,21 +286,48 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 				(unsigned long)emulate_step_addr,
 				BRANCH_SET_LINK);
 
-	if (!branch_op_callback || !branch_emulate_step)
+	if (!branch_op_callback || !branch_emulate_step) {
+		rc = -ERANGE;
 		goto error;
+	}
 
-	patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
-	patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
+	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;
+	}
 
 	/*
 	 * 3. load instruction to be emulated into relevant register, and
 	 */
-	patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+	if (patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX)) {
+		rc = -EFAULT;
+		goto error;
+	}
 
 	/*
 	 * 4. branch back from trampoline
 	 */
-	patch_branch(buff + TMPL_RET_IDX, (unsigned long)nip, 0);
+	rc = patch_branch(buff + TMPL_RET_IDX, (unsigned long)nip, 0);
+	if (rc) {
+		pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
+				__func__, __LINE__,
+				(void *)(buff + TMPL_RET_IDX), rc);
+		rc = -EFAULT;
+		goto error;
+	}
 
 	flush_icache_range((unsigned long)buff,
 			   (unsigned long)(&buff[TMPL_END_IDX]));
@@ -284,7 +338,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 
 error:
 	free_ppc_optinsn_slot(buff, 0);
-	return -ERANGE;
+	return rc;
 
 }
 
@@ -307,6 +361,7 @@ void arch_optimize_kprobes(struct list_head *oplist)
 {
 	struct optimized_kprobe *op;
 	struct optimized_kprobe *tmp;
+	int rc;
 
 	list_for_each_entry_safe(op, tmp, oplist, list) {
 		/*
@@ -315,9 +370,13 @@ void arch_optimize_kprobes(struct list_head *oplist)
 		 */
 		memcpy(op->optinsn.copied_insn, op->kp.addr,
 					       RELATIVEJUMP_SIZE);
-		patch_instruction(op->kp.addr,
+		rc = patch_instruction(op->kp.addr,
 			create_branch((unsigned int *)op->kp.addr,
 				      (unsigned long)op->optinsn.insn, 0));
+		if (rc)
+			pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
+					__func__, __LINE__,
+					(void *)(op->kp.addr), rc);
 		list_del_init(&op->list);
 	}
 }
-- 
2.25.1

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.rao@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

Re: [PATCH 2/3] powerpc/ftrace: Simplify error checking when patching instructions

[Christophe Leroy]

Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> Introduce a macro PATCH_INSN() to simplify instruction patching, and to
> make the error messages more uniform and useful:
> - print an error message that includes the original return value
> - print the function name and line numbers, so that the offending
>    location is clear
> - always return -EPERM, which ftrace_bug() expects for proper error
>    handling
> 
> Also eliminate use of patch_branch() since most such uses already call
> create_branch() for error checking before patching. Instead, use the
> return value from create_branch() with PATCH_INSN().

I have the same comment here as for patch 3, this kind of macro hides 
the return action and can be dangerous.

What about implementing a macro that takes an explicit label as third 
argument and jump to that label in case of error ? On the same model as 
unsafe_put_user() ?

Christophe

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Christophe Leroy]

Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> With STRICT_KERNEL_RWX, we are currently ignoring return value from
> __patch_instruction() in do_patch_instruction(), resulting in the error
> not being propagated back. Fix the same.

Good patch.

Be aware that there is ongoing work which tend to wanting to replace 
error reporting by BUG_ON() . See 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

> 
> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/lib/code-patching.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 3345f039a876..5c713a6c0bd8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>   
>   static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   {
> -	int err;
> +	int err, rc = 0;
>   	unsigned int *patch_addr = NULL;
>   	unsigned long flags;
>   	unsigned long text_poke_addr;
> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   	patch_addr = (unsigned int *)(text_poke_addr) +
>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>   
> -	__patch_instruction(addr, instr, patch_addr);
> +	rc = __patch_instruction(addr, instr, patch_addr);
>   
>   	err = unmap_patch_area(text_poke_addr);
>   	if (err)
> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   out:
>   	local_irq_restore(flags);
>   
> -	return err;
> +	return rc ? rc : err;

That's not really consistent. __patch_instruction() and 
unmap_patch_area() return a valid minus errno, while in case of 
map_patch_area() failure, err has value -1

>   }
>   #else /* !CONFIG_STRICT_KERNEL_RWX */
>   
> 

Christophe

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Steven Rostedt]

On Thu, 23 Apr 2020 18:21:14 +0200
Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > __patch_instruction() in do_patch_instruction(), resulting in the error
> > not being propagated back. Fix the same.  
> 
> Good patch.
> 
> Be aware that there is ongoing work which tend to wanting to replace 
> error reporting by BUG_ON() . See 
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

Thanks for the reference. I still believe that WARN_ON() should be used in
99% of the cases, including here. And only do a BUG_ON() when you know
there's no recovering from it.

In fact, there's still BUG_ON()s in my code that I need to convert to
WARN_ON() (it was written when BUG_ON() was still acceptable ;-)

-- Steve

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

[Steven Rostedt]

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 ?

#define PATCH_INSN(addr, instr) \
({
	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);	     \
	rc;								     \
})


Then you can just do:

	ret = PATCH_INSN(...);
	if (ret)
		return ret;

in the code.

-- Steve

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Naveen N. Rao]

Christophe Leroy wrote:
> 
> 
> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> __patch_instruction() in do_patch_instruction(), resulting in the error
>> not being propagated back. Fix the same.
> 
> Good patch.
> 
> Be aware that there is ongoing work which tend to wanting to replace 
> error reporting by BUG_ON() . See 
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

Hah, I see that you pointed out this exact issue in your review there!

I had noticed this when Russell's series for STRICT_MODULE_RWX started 
causing kretprobe failures, due to one of the early boot-time patching 
failing silently.

I'll defer to Michael on which patch he prefers to take, between this 
one and the series you point out above.

> 
>> 
>> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/lib/code-patching.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>> index 3345f039a876..5c713a6c0bd8 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>>   
>>   static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   {
>> -	int err;
>> +	int err, rc = 0;
>>   	unsigned int *patch_addr = NULL;
>>   	unsigned long flags;
>>   	unsigned long text_poke_addr;
>> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   	patch_addr = (unsigned int *)(text_poke_addr) +
>>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>>   
>> -	__patch_instruction(addr, instr, patch_addr);
>> +	rc = __patch_instruction(addr, instr, patch_addr);
>>   
>>   	err = unmap_patch_area(text_poke_addr);
>>   	if (err)
>> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   out:
>>   	local_irq_restore(flags);
>>   
>> -	return err;
>> +	return rc ? rc : err;
> 
> That's not really consistent. __patch_instruction() and 
> unmap_patch_area() return a valid minus errno, while in case of 
> map_patch_area() failure, err has value -1

Not sure I follow -- I'm not changing what would be returned in those 
cases, just also capturing return value from __patch_instruction().

If anything, I've considered the different return codes to be a good 
thing -- return code gives you a clear idea of what exactly failed.


- Naveen

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Naveen N. Rao]

Hi Steve,

Steven Rostedt wrote:
> On Thu, 23 Apr 2020 18:21:14 +0200
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> 
>> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> > With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> > __patch_instruction() in do_patch_instruction(), resulting in the error
>> > not being propagated back. Fix the same.  
>> 
>> Good patch.
>> 
>> Be aware that there is ongoing work which tend to wanting to replace 
>> error reporting by BUG_ON() . See 
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
> 
> Thanks for the reference. I still believe that WARN_ON() should be used in
> 99% of the cases, including here. And only do a BUG_ON() when you know
> there's no recovering from it.

I'm not sure if you meant that we should have a WARN_ON() in 
patch_instruction(), or if it was about the users of 
patch_instruction(). As you're well aware, ftrace likes to do its own 
WARN_ON() if any of its operations fail through ftrace_bug(). That was 
the reason I didn't add anything here.


- Naveen

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

[Naveen N. Rao]

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.

> 
> #define PATCH_INSN(addr, instr) \
> ({
> 	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);	     \
> 	rc;								     \
> })
> 
> 
> Then you can just do:
> 
> 	ret = PATCH_INSN(...);
> 	if (ret)
> 		return ret;
> 
> in the code.

That's really nice. However, in this case, I guess I can simply use an 
inline function? The primary reason I used the macro was for including a 
'return' statement in it.


Thanks for the review!
- Naveen

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Steven Rostedt]

On Fri, 24 Apr 2020 23:37:06 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> >> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :  
> >> > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> >> > __patch_instruction() in do_patch_instruction(), resulting in the error
> >> > not being propagated back. Fix the same.    
> >> 
> >> Good patch.
> >> 
> >> Be aware that there is ongoing work which tend to wanting to replace 
> >> error reporting by BUG_ON() . See 
> >> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003  
> > 
> > Thanks for the reference. I still believe that WARN_ON() should be used in
> > 99% of the cases, including here. And only do a BUG_ON() when you know
> > there's no recovering from it.  
> 
> I'm not sure if you meant that we should have a WARN_ON() in 
> patch_instruction(), or if it was about the users of 
> patch_instruction(). As you're well aware, ftrace likes to do its own 
> WARN_ON() if any of its operations fail through ftrace_bug(). That was 
> the reason I didn't add anything here.

I'm fine with that too, and better reason not to call BUG_ON(), because I'm
guessing if we crash, we never make it to the ftrace_bug() which reports
information that can be used to debug what went wrong.

-- Steve

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

[Steven Rostedt]

On Fri, 24 Apr 2020 23:56:25 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> > #define PATCH_INSN(addr, instr) \
> > ({
> > 	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);	     \
> > 	rc;								     \
> > })
> > 
> > 
> > Then you can just do:
> > 
> > 	ret = PATCH_INSN(...);
> > 	if (ret)
> > 		return ret;
> > 
> > in the code.  
> 
> That's really nice. However, in this case, I guess I can simply use an 
> inline function? The primary reason I used the macro was for including a 
> 'return' statement in it.

I thought the primary reason was the __func__, __LINE__ which wont work as
expected as an inline.

-- Steve

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Christopher M. Riedl]

On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
> On Thu, 23 Apr 2020 18:21:14 +0200
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
> 
> > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > > __patch_instruction() in do_patch_instruction(), resulting in the error
> > > not being propagated back. Fix the same.  
> > 
> > Good patch.
> > 
> > Be aware that there is ongoing work which tend to wanting to replace 
> > error reporting by BUG_ON() . See 
> > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
>
> 
> Thanks for the reference. I still believe that WARN_ON() should be used
> in
> 99% of the cases, including here. And only do a BUG_ON() when you know
> there's no recovering from it.
>
> 
> In fact, there's still BUG_ON()s in my code that I need to convert to
> WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
>
Figured I'd chime in since I am working on that other series :) The
BUG_ON()s are _only_ in the init code to set things up to allow a
temporary mapping for patching a STRICT_RWX kernel later. There's no
ongoing work to "replace error reporting by BUG_ON()". If that initial
setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
warrants a BUG_ON(). I am still working on v2 of my RFC which does
return any __patch_instruction() error back to the caller of
patch_instruction() similar to this patch.
> 
> -- Steve
>
> 
>
> 

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

[Naveen N. Rao]

Steven Rostedt wrote:
> On Fri, 24 Apr 2020 23:56:25 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> > #define PATCH_INSN(addr, instr) \
>> > ({
>> > 	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);	     \
>> > 	rc;								     \
>> > })
>> > 
>> > 
>> > Then you can just do:
>> > 
>> > 	ret = PATCH_INSN(...);
>> > 	if (ret)
>> > 		return ret;
>> > 
>> > in the code.  
>> 
>> That's really nice. However, in this case, I guess I can simply use an 
>> inline function? The primary reason I used the macro was for including a 
>> 'return' statement in it.
> 
> I thought the primary reason was the __func__, __LINE__ which wont work as
> expected as an inline.

Ugh, you're right indeed. I clearly didn't think it through. :facepalm:

I'll use this variant.


Regards,
Naveen

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

[Christophe Leroy]

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

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

[Steven Rostedt]

On Sat, 25 Apr 2020 10:11:56 +0000
Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> 
> 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)						     \

With the explicit label as a parameter, makes it more evident that it
will do something (like jump) with that label.

I like this solution the best!

-- Steve


>   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)
> 

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Steven Rostedt]

On Fri, 24 Apr 2020 14:26:02 -0500
"Christopher M. Riedl" <cmr@informatik.wtf> wrote:

> On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
> > On Thu, 23 Apr 2020 18:21:14 +0200
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >   
> > > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :  
> > > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > > > __patch_instruction() in do_patch_instruction(), resulting in the error
> > > > not being propagated back. Fix the same.    
> > > 
> > > Good patch.
> > > 
> > > Be aware that there is ongoing work which tend to wanting to replace 
> > > error reporting by BUG_ON() . See 
> > > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003  
> >
> > 
> > Thanks for the reference. I still believe that WARN_ON() should be used
> > in
> > 99% of the cases, including here. And only do a BUG_ON() when you know
> > there's no recovering from it.
> >
> > 
> > In fact, there's still BUG_ON()s in my code that I need to convert to
> > WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
> >  
> Figured I'd chime in since I am working on that other series :) The
> BUG_ON()s are _only_ in the init code to set things up to allow a
> temporary mapping for patching a STRICT_RWX kernel later. There's no
> ongoing work to "replace error reporting by BUG_ON()". If that initial
> setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
> warrants a BUG_ON(). I am still working on v2 of my RFC which does
> return any __patch_instruction() error back to the caller of
> patch_instruction() similar to this patch.


I agree certain locations may warrant a BUG_ON(), but I wouldn't make a
generic operation like patch_instruction() BUG, as it may be used in
cases that do not warrant it (like setting up ftrace).

Deciding to BUG on not based on the return code of patch_instruction()
is the way to go IMO.

-- Steve

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Steven Rostedt]

On Sat, 25 Apr 2020 10:10:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Deciding to BUG on not based on the return code of patch_instruction()

That was suppose to be "to BUG on or not,"

-- Steve

> is the way to go IMO.

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

[Naveen N. Rao]

Christophe Leroy wrote:
> 
> 
> 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)

My earlier complaint was that this would still add a flow control 
statement, so didn't look to immediately address your original concern.  
However, I suppose introduction of an explicit label makes things a bit 
better.

In addition:

<snip>
> @@ -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);

I like how this variant can cover additional uses of patch_instruction() 
here.

I will use this variant. Thanks for the suggestion!


- Naveen

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

[Naveen N. Rao]

Steven Rostedt wrote:
> On Sat, 25 Apr 2020 10:11:56 +0000
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> 
>> 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)						     \
> 
> With the explicit label as a parameter, makes it more evident that it
> will do something (like jump) with that label.

I think I will also rename the macro to PATCH_INSN_OR_GOTO() to make it 
super evident :)

> 
> I like this solution the best!

Thanks for the feedback.


- Naveen

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Naveen N. Rao]

Christopher M. Riedl wrote:
> On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
>> On Thu, 23 Apr 2020 18:21:14 +0200
>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>> 
>> > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> > > __patch_instruction() in do_patch_instruction(), resulting in the error
>> > > not being propagated back. Fix the same.  
>> > 
>> > Good patch.
>> > 
>> > Be aware that there is ongoing work which tend to wanting to replace 
>> > error reporting by BUG_ON() . See 
>> > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
>>
>> 
>> Thanks for the reference. I still believe that WARN_ON() should be used
>> in
>> 99% of the cases, including here. And only do a BUG_ON() when you know
>> there's no recovering from it.
>>
>> 
>> In fact, there's still BUG_ON()s in my code that I need to convert to
>> WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
>>
> Figured I'd chime in since I am working on that other series :) The
> BUG_ON()s are _only_ in the init code to set things up to allow a
> temporary mapping for patching a STRICT_RWX kernel later. There's no
> ongoing work to "replace error reporting by BUG_ON()". If that initial
> setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
> warrants a BUG_ON(). I am still working on v2 of my RFC which does
> return any __patch_instruction() error back to the caller of
> patch_instruction() similar to this patch.

Ok, that's good to know. I will drop this patch from my series, since 
this can be done independently of the other changes.

- Naveen

Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

[Christophe Leroy]

Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> With STRICT_KERNEL_RWX, we are currently ignoring return value from
> __patch_instruction() in do_patch_instruction(), resulting in the error
> not being propagated back. Fix the same.
> 
> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

A similar patch was merged as 
https://github.com/linuxppc/linux/commit/a3483c3dd18c136785a31406fe27210649fc4fba#diff-e084bb6dc223aec74e7fc4208b7b260acc571bd5b50c9b709ec3de175cb1a979

Christophe


> ---
>   arch/powerpc/lib/code-patching.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 3345f039a876..5c713a6c0bd8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>   
>   static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   {
> -	int err;
> +	int err, rc = 0;
>   	unsigned int *patch_addr = NULL;
>   	unsigned long flags;
>   	unsigned long text_poke_addr;
> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   	patch_addr = (unsigned int *)(text_poke_addr) +
>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>   
> -	__patch_instruction(addr, instr, patch_addr);
> +	rc = __patch_instruction(addr, instr, patch_addr);
>   
>   	err = unmap_patch_area(text_poke_addr);
>   	if (err)
> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>   out:
>   	local_irq_restore(flags);
>   
> -	return err;
> +	return rc ? rc : err;
>   }
>   #else /* !CONFIG_STRICT_KERNEL_RWX */
>