diff mbox series

[RFC,1/9] fprobe: Pass entry_data to handlers

Message ID 166792256330.919356.14776044963704066797.stgit@devnote3 (mailing list archive)
State Superseded
Headers show
Series tracing: Add fprobe events | expand

Commit Message

Masami Hiramatsu (Google) Nov. 8, 2022, 3:49 p.m. UTC 
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Pass the private entry_data to the entry and exit handlers so that
they can share the context data, something like saved function
arguments etc.
User must specify the private entry_data size by @entry_data_size
field before registering the fprobe.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/fprobe.h          |    8 ++++++--
 kernel/trace/bpf_trace.c        |    2 +-
 kernel/trace/fprobe.c           |   21 ++++++++++++++-------
 lib/test_fprobe.c               |    6 ++++--
 samples/fprobe/fprobe_example.c |    6 ++++--
 5 files changed, 29 insertions(+), 14 deletions(-)

Comments

Steven Rostedt Jan. 18, 2023, 1:43 a.m. UTC | #1 
On Wed,  9 Nov 2022 00:49:23 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index e8143e368074..fa25d09c9d57 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -17,14 +17,16 @@
>  struct fprobe_rethook_node {
>  	struct rethook_node node;
>  	unsigned long entry_ip;
> +	char data[];
>  };
>  
>  static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
>  			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
>  {
>  	struct fprobe_rethook_node *fpr;
> -	struct rethook_node *rh;
> +	struct rethook_node *rh = NULL;
>  	struct fprobe *fp;
> +	void *entry_data = NULL;
>  	int bit;
>  
>  	fp = container_of(ops, struct fprobe, ops);
> @@ -37,9 +39,6 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
>  		return;
>  	}
>  
> -	if (fp->entry_handler)
> -		fp->entry_handler(fp, ip, ftrace_get_regs(fregs));
> -
>  	if (fp->exit_handler) {
>  		rh = rethook_try_get(fp->rethook);
>  		if (!rh) {
> @@ -48,9 +47,16 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
>  		}
>  		fpr = container_of(rh, struct fprobe_rethook_node, node);
>  		fpr->entry_ip = ip;
> -		rethook_hook(rh, ftrace_get_regs(fregs), true);
> +		if (fp->entry_data_size)
> +			entry_data = fpr->data;
>  	}
>  
> +	if (fp->entry_handler)
> +		fp->entry_handler(fp, ip, ftrace_get_regs(fregs), entry_data);
> +
> +	if (rh)
> +		rethook_hook(rh, ftrace_get_regs(fregs), true);
> +
>  out:
>  	ftrace_test_recursion_unlock(bit);
>  }
> @@ -81,7 +87,8 @@ static void fprobe_exit_handler(struct rethook_node *rh, void *data,
>  
>  	fpr = container_of(rh, struct fprobe_rethook_node, node);
>  
> -	fp->exit_handler(fp, fpr->entry_ip, regs);
> +	fp->exit_handler(fp, fpr->entry_ip, regs,
> +			 fp->entry_data_size ? (void *)fpr->data : NULL);
>  }
>  NOKPROBE_SYMBOL(fprobe_exit_handler);
>  
> @@ -146,7 +153,7 @@ static int fprobe_init_rethook(struct fprobe *fp, int num)
>  	for (i = 0; i < size; i++) {
>  		struct fprobe_rethook_node *node;
>  
> -		node = kzalloc(sizeof(*node), GFP_KERNEL);
> +		node = kzalloc(sizeof(*node) + fp->entry_data_size, GFP_KERNEL);

		node = kzalloc(struct_size(node, data, fp->entry_data_size), GFP_KERNEL);

Should use struct_size() for any tail structure allocations.

-- Steve


>  		if (!node) {
>  			rethook_free(fp->rethook);
>  			fp->rethook = NULL;
> diff --git a/lib/test_fprobe.c b/lib/test_fprobe.c
> index e0381b3ec410..34fa5a5bbda1 100644
Masami Hiramatsu (Google) Jan. 20, 2023, 3:48 a.m. UTC | #2 
On Tue, 17 Jan 2023 20:43:09 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed,  9 Nov 2022 00:49:23 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index e8143e368074..fa25d09c9d57 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -17,14 +17,16 @@
> >  struct fprobe_rethook_node {
> >  	struct rethook_node node;
> >  	unsigned long entry_ip;
> > +	char data[];
> >  };
> >  
> >  static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
> >  			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
> >  {
> >  	struct fprobe_rethook_node *fpr;
> > -	struct rethook_node *rh;
> > +	struct rethook_node *rh = NULL;
> >  	struct fprobe *fp;
> > +	void *entry_data = NULL;
> >  	int bit;
> >  
> >  	fp = container_of(ops, struct fprobe, ops);
> > @@ -37,9 +39,6 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
> >  		return;
> >  	}
> >  
> > -	if (fp->entry_handler)
> > -		fp->entry_handler(fp, ip, ftrace_get_regs(fregs));
> > -
> >  	if (fp->exit_handler) {
> >  		rh = rethook_try_get(fp->rethook);
> >  		if (!rh) {
> > @@ -48,9 +47,16 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
> >  		}
> >  		fpr = container_of(rh, struct fprobe_rethook_node, node);
> >  		fpr->entry_ip = ip;
> > -		rethook_hook(rh, ftrace_get_regs(fregs), true);
> > +		if (fp->entry_data_size)
> > +			entry_data = fpr->data;
> >  	}
> >  
> > +	if (fp->entry_handler)
> > +		fp->entry_handler(fp, ip, ftrace_get_regs(fregs), entry_data);
> > +
> > +	if (rh)
> > +		rethook_hook(rh, ftrace_get_regs(fregs), true);
> > +
> >  out:
> >  	ftrace_test_recursion_unlock(bit);
> >  }
> > @@ -81,7 +87,8 @@ static void fprobe_exit_handler(struct rethook_node *rh, void *data,
> >  
> >  	fpr = container_of(rh, struct fprobe_rethook_node, node);
> >  
> > -	fp->exit_handler(fp, fpr->entry_ip, regs);
> > +	fp->exit_handler(fp, fpr->entry_ip, regs,
> > +			 fp->entry_data_size ? (void *)fpr->data : NULL);
> >  }
> >  NOKPROBE_SYMBOL(fprobe_exit_handler);
> >  
> > @@ -146,7 +153,7 @@ static int fprobe_init_rethook(struct fprobe *fp, int num)
> >  	for (i = 0; i < size; i++) {
> >  		struct fprobe_rethook_node *node;
> >  
> > -		node = kzalloc(sizeof(*node), GFP_KERNEL);
> > +		node = kzalloc(sizeof(*node) + fp->entry_data_size, GFP_KERNEL);
> 
> 		node = kzalloc(struct_size(node, data, fp->entry_data_size), GFP_KERNEL);
> 
> Should use struct_size() for any tail structure allocations.

OK, I'll use that.

Thank you!

> 
> -- Steve
> 
> 
> >  		if (!node) {
> >  			rethook_free(fp->rethook);
> >  			fp->rethook = NULL;
> > diff --git a/lib/test_fprobe.c b/lib/test_fprobe.c
> > index e0381b3ec410..34fa5a5bbda1 100644
diff mbox series

Patch

diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
index 1c2bde0ead73..e0d4e6136249 100644
--- a/include/linux/fprobe.h
+++ b/include/linux/fprobe.h
@@ -13,6 +13,7 @@ 
  * @nmissed: The counter for missing events.
  * @flags: The status flag.
  * @rethook: The rethook data structure. (internal data)
+ * @entry_data_size: The private data storage size.
  * @entry_handler: The callback function for function entry.
  * @exit_handler: The callback function for function exit.
  */
@@ -29,9 +30,12 @@  struct fprobe {
 	unsigned long		nmissed;
 	unsigned int		flags;
 	struct rethook		*rethook;
+	size_t			entry_data_size;
 
-	void (*entry_handler)(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs);
-	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs);
+	void (*entry_handler)(struct fprobe *fp, unsigned long entry_ip,
+			      struct pt_regs *regs, void *entry_data);
+	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
+			     struct pt_regs *regs, void *entry_data);
 };
 
 /* This fprobe is soft-disabled. */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 1ed08967fb97..68c369452680 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2625,7 +2625,7 @@  kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 
 static void
 kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
-			  struct pt_regs *regs)
+			  struct pt_regs *regs, void *data)
 {
 	struct bpf_kprobe_multi_link *link;
 
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index e8143e368074..fa25d09c9d57 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -17,14 +17,16 @@ 
 struct fprobe_rethook_node {
 	struct rethook_node node;
 	unsigned long entry_ip;
+	char data[];
 };
 
 static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
 			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	struct fprobe_rethook_node *fpr;
-	struct rethook_node *rh;
+	struct rethook_node *rh = NULL;
 	struct fprobe *fp;
+	void *entry_data = NULL;
 	int bit;
 
 	fp = container_of(ops, struct fprobe, ops);
@@ -37,9 +39,6 @@  static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
 		return;
 	}
 
-	if (fp->entry_handler)
-		fp->entry_handler(fp, ip, ftrace_get_regs(fregs));
-
 	if (fp->exit_handler) {
 		rh = rethook_try_get(fp->rethook);
 		if (!rh) {
@@ -48,9 +47,16 @@  static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
 		}
 		fpr = container_of(rh, struct fprobe_rethook_node, node);
 		fpr->entry_ip = ip;
-		rethook_hook(rh, ftrace_get_regs(fregs), true);
+		if (fp->entry_data_size)
+			entry_data = fpr->data;
 	}
 
+	if (fp->entry_handler)
+		fp->entry_handler(fp, ip, ftrace_get_regs(fregs), entry_data);
+
+	if (rh)
+		rethook_hook(rh, ftrace_get_regs(fregs), true);
+
 out:
 	ftrace_test_recursion_unlock(bit);
 }
@@ -81,7 +87,8 @@  static void fprobe_exit_handler(struct rethook_node *rh, void *data,
 
 	fpr = container_of(rh, struct fprobe_rethook_node, node);
 
-	fp->exit_handler(fp, fpr->entry_ip, regs);
+	fp->exit_handler(fp, fpr->entry_ip, regs,
+			 fp->entry_data_size ? (void *)fpr->data : NULL);
 }
 NOKPROBE_SYMBOL(fprobe_exit_handler);
 
@@ -146,7 +153,7 @@  static int fprobe_init_rethook(struct fprobe *fp, int num)
 	for (i = 0; i < size; i++) {
 		struct fprobe_rethook_node *node;
 
-		node = kzalloc(sizeof(*node), GFP_KERNEL);
+		node = kzalloc(sizeof(*node) + fp->entry_data_size, GFP_KERNEL);
 		if (!node) {
 			rethook_free(fp->rethook);
 			fp->rethook = NULL;
diff --git a/lib/test_fprobe.c b/lib/test_fprobe.c
index e0381b3ec410..34fa5a5bbda1 100644
--- a/lib/test_fprobe.c
+++ b/lib/test_fprobe.c
@@ -30,7 +30,8 @@  static noinline u32 fprobe_selftest_target2(u32 value)
 	return (value / div_factor) + 1;
 }
 
-static notrace void fp_entry_handler(struct fprobe *fp, unsigned long ip, struct pt_regs *regs)
+static notrace void fp_entry_handler(struct fprobe *fp, unsigned long ip,
+				     struct pt_regs *regs, void *data)
 {
 	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	/* This can be called on the fprobe_selftest_target and the fprobe_selftest_target2 */
@@ -39,7 +40,8 @@  static notrace void fp_entry_handler(struct fprobe *fp, unsigned long ip, struct
 	entry_val = (rand1 / div_factor);
 }
 
-static notrace void fp_exit_handler(struct fprobe *fp, unsigned long ip, struct pt_regs *regs)
+static notrace void fp_exit_handler(struct fprobe *fp, unsigned long ip,
+				    struct pt_regs *regs, void *data)
 {
 	unsigned long ret = regs_return_value(regs);
 
diff --git a/samples/fprobe/fprobe_example.c b/samples/fprobe/fprobe_example.c
index e22da8573116..dd794990ad7e 100644
--- a/samples/fprobe/fprobe_example.c
+++ b/samples/fprobe/fprobe_example.c
@@ -48,7 +48,8 @@  static void show_backtrace(void)
 	stack_trace_print(stacks, len, 24);
 }
 
-static void sample_entry_handler(struct fprobe *fp, unsigned long ip, struct pt_regs *regs)
+static void sample_entry_handler(struct fprobe *fp, unsigned long ip,
+				 struct pt_regs *regs, void *data)
 {
 	if (use_trace)
 		/*
@@ -63,7 +64,8 @@  static void sample_entry_handler(struct fprobe *fp, unsigned long ip, struct pt_
 		show_backtrace();
 }
 
-static void sample_exit_handler(struct fprobe *fp, unsigned long ip, struct pt_regs *regs)
+static void sample_exit_handler(struct fprobe *fp, unsigned long ip, struct pt_regs *regs,
+				void *data)
 {
 	unsigned long rip = instruction_pointer(regs);