[PATCH 0/5] powerpc/kprobes: preempt related changes and cleanups

[PATCH 0/5] powerpc/kprobes: preempt related changes and cleanups

[Naveen N. Rao]

This series attempts to address some of the concerns raised in 
https://github.com/linuxppc/issues/issues/440

The last two patches are minor cleanups in related kprobes code.

- Naveen


Naveen N. Rao (5):
  powerpc/kprobes: Remove preempt disable around call to get_kprobe() in
    arch_prepare_kprobe()
  powerpc/kprobes: Have optimized_callback() use preempt_enable()
  powerpc/kprobes: Use preempt_enable() rather than the no_resched
    variant
  powerpc/kprobes: Setup consistent pt_regs across kprobes, optprobes
    and KPROBES_ON_FTRACE
  powerpc/kprobes: Remove unnecessary headers from kprobes

 arch/powerpc/kernel/kprobes-ftrace.c        |  4 ----
 arch/powerpc/kernel/kprobes.c               | 16 ++++++----------
 arch/powerpc/kernel/optprobes.c             |  2 +-
 arch/powerpc/kernel/optprobes_head.S        |  5 +----
 arch/powerpc/kernel/trace/ftrace_mprofile.S |  6 ++++++
 5 files changed, 14 insertions(+), 19 deletions(-)


base-commit: 7dc2a00fdd44a4d0c3bac9fd10558b3933586a0c
-- 
2.38.0

[PATCH 1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()

[Naveen N. Rao]

arch_prepare_kprobe() is called from register_kprobe() via
prepare_kprobe(), or through register_aggr_kprobe(), both with the
kprobe_mutex held. Per the comment for get_kprobe():
  /*
   * This routine is called either:
   *	- under the 'kprobe_mutex' - during kprobe_[un]register().
   *				OR
   *	- with preemption disabled - from architecture specific code.
   */

As such, there is no need to disable preemption around the call to
get_kprobe(). Drop the same.

Reported-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index bd7b1a03545948..88f42de681e1f8 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -158,9 +158,7 @@ int arch_prepare_kprobe(struct kprobe *p)
 		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
 		ret = -EINVAL;
 	}
-	preempt_disable();
 	prev = get_kprobe(p->addr - 1);
-	preempt_enable_no_resched();
 
 	/*
 	 * When prev is a ftrace-based kprobe, we don't have an insn, and it
-- 
2.38.0

[PATCH 2/5] powerpc/kprobes: Have optimized_callback() use preempt_enable()

[Naveen N. Rao]

Similar to x86 commit 2e62024c265aa6 ("kprobes/x86: Use preempt_enable()
in optimized_callback()"), change powerpc optprobes to use
preempt_enable() rather than preempt_enable_no_resched() since powerpc
also removed irq disabling for optprobes in commit f72180cc93a2c6
("powerpc/kprobes: Do not disable interrupts for optprobes and
kprobes_on_ftrace").

Reported-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/optprobes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 3b1c2236cbee57..004fae2044a3e0 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -112,7 +112,7 @@ static void optimized_callback(struct optimized_kprobe *op,
 		__this_cpu_write(current_kprobe, NULL);
 	}
 
-	preempt_enable_no_resched();
+	preempt_enable();
 }
 NOKPROBE_SYMBOL(optimized_callback);
 
-- 
2.38.0

[PATCH 3/5] powerpc/kprobes: Use preempt_enable() rather than the no_resched variant

[Naveen N. Rao]

preempt_enable_no_resched() is just the same as preempt_enable() when we
are in a irqs disabled context. kprobe_handler() and the post/fault
handlers are all called with irqs disabled. As such, convert those to
just use preempt_enable().

Reported-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 88f42de681e1f8..86ca5a61ea9afb 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -369,7 +369,7 @@ int kprobe_handler(struct pt_regs *regs)
 
 			if (ret > 0) {
 				restore_previous_kprobe(kcb);
-				preempt_enable_no_resched();
+				preempt_enable();
 				return 1;
 			}
 		}
@@ -382,7 +382,7 @@ int kprobe_handler(struct pt_regs *regs)
 	if (p->pre_handler && p->pre_handler(p, regs)) {
 		/* handler changed execution path, so skip ss setup */
 		reset_current_kprobe();
-		preempt_enable_no_resched();
+		preempt_enable();
 		return 1;
 	}
 
@@ -395,7 +395,7 @@ int kprobe_handler(struct pt_regs *regs)
 
 			kcb->kprobe_status = KPROBE_HIT_SSDONE;
 			reset_current_kprobe();
-			preempt_enable_no_resched();
+			preempt_enable();
 			return 1;
 		}
 	}
@@ -404,7 +404,7 @@ int kprobe_handler(struct pt_regs *regs)
 	return 1;
 
 no_kprobe:
-	preempt_enable_no_resched();
+	preempt_enable();
 	return ret;
 }
 NOKPROBE_SYMBOL(kprobe_handler);
@@ -490,7 +490,7 @@ int kprobe_post_handler(struct pt_regs *regs)
 	}
 	reset_current_kprobe();
 out:
-	preempt_enable_no_resched();
+	preempt_enable();
 
 	/*
 	 * if somebody else is singlestepping across a probe point, msr
@@ -529,7 +529,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 			restore_previous_kprobe(kcb);
 		else
 			reset_current_kprobe();
-		preempt_enable_no_resched();
+		preempt_enable();
 		break;
 	case KPROBE_HIT_ACTIVE:
 	case KPROBE_HIT_SSDONE:
-- 
2.38.0

[PATCH 4/5] powerpc/kprobes: Setup consistent pt_regs across kprobes, optprobes and KPROBES_ON_FTRACE

[Naveen N. Rao]

Ensure a more consistent pt_regs across kprobes, optprobes and
KPROBES_ON_FTRACE:
- Drop setting trap to 0x700 under optprobes. This is not accurate and
  is unnecessary. Instead, zero it out for both optprobes and
  KPROBES_ON_FTRACE.
- Save irq soft mask in the ftrace handler, similar to what we do in
  optprobes and trap-based kprobes.
- Drop setting orig_gpr3 and result to zero in optprobes. These are not
  relevant under kprobes and should not be used by the handlers.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/optprobes_head.S        | 5 +----
 arch/powerpc/kernel/trace/ftrace_mprofile.S | 6 ++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index cd4e7bc32609d3..06df09b4e8b155 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -49,11 +49,8 @@ optprobe_template_entry:
 	/* Save SPRS */
 	mfmsr	r5
 	PPC_STL	r5,_MSR(r1)
-	li	r5,0x700
-	PPC_STL	r5,_TRAP(r1)
 	li	r5,0
-	PPC_STL	r5,ORIG_GPR3(r1)
-	PPC_STL	r5,RESULT(r1)
+	PPC_STL	r5,_TRAP(r1)
 	mfctr	r5
 	PPC_STL	r5,_CTR(r1)
 	mflr	r5
diff --git a/arch/powerpc/kernel/trace/ftrace_mprofile.S b/arch/powerpc/kernel/trace/ftrace_mprofile.S
index d031093bc43671..f82004089426e6 100644
--- a/arch/powerpc/kernel/trace/ftrace_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_mprofile.S
@@ -107,6 +107,12 @@
 	PPC_STL	r9, _CTR(r1)
 	PPC_STL	r10, _XER(r1)
 	PPC_STL	r11, _CCR(r1)
+#ifdef CONFIG_PPC64
+	lbz     r7, PACAIRQSOFTMASK(r13)
+	std     r7, SOFTE(r1)
+#endif
+	li	r8, 0
+	PPC_STL	r8, _TRAP(r1)
 	.endif
 
 	/* Load &pt_regs in r6 for call below */
-- 
2.38.0

[PATCH 5/5] powerpc/kprobes: Remove unnecessary headers from kprobes

[Naveen N. Rao]

Many of these headers are not necessary since those are included
indirectly, or the code using those headers has been removed.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes-ftrace.c | 4 ----
 arch/powerpc/kernel/kprobes.c        | 2 --
 2 files changed, 6 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c
index 072ebe7f290ba7..08ed8a158fd724 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -7,10 +7,6 @@
  *		  IBM Corporation
  */
 #include <linux/kprobes.h>
-#include <linux/ptrace.h>
-#include <linux/hardirq.h>
-#include <linux/preempt.h>
-#include <linux/ftrace.h>
 
 /* Ftrace callback handler for kprobes */
 void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 86ca5a61ea9afb..3bf2507f07e6c6 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -14,8 +14,6 @@
  */
 
 #include <linux/kprobes.h>
-#include <linux/ptrace.h>
-#include <linux/preempt.h>
 #include <linux/extable.h>
 #include <linux/kdebug.h>
 #include <linux/slab.h>
-- 
2.38.0

Re: [PATCH 5/5] powerpc/kprobes: Remove unnecessary headers from kprobes

[Christophe Leroy]

Le 20/10/2022 à 19:29, Naveen N. Rao a écrit :
> Many of these headers are not necessary since those are included
> indirectly, or the code using those headers has been removed.

It is usually not a good idea to not include headers because they are 
already included indirectly. If one day for some reason de indirect link 
gets removed, then your file doesn't build anymore.

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/kprobes-ftrace.c | 4 ----
>   arch/powerpc/kernel/kprobes.c        | 2 --
>   2 files changed, 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c
> index 072ebe7f290ba7..08ed8a158fd724 100644
> --- a/arch/powerpc/kernel/kprobes-ftrace.c
> +++ b/arch/powerpc/kernel/kprobes-ftrace.c
> @@ -7,10 +7,6 @@
>    *		  IBM Corporation
>    */
>   #include <linux/kprobes.h>
> -#include <linux/ptrace.h>
> -#include <linux/hardirq.h>
> -#include <linux/preempt.h>
> -#include <linux/ftrace.h>
>   
>   /* Ftrace callback handler for kprobes */
>   void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 86ca5a61ea9afb..3bf2507f07e6c6 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -14,8 +14,6 @@
>    */
>   
>   #include <linux/kprobes.h>
> -#include <linux/ptrace.h>
> -#include <linux/preempt.h>
>   #include <linux/extable.h>
>   #include <linux/kdebug.h>
>   #include <linux/slab.h>

Re: [PATCH 1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()

[Nicholas Piggin]

On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
> arch_prepare_kprobe() is called from register_kprobe() via
> prepare_kprobe(), or through register_aggr_kprobe(), both with the
> kprobe_mutex held. Per the comment for get_kprobe():
>   /*
>    * This routine is called either:
>    *	- under the 'kprobe_mutex' - during kprobe_[un]register().
>    *				OR
>    *	- with preemption disabled - from architecture specific code.
>    */

That comment should read [un]register_kprobe(), right?

>
> As such, there is no need to disable preemption around the call to
> get_kprobe(). Drop the same.

And prepare_kprobe() and register_aggr_kprobe() are both called with
kprobe_mutex held so you rely on the same protection. This seems to
fix a lost-resched bug with preempt kernels too.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index bd7b1a03545948..88f42de681e1f8 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -158,9 +158,7 @@ int arch_prepare_kprobe(struct kprobe *p)
>  		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
>  		ret = -EINVAL;
>  	}
> -	preempt_disable();
>  	prev = get_kprobe(p->addr - 1);
> -	preempt_enable_no_resched();
>  
>  	/*
>  	 * When prev is a ftrace-based kprobe, we don't have an insn, and it
> -- 
> 2.38.0

Re: [PATCH 2/5] powerpc/kprobes: Have optimized_callback() use preempt_enable()

[Nicholas Piggin]

On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
> Similar to x86 commit 2e62024c265aa6 ("kprobes/x86: Use preempt_enable()
> in optimized_callback()"), change powerpc optprobes to use
> preempt_enable() rather than preempt_enable_no_resched() since powerpc
> also removed irq disabling for optprobes in commit f72180cc93a2c6
> ("powerpc/kprobes: Do not disable interrupts for optprobes and
> kprobes_on_ftrace").

Looks okay. Even if we did have irqs disabled here, we should just use
preempt_enable(), which nests properly inside or outside local irqs.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/optprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 3b1c2236cbee57..004fae2044a3e0 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -112,7 +112,7 @@ static void optimized_callback(struct optimized_kprobe *op,
>  		__this_cpu_write(current_kprobe, NULL);
>  	}
>  
> -	preempt_enable_no_resched();
> +	preempt_enable();
>  }
>  NOKPROBE_SYMBOL(optimized_callback);
>  
> -- 
> 2.38.0

Re: [PATCH 3/5] powerpc/kprobes: Use preempt_enable() rather than the no_resched variant

[Nicholas Piggin]

On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
> preempt_enable_no_resched() is just the same as preempt_enable() when we
> are in a irqs disabled context. kprobe_handler() and the post/fault
> handlers are all called with irqs disabled. As such, convert those to
> just use preempt_enable().

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 88f42de681e1f8..86ca5a61ea9afb 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -369,7 +369,7 @@ int kprobe_handler(struct pt_regs *regs)
>  
>  			if (ret > 0) {
>  				restore_previous_kprobe(kcb);
> -				preempt_enable_no_resched();
> +				preempt_enable();
>  				return 1;
>  			}
>  		}
> @@ -382,7 +382,7 @@ int kprobe_handler(struct pt_regs *regs)
>  	if (p->pre_handler && p->pre_handler(p, regs)) {
>  		/* handler changed execution path, so skip ss setup */
>  		reset_current_kprobe();
> -		preempt_enable_no_resched();
> +		preempt_enable();
>  		return 1;
>  	}
>  
> @@ -395,7 +395,7 @@ int kprobe_handler(struct pt_regs *regs)
>  
>  			kcb->kprobe_status = KPROBE_HIT_SSDONE;
>  			reset_current_kprobe();
> -			preempt_enable_no_resched();
> +			preempt_enable();
>  			return 1;
>  		}
>  	}
> @@ -404,7 +404,7 @@ int kprobe_handler(struct pt_regs *regs)
>  	return 1;
>  
>  no_kprobe:
> -	preempt_enable_no_resched();
> +	preempt_enable();
>  	return ret;
>  }
>  NOKPROBE_SYMBOL(kprobe_handler);
> @@ -490,7 +490,7 @@ int kprobe_post_handler(struct pt_regs *regs)
>  	}
>  	reset_current_kprobe();
>  out:
> -	preempt_enable_no_resched();
> +	preempt_enable();
>  
>  	/*
>  	 * if somebody else is singlestepping across a probe point, msr
> @@ -529,7 +529,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>  			restore_previous_kprobe(kcb);
>  		else
>  			reset_current_kprobe();
> -		preempt_enable_no_resched();
> +		preempt_enable();
>  		break;
>  	case KPROBE_HIT_ACTIVE:
>  	case KPROBE_HIT_SSDONE:
> -- 
> 2.38.0

Re: [PATCH 4/5] powerpc/kprobes: Setup consistent pt_regs across kprobes, optprobes and KPROBES_ON_FTRACE

[Nicholas Piggin]

On Fri Oct 21, 2022 at 3:29 AM AEST, Naveen N. Rao wrote:
> Ensure a more consistent pt_regs across kprobes, optprobes and
> KPROBES_ON_FTRACE:
> - Drop setting trap to 0x700 under optprobes. This is not accurate and
>   is unnecessary. Instead, zero it out for both optprobes and
>   KPROBES_ON_FTRACE.

Okay I think.

> - Save irq soft mask in the ftrace handler, similar to what we do in
>   optprobes and trap-based kprobes.

This advertises the irqs status of regs correctly, whereas previously
it was uninitialised.

> - Drop setting orig_gpr3 and result to zero in optprobes. These are not
>   relevant under kprobes and should not be used by the handlers.

This is for CFAR, which we can't get anyway because we just branched
here. I would rather zero it explicitly though.

Otherwise,

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/optprobes_head.S        | 5 +----
>  arch/powerpc/kernel/trace/ftrace_mprofile.S | 6 ++++++
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
> index cd4e7bc32609d3..06df09b4e8b155 100644
> --- a/arch/powerpc/kernel/optprobes_head.S
> +++ b/arch/powerpc/kernel/optprobes_head.S
> @@ -49,11 +49,8 @@ optprobe_template_entry:
>  	/* Save SPRS */
>  	mfmsr	r5
>  	PPC_STL	r5,_MSR(r1)
> -	li	r5,0x700
> -	PPC_STL	r5,_TRAP(r1)
>  	li	r5,0
> -	PPC_STL	r5,ORIG_GPR3(r1)
> -	PPC_STL	r5,RESULT(r1)
> +	PPC_STL	r5,_TRAP(r1)
>  	mfctr	r5
>  	PPC_STL	r5,_CTR(r1)
>  	mflr	r5
> diff --git a/arch/powerpc/kernel/trace/ftrace_mprofile.S b/arch/powerpc/kernel/trace/ftrace_mprofile.S
> index d031093bc43671..f82004089426e6 100644
> --- a/arch/powerpc/kernel/trace/ftrace_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_mprofile.S
> @@ -107,6 +107,12 @@
>  	PPC_STL	r9, _CTR(r1)
>  	PPC_STL	r10, _XER(r1)
>  	PPC_STL	r11, _CCR(r1)
> +#ifdef CONFIG_PPC64
> +	lbz     r7, PACAIRQSOFTMASK(r13)
> +	std     r7, SOFTE(r1)
> +#endif
> +	li	r8, 0
> +	PPC_STL	r8, _TRAP(r1)
>  	.endif
>  
>  	/* Load &pt_regs in r6 for call below */
> -- 
> 2.38.0

Re: [PATCH 0/5] powerpc/kprobes: preempt related changes and cleanups

[Nicholas Piggin]

+linux-arch

FYI most preempt_enable_no_resched() calls outside of core scheduler
code come from kprobes copy-and-paste, and can possibly be fixed in
similar ways as this series. It's a bit of a footgun API that should
be banished from outside kernel/sched/, at least without an explicit
comment for each case that explains the need and how the missed
resched is resolved or not applicable.

Thanks,
Nick

On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
> This series attempts to address some of the concerns raised in 
> https://github.com/linuxppc/issues/issues/440
>
> The last two patches are minor cleanups in related kprobes code.
>
> - Naveen
>
>
> Naveen N. Rao (5):
>   powerpc/kprobes: Remove preempt disable around call to get_kprobe() in
>     arch_prepare_kprobe()
>   powerpc/kprobes: Have optimized_callback() use preempt_enable()
>   powerpc/kprobes: Use preempt_enable() rather than the no_resched
>     variant
>   powerpc/kprobes: Setup consistent pt_regs across kprobes, optprobes
>     and KPROBES_ON_FTRACE
>   powerpc/kprobes: Remove unnecessary headers from kprobes
>
>  arch/powerpc/kernel/kprobes-ftrace.c        |  4 ----
>  arch/powerpc/kernel/kprobes.c               | 16 ++++++----------
>  arch/powerpc/kernel/optprobes.c             |  2 +-
>  arch/powerpc/kernel/optprobes_head.S        |  5 +----
>  arch/powerpc/kernel/trace/ftrace_mprofile.S |  6 ++++++
>  5 files changed, 14 insertions(+), 19 deletions(-)
>
>
> base-commit: 7dc2a00fdd44a4d0c3bac9fd10558b3933586a0c
> -- 
> 2.38.0

Re: [PATCH 0/5] powerpc/kprobes: preempt related changes and cleanups

[Nicholas Piggin]

+linux-arch

FYI most preempt_enable_no_resched() calls outside of core scheduler
code come from kprobes copy-and-paste, and can possibly be fixed in
similar ways as this series. It's a bit of a footgun API that should
be banished from outside kernel/sched/, at least without an explicit
comment for each case that explains the need and how the missed
resched is resolved or not applicable.

Thanks,
Nick

On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
> This series attempts to address some of the concerns raised in 
> https://github.com/linuxppc/issues/issues/440
>
> The last two patches are minor cleanups in related kprobes code.
>
> - Naveen
>
>
> Naveen N. Rao (5):
>   powerpc/kprobes: Remove preempt disable around call to get_kprobe() in
>     arch_prepare_kprobe()
>   powerpc/kprobes: Have optimized_callback() use preempt_enable()
>   powerpc/kprobes: Use preempt_enable() rather than the no_resched
>     variant
>   powerpc/kprobes: Setup consistent pt_regs across kprobes, optprobes
>     and KPROBES_ON_FTRACE
>   powerpc/kprobes: Remove unnecessary headers from kprobes
>
>  arch/powerpc/kernel/kprobes-ftrace.c        |  4 ----
>  arch/powerpc/kernel/kprobes.c               | 16 ++++++----------
>  arch/powerpc/kernel/optprobes.c             |  2 +-
>  arch/powerpc/kernel/optprobes_head.S        |  5 +----
>  arch/powerpc/kernel/trace/ftrace_mprofile.S |  6 ++++++
>  5 files changed, 14 insertions(+), 19 deletions(-)
>
>
> base-commit: 7dc2a00fdd44a4d0c3bac9fd10558b3933586a0c
> -- 
> 2.38.0

Re: [PATCH 5/5] powerpc/kprobes: Remove unnecessary headers from kprobes

[Naveen N. Rao]

Christophe Leroy wrote:
> 
> 
> Le 20/10/2022 à 19:29, Naveen N. Rao a écrit :
>> Many of these headers are not necessary since those are included
>> indirectly, or the code using those headers has been removed.
> 
> It is usually not a good idea to not include headers because they are 
> already included indirectly. If one day for some reason de indirect link 
> gets removed, then your file doesn't build anymore.

I agree with that in general.

The motivation for this patch was to remove <linux/preempt.h> from 
kprobes-ftrace.c since we got rid of explicit preempt disable/enable 
calls in the past. We have also removed explicit irq disable from this 
path. <linux/kprobes.h> includes <linux/ftrace.h> and <asm/kprobes.h> 
includes <linux/ptrace.h> so those could go too.

I don't expect those header dependencies to change, so I think this 
patch is ok.


- Naveen

Re: [PATCH 1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()

[Naveen N. Rao]

Nicholas Piggin wrote:
> On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
>> arch_prepare_kprobe() is called from register_kprobe() via
>> prepare_kprobe(), or through register_aggr_kprobe(), both with the
>> kprobe_mutex held. Per the comment for get_kprobe():
>>   /*
>>    * This routine is called either:
>>    *	- under the 'kprobe_mutex' - during kprobe_[un]register().
>>    *				OR
>>    *	- with preemption disabled - from architecture specific code.
>>    */
> 
> That comment should read [un]register_kprobe(), right?

Ugh, yes!

> 
>>
>> As such, there is no need to disable preemption around the call to
>> get_kprobe(). Drop the same.
> 
> And prepare_kprobe() and register_aggr_kprobe() are both called with
> kprobe_mutex held so you rely on the same protection. This seems to
> fix a lost-resched bug with preempt kernels too.
> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Naveen

Re: [PATCH 4/5] powerpc/kprobes: Setup consistent pt_regs across kprobes, optprobes and KPROBES_ON_FTRACE

[Naveen N. Rao]

Nicholas Piggin wrote:
> On Fri Oct 21, 2022 at 3:29 AM AEST, Naveen N. Rao wrote:
>> Ensure a more consistent pt_regs across kprobes, optprobes and
>> KPROBES_ON_FTRACE:
>> - Drop setting trap to 0x700 under optprobes. This is not accurate and
>>   is unnecessary. Instead, zero it out for both optprobes and
>>   KPROBES_ON_FTRACE.
> 
> Okay I think.
> 
>> - Save irq soft mask in the ftrace handler, similar to what we do in
>>   optprobes and trap-based kprobes.
> 
> This advertises the irqs status of regs correctly, whereas previously
> it was uninitialised.
> 
>> - Drop setting orig_gpr3 and result to zero in optprobes. These are not
>>   relevant under kprobes and should not be used by the handlers.
> 
> This is for CFAR, which we can't get anyway because we just branched
> here. I would rather zero it explicitly though.

Is there a strong reason to zero those out?

The reason I dropped zero'ing of orig_gpr3 and result is to make 
optprobes consistent with KPROBES_ON_FTRACE. If we want to retain 
zero'ing orig_gpr3/result for optprobes, I think we should then go ahead 
and zero those out in ftrace_regs_caller too.

Thanks,
Naveen

Re: (subset) [PATCH 0/5] powerpc/kprobes: preempt related changes and cleanups

[Michael Ellerman]

On Thu, 20 Oct 2022 22:58:56 +0530, Naveen N. Rao wrote:
> This series attempts to address some of the concerns raised in
> https://github.com/linuxppc/issues/issues/440
> 
> The last two patches are minor cleanups in related kprobes code.
> 
> - Naveen
> 
> [...]

Applied to powerpc/next.

[1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()
      https://git.kernel.org/powerpc/c/2fa9482334b0593b7edc371a13c0cca81daaa89e
[2/5] powerpc/kprobes: Have optimized_callback() use preempt_enable()
      https://git.kernel.org/powerpc/c/04ec5d5782fb346c291a05a2efe59483d8ada4c4
[3/5] powerpc/kprobes: Use preempt_enable() rather than the no_resched variant
      https://git.kernel.org/powerpc/c/266b1991a433cd55bb86a933216b3f6762737d47

cheers