Re: [PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs
On Thu 2020-11-19 09:07:58, Steven Rostedt wrote:
> On Thu, 19 Nov 2020 12:05:44 +0100
> Petr Mladek wrote:
>
> > > void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> > > -struct ftrace_ops *ops, struct pt_regs *regs)
> > > +struct ftrace_ops *ops, struct ftrace_regs *fregs)
> > > {
> > > int bit;
> > > bool lr_saver = false;
> > > struct kprobe *p;
> > > struct kprobe_ctlblk *kcb;
> > > + struct pt_regs *regs;
> > >
> > > bit = ftrace_test_recursion_trylock(ip, parent_ip);
> > > if (bit < 0)
> > > return;
> > >
> > > + regs = ftrace_get_regs(fregs);
> >
> > Should we check for NULL here?
> > Same in all callers?
>
> If regs is NULL that's a major bug.
>
> It's no different than what we have today. If you set FL_SAVE_REGS, then
> the regs parameter will have regs. If you don't, it will be NULL. We don't
> check regs for NULL today, so we shouldn't need to check regs for NULL with
> this.
>
> One of my bootup tests checks if this works. I work hard to make sure that
> regs is set for everything that wants it, otherwise bad things happen.
>
> In other words, the functionality in this regard hasn't changed with this
> patch.
Thanks for explanation. Feel free to use:
Acked-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs
On Thu, 19 Nov 2020 12:05:44 +0100
Petr Mladek wrote:
> > void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> > - struct ftrace_ops *ops, struct pt_regs *regs)
> > + struct ftrace_ops *ops, struct ftrace_regs *fregs)
> > {
> > int bit;
> > bool lr_saver = false;
> > struct kprobe *p;
> > struct kprobe_ctlblk *kcb;
> > + struct pt_regs *regs;
> >
> > bit = ftrace_test_recursion_trylock(ip, parent_ip);
> > if (bit < 0)
> > return;
> >
> > + regs = ftrace_get_regs(fregs);
>
> Should we check for NULL here?
> Same in all callers?
If regs is NULL that's a major bug.
It's no different than what we have today. If you set FL_SAVE_REGS, then
the regs parameter will have regs. If you don't, it will be NULL. We don't
check regs for NULL today, so we shouldn't need to check regs for NULL with
this.
One of my bootup tests checks if this works. I work hard to make sure that
regs is set for everything that wants it, otherwise bad things happen.
In other words, the functionality in this regard hasn't changed with this
patch.
-- Steve
>
> > preempt_disable_notrace();
> > p = get_kprobe((kprobe_opcode_t *)ip);
> > if (!p) {
>
> Otherwise, the patch is pretty strightforard and looks good to me.
Re: [PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs
On Fri 2020-11-13 12:18:12, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)"
>
> In preparation to have arguments of a function passed to callbacks attached
> to functions as default, change the default callback prototype to receive a
> struct ftrace_regs as the forth parameter instead of a pt_regs.
>
> For callbacks that set the FL_SAVE_REGS flag in their ftrace_ops flags, they
> will now need to get the pt_regs via a ftrace_get_regs() helper call. If
> this is called by a callback that their ftrace_ops did not have a
> FL_SAVE_REGS flag set, it that helper function will return NULL.
>
> This will allow the ftrace_regs to hold enough just to get the parameters
> and stack pointer, but without the worry that callbacks may have a pt_regs
> that is not completely filled.
>
> diff --git a/arch/csky/kernel/probes/ftrace.c
> b/arch/csky/kernel/probes/ftrace.c
> index f30b179924ef..ae2b1c7b3b5c 100644
> --- a/arch/csky/kernel/probes/ftrace.c
> +++ b/arch/csky/kernel/probes/ftrace.c
> @@ -11,17 +11,19 @@ int arch_check_ftrace_location(struct kprobe *p)
>
> /* Ftrace callback handler for kprobes -- called under preepmt disabed */
> void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> -struct ftrace_ops *ops, struct pt_regs *regs)
> +struct ftrace_ops *ops, struct ftrace_regs *fregs)
> {
> int bit;
> bool lr_saver = false;
> struct kprobe *p;
> struct kprobe_ctlblk *kcb;
> + struct pt_regs *regs;
>
> bit = ftrace_test_recursion_trylock(ip, parent_ip);
> if (bit < 0)
> return;
>
> + regs = ftrace_get_regs(fregs);
Should we check for NULL here?
Same in all callers?
> preempt_disable_notrace();
> p = get_kprobe((kprobe_opcode_t *)ip);
> if (!p) {
Otherwise, the patch is pretty strightforard and looks good to me.
Best Regards,
Petr
[PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs
From: "Steven Rostedt (VMware)"
In preparation to have arguments of a function passed to callbacks attached
to functions as default, change the default callback prototype to receive a
struct ftrace_regs as the forth parameter instead of a pt_regs.
For callbacks that set the FL_SAVE_REGS flag in their ftrace_ops flags, they
will now need to get the pt_regs via a ftrace_get_regs() helper call. If
this is called by a callback that their ftrace_ops did not have a
FL_SAVE_REGS flag set, it that helper function will return NULL.
This will allow the ftrace_regs to hold enough just to get the parameters
and stack pointer, but without the worry that callbacks may have a pt_regs
that is not completely filled.
Acked-by: Peter Zijlstra (Intel)
Reviewed-by: Masami Hiramatsu
Signed-off-by: Steven Rostedt (VMware)
---
arch/csky/kernel/probes/ftrace.c | 4 +++-
arch/nds32/kernel/ftrace.c | 4 ++--
arch/parisc/kernel/ftrace.c | 8 +---
arch/powerpc/kernel/kprobes-ftrace.c | 4 +++-
arch/s390/kernel/ftrace.c| 4 +++-
arch/x86/kernel/kprobes/ftrace.c | 3 ++-
fs/pstore/ftrace.c | 2 +-
include/linux/ftrace.h | 16 ++--
include/linux/kprobes.h | 2 +-
kernel/livepatch/patch.c | 3 ++-
kernel/trace/ftrace.c| 27 +++
kernel/trace/trace_event_perf.c | 2 +-
kernel/trace/trace_events.c | 2 +-
kernel/trace/trace_functions.c | 9 -
kernel/trace/trace_irqsoff.c | 2 +-
kernel/trace/trace_sched_wakeup.c| 2 +-
kernel/trace/trace_selftest.c| 20 +++-
kernel/trace/trace_stack.c | 2 +-
18 files changed, 71 insertions(+), 45 deletions(-)
diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
index f30b179924ef..ae2b1c7b3b5c 100644
--- a/arch/csky/kernel/probes/ftrace.c
+++ b/arch/csky/kernel/probes/ftrace.c
@@ -11,17 +11,19 @@ int arch_check_ftrace_location(struct kprobe *p)
/* Ftrace callback handler for kprobes -- called under preepmt disabed */
void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
- struct ftrace_ops *ops, struct pt_regs *regs)
+ struct ftrace_ops *ops, struct ftrace_regs *fregs)
{
int bit;
bool lr_saver = false;
struct kprobe *p;
struct kprobe_ctlblk *kcb;
+ struct pt_regs *regs;
bit = ftrace_test_recursion_trylock(ip, parent_ip);
if (bit < 0)
return;
+ regs = ftrace_get_regs(fregs);
preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (!p) {
diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c
index 3763b3f8c3db..414f8a780cc3 100644
--- a/arch/nds32/kernel/ftrace.c
+++ b/arch/nds32/kernel/ftrace.c
@@ -10,7 +10,7 @@ extern void (*ftrace_trace_function)(unsigned long, unsigned
long,
extern void ftrace_graph_caller(void);
noinline void __naked ftrace_stub(unsigned long ip, unsigned long parent_ip,
- struct ftrace_ops *op, struct pt_regs *regs)
+ struct ftrace_ops *op, struct ftrace_regs
*fregs)
{
__asm__ (""); /* avoid to optimize as pure function */
}
@@ -38,7 +38,7 @@ EXPORT_SYMBOL(_mcount);
#else /* CONFIG_DYNAMIC_FTRACE */
noinline void __naked ftrace_stub(unsigned long ip, unsigned long parent_ip,
- struct ftrace_ops *op, struct pt_regs *regs)
+ struct ftrace_ops *op, struct ftrace_regs
*fregs)
{
__asm__ (""); /* avoid to optimize as pure function */
}
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 1c5d3732bda2..0a1e75af5382 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -51,7 +51,7 @@ static void __hot prepare_ftrace_return(unsigned long *parent,
void notrace __hot ftrace_function_trampoline(unsigned long parent,
unsigned long self_addr,
unsigned long org_sp_gr3,
- struct pt_regs *regs)
+ struct ftrace_regs *fregs)
{
#ifndef CONFIG_DYNAMIC_FTRACE
extern ftrace_func_t ftrace_trace_function;
@@ -61,7 +61,7 @@ void notrace __hot ftrace_function_trampoline(unsigned long
parent,
if (function_trace_op->flags & FTRACE_OPS_FL_ENABLED &&
ftrace_trace_function != ftrace_stub)
ftrace_trace_function(self_addr, parent,
- function_trace_op, regs);
+ function_trace_op, fregs);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (dereference_function_descriptor(ftrace_graph_return) !=
@@ -204,9 +204,10 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace
*rec,
#ifdef CONFIG_KPROBES_ON_FTRACE
