[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:
>=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:
>> ...
>=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:
>=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:
>>=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

=