LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] error-injection: simplify code and powerpc support
@ 2018-05-29 12:36 Naveen N. Rao
  2018-05-29 12:36 ` [PATCH 1/2] error-injection: Simplify arch specific helpers Naveen N. Rao
  2018-05-29 12:36 ` [PATCH 2/2] powerpc: Add support for function error injection Naveen N. Rao
  0 siblings, 2 replies; 11+ messages in thread
From: Naveen N. Rao @ 2018-05-29 12:36 UTC (permalink / raw)
  To: Ingo Molnar, Michael Ellerman, Masami Hiramatsu, Josef Bacik
  Cc: linuxppc-dev, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] error-injection: Simplify arch specific helpers
  2018-05-29 12:36 [PATCH 0/2] error-injection: simplify code and powerpc support Naveen N. Rao
@ 2018-05-29 12:36 ` Naveen N. Rao
  2018-05-30  8:41   ` Masami Hiramatsu
  2018-05-29 12:36 ` [PATCH 2/2] powerpc: Add support for function error injection Naveen N. Rao
  1 sibling, 1 reply; 11+ messages in thread
From: Naveen N. Rao @ 2018-05-29 12:36 UTC (permalink / raw)
  To: Ingo Molnar, Michael Ellerman, Masami Hiramatsu, Josef Bacik
  Cc: linuxppc-dev, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] powerpc: Add support for function error injection
  2018-05-29 12:36 [PATCH 0/2] error-injection: simplify code and powerpc support Naveen N. Rao
  2018-05-29 12:36 ` [PATCH 1/2] error-injection: Simplify arch specific helpers Naveen N. Rao
@ 2018-05-29 12:36 ` Naveen N. Rao
  2018-05-31  4:57   ` Michael Ellerman
  1 sibling, 1 reply; 11+ messages in thread
From: Naveen N. Rao @ 2018-05-29 12:36 UTC (permalink / raw)
  To: Ingo Molnar, Michael Ellerman, Masami Hiramatsu, Josef Bacik
  Cc: linuxppc-dev, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] error-injection: Simplify arch specific helpers
  2018-05-29 12:36 ` [PATCH 1/2] error-injection: Simplify arch specific helpers Naveen N. Rao
@ 2018-05-30  8:41   ` Masami Hiramatsu
  2018-05-31 10:09     ` Naveen N. Rao
  0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2018-05-30  8:41 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Ingo Molnar, Michael Ellerman, Josef Bacik, linuxppc-dev, linux-kernel

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] powerpc: Add support for function error injection
  2018-05-29 12:36 ` [PATCH 2/2] powerpc: Add support for function error injection Naveen N. Rao
@ 2018-05-31  4:57   ` Michael Ellerman
  2018-05-31 10:11     ` Naveen N. Rao
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Ellerman @ 2018-05-31  4:57 UTC (permalink / raw)
  To: Naveen N. Rao, Ingo Molnar, Masami Hiramatsu, Josef Bacik
  Cc: linuxppc-dev, linux-kernel

"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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] error-injection: Simplify arch specific helpers
  2018-05-30  8:41   ` Masami Hiramatsu
@ 2018-05-31 10:09     ` Naveen N. Rao
  2018-06-01 23:12       ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: Naveen N. Rao @ 2018-05-31 10:09 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Josef Bacik, linux-kernel, linuxppc-dev, Ingo Molnar, Michael Ellerman

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] powerpc: Add support for function error injection
  2018-05-31  4:57   ` Michael Ellerman
@ 2018-05-31 10:11     ` Naveen N. Rao
  2018-05-31 14:13       ` Michael Ellerman
  0 siblings, 1 reply; 11+ messages in thread
From: Naveen N. Rao @ 2018-05-31 10:11 UTC (permalink / raw)
  To: Josef Bacik, Masami Hiramatsu, Ingo Molnar, Michael Ellerman
  Cc: linux-kernel, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] powerpc: Add support for function error injection
  2018-05-31 10:11     ` Naveen N. Rao
@ 2018-05-31 14:13       ` Michael Ellerman
  2018-05-31 19:06         ` Naveen N. Rao
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Ellerman @ 2018-05-31 14:13 UTC (permalink / raw)
  To: Naveen N. Rao, Josef Bacik, Masami Hiramatsu, Ingo Molnar
  Cc: linux-kernel, linuxppc-dev

"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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] powerpc: Add support for function error injection
  2018-05-31 14:13       ` Michael Ellerman
@ 2018-05-31 19:06         ` Naveen N. Rao
  2018-06-06 10:38           ` Naveen N. Rao
  0 siblings, 1 reply; 11+ messages in thread
From: Naveen N. Rao @ 2018-05-31 19:06 UTC (permalink / raw)
  To: Josef Bacik, Masami Hiramatsu, Ingo Molnar, Michael Ellerman
  Cc: linux-kernel, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] error-injection: Simplify arch specific helpers
  2018-05-31 10:09     ` Naveen N. Rao
@ 2018-06-01 23:12       ` Masami Hiramatsu
  0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2018-06-01 23:12 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Josef Bacik, linux-kernel, linuxppc-dev, Ingo Molnar, Michael Ellerman

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] powerpc: Add support for function error injection
  2018-05-31 19:06         ` Naveen N. Rao
@ 2018-06-06 10:38           ` Naveen N. Rao
  0 siblings, 0 replies; 11+ messages in thread
From: Naveen N. Rao @ 2018-06-06 10:38 UTC (permalink / raw)
  To: Josef Bacik, Masami Hiramatsu, Ingo Molnar, Michael Ellerman
  Cc: linux-kernel, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-06-06 10:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29 12:36 [PATCH 0/2] error-injection: simplify code and powerpc support Naveen N. Rao
2018-05-29 12:36 ` [PATCH 1/2] error-injection: Simplify arch specific helpers Naveen N. Rao
2018-05-30  8:41   ` Masami Hiramatsu
2018-05-31 10:09     ` Naveen N. Rao
2018-06-01 23:12       ` Masami Hiramatsu
2018-05-29 12:36 ` [PATCH 2/2] powerpc: Add support for function error injection Naveen N. Rao
2018-05-31  4:57   ` Michael Ellerman
2018-05-31 10:11     ` Naveen N. Rao
2018-05-31 14:13       ` Michael Ellerman
2018-05-31 19:06         ` Naveen N. Rao
2018-06-06 10:38           ` Naveen N. Rao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).