| Message ID | 20201103074244.GA5615@ls3530.fritz.box (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | parisc: ftrace: get_kprobe() must be called with preempt disabled | expand |
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); >
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
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
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);
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>