diff mbox series

[4/4] rehook, fprobe: mark rethook related functions notrace

Message ID 238bad4335d029072ca6000fb404f47376197f39.1684120990.git.zegao@tencent.com
State New
Headers
Series Make fpobe + rethook immune to recursion |

Commit Message

Ze Gao May 15, 2023, 3:26 a.m. UTC 
  These functions are already marked as NOKPROBE to prevent recusion and
we have the same reason to blacklist them if rethook is used with fprobe,
since they are beyond the recursion-free region ftrace can guard.

Signed-off-by: Ze Gao <zegao@tencent.com>
---
 arch/riscv/kernel/probes/rethook.c | 4 ++--
 arch/s390/kernel/rethook.c         | 6 +++---
 arch/x86/kernel/rethook.c          | 8 +++++---
 kernel/trace/rethook.c             | 8 ++++----
 4 files changed, 14 insertions(+), 12 deletions(-)

Comments

Masami Hiramatsu (Google) May 16, 2023, 4:28 a.m. UTC | #1 
On Mon, 15 May 2023 11:26:41 +0800
Ze Gao <zegao2021@gmail.com> wrote:

> These functions are already marked as NOKPROBE to prevent recusion and
> we have the same reason to blacklist them if rethook is used with fprobe,
> since they are beyond the recursion-free region ftrace can guard.
> 
> Signed-off-by: Ze Gao <zegao@tencent.com>
> ---
>  arch/riscv/kernel/probes/rethook.c | 4 ++--
>  arch/s390/kernel/rethook.c         | 6 +++---
>  arch/x86/kernel/rethook.c          | 8 +++++---
>  kernel/trace/rethook.c             | 8 ++++----

Except for the kernel/trace/rethook.c, those looks good to me.
Could you drop notrace from kernel/trace/rethook.c? As Steve mentioned
all functions in that file is automatically notraced.

Thank you,

>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/riscv/kernel/probes/rethook.c b/arch/riscv/kernel/probes/rethook.c
> index 5c27c1f50989..803c412a1bea 100644
> --- a/arch/riscv/kernel/probes/rethook.c
> +++ b/arch/riscv/kernel/probes/rethook.c
> @@ -8,14 +8,14 @@
>  #include "rethook.h"
>  
>  /* This is called from arch_rethook_trampoline() */
> -unsigned long __used arch_rethook_trampoline_callback(struct pt_regs *regs)
> +unsigned long __used notrace arch_rethook_trampoline_callback(struct pt_regs *regs)
>  {
>  	return rethook_trampoline_handler(regs, regs->s0);
>  }
>  
>  NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
>  
> -void arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
> +void notrace arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
>  {
>  	rhn->ret_addr = regs->ra;
>  	rhn->frame = regs->s0;
> diff --git a/arch/s390/kernel/rethook.c b/arch/s390/kernel/rethook.c
> index af10e6bdd34e..ad52119826c1 100644
> --- a/arch/s390/kernel/rethook.c
> +++ b/arch/s390/kernel/rethook.c
> @@ -3,7 +3,7 @@
>  #include <linux/kprobes.h>
>  #include "rethook.h"
>  
> -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> +void notrace arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
>  {
>  	rh->ret_addr = regs->gprs[14];
>  	rh->frame = regs->gprs[15];
> @@ -13,7 +13,7 @@ void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mc
>  }
>  NOKPROBE_SYMBOL(arch_rethook_prepare);
>  
> -void arch_rethook_fixup_return(struct pt_regs *regs,
> +void notrace arch_rethook_fixup_return(struct pt_regs *regs,
>  			       unsigned long correct_ret_addr)
>  {
>  	/* Replace fake return address with real one. */
> @@ -24,7 +24,7 @@ NOKPROBE_SYMBOL(arch_rethook_fixup_return);
>  /*
>   * Called from arch_rethook_trampoline
>   */
> -unsigned long arch_rethook_trampoline_callback(struct pt_regs *regs)
> +unsigned long notrace arch_rethook_trampoline_callback(struct pt_regs *regs)
>  {
>  	return rethook_trampoline_handler(regs, regs->gprs[15]);
>  }
> diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
> index 8a1c0111ae79..1f7cef86f73d 100644
> --- a/arch/x86/kernel/rethook.c
> +++ b/arch/x86/kernel/rethook.c
> @@ -64,7 +64,8 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline);
>  /*
>   * Called from arch_rethook_trampoline
>   */
> -__used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
> +__used __visible void notrace arch_rethook_trampoline_callback(struct pt_regs
> +		*regs)
>  {
>  	unsigned long *frame_pointer;
>  
> @@ -104,7 +105,7 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
>  STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
>  
>  /* This is called from rethook_trampoline_handler(). */
> -void arch_rethook_fixup_return(struct pt_regs *regs,
> +void notrace arch_rethook_fixup_return(struct pt_regs *regs,
>  			       unsigned long correct_ret_addr)
>  {
>  	unsigned long *frame_pointer = (void *)(regs + 1);
> @@ -114,7 +115,8 @@ void arch_rethook_fixup_return(struct pt_regs *regs,
>  }
>  NOKPROBE_SYMBOL(arch_rethook_fixup_return);
>  
> -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> +void notrace arch_rethook_prepare(struct rethook_node *rh, struct pt_regs
> +		*regs, bool mcount)
>  {
>  	unsigned long *stack = (unsigned long *)regs->sp;
>  
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index 60f6cb2b486b..e551e86d3927 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -127,7 +127,7 @@ static void free_rethook_node_rcu(struct rcu_head *head)
>   * Return back the @node to @node::rethook. If the @node::rethook is already
>   * marked as freed, this will free the @node.
>   */
> -void rethook_recycle(struct rethook_node *node)
> +void notrace rethook_recycle(struct rethook_node *node)
>  {
>  	lockdep_assert_preemption_disabled();
>  
> @@ -194,7 +194,7 @@ void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount)
>  NOKPROBE_SYMBOL(rethook_hook);
>  
>  /* This assumes the 'tsk' is the current task or is not running. */
> -static unsigned long __rethook_find_ret_addr(struct task_struct *tsk,
> +static unsigned long notrace __rethook_find_ret_addr(struct task_struct *tsk,
>  					     struct llist_node **cur)
>  {
>  	struct rethook_node *rh = NULL;
> @@ -256,7 +256,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
>  }
>  NOKPROBE_SYMBOL(rethook_find_ret_addr);
>  
> -void __weak arch_rethook_fixup_return(struct pt_regs *regs,
> +void __weak notrace arch_rethook_fixup_return(struct pt_regs *regs,
>  				      unsigned long correct_ret_addr)
>  {
>  	/*
> @@ -268,7 +268,7 @@ void __weak arch_rethook_fixup_return(struct pt_regs *regs,
>  }
>  
>  /* This function will be called from each arch-defined trampoline. */
> -unsigned long rethook_trampoline_handler(struct pt_regs *regs,
> +unsigned long notrace rethook_trampoline_handler(struct pt_regs *regs,
>  					 unsigned long frame)
>  {
>  	struct llist_node *first, *node = NULL;
> -- 
> 2.40.1
>
Ze Gao May 16, 2023, 7:26 a.m. UTC | #2 
Hi Masami,

Thanks for your review. I've applied the makefile trick to arch files
specific to rethook just as
mentioned by Steven. And here is the link:

https://lore.kernel.org/linux-trace-kernel/20230516071830.8190-2-zegao@tencent.com/T/#m503e513071e82d5234d80a1b9e15eb126e334608

Unnecessary notrace annotations have been dropped in this new series.

Thank you,
Ze

On Tue, May 16, 2023 at 12:28 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Mon, 15 May 2023 11:26:41 +0800
> Ze Gao <zegao2021@gmail.com> wrote:
>
> > These functions are already marked as NOKPROBE to prevent recusion and
> > we have the same reason to blacklist them if rethook is used with fprobe,
> > since they are beyond the recursion-free region ftrace can guard.
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> >  arch/riscv/kernel/probes/rethook.c | 4 ++--
> >  arch/s390/kernel/rethook.c         | 6 +++---
> >  arch/x86/kernel/rethook.c          | 8 +++++---
> >  kernel/trace/rethook.c             | 8 ++++----
>
> Except for the kernel/trace/rethook.c, those looks good to me.
> Could you drop notrace from kernel/trace/rethook.c? As Steve mentioned
> all functions in that file is automatically notraced.
>
> Thank you,
>
> >  4 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/probes/rethook.c b/arch/riscv/kernel/probes/rethook.c
> > index 5c27c1f50989..803c412a1bea 100644
> > --- a/arch/riscv/kernel/probes/rethook.c
> > +++ b/arch/riscv/kernel/probes/rethook.c
> > @@ -8,14 +8,14 @@
> >  #include "rethook.h"
> >
> >  /* This is called from arch_rethook_trampoline() */
> > -unsigned long __used arch_rethook_trampoline_callback(struct pt_regs *regs)
> > +unsigned long __used notrace arch_rethook_trampoline_callback(struct pt_regs *regs)
> >  {
> >       return rethook_trampoline_handler(regs, regs->s0);
> >  }
> >
> >  NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
> >
> > -void arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
> > +void notrace arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
> >  {
> >       rhn->ret_addr = regs->ra;
> >       rhn->frame = regs->s0;
> > diff --git a/arch/s390/kernel/rethook.c b/arch/s390/kernel/rethook.c
> > index af10e6bdd34e..ad52119826c1 100644
> > --- a/arch/s390/kernel/rethook.c
> > +++ b/arch/s390/kernel/rethook.c
> > @@ -3,7 +3,7 @@
> >  #include <linux/kprobes.h>
> >  #include "rethook.h"
> >
> > -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> > +void notrace arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> >  {
> >       rh->ret_addr = regs->gprs[14];
> >       rh->frame = regs->gprs[15];
> > @@ -13,7 +13,7 @@ void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mc
> >  }
> >  NOKPROBE_SYMBOL(arch_rethook_prepare);
> >
> > -void arch_rethook_fixup_return(struct pt_regs *regs,
> > +void notrace arch_rethook_fixup_return(struct pt_regs *regs,
> >                              unsigned long correct_ret_addr)
> >  {
> >       /* Replace fake return address with real one. */
> > @@ -24,7 +24,7 @@ NOKPROBE_SYMBOL(arch_rethook_fixup_return);
> >  /*
> >   * Called from arch_rethook_trampoline
> >   */
> > -unsigned long arch_rethook_trampoline_callback(struct pt_regs *regs)
> > +unsigned long notrace arch_rethook_trampoline_callback(struct pt_regs *regs)
> >  {
> >       return rethook_trampoline_handler(regs, regs->gprs[15]);
> >  }
> > diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
> > index 8a1c0111ae79..1f7cef86f73d 100644
> > --- a/arch/x86/kernel/rethook.c
> > +++ b/arch/x86/kernel/rethook.c
> > @@ -64,7 +64,8 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline);
> >  /*
> >   * Called from arch_rethook_trampoline
> >   */
> > -__used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
> > +__used __visible void notrace arch_rethook_trampoline_callback(struct pt_regs
> > +             *regs)
> >  {
> >       unsigned long *frame_pointer;
> >
> > @@ -104,7 +105,7 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
> >  STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
> >
> >  /* This is called from rethook_trampoline_handler(). */
> > -void arch_rethook_fixup_return(struct pt_regs *regs,
> > +void notrace arch_rethook_fixup_return(struct pt_regs *regs,
> >                              unsigned long correct_ret_addr)
> >  {
> >       unsigned long *frame_pointer = (void *)(regs + 1);
> > @@ -114,7 +115,8 @@ void arch_rethook_fixup_return(struct pt_regs *regs,
> >  }
> >  NOKPROBE_SYMBOL(arch_rethook_fixup_return);
> >
> > -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> > +void notrace arch_rethook_prepare(struct rethook_node *rh, struct pt_regs
> > +             *regs, bool mcount)
> >  {
> >       unsigned long *stack = (unsigned long *)regs->sp;
> >
> > diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> > index 60f6cb2b486b..e551e86d3927 100644
> > --- a/kernel/trace/rethook.c
> > +++ b/kernel/trace/rethook.c
> > @@ -127,7 +127,7 @@ static void free_rethook_node_rcu(struct rcu_head *head)
> >   * Return back the @node to @node::rethook. If the @node::rethook is already
> >   * marked as freed, this will free the @node.
> >   */
> > -void rethook_recycle(struct rethook_node *node)
> > +void notrace rethook_recycle(struct rethook_node *node)
> >  {
> >       lockdep_assert_preemption_disabled();
> >
> > @@ -194,7 +194,7 @@ void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount)
> >  NOKPROBE_SYMBOL(rethook_hook);
> >
> >  /* This assumes the 'tsk' is the current task or is not running. */
> > -static unsigned long __rethook_find_ret_addr(struct task_struct *tsk,
> > +static unsigned long notrace __rethook_find_ret_addr(struct task_struct *tsk,
> >                                            struct llist_node **cur)
> >  {
> >       struct rethook_node *rh = NULL;
> > @@ -256,7 +256,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
> >  }
> >  NOKPROBE_SYMBOL(rethook_find_ret_addr);
> >
> > -void __weak arch_rethook_fixup_return(struct pt_regs *regs,
> > +void __weak notrace arch_rethook_fixup_return(struct pt_regs *regs,
> >                                     unsigned long correct_ret_addr)
> >  {
> >       /*
> > @@ -268,7 +268,7 @@ void __weak arch_rethook_fixup_return(struct pt_regs *regs,
> >  }
> >
> >  /* This function will be called from each arch-defined trampoline. */
> > -unsigned long rethook_trampoline_handler(struct pt_regs *regs,
> > +unsigned long notrace rethook_trampoline_handler(struct pt_regs *regs,
> >                                        unsigned long frame)
> >  {
> >       struct llist_node *first, *node = NULL;
> > --
> > 2.40.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
diff mbox series

Patch

diff --git a/arch/riscv/kernel/probes/rethook.c b/arch/riscv/kernel/probes/rethook.c
index 5c27c1f50989..803c412a1bea 100644
--- a/arch/riscv/kernel/probes/rethook.c
+++ b/arch/riscv/kernel/probes/rethook.c
@@ -8,14 +8,14 @@ 
 #include "rethook.h"
 
 /* This is called from arch_rethook_trampoline() */
-unsigned long __used arch_rethook_trampoline_callback(struct pt_regs *regs)
+unsigned long __used notrace arch_rethook_trampoline_callback(struct pt_regs *regs)
 {
 	return rethook_trampoline_handler(regs, regs->s0);
 }
 
 NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
 
-void arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
+void notrace arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
 {
 	rhn->ret_addr = regs->ra;
 	rhn->frame = regs->s0;
diff --git a/arch/s390/kernel/rethook.c b/arch/s390/kernel/rethook.c
index af10e6bdd34e..ad52119826c1 100644
--- a/arch/s390/kernel/rethook.c
+++ b/arch/s390/kernel/rethook.c
@@ -3,7 +3,7 @@ 
 #include <linux/kprobes.h>
 #include "rethook.h"
 
-void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
+void notrace arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
 {
 	rh->ret_addr = regs->gprs[14];
 	rh->frame = regs->gprs[15];
@@ -13,7 +13,7 @@  void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mc
 }
 NOKPROBE_SYMBOL(arch_rethook_prepare);
 
-void arch_rethook_fixup_return(struct pt_regs *regs,
+void notrace arch_rethook_fixup_return(struct pt_regs *regs,
 			       unsigned long correct_ret_addr)
 {
 	/* Replace fake return address with real one. */
@@ -24,7 +24,7 @@  NOKPROBE_SYMBOL(arch_rethook_fixup_return);
 /*
  * Called from arch_rethook_trampoline
  */
-unsigned long arch_rethook_trampoline_callback(struct pt_regs *regs)
+unsigned long notrace arch_rethook_trampoline_callback(struct pt_regs *regs)
 {
 	return rethook_trampoline_handler(regs, regs->gprs[15]);
 }
diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
index 8a1c0111ae79..1f7cef86f73d 100644
--- a/arch/x86/kernel/rethook.c
+++ b/arch/x86/kernel/rethook.c
@@ -64,7 +64,8 @@  NOKPROBE_SYMBOL(arch_rethook_trampoline);
 /*
  * Called from arch_rethook_trampoline
  */
-__used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
+__used __visible void notrace arch_rethook_trampoline_callback(struct pt_regs
+		*regs)
 {
 	unsigned long *frame_pointer;
 
@@ -104,7 +105,7 @@  NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
 STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
 
 /* This is called from rethook_trampoline_handler(). */
-void arch_rethook_fixup_return(struct pt_regs *regs,
+void notrace arch_rethook_fixup_return(struct pt_regs *regs,
 			       unsigned long correct_ret_addr)
 {
 	unsigned long *frame_pointer = (void *)(regs + 1);
@@ -114,7 +115,8 @@  void arch_rethook_fixup_return(struct pt_regs *regs,
 }
 NOKPROBE_SYMBOL(arch_rethook_fixup_return);
 
-void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
+void notrace arch_rethook_prepare(struct rethook_node *rh, struct pt_regs
+		*regs, bool mcount)
 {
 	unsigned long *stack = (unsigned long *)regs->sp;
 
diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
index 60f6cb2b486b..e551e86d3927 100644
--- a/kernel/trace/rethook.c
+++ b/kernel/trace/rethook.c
@@ -127,7 +127,7 @@  static void free_rethook_node_rcu(struct rcu_head *head)
  * Return back the @node to @node::rethook. If the @node::rethook is already
  * marked as freed, this will free the @node.
  */
-void rethook_recycle(struct rethook_node *node)
+void notrace rethook_recycle(struct rethook_node *node)
 {
 	lockdep_assert_preemption_disabled();
 
@@ -194,7 +194,7 @@  void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount)
 NOKPROBE_SYMBOL(rethook_hook);
 
 /* This assumes the 'tsk' is the current task or is not running. */
-static unsigned long __rethook_find_ret_addr(struct task_struct *tsk,
+static unsigned long notrace __rethook_find_ret_addr(struct task_struct *tsk,
 					     struct llist_node **cur)
 {
 	struct rethook_node *rh = NULL;
@@ -256,7 +256,7 @@  unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
 }
 NOKPROBE_SYMBOL(rethook_find_ret_addr);
 
-void __weak arch_rethook_fixup_return(struct pt_regs *regs,
+void __weak notrace arch_rethook_fixup_return(struct pt_regs *regs,
 				      unsigned long correct_ret_addr)
 {
 	/*
@@ -268,7 +268,7 @@  void __weak arch_rethook_fixup_return(struct pt_regs *regs,
 }
 
 /* This function will be called from each arch-defined trampoline. */
-unsigned long rethook_trampoline_handler(struct pt_regs *regs,
+unsigned long notrace rethook_trampoline_handler(struct pt_regs *regs,
 					 unsigned long frame)
 {
 	struct llist_node *first, *node = NULL;