[PATCH 0/2] error-injection: simplify code and powerpc support

[PATCH 0/2] error-injection: simplify code and powerpc support

[Naveen N. Rao]

The first patch simplifies code around function error-injection by 
limiting the need for arch-specific helpers. The second patch adds 
support for powerpc.

- Naveen

Naveen N. Rao (2):
  error-injection: Simplify arch specific helpers
  powerpc: Add support for function error injection

 arch/powerpc/Kconfig                       |  1 +
 arch/powerpc/include/asm/error-injection.h |  9 +++++++++
 arch/powerpc/include/asm/ptrace.h          |  5 +++++
 arch/x86/include/asm/error-injection.h     |  6 +-----
 arch/x86/lib/Makefile                      |  1 -
 arch/x86/lib/error-inject.c                | 20 --------------------
 include/asm-generic/error-injection.h      |  6 ++++++
 include/linux/error-injection.h            |  1 +
 kernel/fail_function.c                     |  2 +-
 kernel/trace/bpf_trace.c                   |  2 +-
 lib/error-inject.c                         |  8 ++++++++
 11 files changed, 33 insertions(+), 28 deletions(-)
 create mode 100644 arch/powerpc/include/asm/error-injection.h
 delete mode 100644 arch/x86/lib/error-inject.c

-- 
2.17.0

[PATCH 1/2] error-injection: Simplify arch specific helpers

[Naveen N. Rao]

We already have an arch-independent way to set the instruction pointer
with instruction_pointer_set(). Using this allows us to get rid of the
need for override_function_with_return() that each architecture has to
implement.

Furthermore, just_return_func() only has to encode arch-specific
assembly instructions to return from a function. Introduce a macro
ARCH_FUNC_RET to provide the arch-specific instruction and move over
just_return_func() to generic code.

With these changes, architectures that already support kprobes, only
just need to ensure they provide regs_set_return_value(), GET_IP() (for
instruction_pointer_set()), and ARCH_FUNC_RET to support error
injection.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/x86/include/asm/error-injection.h |  6 +-----
 arch/x86/lib/Makefile                  |  1 -
 arch/x86/lib/error-inject.c            | 20 --------------------
 include/asm-generic/error-injection.h  |  6 ++++++
 include/linux/error-injection.h        |  1 +
 kernel/fail_function.c                 |  2 +-
 kernel/trace/bpf_trace.c               |  2 +-
 lib/error-inject.c                     |  8 ++++++++
 8 files changed, 18 insertions(+), 28 deletions(-)
 delete mode 100644 arch/x86/lib/error-inject.c

diff --git a/arch/x86/include/asm/error-injection.h b/arch/x86/include/asm/error-injection.h
index 47b7a1296245..f3f22e237b86 100644
--- a/arch/x86/include/asm/error-injection.h
+++ b/arch/x86/include/asm/error-injection.h
@@ -2,12 +2,8 @@
 #ifndef _ASM_ERROR_INJECTION_H
 #define _ASM_ERROR_INJECTION_H
 
-#include <linux/compiler.h>
-#include <linux/linkage.h>
-#include <asm/ptrace.h>
 #include <asm-generic/error-injection.h>
 
-asmlinkage void just_return_func(void);
-void override_function_with_return(struct pt_regs *regs);
+#define ARCH_FUNC_RET	"ret"
 
 #endif /* _ASM_ERROR_INJECTION_H */
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 25a972c61b0a..f23934bbaf4e 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -26,7 +26,6 @@ lib-y += memcpy_$(BITS).o
 lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
 lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-eval.o
 lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
-lib-$(CONFIG_FUNCTION_ERROR_INJECTION)	+= error-inject.o
 lib-$(CONFIG_RETPOLINE) += retpoline.o
 
 obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o
diff --git a/arch/x86/lib/error-inject.c b/arch/x86/lib/error-inject.c
deleted file mode 100644
index 3cdf06128d13..000000000000
--- a/arch/x86/lib/error-inject.c
+++ /dev/null
@@ -1,20 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <linux/error-injection.h>
-#include <linux/kprobes.h>
-
-asmlinkage void just_return_func(void);
-
-asm(
-	".type just_return_func, @function\n"
-	".globl just_return_func\n"
-	"just_return_func:\n"
-	"	ret\n"
-	".size just_return_func, .-just_return_func\n"
-);
-
-void override_function_with_return(struct pt_regs *regs)
-{
-	regs->ip = (unsigned long)&just_return_func;
-}
-NOKPROBE_SYMBOL(override_function_with_return);
diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
index 296c65442f00..8ac152cc204a 100644
--- a/include/asm-generic/error-injection.h
+++ b/include/asm-generic/error-injection.h
@@ -3,6 +3,9 @@
 #define _ASM_GENERIC_ERROR_INJECTION_H
 
 #if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#include <linux/compiler.h>
+#include <linux/linkage.h>
+
 enum {
 	EI_ETYPE_NONE,		/* Dummy value for undefined case */
 	EI_ETYPE_NULL,		/* Return NULL if failure */
@@ -27,6 +30,9 @@ static struct error_injection_entry __used				\
 		.addr = (unsigned long)fname,				\
 		.etype = EI_ETYPE_##_etype,				\
 	};
+
+asmlinkage void just_return_func(void);
+
 #else
 #define ALLOW_ERROR_INJECTION(fname, _etype)
 #endif
diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h
index 280c61ecbf20..f4a0b23423d2 100644
--- a/include/linux/error-injection.h
+++ b/include/linux/error-injection.h
@@ -4,6 +4,7 @@
 
 #ifdef CONFIG_FUNCTION_ERROR_INJECTION
 
+#include <linux/types.h>
 #include <asm/error-injection.h>
 
 extern bool within_error_injection_list(unsigned long addr);
diff --git a/kernel/fail_function.c b/kernel/fail_function.c
index 1d5632d8bbcc..0ae2ca4a29e8 100644
--- a/kernel/fail_function.c
+++ b/kernel/fail_function.c
@@ -183,7 +183,7 @@ static int fei_kprobe_handler(struct kprobe *kp, struct pt_regs *regs)
 
 	if (should_fail(&fei_fault_attr, 1)) {
 		regs_set_return_value(regs, attr->retval);
-		override_function_with_return(regs);
+		instruction_pointer_set(regs, (unsigned long)&just_return_func);
 		/* Kprobe specific fixup */
 		reset_current_kprobe();
 		preempt_enable_no_resched();
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 56ba0f2a01db..23f1f4ffda6c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -84,7 +84,7 @@ EXPORT_SYMBOL_GPL(trace_call_bpf);
 BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
 {
 	regs_set_return_value(regs, rc);
-	override_function_with_return(regs);
+	instruction_pointer_set(regs, (unsigned long)&just_return_func);
 	return 0;
 }
 
diff --git a/lib/error-inject.c b/lib/error-inject.c
index c0d4600f4896..7fdc92b5babc 100644
--- a/lib/error-inject.c
+++ b/lib/error-inject.c
@@ -20,6 +20,14 @@ struct ei_entry {
 	void *priv;
 };
 
+asm(
+	".type just_return_func, @function\n"
+	".globl just_return_func\n"
+	"just_return_func:\n"
+	ARCH_FUNC_RET "\n"
+	".size just_return_func, .-just_return_func\n"
+);
+
 bool within_error_injection_list(unsigned long addr)
 {
 	struct ei_entry *ent;
-- 
2.17.0

[PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/Kconfig                       | 1 +
 arch/powerpc/include/asm/error-injection.h | 9 +++++++++
 arch/powerpc/include/asm/ptrace.h          | 5 +++++
 3 files changed, 15 insertions(+)
 create mode 100644 arch/powerpc/include/asm/error-injection.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 23247fa551e7..ed1ab693f945 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -194,6 +194,7 @@ config PPC
 	select HAVE_EBPF_JIT			if PPC64
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
 	select HAVE_FTRACE_MCOUNT_RECORD
+	select HAVE_FUNCTION_ERROR_INJECTION
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACER
 	select HAVE_GCC_PLUGINS
diff --git a/arch/powerpc/include/asm/error-injection.h b/arch/powerpc/include/asm/error-injection.h
new file mode 100644
index 000000000000..b596eca04ef9
--- /dev/null
+++ b/arch/powerpc/include/asm/error-injection.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ERROR_INJECTION_H
+#define _ASM_ERROR_INJECTION_H
+
+#include <asm-generic/error-injection.h>
+
+#define ARCH_FUNC_RET	"blr"
+
+#endif /* _ASM_ERROR_INJECTION_H */
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index e4923686e43a..c0705296c2f0 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -101,6 +101,11 @@ static inline long regs_return_value(struct pt_regs *regs)
 		return -regs->gpr[3];
 }
 
+static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
+{
+	regs->gpr[3] = rc;
+}
+
 #ifdef __powerpc64__
 #define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
 #else
-- 
2.17.0

Re: [PATCH 1/2] error-injection: Simplify arch specific helpers

[Masami Hiramatsu]

On Tue, 29 May 2018 18:06:02 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> We already have an arch-independent way to set the instruction pointer
> with instruction_pointer_set(). Using this allows us to get rid of the
> need for override_function_with_return() that each architecture has to
> implement.
> 
> Furthermore, just_return_func() only has to encode arch-specific
> assembly instructions to return from a function. Introduce a macro
> ARCH_FUNC_RET to provide the arch-specific instruction and move over
> just_return_func() to generic code.
> 
> With these changes, architectures that already support kprobes, only
> just need to ensure they provide regs_set_return_value(), GET_IP() (for
> instruction_pointer_set()), and ARCH_FUNC_RET to support error
> injection.

Nice! the code basically good to me. Just one comment, ARCH_FUNC_RET sounds
like a function. Maybe ARCH_RETURN_INSTRUCTION will be better name, isn't it? :)

Thank you,

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/x86/include/asm/error-injection.h |  6 +-----
>  arch/x86/lib/Makefile                  |  1 -
>  arch/x86/lib/error-inject.c            | 20 --------------------
>  include/asm-generic/error-injection.h  |  6 ++++++
>  include/linux/error-injection.h        |  1 +
>  kernel/fail_function.c                 |  2 +-
>  kernel/trace/bpf_trace.c               |  2 +-
>  lib/error-inject.c                     |  8 ++++++++
>  8 files changed, 18 insertions(+), 28 deletions(-)
>  delete mode 100644 arch/x86/lib/error-inject.c
> 
> diff --git a/arch/x86/include/asm/error-injection.h b/arch/x86/include/asm/error-injection.h
> index 47b7a1296245..f3f22e237b86 100644
> --- a/arch/x86/include/asm/error-injection.h
> +++ b/arch/x86/include/asm/error-injection.h
> @@ -2,12 +2,8 @@
>  #ifndef _ASM_ERROR_INJECTION_H
>  #define _ASM_ERROR_INJECTION_H
>  
> -#include <linux/compiler.h>
> -#include <linux/linkage.h>
> -#include <asm/ptrace.h>
>  #include <asm-generic/error-injection.h>
>  
> -asmlinkage void just_return_func(void);
> -void override_function_with_return(struct pt_regs *regs);
> +#define ARCH_FUNC_RET	"ret"
>  
>  #endif /* _ASM_ERROR_INJECTION_H */
> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> index 25a972c61b0a..f23934bbaf4e 100644
> --- a/arch/x86/lib/Makefile
> +++ b/arch/x86/lib/Makefile
> @@ -26,7 +26,6 @@ lib-y += memcpy_$(BITS).o
>  lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
>  lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-eval.o
>  lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
> -lib-$(CONFIG_FUNCTION_ERROR_INJECTION)	+= error-inject.o
>  lib-$(CONFIG_RETPOLINE) += retpoline.o
>  
>  obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o
> diff --git a/arch/x86/lib/error-inject.c b/arch/x86/lib/error-inject.c
> deleted file mode 100644
> index 3cdf06128d13..000000000000
> --- a/arch/x86/lib/error-inject.c
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -
> -#include <linux/error-injection.h>
> -#include <linux/kprobes.h>
> -
> -asmlinkage void just_return_func(void);
> -
> -asm(
> -	".type just_return_func, @function\n"
> -	".globl just_return_func\n"
> -	"just_return_func:\n"
> -	"	ret\n"
> -	".size just_return_func, .-just_return_func\n"
> -);
> -
> -void override_function_with_return(struct pt_regs *regs)
> -{
> -	regs->ip = (unsigned long)&just_return_func;
> -}
> -NOKPROBE_SYMBOL(override_function_with_return);
> diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
> index 296c65442f00..8ac152cc204a 100644
> --- a/include/asm-generic/error-injection.h
> +++ b/include/asm-generic/error-injection.h
> @@ -3,6 +3,9 @@
>  #define _ASM_GENERIC_ERROR_INJECTION_H
>  
>  #if defined(__KERNEL__) && !defined(__ASSEMBLY__)
> +#include <linux/compiler.h>
> +#include <linux/linkage.h>
> +
>  enum {
>  	EI_ETYPE_NONE,		/* Dummy value for undefined case */
>  	EI_ETYPE_NULL,		/* Return NULL if failure */
> @@ -27,6 +30,9 @@ static struct error_injection_entry __used				\
>  		.addr = (unsigned long)fname,				\
>  		.etype = EI_ETYPE_##_etype,				\
>  	};
> +
> +asmlinkage void just_return_func(void);
> +
>  #else
>  #define ALLOW_ERROR_INJECTION(fname, _etype)
>  #endif
> diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h
> index 280c61ecbf20..f4a0b23423d2 100644
> --- a/include/linux/error-injection.h
> +++ b/include/linux/error-injection.h
> @@ -4,6 +4,7 @@
>  
>  #ifdef CONFIG_FUNCTION_ERROR_INJECTION
>  
> +#include <linux/types.h>
>  #include <asm/error-injection.h>
>  
>  extern bool within_error_injection_list(unsigned long addr);
> diff --git a/kernel/fail_function.c b/kernel/fail_function.c
> index 1d5632d8bbcc..0ae2ca4a29e8 100644
> --- a/kernel/fail_function.c
> +++ b/kernel/fail_function.c
> @@ -183,7 +183,7 @@ static int fei_kprobe_handler(struct kprobe *kp, struct pt_regs *regs)
>  
>  	if (should_fail(&fei_fault_attr, 1)) {
>  		regs_set_return_value(regs, attr->retval);
> -		override_function_with_return(regs);
> +		instruction_pointer_set(regs, (unsigned long)&just_return_func);
>  		/* Kprobe specific fixup */
>  		reset_current_kprobe();
>  		preempt_enable_no_resched();
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 56ba0f2a01db..23f1f4ffda6c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -84,7 +84,7 @@ EXPORT_SYMBOL_GPL(trace_call_bpf);
>  BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
>  {
>  	regs_set_return_value(regs, rc);
> -	override_function_with_return(regs);
> +	instruction_pointer_set(regs, (unsigned long)&just_return_func);
>  	return 0;
>  }
>  
> diff --git a/lib/error-inject.c b/lib/error-inject.c
> index c0d4600f4896..7fdc92b5babc 100644
> --- a/lib/error-inject.c
> +++ b/lib/error-inject.c
> @@ -20,6 +20,14 @@ struct ei_entry {
>  	void *priv;
>  };
>  
> +asm(
> +	".type just_return_func, @function\n"
> +	".globl just_return_func\n"
> +	"just_return_func:\n"
> +	ARCH_FUNC_RET "\n"
> +	".size just_return_func, .-just_return_func\n"
> +);
> +
>  bool within_error_injection_list(unsigned long addr)
>  {
>  	struct ei_entry *ent;
> -- 
> 2.17.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> ...

A change log is always nice even if it's short :)

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/Kconfig                       | 1 +
>  arch/powerpc/include/asm/error-injection.h | 9 +++++++++
>  arch/powerpc/include/asm/ptrace.h          | 5 +++++
>  3 files changed, 15 insertions(+)
>  create mode 100644 arch/powerpc/include/asm/error-injection.h

This looks fine to me, it's probably easiest if it goes in via tip along
with patch 1.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 23247fa551e7..ed1ab693f945 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -194,6 +194,7 @@ config PPC
>  	select HAVE_EBPF_JIT			if PPC64
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>  	select HAVE_FTRACE_MCOUNT_RECORD
> +	select HAVE_FUNCTION_ERROR_INJECTION
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_GCC_PLUGINS
> diff --git a/arch/powerpc/include/asm/error-injection.h b/arch/powerpc/include/asm/error-injection.h
> new file mode 100644
> index 000000000000..b596eca04ef9
> --- /dev/null
> +++ b/arch/powerpc/include/asm/error-injection.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ERROR_INJECTION_H
> +#define _ASM_ERROR_INJECTION_H
> +
> +#include <asm-generic/error-injection.h>
> +
> +#define ARCH_FUNC_RET	"blr"
> +
> +#endif /* _ASM_ERROR_INJECTION_H */
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index e4923686e43a..c0705296c2f0 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -101,6 +101,11 @@ static inline long regs_return_value(struct pt_regs *regs)
>  		return -regs->gpr[3];
>  }
>  
> +static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
> +{
> +	regs->gpr[3] = rc;
> +}
> +
>  #ifdef __powerpc64__
>  #define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
>  #else
> -- 
> 2.17.0

Re: [PATCH 1/2] error-injection: Simplify arch specific helpers

[Naveen N. Rao]

Masami Hiramatsu wrote:
> On Tue, 29 May 2018 18:06:02 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> We already have an arch-independent way to set the instruction pointer
>> with instruction_pointer_set(). Using this allows us to get rid of the
>> need for override_function_with_return() that each architecture has to
>> implement.
>> 
>> Furthermore, just_return_func() only has to encode arch-specific
>> assembly instructions to return from a function. Introduce a macro
>> ARCH_FUNC_RET to provide the arch-specific instruction and move over
>> just_return_func() to generic code.
>> 
>> With these changes, architectures that already support kprobes, only
>> just need to ensure they provide regs_set_return_value(), GET_IP() (for
>> instruction_pointer_set()), and ARCH_FUNC_RET to support error
>> injection.
> 
> Nice! the code basically good to me. Just one comment, ARCH_FUNC_RET sounds
> like a function. Maybe ARCH_RETURN_INSTRUCTION will be better name, isn't it? :)

Sure -- I thought of writing ARCH_FUNCTION_RETURN, but felt that was too 
verbose. How about ARCH_FUNC_RET_INST?

Thanks for the review,
Naveen

Re: [PATCH 1/2] error-injection: Simplify arch specific helpers

[Naveen N. Rao]

Masami Hiramatsu wrote:
> On Tue, 29 May 2018 18:06:02 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> We already have an arch-independent way to set the instruction pointer
>> with instruction_pointer_set(). Using this allows us to get rid of the
>> need for override_function_with_return() that each architecture has to
>> implement.
>>=20
>> Furthermore, just_return_func() only has to encode arch-specific
>> assembly instructions to return from a function. Introduce a macro
>> ARCH_FUNC_RET to provide the arch-specific instruction and move over
>> just_return_func() to generic code.
>>=20
>> With these changes, architectures that already support kprobes, only
>> just need to ensure they provide regs_set_return_value(), GET_IP() (for
>> instruction_pointer_set()), and ARCH_FUNC_RET to support error
>> injection.
>=20
> Nice! the code basically good to me. Just one comment, ARCH_FUNC_RET soun=
ds
> like a function. Maybe ARCH_RETURN_INSTRUCTION will be better name, isn't=
 it? :)

Sure -- I thought of writing ARCH_FUNCTION_RETURN, but felt that was too=20
verbose. How about ARCH_FUNC_RET_INST?

Thanks for the review,
Naveen

=

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> ...
> 
> A change log is always nice even if it's short :)

I tried, but really couldn't come up with anything more to write. I'll 
try harder for v2 :)

> 
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/Kconfig                       | 1 +
>>  arch/powerpc/include/asm/error-injection.h | 9 +++++++++
>>  arch/powerpc/include/asm/ptrace.h          | 5 +++++
>>  3 files changed, 15 insertions(+)
>>  create mode 100644 arch/powerpc/include/asm/error-injection.h
> 
> This looks fine to me, it's probably easiest if it goes in via tip along
> with patch 1.
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>

Thanks for the review. I'll base v2 on -tip


- Naveen

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> ...
>=20
> A change log is always nice even if it's short :)

I tried, but really couldn't come up with anything more to write. I'll=20
try harder for v2 :)

>=20
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/Kconfig                       | 1 +
>>  arch/powerpc/include/asm/error-injection.h | 9 +++++++++
>>  arch/powerpc/include/asm/ptrace.h          | 5 +++++
>>  3 files changed, 15 insertions(+)
>>  create mode 100644 arch/powerpc/include/asm/error-injection.h
>=20
> This looks fine to me, it's probably easiest if it goes in via tip along
> with patch 1.
>=20
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>

Thanks for the review. I'll base v2 on -tip


- Naveen

=

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> Michael Ellerman wrote:
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>>> ...
>> 
>> A change log is always nice even if it's short :)
>
> I tried, but really couldn't come up with anything more to write. I'll 
> try harder for v2 :)

Yeah true.

You could just say "All that's required is to define x and y and select
the Kconfig symbol".

>> This looks fine to me, it's probably easiest if it goes in via tip along
>> with patch 1.
>> 
>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>
> Thanks for the review. I'll base v2 on -tip

I guess if it doesn't already apply to tip you should rebase it. You've
probably missed 4.18 anyway.

cheers

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
>> Michael Ellerman wrote:
>>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>>>> ...
>>> 
>>> A change log is always nice even if it's short :)
>>
>> I tried, but really couldn't come up with anything more to write. I'll 
>> try harder for v2 :)
> 
> Yeah true.
> 
> You could just say "All that's required is to define x and y and select
> the Kconfig symbol".

Ok, sure.

> 
>>> This looks fine to me, it's probably easiest if it goes in via tip along
>>> with patch 1.
>>> 
>>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>
>> Thanks for the review. I'll base v2 on -tip
> 
> I guess if it doesn't already apply to tip you should rebase it. You've
> probably missed 4.18 anyway.

Oh ok. I just tried and it seems to apply just fine. I'll post v2 after 
giving this a quick test.

- Naveen

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>=20
>> Michael Ellerman wrote:
>>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>>>> ...
>>>=20
>>> A change log is always nice even if it's short :)
>>
>> I tried, but really couldn't come up with anything more to write. I'll=20
>> try harder for v2 :)
>=20
> Yeah true.
>=20
> You could just say "All that's required is to define x and y and select
> the Kconfig symbol".

Ok, sure.

>=20
>>> This looks fine to me, it's probably easiest if it goes in via tip alon=
g
>>> with patch 1.
>>>=20
>>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>
>> Thanks for the review. I'll base v2 on -tip
>=20
> I guess if it doesn't already apply to tip you should rebase it. You've
> probably missed 4.18 anyway.

Oh ok. I just tried and it seems to apply just fine. I'll post v2 after=20
giving this a quick test.

- Naveen

=

Re: [PATCH 1/2] error-injection: Simplify arch specific helpers

[Masami Hiramatsu]

On Thu, 31 May 2018 15:39:03 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Masami Hiramatsu wrote:
> > On Tue, 29 May 2018 18:06:02 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> >> We already have an arch-independent way to set the instruction pointer
> >> with instruction_pointer_set(). Using this allows us to get rid of the
> >> need for override_function_with_return() that each architecture has to
> >> implement.
> >> 
> >> Furthermore, just_return_func() only has to encode arch-specific
> >> assembly instructions to return from a function. Introduce a macro
> >> ARCH_FUNC_RET to provide the arch-specific instruction and move over
> >> just_return_func() to generic code.
> >> 
> >> With these changes, architectures that already support kprobes, only
> >> just need to ensure they provide regs_set_return_value(), GET_IP() (for
> >> instruction_pointer_set()), and ARCH_FUNC_RET to support error
> >> injection.
> > 
> > Nice! the code basically good to me. Just one comment, ARCH_FUNC_RET sounds
> > like a function. Maybe ARCH_RETURN_INSTRUCTION will be better name, isn't it? :)
> 
> Sure -- I thought of writing ARCH_FUNCTION_RETURN, but felt that was too 
> verbose. How about ARCH_FUNC_RET_INST?

It is OK if we can recognize it is an instruction.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Naveen N. Rao wrote:
> Michael Ellerman wrote:
>> 
>> I guess if it doesn't already apply to tip you should rebase it. You've
>> probably missed 4.18 anyway.
> 
> Oh ok. I just tried and it seems to apply just fine. I'll post v2 after 
> giving this a quick test.

I didn't post a v2 since I have decided against using this approach. The 
reason for that is Masami's series to remove jprobes. The discussion 
there reminded me that we can't easily override functions with kprobes 
on powerpc. Though it works for this particular scenario, we would just 
be setting a bad example.

As such, I won't be changing generic code, but will simply make the 
necessary changes in powerpc code.

Sorry for the noise.

- Naveen

Re: [PATCH 2/2] powerpc: Add support for function error injection

[Naveen N. Rao]

Naveen N. Rao wrote:
> Michael Ellerman wrote:
>>=20
>> I guess if it doesn't already apply to tip you should rebase it. You've
>> probably missed 4.18 anyway.
>=20
> Oh ok. I just tried and it seems to apply just fine. I'll post v2 after=20
> giving this a quick test.

I didn't post a v2 since I have decided against using this approach. The=20
reason for that is Masami's series to remove jprobes. The discussion=20
there reminded me that we can't easily override functions with kprobes=20
on powerpc. Though it works for this particular scenario, we would just=20
be setting a bad example.

As such, I won't be changing generic code, but will simply make the=20
necessary changes in powerpc code.

Sorry for the noise.

- Naveen

=