[PATCH] parisc: ftrace: get_kprobe() must be called with preempt disabled

[PATCH] parisc: ftrace: get_kprobe() must be called with preempt disabled

[Helge Deller]

As noticed by Masami Hiramatsu, get_kprobe() must be called with preempt
disabled.

Noticed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 63e3ecb9da81..dd356ad18aca 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -207,13 +212,22 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 			   struct ftrace_ops *ops, struct pt_regs *regs)
 {
 	struct kprobe_ctlblk *kcb;
-	struct kprobe *p = get_kprobe((kprobe_opcode_t *)ip);
+	struct kprobe *p;
+
+	/*
+	 * We don't want to be preempted for the entire
+	 * duration of kprobe processing
+	 */
+	preempt_disable();
+	p = get_kprobe((kprobe_opcode_t *)ip);

 	if (unlikely(!p) || kprobe_disabled(p))
+		preempt_enable_no_resched();
 		return;

 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(p);
+		preempt_enable_no_resched();
 		return;
 	}

@@ -235,6 +249,8 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		}
 	}
 	__this_cpu_write(current_kprobe, NULL);
+
+	preempt_enable_no_resched();
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);

Re: [PATCH] parisc: ftrace: get_kprobe() must be called with preempt disabled

[Masami Hiramatsu]

On Tue, 3 Nov 2020 08:42:44 +0100
Helge Deller <deller@gmx.de> wrote:

> As noticed by Masami Hiramatsu, get_kprobe() must be called with preempt
> disabled.

Doesn't parisc ftrace implementation preempt off? Then it is required.

Steve, can we expect that op->func() is called under preempt off always
on any arch or is it arch dependent?

Thank you,

> 
> Noticed-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
> index 63e3ecb9da81..dd356ad18aca 100644
> --- a/arch/parisc/kernel/ftrace.c
> +++ b/arch/parisc/kernel/ftrace.c
> @@ -207,13 +212,22 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
>  			   struct ftrace_ops *ops, struct pt_regs *regs)
>  {
>  	struct kprobe_ctlblk *kcb;
> -	struct kprobe *p = get_kprobe((kprobe_opcode_t *)ip);
> +	struct kprobe *p;
> +
> +	/*
> +	 * We don't want to be preempted for the entire
> +	 * duration of kprobe processing
> +	 */
> +	preempt_disable();
> +	p = get_kprobe((kprobe_opcode_t *)ip);
> 
>  	if (unlikely(!p) || kprobe_disabled(p))
> +		preempt_enable_no_resched();
>  		return;
> 
>  	if (kprobe_running()) {
>  		kprobes_inc_nmissed_count(p);
> +		preempt_enable_no_resched();
>  		return;
>  	}
> 
> @@ -235,6 +249,8 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
>  		}
>  	}
>  	__this_cpu_write(current_kprobe, NULL);
> +
> +	preempt_enable_no_resched();
>  }
>  NOKPROBE_SYMBOL(kprobe_ftrace_handler);
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] parisc: ftrace: get_kprobe() must be called with preempt disabled

[Steven Rostedt]

On Fri, 6 Nov 2020 11:17:58 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Tue, 3 Nov 2020 08:42:44 +0100
> Helge Deller <deller@gmx.de> wrote:
> 
> > As noticed by Masami Hiramatsu, get_kprobe() must be called with preempt
> > disabled.  
> 
> Doesn't parisc ftrace implementation preempt off? Then it is required.
> 
> Steve, can we expect that op->func() is called under preempt off always
> on any arch or is it arch dependent?
> 

Currently all kprobe function callbacks are called with preempt
disabled, because it doesn't have the RECURSIVE_SAFE flag set. And for
callbacks without that set, it goes through a helper function that
disables preemption. My current patch set (which is not upstreamed yet)
is changing that, and I just posted a new patch series that does the
preempt disable before the get_kprobe() function for parsic and other
archs, to handle the new code.

Thus, parsic is currently OK (without my new patch set, it's called
with preemption disabled).

-- Steve

Re: [PATCH] parisc: ftrace: get_kprobe() must be called with preempt disabled

[Helge Deller]

On 11/6/20 3:43 AM, Steven Rostedt wrote:
> On Fri, 6 Nov 2020 11:17:58 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
>> On Tue, 3 Nov 2020 08:42:44 +0100
>> Helge Deller <deller@gmx.de> wrote:
>>
>>> As noticed by Masami Hiramatsu, get_kprobe() must be called with preempt
>>> disabled.
>>
>> Doesn't parisc ftrace implementation preempt off? Then it is required.
>>
>> Steve, can we expect that op->func() is called under preempt off always
>> on any arch or is it arch dependent?
>>
>
> Currently all kprobe function callbacks are called with preempt
> disabled, because it doesn't have the RECURSIVE_SAFE flag set. And for
> callbacks without that set, it goes through a helper function that
> disables preemption. My current patch set (which is not upstreamed yet)
> is changing that, and I just posted a new patch series that does the
> preempt disable before the get_kprobe() function for parsic and other
> archs, to handle the new code.

Seems good. Thanks!

> Thus, parsic is currently OK (without my new patch set, it's called
> with preemption disabled).

Ok.

Helge