linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
@ 2020-01-10  1:45 Masami Hiramatsu
  2020-01-10  1:47 ` Masami Hiramatsu
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-10  1:45 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Steven Rostedt
  Cc: Jiri Olsa, Peter Zijlstra, Ingo Molnar, Naveen N . Rao,
	Anil S Keshavamurthy, David S . Miller, Namhyung Kim,
	Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

Fix double perf_event linking to trace_uprobe_filter on
multiple uprobe event by moving trace_uprobe_filter under
trace_probe_event.

In uprobe perf event, trace_uprobe_filter data structure is
managing target mm filters (in perf_event) related to each
uprobe event.

Since commit 60d53e2c3b75 ("tracing/probe: Split trace_event
related data from trace_probe") left the trace_uprobe_filter
data structure in trace_uprobe, if a trace_probe_event has
multiple trace_uprobe (multi-probe event), a perf_event is
added to different trace_uprobe_filter on each trace_uprobe.
This leads a linked list corruption.

To fix this issue, move trace_uprobe_filter to trace_probe_event
and link it once on each event instead of each probe.

Fixes: 60d53e2c3b75 ("tracing/probe: Split trace_event related data from trace_probe")
Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
URL: https://lkml.kernel.org/r/20200108171611.GA8472@kernel.org
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |    2 -
 kernel/trace/trace_probe.c  |    5 +-
 kernel/trace/trace_probe.h  |    3 +
 kernel/trace/trace_uprobe.c |  124 ++++++++++++++++++++++++++++---------------
 4 files changed, 86 insertions(+), 48 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 7f890262c8a3..3e5f9c7d939c 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -290,7 +290,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
 	INIT_HLIST_NODE(&tk->rp.kp.hlist);
 	INIT_LIST_HEAD(&tk->rp.kp.list);
 
-	ret = trace_probe_init(&tk->tp, event, group);
+	ret = trace_probe_init(&tk->tp, event, group, 0);
 	if (ret < 0)
 		goto error;
 
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 905b10af5d5c..bba18cf44a30 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -984,7 +984,7 @@ void trace_probe_cleanup(struct trace_probe *tp)
 }
 
 int trace_probe_init(struct trace_probe *tp, const char *event,
-		     const char *group)
+		     const char *group, size_t event_data_size)
 {
 	struct trace_event_call *call;
 	int ret = 0;
@@ -992,7 +992,8 @@ int trace_probe_init(struct trace_probe *tp, const char *event,
 	if (!event || !group)
 		return -EINVAL;
 
-	tp->event = kzalloc(sizeof(struct trace_probe_event), GFP_KERNEL);
+	tp->event = kzalloc(sizeof(struct trace_probe_event) + event_data_size,
+			    GFP_KERNEL);
 	if (!tp->event)
 		return -ENOMEM;
 
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 4ee703728aec..03e4e180058d 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -230,6 +230,7 @@ struct trace_probe_event {
 	struct trace_event_call		call;
 	struct list_head 		files;
 	struct list_head		probes;
+	char				data[0];
 };
 
 struct trace_probe {
@@ -322,7 +323,7 @@ static inline bool trace_probe_has_single_file(struct trace_probe *tp)
 }
 
 int trace_probe_init(struct trace_probe *tp, const char *event,
-		     const char *group);
+		     const char *group, size_t event_data_size);
 void trace_probe_cleanup(struct trace_probe *tp);
 int trace_probe_append(struct trace_probe *tp, struct trace_probe *to);
 void trace_probe_unlink(struct trace_probe *tp);
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 352073d36585..f66e202fec13 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -60,7 +60,6 @@ static struct dyn_event_operations trace_uprobe_ops = {
  */
 struct trace_uprobe {
 	struct dyn_event		devent;
-	struct trace_uprobe_filter	filter;
 	struct uprobe_consumer		consumer;
 	struct path			path;
 	struct inode			*inode;
@@ -264,6 +263,14 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
 }
 NOKPROBE_SYMBOL(process_fetch_insn)
 
+static struct trace_uprobe_filter *
+trace_uprobe_get_filter(struct trace_uprobe *tu)
+{
+	struct trace_probe_event *event = tu->tp.event;
+
+	return (struct trace_uprobe_filter *)&event->data[0];
+}
+
 static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
 {
 	rwlock_init(&filter->rwlock);
@@ -351,7 +358,8 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	if (!tu)
 		return ERR_PTR(-ENOMEM);
 
-	ret = trace_probe_init(&tu->tp, event, group);
+	ret = trace_probe_init(&tu->tp, event, group,
+				sizeof(struct trace_uprobe_filter));
 	if (ret < 0)
 		goto error;
 
@@ -359,7 +367,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	tu->consumer.handler = uprobe_dispatcher;
 	if (is_ret)
 		tu->consumer.ret_handler = uretprobe_dispatcher;
-	init_trace_uprobe_filter(&tu->filter);
+	init_trace_uprobe_filter(trace_uprobe_get_filter(tu));
 	return tu;
 
 error:
@@ -1067,13 +1075,14 @@ static void __probe_event_disable(struct trace_probe *tp)
 	struct trace_probe *pos;
 	struct trace_uprobe *tu;
 
+	tu = container_of(tp, struct trace_uprobe, tp);
+	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
+
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
 		tu = container_of(pos, struct trace_uprobe, tp);
 		if (!tu->inode)
 			continue;
 
-		WARN_ON(!uprobe_filter_is_empty(&tu->filter));
-
 		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
 		tu->inode = NULL;
 	}
@@ -1108,7 +1117,7 @@ static int probe_event_enable(struct trace_event_call *call,
 	}
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
+	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
 
 	if (enabled)
 		return 0;
@@ -1205,39 +1214,39 @@ __uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
 }
 
 static inline bool
-uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
+trace_uprobe_filter_event(struct trace_uprobe_filter *filter,
+			  struct perf_event *event)
 {
-	return __uprobe_perf_filter(&tu->filter, event->hw.target->mm);
+	return __uprobe_perf_filter(filter, event->hw.target->mm);
 }
 
-static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
+static bool trace_uprobe_filter_remove(struct trace_uprobe_filter *filter,
+				       struct perf_event *event)
 {
 	bool done;
 
-	write_lock(&tu->filter.rwlock);
+	write_lock(&filter->rwlock);
 	if (event->hw.target) {
 		list_del(&event->hw.tp_list);
-		done = tu->filter.nr_systemwide ||
+		done = filter->nr_systemwide ||
 			(event->hw.target->flags & PF_EXITING) ||
-			uprobe_filter_event(tu, event);
+			trace_uprobe_filter_event(filter, event);
 	} else {
-		tu->filter.nr_systemwide--;
-		done = tu->filter.nr_systemwide;
+		filter->nr_systemwide--;
+		done = filter->nr_systemwide;
 	}
-	write_unlock(&tu->filter.rwlock);
-
-	if (!done)
-		return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+	write_unlock(&filter->rwlock);
 
-	return 0;
+	return done;
 }
 
-static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
+/* This returns true if the filter always covers target mm */
+static bool trace_uprobe_filter_add(struct trace_uprobe_filter *filter,
+				    struct perf_event *event)
 {
 	bool done;
-	int err;
 
-	write_lock(&tu->filter.rwlock);
+	write_lock(&filter->rwlock);
 	if (event->hw.target) {
 		/*
 		 * event->parent != NULL means copy_process(), we can avoid
@@ -1247,28 +1256,21 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
 		 * attr.enable_on_exec means that exec/mmap will install the
 		 * breakpoints we need.
 		 */
-		done = tu->filter.nr_systemwide ||
+		done = filter->nr_systemwide ||
 			event->parent || event->attr.enable_on_exec ||
-			uprobe_filter_event(tu, event);
-		list_add(&event->hw.tp_list, &tu->filter.perf_events);
+			trace_uprobe_filter_event(filter, event);
+		list_add(&event->hw.tp_list, &filter->perf_events);
 	} else {
-		done = tu->filter.nr_systemwide;
-		tu->filter.nr_systemwide++;
+		done = filter->nr_systemwide;
+		filter->nr_systemwide++;
 	}
-	write_unlock(&tu->filter.rwlock);
+	write_unlock(&filter->rwlock);
 
-	err = 0;
-	if (!done) {
-		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
-		if (err)
-			uprobe_perf_close(tu, event);
-	}
-	return err;
+	return done;
 }
 
-static int uprobe_perf_multi_call(struct trace_event_call *call,
-				  struct perf_event *event,
-		int (*op)(struct trace_uprobe *tu, struct perf_event *event))
+static int uprobe_perf_close(struct trace_event_call *call,
+			     struct perf_event *event)
 {
 	struct trace_probe *pos, *tp;
 	struct trace_uprobe *tu;
@@ -1278,25 +1280,59 @@ static int uprobe_perf_multi_call(struct trace_event_call *call,
 	if (WARN_ON_ONCE(!tp))
 		return -ENODEV;
 
+	tu = container_of(tp, struct trace_uprobe, tp);
+	if (trace_uprobe_filter_remove(trace_uprobe_get_filter(tu), event))
+		return 0;
+
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
 		tu = container_of(pos, struct trace_uprobe, tp);
-		ret = op(tu, event);
+		ret = uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
 		if (ret)
 			break;
 	}
 
 	return ret;
 }
+
+static int uprobe_perf_open(struct trace_event_call *call,
+			    struct perf_event *event)
+{
+	struct trace_probe *pos, *tp;
+	struct trace_uprobe *tu;
+	int err = 0;
+
+	tp = trace_probe_primary_from_call(call);
+	if (WARN_ON_ONCE(!tp))
+		return -ENODEV;
+
+	tu = container_of(tp, struct trace_uprobe, tp);
+	if (trace_uprobe_filter_add(trace_uprobe_get_filter(tu), event))
+		return 0;
+
+	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
+		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+		if (err) {
+			uprobe_perf_close(call, event);
+			break;
+		}
+	}
+
+	return err;
+}
+
 static bool uprobe_perf_filter(struct uprobe_consumer *uc,
 				enum uprobe_filter_ctx ctx, struct mm_struct *mm)
 {
+	struct trace_uprobe_filter *filter;
 	struct trace_uprobe *tu;
 	int ret;
 
 	tu = container_of(uc, struct trace_uprobe, consumer);
-	read_lock(&tu->filter.rwlock);
-	ret = __uprobe_perf_filter(&tu->filter, mm);
-	read_unlock(&tu->filter.rwlock);
+	filter = trace_uprobe_get_filter(tu);
+
+	read_lock(&filter->rwlock);
+	ret = __uprobe_perf_filter(filter, mm);
+	read_unlock(&filter->rwlock);
 
 	return ret;
 }
@@ -1419,10 +1455,10 @@ trace_uprobe_register(struct trace_event_call *event, enum trace_reg type,
 		return 0;
 
 	case TRACE_REG_PERF_OPEN:
-		return uprobe_perf_multi_call(event, data, uprobe_perf_open);
+		return uprobe_perf_open(event, data);
 
 	case TRACE_REG_PERF_CLOSE:
-		return uprobe_perf_multi_call(event, data, uprobe_perf_close);
+		return uprobe_perf_close(event, data);
 
 #endif
 	default:


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-10  1:45 [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Masami Hiramatsu
@ 2020-01-10  1:47 ` Masami Hiramatsu
  2020-01-14 17:35 ` Arnaldo Carvalho de Melo
  2020-01-20 12:40 ` Peter Zijlstra
  2 siblings, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-10  1:47 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, Steven Rostedt, Jiri Olsa,
	Peter Zijlstra, Ingo Molnar, Naveen N . Rao,
	Anil S Keshavamurthy, David S . Miller, Namhyung Kim,
	Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Fri, 10 Jan 2020 10:45:39 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix double perf_event linking to trace_uprobe_filter on
> multiple uprobe event by moving trace_uprobe_filter under
> trace_probe_event.
> 
> In uprobe perf event, trace_uprobe_filter data structure is
> managing target mm filters (in perf_event) related to each
> uprobe event.
> 
> Since commit 60d53e2c3b75 ("tracing/probe: Split trace_event
> related data from trace_probe") left the trace_uprobe_filter
> data structure in trace_uprobe, if a trace_probe_event has
> multiple trace_uprobe (multi-probe event), a perf_event is
> added to different trace_uprobe_filter on each trace_uprobe.
> This leads a linked list corruption.
> 
> To fix this issue, move trace_uprobe_filter to trace_probe_event
> and link it once on each event instead of each probe.
> 
> Fixes: 60d53e2c3b75 ("tracing/probe: Split trace_event related data from trace_probe")
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> URL: https://lkml.kernel.org/r/20200108171611.GA8472@kernel.org

Oops, s/URL/Link/ 

Thanks,

> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |    2 -
>  kernel/trace/trace_probe.c  |    5 +-
>  kernel/trace/trace_probe.h  |    3 +
>  kernel/trace/trace_uprobe.c |  124 ++++++++++++++++++++++++++++---------------
>  4 files changed, 86 insertions(+), 48 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 7f890262c8a3..3e5f9c7d939c 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -290,7 +290,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
>  	INIT_HLIST_NODE(&tk->rp.kp.hlist);
>  	INIT_LIST_HEAD(&tk->rp.kp.list);
>  
> -	ret = trace_probe_init(&tk->tp, event, group);
> +	ret = trace_probe_init(&tk->tp, event, group, 0);
>  	if (ret < 0)
>  		goto error;
>  
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 905b10af5d5c..bba18cf44a30 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -984,7 +984,7 @@ void trace_probe_cleanup(struct trace_probe *tp)
>  }
>  
>  int trace_probe_init(struct trace_probe *tp, const char *event,
> -		     const char *group)
> +		     const char *group, size_t event_data_size)
>  {
>  	struct trace_event_call *call;
>  	int ret = 0;
> @@ -992,7 +992,8 @@ int trace_probe_init(struct trace_probe *tp, const char *event,
>  	if (!event || !group)
>  		return -EINVAL;
>  
> -	tp->event = kzalloc(sizeof(struct trace_probe_event), GFP_KERNEL);
> +	tp->event = kzalloc(sizeof(struct trace_probe_event) + event_data_size,
> +			    GFP_KERNEL);
>  	if (!tp->event)
>  		return -ENOMEM;
>  
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 4ee703728aec..03e4e180058d 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -230,6 +230,7 @@ struct trace_probe_event {
>  	struct trace_event_call		call;
>  	struct list_head 		files;
>  	struct list_head		probes;
> +	char				data[0];
>  };
>  
>  struct trace_probe {
> @@ -322,7 +323,7 @@ static inline bool trace_probe_has_single_file(struct trace_probe *tp)
>  }
>  
>  int trace_probe_init(struct trace_probe *tp, const char *event,
> -		     const char *group);
> +		     const char *group, size_t event_data_size);
>  void trace_probe_cleanup(struct trace_probe *tp);
>  int trace_probe_append(struct trace_probe *tp, struct trace_probe *to);
>  void trace_probe_unlink(struct trace_probe *tp);
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 352073d36585..f66e202fec13 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -60,7 +60,6 @@ static struct dyn_event_operations trace_uprobe_ops = {
>   */
>  struct trace_uprobe {
>  	struct dyn_event		devent;
> -	struct trace_uprobe_filter	filter;
>  	struct uprobe_consumer		consumer;
>  	struct path			path;
>  	struct inode			*inode;
> @@ -264,6 +263,14 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
>  }
>  NOKPROBE_SYMBOL(process_fetch_insn)
>  
> +static struct trace_uprobe_filter *
> +trace_uprobe_get_filter(struct trace_uprobe *tu)
> +{
> +	struct trace_probe_event *event = tu->tp.event;
> +
> +	return (struct trace_uprobe_filter *)&event->data[0];
> +}
> +
>  static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
>  {
>  	rwlock_init(&filter->rwlock);
> @@ -351,7 +358,8 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
>  	if (!tu)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = trace_probe_init(&tu->tp, event, group);
> +	ret = trace_probe_init(&tu->tp, event, group,
> +				sizeof(struct trace_uprobe_filter));
>  	if (ret < 0)
>  		goto error;
>  
> @@ -359,7 +367,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
>  	tu->consumer.handler = uprobe_dispatcher;
>  	if (is_ret)
>  		tu->consumer.ret_handler = uretprobe_dispatcher;
> -	init_trace_uprobe_filter(&tu->filter);
> +	init_trace_uprobe_filter(trace_uprobe_get_filter(tu));
>  	return tu;
>  
>  error:
> @@ -1067,13 +1075,14 @@ static void __probe_event_disable(struct trace_probe *tp)
>  	struct trace_probe *pos;
>  	struct trace_uprobe *tu;
>  
> +	tu = container_of(tp, struct trace_uprobe, tp);
> +	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
> +
>  	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
>  		tu = container_of(pos, struct trace_uprobe, tp);
>  		if (!tu->inode)
>  			continue;
>  
> -		WARN_ON(!uprobe_filter_is_empty(&tu->filter));
> -
>  		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
>  		tu->inode = NULL;
>  	}
> @@ -1108,7 +1117,7 @@ static int probe_event_enable(struct trace_event_call *call,
>  	}
>  
>  	tu = container_of(tp, struct trace_uprobe, tp);
> -	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
> +	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
>  
>  	if (enabled)
>  		return 0;
> @@ -1205,39 +1214,39 @@ __uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
>  }
>  
>  static inline bool
> -uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
> +trace_uprobe_filter_event(struct trace_uprobe_filter *filter,
> +			  struct perf_event *event)
>  {
> -	return __uprobe_perf_filter(&tu->filter, event->hw.target->mm);
> +	return __uprobe_perf_filter(filter, event->hw.target->mm);
>  }
>  
> -static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
> +static bool trace_uprobe_filter_remove(struct trace_uprobe_filter *filter,
> +				       struct perf_event *event)
>  {
>  	bool done;
>  
> -	write_lock(&tu->filter.rwlock);
> +	write_lock(&filter->rwlock);
>  	if (event->hw.target) {
>  		list_del(&event->hw.tp_list);
> -		done = tu->filter.nr_systemwide ||
> +		done = filter->nr_systemwide ||
>  			(event->hw.target->flags & PF_EXITING) ||
> -			uprobe_filter_event(tu, event);
> +			trace_uprobe_filter_event(filter, event);
>  	} else {
> -		tu->filter.nr_systemwide--;
> -		done = tu->filter.nr_systemwide;
> +		filter->nr_systemwide--;
> +		done = filter->nr_systemwide;
>  	}
> -	write_unlock(&tu->filter.rwlock);
> -
> -	if (!done)
> -		return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> +	write_unlock(&filter->rwlock);
>  
> -	return 0;
> +	return done;
>  }
>  
> -static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
> +/* This returns true if the filter always covers target mm */
> +static bool trace_uprobe_filter_add(struct trace_uprobe_filter *filter,
> +				    struct perf_event *event)
>  {
>  	bool done;
> -	int err;
>  
> -	write_lock(&tu->filter.rwlock);
> +	write_lock(&filter->rwlock);
>  	if (event->hw.target) {
>  		/*
>  		 * event->parent != NULL means copy_process(), we can avoid
> @@ -1247,28 +1256,21 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
>  		 * attr.enable_on_exec means that exec/mmap will install the
>  		 * breakpoints we need.
>  		 */
> -		done = tu->filter.nr_systemwide ||
> +		done = filter->nr_systemwide ||
>  			event->parent || event->attr.enable_on_exec ||
> -			uprobe_filter_event(tu, event);
> -		list_add(&event->hw.tp_list, &tu->filter.perf_events);
> +			trace_uprobe_filter_event(filter, event);
> +		list_add(&event->hw.tp_list, &filter->perf_events);
>  	} else {
> -		done = tu->filter.nr_systemwide;
> -		tu->filter.nr_systemwide++;
> +		done = filter->nr_systemwide;
> +		filter->nr_systemwide++;
>  	}
> -	write_unlock(&tu->filter.rwlock);
> +	write_unlock(&filter->rwlock);
>  
> -	err = 0;
> -	if (!done) {
> -		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> -		if (err)
> -			uprobe_perf_close(tu, event);
> -	}
> -	return err;
> +	return done;
>  }
>  
> -static int uprobe_perf_multi_call(struct trace_event_call *call,
> -				  struct perf_event *event,
> -		int (*op)(struct trace_uprobe *tu, struct perf_event *event))
> +static int uprobe_perf_close(struct trace_event_call *call,
> +			     struct perf_event *event)
>  {
>  	struct trace_probe *pos, *tp;
>  	struct trace_uprobe *tu;
> @@ -1278,25 +1280,59 @@ static int uprobe_perf_multi_call(struct trace_event_call *call,
>  	if (WARN_ON_ONCE(!tp))
>  		return -ENODEV;
>  
> +	tu = container_of(tp, struct trace_uprobe, tp);
> +	if (trace_uprobe_filter_remove(trace_uprobe_get_filter(tu), event))
> +		return 0;
> +
>  	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
>  		tu = container_of(pos, struct trace_uprobe, tp);
> -		ret = op(tu, event);
> +		ret = uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
>  		if (ret)
>  			break;
>  	}
>  
>  	return ret;
>  }
> +
> +static int uprobe_perf_open(struct trace_event_call *call,
> +			    struct perf_event *event)
> +{
> +	struct trace_probe *pos, *tp;
> +	struct trace_uprobe *tu;
> +	int err = 0;
> +
> +	tp = trace_probe_primary_from_call(call);
> +	if (WARN_ON_ONCE(!tp))
> +		return -ENODEV;
> +
> +	tu = container_of(tp, struct trace_uprobe, tp);
> +	if (trace_uprobe_filter_add(trace_uprobe_get_filter(tu), event))
> +		return 0;
> +
> +	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> +		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> +		if (err) {
> +			uprobe_perf_close(call, event);
> +			break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
>  static bool uprobe_perf_filter(struct uprobe_consumer *uc,
>  				enum uprobe_filter_ctx ctx, struct mm_struct *mm)
>  {
> +	struct trace_uprobe_filter *filter;
>  	struct trace_uprobe *tu;
>  	int ret;
>  
>  	tu = container_of(uc, struct trace_uprobe, consumer);
> -	read_lock(&tu->filter.rwlock);
> -	ret = __uprobe_perf_filter(&tu->filter, mm);
> -	read_unlock(&tu->filter.rwlock);
> +	filter = trace_uprobe_get_filter(tu);
> +
> +	read_lock(&filter->rwlock);
> +	ret = __uprobe_perf_filter(filter, mm);
> +	read_unlock(&filter->rwlock);
>  
>  	return ret;
>  }
> @@ -1419,10 +1455,10 @@ trace_uprobe_register(struct trace_event_call *event, enum trace_reg type,
>  		return 0;
>  
>  	case TRACE_REG_PERF_OPEN:
> -		return uprobe_perf_multi_call(event, data, uprobe_perf_open);
> +		return uprobe_perf_open(event, data);
>  
>  	case TRACE_REG_PERF_CLOSE:
> -		return uprobe_perf_multi_call(event, data, uprobe_perf_close);
> +		return uprobe_perf_close(event, data);
>  
>  #endif
>  	default:
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-10  1:45 [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Masami Hiramatsu
  2020-01-10  1:47 ` Masami Hiramatsu
@ 2020-01-14 17:35 ` Arnaldo Carvalho de Melo
  2020-01-14 17:44   ` Steven Rostedt
  2020-01-20 12:40 ` Peter Zijlstra
  2 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-01-14 17:35 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Jiri Olsa, Peter Zijlstra, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

Em Fri, Jan 10, 2020 at 10:45:39AM +0900, Masami Hiramatsu escreveu:
> Fix double perf_event linking to trace_uprobe_filter on
> multiple uprobe event by moving trace_uprobe_filter under
> trace_probe_event.
> 
> In uprobe perf event, trace_uprobe_filter data structure is
> managing target mm filters (in perf_event) related to each
> uprobe event.
> 
> Since commit 60d53e2c3b75 ("tracing/probe: Split trace_event
> related data from trace_probe") left the trace_uprobe_filter
> data structure in trace_uprobe, if a trace_probe_event has
> multiple trace_uprobe (multi-probe event), a perf_event is
> added to different trace_uprobe_filter on each trace_uprobe.
> This leads a linked list corruption.
> 
> To fix this issue, move trace_uprobe_filter to trace_probe_event
> and link it once on each event instead of each probe.
> 
> Fixes: 60d53e2c3b75 ("tracing/probe: Split trace_event related data from trace_probe")
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> URL: https://lkml.kernel.org/r/20200108171611.GA8472@kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks, now it works as expected:

Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

If you don't mind I can put it into my next perf/urgent pull req to
Ingo/Thomas,

And here is the sequence to check that libbpf routines linked statically
with perf are in fact being used when using 'perf trace' with one of its
events being a .c file, which gets interpreted as a bpf program, gets
compiled, and then the .o is handed over to libbpf on its way to the
kernel, in this specific case it'll collect, among other things, the
open, openat, open* pathnames, then we see that perf stat counts how
many times those routines were called:

[root@quaco ~]# perf probe -x ~/bin/perf bpf_[op]*__*
Added new events:
  probe_perf:bpf_output__fprintf (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_output__printer (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__bpil_offs_to_addr (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__bpil_addr_to_offs (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__get_prog_info_linear (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__attach_trace (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__attach_raw_tracepoint (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__attach_tracepoint (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__attach_uprobe (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__attach_kprobe (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__attach_perf_event (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__find_map_by_offset (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__find_map_fd_by_name (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__find_map_by_name (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_expected_attach_type (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__get_expected_attach_type (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_tracing (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_tracing (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_perf_event (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_perf_event (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_xdp (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_xdp (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_raw_tracepoint (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_raw_tracepoint (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_tracepoint (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_tracepoint (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_sched_act (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_sched_act (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_sched_cls (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_sched_cls (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_kprobe (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_kprobe (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_socket_filter (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_socket_filter (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_type (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_type (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__get_type (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__nth_fd (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_prep (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__size (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__fd (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__title (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_ifindex (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__priv (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__set_priv (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__prev (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__next (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__priv (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__set_priv (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__btf_fd (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__btf (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__kversion (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__name (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__next (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__close (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__pin (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__unpin_programs (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__pin_programs (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__unpin_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__pin_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__unpin (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__pin (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__unpin_instance (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__pin_instance (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__load (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__load_xattr (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__unload (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__open_buffer (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__open_mem (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__open_file (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__open (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__open_xattr (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__load_progs (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__is_function_storage (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__load (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__collect_reloc (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__relocate (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__relocate (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__reloc_text (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__relocate_core (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__create_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__populate_internal_map (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__reuse_map (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__probe_caps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__probe_array_mmap (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__probe_btf_datasec (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__probe_btf_func (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__probe_global_data (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__probe_name (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__collect_reloc (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__record_reloc (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__section_to_libbpf_map_type (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__shndx_is_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__shndx_is_data (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__find_program_by_title (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__find_prog_by_idx (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__elf_collect (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__sanitize_and_load_btf (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_btf (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__is_btf_mandatory (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__sanitize_btf_ext (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__sanitize_btf (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_user_btf_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_user_btf_map (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_user_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_global_data_maps (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_internal_map (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__add_map (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__variable_offset (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__section_size (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_kversion (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_license (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__check_endianness (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__elf_init (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__elf_finish (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__new (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__init_prog_names (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_object__add_program (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__init (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__exit (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_program__unload (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_prog_linfo__lfind (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_prog_linfo__lfind_addr_func (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_prog_linfo__new (on bpf_[op]*__* in /home/acme/bin/perf)
  probe_perf:bpf_prog_linfo__free (on bpf_[op]*__* in /home/acme/bin/perf)

You can now use it in all perf tools, such as:

	perf record -e probe_perf:bpf_prog_linfo__free -aR sleep 1

[root@quaco ~]# #perf stat -e probe_perf:bpf* perf trace
[root@quaco ~]# cat ~/.perfconfig.OFF
[llvm]
	dump-obj = true
	clang-opt = -g
[trace]
	add_events = /home/acme/git/perf/tools/perf/examples/bpf/augmented_raw_syscalls.o
	#add_events = /home/acme/augmented_raw_syscalls.c
	#add_events = /home/acme/git/perf/tools/perf/examples/bpf/augmented_raw_syscalls.c
	#add_events = /wb/augmented_raw_syscalls.o
	show_zeros = yes
	show_duration = no
	no_inherit = yes
	#show_timestamp = no
	#show_arg_names = no
	args_alignment = 40
	#show_prefix = yes

[annotate]

	hide_src_code = false
[root@quaco ~]# mv ~/.perfconfig ~/.perfconfig.OFF
mv: cannot stat '/root/.perfconfig': No such file or directory
[root@quaco ~]#
[root@quaco ~]# perf stat -e probe_perf:bpf* perf trace -e /home/acme/git/perf/tools/perf/examples/bpf/augmented_raw_syscalls.c,open* sleep 1
     0.000 ( 0.009 ms): sleep/16207 openat(dfd: CWD, filename: "/etc/ld.so.cache", flags: RDONLY|CLOEXEC) = 3
     0.031 ( 0.005 ms): sleep/16207 openat(dfd: CWD, filename: "/lib64/libc.so.6", flags: RDONLY|CLOEXEC) = 3
     0.308 ( 0.008 ms): sleep/16207 openat(dfd: CWD, filename: "", flags: RDONLY|CLOEXEC)                 = 3

 Performance counter stats for 'perf trace -e /home/acme/git/perf/tools/perf/examples/bpf/augmented_raw_syscalls.c,open* sleep 1':

                 0      probe_perf:bpf_prog_linfo__free
                 0      probe_perf:bpf_prog_linfo__new
                 0      probe_perf:bpf_prog_linfo__lfind_addr_func
                 0      probe_perf:bpf_prog_linfo__lfind
                18      probe_perf:bpf_program__unload
                 9      probe_perf:bpf_program__exit
                 9      probe_perf:bpf_program__init
                 9      probe_perf:bpf_object__add_program
                 1      probe_perf:bpf_object__init_prog_names
                 1      probe_perf:bpf_object__new
                 2      probe_perf:bpf_object__elf_finish
                 1      probe_perf:bpf_object__elf_init
                 1      probe_perf:bpf_object__check_endianness
                 1      probe_perf:bpf_object__init_license
                 1      probe_perf:bpf_object__init_kversion
                 0      probe_perf:bpf_object__section_size
                 0      probe_perf:bpf_object__variable_offset
                 6      probe_perf:bpf_object__add_map
                 0      probe_perf:bpf_object__init_internal_map
                 1      probe_perf:bpf_object__init_global_data_maps
                 1      probe_perf:bpf_object__init_user_maps
                 0      probe_perf:bpf_object__init_user_btf_map
                 1      probe_perf:bpf_object__init_user_btf_maps
                 1      probe_perf:bpf_object__init_maps
                 0      probe_perf:bpf_object__sanitize_btf
                 0      probe_perf:bpf_object__sanitize_btf_ext
                 1      probe_perf:bpf_object__is_btf_mandatory
                 1      probe_perf:bpf_object__init_btf
                 1      probe_perf:bpf_object__sanitize_and_load_btf
                 1      probe_perf:bpf_object__elf_collect
                 8      probe_perf:bpf_object__find_prog_by_idx
                15      probe_perf:bpf_object__find_program_by_title
                 0      probe_perf:bpf_object__shndx_is_data
                17      probe_perf:bpf_object__shndx_is_maps
                17      probe_perf:bpf_object__section_to_libbpf_map_type
                17      probe_perf:bpf_program__record_reloc
                 8      probe_perf:bpf_program__collect_reloc
                 1      probe_perf:bpf_object__probe_name
                 1      probe_perf:bpf_object__probe_global_data
                 1      probe_perf:bpf_object__probe_btf_func
                 1      probe_perf:bpf_object__probe_btf_datasec
                 1      probe_perf:bpf_object__probe_array_mmap
                 1      probe_perf:bpf_object__probe_caps
                 0      probe_perf:bpf_object__reuse_map
                 0      probe_perf:bpf_object__populate_internal_map
                 1      probe_perf:bpf_object__create_maps
                 0      probe_perf:bpf_object__relocate_core
                 0      probe_perf:bpf_program__reloc_text
                 9      probe_perf:bpf_program__relocate
                 1      probe_perf:bpf_object__relocate
                 1      probe_perf:bpf_object__collect_reloc
                 9      probe_perf:bpf_program__load
               154      probe_perf:bpf_program__is_function_storage
                 1      probe_perf:bpf_object__load_progs
                 0      probe_perf:bpf_object__open_xattr
                 0      probe_perf:bpf_object__open
                 0      probe_perf:bpf_object__open_file
                 1      probe_perf:bpf_object__open_mem
                 1      probe_perf:bpf_object__open_buffer
                 1      probe_perf:bpf_object__unload
                 1      probe_perf:bpf_object__load_xattr
                 1      probe_perf:bpf_object__load
                 0      probe_perf:bpf_program__pin_instance
                 0      probe_perf:bpf_program__unpin_instance
                 0      probe_perf:bpf_program__pin
                 0      probe_perf:bpf_program__unpin
                 0      probe_perf:bpf_object__pin_maps
                 0      probe_perf:bpf_object__unpin_maps
                 0      probe_perf:bpf_object__pin_programs
                 0      probe_perf:bpf_object__unpin_programs
                 0      probe_perf:bpf_object__pin
                 1      probe_perf:bpf_object__close
                17      probe_perf:bpf_object__next
                 0      probe_perf:bpf_object__name
                 0      probe_perf:bpf_object__kversion
                 0      probe_perf:bpf_object__btf
                 0      probe_perf:bpf_object__btf_fd
                 0      probe_perf:bpf_object__set_priv
                 0      probe_perf:bpf_object__priv
               159      probe_perf:bpf_program__next
                 0      probe_perf:bpf_program__prev
                 9      probe_perf:bpf_program__set_priv
                27      probe_perf:bpf_program__priv
                 0      probe_perf:bpf_program__set_ifindex
                 9      probe_perf:bpf_program__title
                15      probe_perf:bpf_program__fd
                 0      probe_perf:bpf_program__size
                 0      probe_perf:bpf_program__set_prep
                15      probe_perf:bpf_program__nth_fd
                 0      probe_perf:bpf_program__get_type
                 9      probe_perf:bpf_program__set_type
                 0      probe_perf:bpf_program__is_type
                 0      probe_perf:bpf_program__set_socket_filter
                 0      probe_perf:bpf_program__is_socket_filter
                 0      probe_perf:bpf_program__set_kprobe
                 0      probe_perf:bpf_program__is_kprobe
                 0      probe_perf:bpf_program__set_sched_cls
                 0      probe_perf:bpf_program__is_sched_cls
                 0      probe_perf:bpf_program__set_sched_act
                 0      probe_perf:bpf_program__is_sched_act
                 9      probe_perf:bpf_program__set_tracepoint
                 0      probe_perf:bpf_program__is_tracepoint
                 0      probe_perf:bpf_program__set_raw_tracepoint
                 0      probe_perf:bpf_program__is_raw_tracepoint
                 0      probe_perf:bpf_program__set_xdp
                 0      probe_perf:bpf_program__is_xdp
                 0      probe_perf:bpf_program__set_perf_event
                 0      probe_perf:bpf_program__is_perf_event
                 0      probe_perf:bpf_program__set_tracing
                 0      probe_perf:bpf_program__is_tracing
                 0      probe_perf:bpf_program__get_expected_attach_type
                 0      probe_perf:bpf_program__set_expected_attach_type
                 4      probe_perf:bpf_object__find_map_by_name
                 0      probe_perf:bpf_object__find_map_fd_by_name
                 0      probe_perf:bpf_object__find_map_by_offset
                 0      probe_perf:bpf_program__attach_perf_event
                 0      probe_perf:bpf_program__attach_kprobe
                 0      probe_perf:bpf_program__attach_uprobe
                 0      probe_perf:bpf_program__attach_tracepoint
                 0      probe_perf:bpf_program__attach_raw_tracepoint
                 0      probe_perf:bpf_program__attach_trace
                 0      probe_perf:bpf_program__get_prog_info_linear
                 0      probe_perf:bpf_program__bpil_addr_to_offs
                 0      probe_perf:bpf_program__bpil_offs_to_addr
                 0      probe_perf:bpf_output__printer
                 0      probe_perf:bpf_output__fprintf

       2.375277776 seconds time elapsed

       0.579446000 seconds user
       0.733851000 seconds sys


[root@quaco ~]#


Thanks,

- Arnaldo


>  kernel/trace/trace_kprobe.c |    2 -
>  kernel/trace/trace_probe.c  |    5 +-
>  kernel/trace/trace_probe.h  |    3 +
>  kernel/trace/trace_uprobe.c |  124 ++++++++++++++++++++++++++++---------------
>  4 files changed, 86 insertions(+), 48 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 7f890262c8a3..3e5f9c7d939c 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -290,7 +290,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
>  	INIT_HLIST_NODE(&tk->rp.kp.hlist);
>  	INIT_LIST_HEAD(&tk->rp.kp.list);
>  
> -	ret = trace_probe_init(&tk->tp, event, group);
> +	ret = trace_probe_init(&tk->tp, event, group, 0);
>  	if (ret < 0)
>  		goto error;
>  
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 905b10af5d5c..bba18cf44a30 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -984,7 +984,7 @@ void trace_probe_cleanup(struct trace_probe *tp)
>  }
>  
>  int trace_probe_init(struct trace_probe *tp, const char *event,
> -		     const char *group)
> +		     const char *group, size_t event_data_size)
>  {
>  	struct trace_event_call *call;
>  	int ret = 0;
> @@ -992,7 +992,8 @@ int trace_probe_init(struct trace_probe *tp, const char *event,
>  	if (!event || !group)
>  		return -EINVAL;
>  
> -	tp->event = kzalloc(sizeof(struct trace_probe_event), GFP_KERNEL);
> +	tp->event = kzalloc(sizeof(struct trace_probe_event) + event_data_size,
> +			    GFP_KERNEL);
>  	if (!tp->event)
>  		return -ENOMEM;
>  
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 4ee703728aec..03e4e180058d 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -230,6 +230,7 @@ struct trace_probe_event {
>  	struct trace_event_call		call;
>  	struct list_head 		files;
>  	struct list_head		probes;
> +	char				data[0];
>  };
>  
>  struct trace_probe {
> @@ -322,7 +323,7 @@ static inline bool trace_probe_has_single_file(struct trace_probe *tp)
>  }
>  
>  int trace_probe_init(struct trace_probe *tp, const char *event,
> -		     const char *group);
> +		     const char *group, size_t event_data_size);
>  void trace_probe_cleanup(struct trace_probe *tp);
>  int trace_probe_append(struct trace_probe *tp, struct trace_probe *to);
>  void trace_probe_unlink(struct trace_probe *tp);
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 352073d36585..f66e202fec13 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -60,7 +60,6 @@ static struct dyn_event_operations trace_uprobe_ops = {
>   */
>  struct trace_uprobe {
>  	struct dyn_event		devent;
> -	struct trace_uprobe_filter	filter;
>  	struct uprobe_consumer		consumer;
>  	struct path			path;
>  	struct inode			*inode;
> @@ -264,6 +263,14 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
>  }
>  NOKPROBE_SYMBOL(process_fetch_insn)
>  
> +static struct trace_uprobe_filter *
> +trace_uprobe_get_filter(struct trace_uprobe *tu)
> +{
> +	struct trace_probe_event *event = tu->tp.event;
> +
> +	return (struct trace_uprobe_filter *)&event->data[0];
> +}
> +
>  static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
>  {
>  	rwlock_init(&filter->rwlock);
> @@ -351,7 +358,8 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
>  	if (!tu)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = trace_probe_init(&tu->tp, event, group);
> +	ret = trace_probe_init(&tu->tp, event, group,
> +				sizeof(struct trace_uprobe_filter));
>  	if (ret < 0)
>  		goto error;
>  
> @@ -359,7 +367,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
>  	tu->consumer.handler = uprobe_dispatcher;
>  	if (is_ret)
>  		tu->consumer.ret_handler = uretprobe_dispatcher;
> -	init_trace_uprobe_filter(&tu->filter);
> +	init_trace_uprobe_filter(trace_uprobe_get_filter(tu));
>  	return tu;
>  
>  error:
> @@ -1067,13 +1075,14 @@ static void __probe_event_disable(struct trace_probe *tp)
>  	struct trace_probe *pos;
>  	struct trace_uprobe *tu;
>  
> +	tu = container_of(tp, struct trace_uprobe, tp);
> +	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
> +
>  	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
>  		tu = container_of(pos, struct trace_uprobe, tp);
>  		if (!tu->inode)
>  			continue;
>  
> -		WARN_ON(!uprobe_filter_is_empty(&tu->filter));
> -
>  		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
>  		tu->inode = NULL;
>  	}
> @@ -1108,7 +1117,7 @@ static int probe_event_enable(struct trace_event_call *call,
>  	}
>  
>  	tu = container_of(tp, struct trace_uprobe, tp);
> -	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
> +	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
>  
>  	if (enabled)
>  		return 0;
> @@ -1205,39 +1214,39 @@ __uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
>  }
>  
>  static inline bool
> -uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
> +trace_uprobe_filter_event(struct trace_uprobe_filter *filter,
> +			  struct perf_event *event)
>  {
> -	return __uprobe_perf_filter(&tu->filter, event->hw.target->mm);
> +	return __uprobe_perf_filter(filter, event->hw.target->mm);
>  }
>  
> -static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
> +static bool trace_uprobe_filter_remove(struct trace_uprobe_filter *filter,
> +				       struct perf_event *event)
>  {
>  	bool done;
>  
> -	write_lock(&tu->filter.rwlock);
> +	write_lock(&filter->rwlock);
>  	if (event->hw.target) {
>  		list_del(&event->hw.tp_list);
> -		done = tu->filter.nr_systemwide ||
> +		done = filter->nr_systemwide ||
>  			(event->hw.target->flags & PF_EXITING) ||
> -			uprobe_filter_event(tu, event);
> +			trace_uprobe_filter_event(filter, event);
>  	} else {
> -		tu->filter.nr_systemwide--;
> -		done = tu->filter.nr_systemwide;
> +		filter->nr_systemwide--;
> +		done = filter->nr_systemwide;
>  	}
> -	write_unlock(&tu->filter.rwlock);
> -
> -	if (!done)
> -		return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> +	write_unlock(&filter->rwlock);
>  
> -	return 0;
> +	return done;
>  }
>  
> -static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
> +/* This returns true if the filter always covers target mm */
> +static bool trace_uprobe_filter_add(struct trace_uprobe_filter *filter,
> +				    struct perf_event *event)
>  {
>  	bool done;
> -	int err;
>  
> -	write_lock(&tu->filter.rwlock);
> +	write_lock(&filter->rwlock);
>  	if (event->hw.target) {
>  		/*
>  		 * event->parent != NULL means copy_process(), we can avoid
> @@ -1247,28 +1256,21 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
>  		 * attr.enable_on_exec means that exec/mmap will install the
>  		 * breakpoints we need.
>  		 */
> -		done = tu->filter.nr_systemwide ||
> +		done = filter->nr_systemwide ||
>  			event->parent || event->attr.enable_on_exec ||
> -			uprobe_filter_event(tu, event);
> -		list_add(&event->hw.tp_list, &tu->filter.perf_events);
> +			trace_uprobe_filter_event(filter, event);
> +		list_add(&event->hw.tp_list, &filter->perf_events);
>  	} else {
> -		done = tu->filter.nr_systemwide;
> -		tu->filter.nr_systemwide++;
> +		done = filter->nr_systemwide;
> +		filter->nr_systemwide++;
>  	}
> -	write_unlock(&tu->filter.rwlock);
> +	write_unlock(&filter->rwlock);
>  
> -	err = 0;
> -	if (!done) {
> -		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> -		if (err)
> -			uprobe_perf_close(tu, event);
> -	}
> -	return err;
> +	return done;
>  }
>  
> -static int uprobe_perf_multi_call(struct trace_event_call *call,
> -				  struct perf_event *event,
> -		int (*op)(struct trace_uprobe *tu, struct perf_event *event))
> +static int uprobe_perf_close(struct trace_event_call *call,
> +			     struct perf_event *event)
>  {
>  	struct trace_probe *pos, *tp;
>  	struct trace_uprobe *tu;
> @@ -1278,25 +1280,59 @@ static int uprobe_perf_multi_call(struct trace_event_call *call,
>  	if (WARN_ON_ONCE(!tp))
>  		return -ENODEV;
>  
> +	tu = container_of(tp, struct trace_uprobe, tp);
> +	if (trace_uprobe_filter_remove(trace_uprobe_get_filter(tu), event))
> +		return 0;
> +
>  	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
>  		tu = container_of(pos, struct trace_uprobe, tp);
> -		ret = op(tu, event);
> +		ret = uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
>  		if (ret)
>  			break;
>  	}
>  
>  	return ret;
>  }
> +
> +static int uprobe_perf_open(struct trace_event_call *call,
> +			    struct perf_event *event)
> +{
> +	struct trace_probe *pos, *tp;
> +	struct trace_uprobe *tu;
> +	int err = 0;
> +
> +	tp = trace_probe_primary_from_call(call);
> +	if (WARN_ON_ONCE(!tp))
> +		return -ENODEV;
> +
> +	tu = container_of(tp, struct trace_uprobe, tp);
> +	if (trace_uprobe_filter_add(trace_uprobe_get_filter(tu), event))
> +		return 0;
> +
> +	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> +		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> +		if (err) {
> +			uprobe_perf_close(call, event);
> +			break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
>  static bool uprobe_perf_filter(struct uprobe_consumer *uc,
>  				enum uprobe_filter_ctx ctx, struct mm_struct *mm)
>  {
> +	struct trace_uprobe_filter *filter;
>  	struct trace_uprobe *tu;
>  	int ret;
>  
>  	tu = container_of(uc, struct trace_uprobe, consumer);
> -	read_lock(&tu->filter.rwlock);
> -	ret = __uprobe_perf_filter(&tu->filter, mm);
> -	read_unlock(&tu->filter.rwlock);
> +	filter = trace_uprobe_get_filter(tu);
> +
> +	read_lock(&filter->rwlock);
> +	ret = __uprobe_perf_filter(filter, mm);
> +	read_unlock(&filter->rwlock);
>  
>  	return ret;
>  }
> @@ -1419,10 +1455,10 @@ trace_uprobe_register(struct trace_event_call *event, enum trace_reg type,
>  		return 0;
>  
>  	case TRACE_REG_PERF_OPEN:
> -		return uprobe_perf_multi_call(event, data, uprobe_perf_open);
> +		return uprobe_perf_open(event, data);
>  
>  	case TRACE_REG_PERF_CLOSE:
> -		return uprobe_perf_multi_call(event, data, uprobe_perf_close);
> +		return uprobe_perf_close(event, data);
>  
>  #endif
>  	default:
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-14 17:35 ` Arnaldo Carvalho de Melo
@ 2020-01-14 17:44   ` Steven Rostedt
  2020-01-14 17:45     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2020-01-14 17:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Masami Hiramatsu, Jiri Olsa, Peter Zijlstra, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Tue, 14 Jan 2020 14:35:35 -0300
Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:

> If you don't mind I can put it into my next perf/urgent pull req to
> Ingo/Thomas,

I was going to pull this into my urgent tree when my tests finish with
my for-next code.

I have one or two other patches I need to apply to urgent as well, so
it's not an issue for me to take this.

-- Steve



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-14 17:44   ` Steven Rostedt
@ 2020-01-14 17:45     ` Arnaldo Carvalho de Melo
  2020-01-14 18:32       ` Steven Rostedt
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-01-14 17:45 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Arnaldo Carvalho de Melo, Masami Hiramatsu, Jiri Olsa,
	Peter Zijlstra, Ingo Molnar, Naveen N . Rao,
	Anil S Keshavamurthy, David S . Miller, Namhyung Kim,
	Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

Em Tue, Jan 14, 2020 at 12:44:04PM -0500, Steven Rostedt escreveu:
> On Tue, 14 Jan 2020 14:35:35 -0300
> Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> 
> > If you don't mind I can put it into my next perf/urgent pull req to
> > Ingo/Thomas,
> 
> I was going to pull this into my urgent tree when my tests finish with
> my for-next code.
> 
> I have one or two other patches I need to apply to urgent as well, so
> it's not an issue for me to take this.

Go ahead, I still need some more time for my tests with the other
patches, please just collect my Tested-by,

Thanks,

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-14 17:45     ` Arnaldo Carvalho de Melo
@ 2020-01-14 18:32       ` Steven Rostedt
  2020-01-15  1:44         ` Masami Hiramatsu
  0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2020-01-14 18:32 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Masami Hiramatsu, Jiri Olsa, Peter Zijlstra, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Tue, 14 Jan 2020 14:45:03 -0300
Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:

> Em Tue, Jan 14, 2020 at 12:44:04PM -0500, Steven Rostedt escreveu:
> > On Tue, 14 Jan 2020 14:35:35 -0300
> > Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> >   
> > > If you don't mind I can put it into my next perf/urgent pull req to
> > > Ingo/Thomas,  
> > 
> > I was going to pull this into my urgent tree when my tests finish with
> > my for-next code.
> > 
> > I have one or two other patches I need to apply to urgent as well, so
> > it's not an issue for me to take this.  
> 
> Go ahead, I still need some more time for my tests with the other
> patches, please just collect my Tested-by,
>

Will do!

This also needs to run through my test suite which will take 13
hours more (after my previous one finishes, which took more than 13
because of all the patches I was testing).

-- Steve

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-14 18:32       ` Steven Rostedt
@ 2020-01-15  1:44         ` Masami Hiramatsu
  0 siblings, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-15  1:44 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Arnaldo Carvalho de Melo, Masami Hiramatsu, Jiri Olsa,
	Peter Zijlstra, Ingo Molnar, Naveen N . Rao,
	Anil S Keshavamurthy, David S . Miller, Namhyung Kim,
	Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Tue, 14 Jan 2020 13:32:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 14 Jan 2020 14:45:03 -0300
> Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> 
> > Em Tue, Jan 14, 2020 at 12:44:04PM -0500, Steven Rostedt escreveu:
> > > On Tue, 14 Jan 2020 14:35:35 -0300
> > > Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> > >   
> > > > If you don't mind I can put it into my next perf/urgent pull req to
> > > > Ingo/Thomas,  
> > > 
> > > I was going to pull this into my urgent tree when my tests finish with
> > > my for-next code.
> > > 
> > > I have one or two other patches I need to apply to urgent as well, so
> > > it's not an issue for me to take this.  
> > 
> > Go ahead, I still need some more time for my tests with the other
> > patches, please just collect my Tested-by,
> >
> 
> Will do!
> 
> This also needs to run through my test suite which will take 13
> hours more (after my previous one finishes, which took more than 13
> because of all the patches I was testing).

Thanks Steve and Arnaldo!

This may not take a long time to be tested but requires perf to
enable multiple perf event.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-10  1:45 [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Masami Hiramatsu
  2020-01-10  1:47 ` Masami Hiramatsu
  2020-01-14 17:35 ` Arnaldo Carvalho de Melo
@ 2020-01-20 12:40 ` Peter Zijlstra
  2020-01-20 15:32   ` Masami Hiramatsu
  2020-01-21  7:46   ` [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe Masami Hiramatsu
  2 siblings, 2 replies; 13+ messages in thread
From: Peter Zijlstra @ 2020-01-20 12:40 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, Steven Rostedt, Jiri Olsa, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Fri, Jan 10, 2020 at 10:45:39AM +0900, Masami Hiramatsu wrote:

> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 4ee703728aec..03e4e180058d 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -230,6 +230,7 @@ struct trace_probe_event {
>  	struct trace_event_call		call;
>  	struct list_head 		files;
>  	struct list_head		probes;
> +	char				data[0];
>  };

Would it make sense to make the above:

	struct trace_uprobe_filter	filter[0];

instead? That would ensure that alignment is respected. While I think
the current code works by accident.

> @@ -264,6 +263,14 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
>  }
>  NOKPROBE_SYMBOL(process_fetch_insn)
>  
> +static struct trace_uprobe_filter *
> +trace_uprobe_get_filter(struct trace_uprobe *tu)
> +{
> +	struct trace_probe_event *event = tu->tp.event;
> +
> +	return (struct trace_uprobe_filter *)&event->data[0];
> +}



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
  2020-01-20 12:40 ` Peter Zijlstra
@ 2020-01-20 15:32   ` Masami Hiramatsu
  2020-01-21  7:46   ` [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe Masami Hiramatsu
  1 sibling, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-20 15:32 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Arnaldo Carvalho de Melo, Steven Rostedt, Jiri Olsa, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Mon, 20 Jan 2020 13:40:22 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Jan 10, 2020 at 10:45:39AM +0900, Masami Hiramatsu wrote:
> 
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 4ee703728aec..03e4e180058d 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -230,6 +230,7 @@ struct trace_probe_event {
> >  	struct trace_event_call		call;
> >  	struct list_head 		files;
> >  	struct list_head		probes;
> > +	char				data[0];
> >  };
> 
> Would it make sense to make the above:
> 
> 	struct trace_uprobe_filter	filter[0];
> 
> instead? That would ensure that alignment is respected. While I think
> the current code works by accident.

Hmm, if we consider the alignment, shouldn't we allocate the structure
with the alignment gap? Currently it just added the 
sizeof(struct trace_uprobe_filter) when kzalloc().

In this case, I think we should introduce a new data structure,
trace_uprobe_event.

Thank you,

> 
> > @@ -264,6 +263,14 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
> >  }
> >  NOKPROBE_SYMBOL(process_fetch_insn)
> >  
> > +static struct trace_uprobe_filter *
> > +trace_uprobe_get_filter(struct trace_uprobe *tu)
> > +{
> > +	struct trace_probe_event *event = tu->tp.event;
> > +
> > +	return (struct trace_uprobe_filter *)&event->data[0];
> > +}
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe
  2020-01-20 12:40 ` Peter Zijlstra
  2020-01-20 15:32   ` Masami Hiramatsu
@ 2020-01-21  7:46   ` Masami Hiramatsu
  2020-01-21 15:50     ` Steven Rostedt
  1 sibling, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-21  7:46 UTC (permalink / raw)
  To: Peter Zijlstra, Steven Rostedt
  Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Naveen N . Rao,
	Anil S Keshavamurthy, David S . Miller, Namhyung Kim,
	Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

Commit 99c9a923e97a ("tracing/uprobe: Fix double perf_event
linking on multiprobe uprobe") moved trace_uprobe_filter on
trace_probe_event. However, since it introduced a flexible
data structure with char array and type casting, the
alignment of trace_uprobe_filter can be broken.

This changes the type of the array to trace_uprobe_filter
data strucure to fix it.

Fixes: 99c9a923e97a ("tracing/uprobe: Fix double perf_event linking on multiprobe uprobe")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |    2 +-
 kernel/trace/trace_probe.c  |    9 ++++++---
 kernel/trace/trace_probe.h  |   10 ++++++++--
 kernel/trace/trace_uprobe.c |   29 +++++++----------------------
 4 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 3e5f9c7d939c..3f54dc2f6e1c 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -290,7 +290,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
 	INIT_HLIST_NODE(&tk->rp.kp.hlist);
 	INIT_LIST_HEAD(&tk->rp.kp.list);
 
-	ret = trace_probe_init(&tk->tp, event, group, 0);
+	ret = trace_probe_init(&tk->tp, event, group, false);
 	if (ret < 0)
 		goto error;
 
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index bba18cf44a30..9ae87be422f2 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -984,16 +984,19 @@ void trace_probe_cleanup(struct trace_probe *tp)
 }
 
 int trace_probe_init(struct trace_probe *tp, const char *event,
-		     const char *group, size_t event_data_size)
+		     const char *group, bool alloc_filter)
 {
 	struct trace_event_call *call;
+	size_t size = sizeof(struct trace_probe_event);
 	int ret = 0;
 
 	if (!event || !group)
 		return -EINVAL;
 
-	tp->event = kzalloc(sizeof(struct trace_probe_event) + event_data_size,
-			    GFP_KERNEL);
+	if (alloc_filter)
+		size += sizeof(struct trace_uprobe_filter);
+
+	tp->event = kzalloc(size, GFP_KERNEL);
 	if (!tp->event)
 		return -ENOMEM;
 
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 03e4e180058d..a0ff9e200ef6 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -223,6 +223,12 @@ struct probe_arg {
 	const struct fetch_type	*type;	/* Type of this argument */
 };
 
+struct trace_uprobe_filter {
+	rwlock_t		rwlock;
+	int			nr_systemwide;
+	struct list_head	perf_events;
+};
+
 /* Event call and class holder */
 struct trace_probe_event {
 	unsigned int			flags;	/* For TP_FLAG_* */
@@ -230,7 +236,7 @@ struct trace_probe_event {
 	struct trace_event_call		call;
 	struct list_head 		files;
 	struct list_head		probes;
-	char				data[0];
+	struct trace_uprobe_filter	filter[0];
 };
 
 struct trace_probe {
@@ -323,7 +329,7 @@ static inline bool trace_probe_has_single_file(struct trace_probe *tp)
 }
 
 int trace_probe_init(struct trace_probe *tp, const char *event,
-		     const char *group, size_t event_data_size);
+		     const char *group, bool alloc_filter);
 void trace_probe_cleanup(struct trace_probe *tp);
 int trace_probe_append(struct trace_probe *tp, struct trace_probe *to);
 void trace_probe_unlink(struct trace_probe *tp);
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f66e202fec13..21fb512d6a66 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -34,12 +34,6 @@ struct uprobe_trace_entry_head {
 #define DATAOF_TRACE_ENTRY(entry, is_return)		\
 	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
 
-struct trace_uprobe_filter {
-	rwlock_t		rwlock;
-	int			nr_systemwide;
-	struct list_head	perf_events;
-};
-
 static int trace_uprobe_create(int argc, const char **argv);
 static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
 static int trace_uprobe_release(struct dyn_event *ev);
@@ -263,14 +257,6 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
 }
 NOKPROBE_SYMBOL(process_fetch_insn)
 
-static struct trace_uprobe_filter *
-trace_uprobe_get_filter(struct trace_uprobe *tu)
-{
-	struct trace_probe_event *event = tu->tp.event;
-
-	return (struct trace_uprobe_filter *)&event->data[0];
-}
-
 static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
 {
 	rwlock_init(&filter->rwlock);
@@ -358,8 +344,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	if (!tu)
 		return ERR_PTR(-ENOMEM);
 
-	ret = trace_probe_init(&tu->tp, event, group,
-				sizeof(struct trace_uprobe_filter));
+	ret = trace_probe_init(&tu->tp, event, group, true);
 	if (ret < 0)
 		goto error;
 
@@ -367,7 +352,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	tu->consumer.handler = uprobe_dispatcher;
 	if (is_ret)
 		tu->consumer.ret_handler = uretprobe_dispatcher;
-	init_trace_uprobe_filter(trace_uprobe_get_filter(tu));
+	init_trace_uprobe_filter(tu->tp.event.filter);
 	return tu;
 
 error:
@@ -1076,7 +1061,7 @@ static void __probe_event_disable(struct trace_probe *tp)
 	struct trace_uprobe *tu;
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
+	WARN_ON(!uprobe_filter_is_empty(tu->tp.event.filter));
 
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
 		tu = container_of(pos, struct trace_uprobe, tp);
@@ -1117,7 +1102,7 @@ static int probe_event_enable(struct trace_event_call *call,
 	}
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
+	WARN_ON(!uprobe_filter_is_empty(tu->tp.event.filter));
 
 	if (enabled)
 		return 0;
@@ -1281,7 +1266,7 @@ static int uprobe_perf_close(struct trace_event_call *call,
 		return -ENODEV;
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	if (trace_uprobe_filter_remove(trace_uprobe_get_filter(tu), event))
+	if (trace_uprobe_filter_remove(tu->tp.event.filter, event))
 		return 0;
 
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
@@ -1306,7 +1291,7 @@ static int uprobe_perf_open(struct trace_event_call *call,
 		return -ENODEV;
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	if (trace_uprobe_filter_add(trace_uprobe_get_filter(tu), event))
+	if (trace_uprobe_filter_add(tu->tp.event.filter, event))
 		return 0;
 
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
@@ -1328,7 +1313,7 @@ static bool uprobe_perf_filter(struct uprobe_consumer *uc,
 	int ret;
 
 	tu = container_of(uc, struct trace_uprobe, consumer);
-	filter = trace_uprobe_get_filter(tu);
+	filter = tu->tp.event.filter;
 
 	read_lock(&filter->rwlock);
 	ret = __uprobe_perf_filter(filter, mm);


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe
  2020-01-21  7:46   ` [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe Masami Hiramatsu
@ 2020-01-21 15:50     ` Steven Rostedt
  2020-01-22  3:08       ` Masami Hiramatsu
  0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2020-01-21 15:50 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 4456 bytes --]

On Tue, 21 Jan 2020 16:46:19 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Commit 99c9a923e97a ("tracing/uprobe: Fix double perf_event
> linking on multiprobe uprobe") moved trace_uprobe_filter on
> trace_probe_event. However, since it introduced a flexible
> data structure with char array and type casting, the
> alignment of trace_uprobe_filter can be broken.
> 
> This changes the type of the array to trace_uprobe_filter
> data strucure to fix it.
> 
> Fixes: 99c9a923e97a ("tracing/uprobe: Fix double perf_event linking on multiprobe uprobe")
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |    2 +-
>  kernel/trace/trace_probe.c  |    9 ++++++---
>  kernel/trace/trace_probe.h  |   10 ++++++++--
>  kernel/trace/trace_uprobe.c |   29 +++++++----------------------
>  4 files changed, 22 insertions(+), 28 deletions(-)


I hit the following build error with this:

/work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘alloc_trace_uprobe’:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c:355:39: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
  init_trace_uprobe_filter(tu->tp.event.filter);
                                       ^
                                       ->
In file included from /work/git/linux-trace.git/arch/x86/include/asm/bug.h:83,
                 from /work/git/linux-trace.git/include/linux/bug.h:5,
                 from /work/git/linux-trace.git/arch/x86/include/asm/paravirt.h:15,
                 from /work/git/linux-trace.git/arch/x86/include/asm/irqflags.h:72,
                 from /work/git/linux-trace.git/include/linux/irqflags.h:16,
                 from /work/git/linux-trace.git/include/linux/rcupdate.h:26,
                 from /work/git/linux-trace.git/include/linux/rbtree.h:22,
                 from /work/git/linux-trace.git/include/linux/key.h:15,
                 from /work/git/linux-trace.git/include/linux/security.h:26,
                 from /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:10:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘__probe_event_disable’:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1064:46: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
  WARN_ON(!uprobe_filter_is_empty(tu->tp.event.filter));
                                              ^
/work/git/linux-trace.git/include/asm-generic/bug.h:113:25: note: in definition of macro ‘WARN_ON’
  int __ret_warn_on = !!(condition);    \
                         ^~~~~~~~~
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘probe_event_enable’:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1105:46: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
  WARN_ON(!uprobe_filter_is_empty(tu->tp.event.filter));
                                              ^
/work/git/linux-trace.git/include/asm-generic/bug.h:113:25: note: in definition of macro ‘WARN_ON’
  int __ret_warn_on = !!(condition);    \
                         ^~~~~~~~~
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘uprobe_perf_close’:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1269:45: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
  if (trace_uprobe_filter_remove(tu->tp.event.filter, event))
                                             ^
                                             ->
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘uprobe_perf_open’:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1294:42: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
  if (trace_uprobe_filter_add(tu->tp.event.filter, event))
                                          ^
                                          ->
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘uprobe_perf_filter’:
/work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1316:23: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
  filter = tu->tp.event.filter;
                       ^
                       ->
make[3]: *** [/work/git/linux-trace.git/scripts/Makefile.build:266: kernel/trace/trace_uprobe.o] Error 1
make[3]: *** Waiting for unfinished jobs....


Config attached.

-- Steve

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33816 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe
  2020-01-21 15:50     ` Steven Rostedt
@ 2020-01-22  3:08       ` Masami Hiramatsu
  2020-01-22  3:23         ` [PATCH v2] " Masami Hiramatsu
  0 siblings, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-22  3:08 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar,
	Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
	Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

On Tue, 21 Jan 2020 10:50:55 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 21 Jan 2020 16:46:19 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Commit 99c9a923e97a ("tracing/uprobe: Fix double perf_event
> > linking on multiprobe uprobe") moved trace_uprobe_filter on
> > trace_probe_event. However, since it introduced a flexible
> > data structure with char array and type casting, the
> > alignment of trace_uprobe_filter can be broken.
> > 
> > This changes the type of the array to trace_uprobe_filter
> > data strucure to fix it.
> > 
> > Fixes: 99c9a923e97a ("tracing/uprobe: Fix double perf_event linking on multiprobe uprobe")
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_kprobe.c |    2 +-
> >  kernel/trace/trace_probe.c  |    9 ++++++---
> >  kernel/trace/trace_probe.h  |   10 ++++++++--
> >  kernel/trace/trace_uprobe.c |   29 +++++++----------------------
> >  4 files changed, 22 insertions(+), 28 deletions(-)
> 
> 
> I hit the following build error with this:

Hmm, I missed something... let me check and update it.

Thank you,

> 
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘alloc_trace_uprobe’:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:355:39: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
>   init_trace_uprobe_filter(tu->tp.event.filter);
>                                        ^
>                                        ->
> In file included from /work/git/linux-trace.git/arch/x86/include/asm/bug.h:83,
>                  from /work/git/linux-trace.git/include/linux/bug.h:5,
>                  from /work/git/linux-trace.git/arch/x86/include/asm/paravirt.h:15,
>                  from /work/git/linux-trace.git/arch/x86/include/asm/irqflags.h:72,
>                  from /work/git/linux-trace.git/include/linux/irqflags.h:16,
>                  from /work/git/linux-trace.git/include/linux/rcupdate.h:26,
>                  from /work/git/linux-trace.git/include/linux/rbtree.h:22,
>                  from /work/git/linux-trace.git/include/linux/key.h:15,
>                  from /work/git/linux-trace.git/include/linux/security.h:26,
>                  from /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:10:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘__probe_event_disable’:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1064:46: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
>   WARN_ON(!uprobe_filter_is_empty(tu->tp.event.filter));
>                                               ^
> /work/git/linux-trace.git/include/asm-generic/bug.h:113:25: note: in definition of macro ‘WARN_ON’
>   int __ret_warn_on = !!(condition);    \
>                          ^~~~~~~~~
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘probe_event_enable’:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1105:46: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
>   WARN_ON(!uprobe_filter_is_empty(tu->tp.event.filter));
>                                               ^
> /work/git/linux-trace.git/include/asm-generic/bug.h:113:25: note: in definition of macro ‘WARN_ON’
>   int __ret_warn_on = !!(condition);    \
>                          ^~~~~~~~~
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘uprobe_perf_close’:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1269:45: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
>   if (trace_uprobe_filter_remove(tu->tp.event.filter, event))
>                                              ^
>                                              ->
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘uprobe_perf_open’:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1294:42: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
>   if (trace_uprobe_filter_add(tu->tp.event.filter, event))
>                                           ^
>                                           ->
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c: In function ‘uprobe_perf_filter’:
> /work/git/linux-trace.git/kernel/trace/trace_uprobe.c:1316:23: error: ‘tu->tp.event’ is a pointer; did you mean to use ‘->’?
>   filter = tu->tp.event.filter;
>                        ^
>                        ->
> make[3]: *** [/work/git/linux-trace.git/scripts/Makefile.build:266: kernel/trace/trace_uprobe.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> 
> 
> Config attached.
> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe
  2020-01-22  3:08       ` Masami Hiramatsu
@ 2020-01-22  3:23         ` Masami Hiramatsu
  0 siblings, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2020-01-22  3:23 UTC (permalink / raw)
  To: Peter Zijlstra, Steven Rostedt
  Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Naveen N . Rao,
	Anil S Keshavamurthy, David S . Miller, Namhyung Kim,
	Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
	Jesper Dangaard Brouer, Linux Kernel Mailing List

Commit 99c9a923e97a ("tracing/uprobe: Fix double perf_event
linking on multiprobe uprobe") moved trace_uprobe_filter on
trace_probe_event. However, since it introduced a flexible
data structure with char array and type casting, the
alignment of trace_uprobe_filter can be broken.

This changes the type of the array to trace_uprobe_filter
data strucure to fix it.

Fixes: 99c9a923e97a ("tracing/uprobe: Fix double perf_event linking on multiprobe uprobe")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
  - Fix build errors against pointer-field mistaking.
---
 kernel/trace/trace_kprobe.c |    2 +-
 kernel/trace/trace_probe.c  |    9 ++++++---
 kernel/trace/trace_probe.h  |   10 ++++++++--
 kernel/trace/trace_uprobe.c |   29 +++++++----------------------
 4 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 3e5f9c7d939c..3f54dc2f6e1c 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -290,7 +290,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
 	INIT_HLIST_NODE(&tk->rp.kp.hlist);
 	INIT_LIST_HEAD(&tk->rp.kp.list);
 
-	ret = trace_probe_init(&tk->tp, event, group, 0);
+	ret = trace_probe_init(&tk->tp, event, group, false);
 	if (ret < 0)
 		goto error;
 
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index bba18cf44a30..9ae87be422f2 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -984,16 +984,19 @@ void trace_probe_cleanup(struct trace_probe *tp)
 }
 
 int trace_probe_init(struct trace_probe *tp, const char *event,
-		     const char *group, size_t event_data_size)
+		     const char *group, bool alloc_filter)
 {
 	struct trace_event_call *call;
+	size_t size = sizeof(struct trace_probe_event);
 	int ret = 0;
 
 	if (!event || !group)
 		return -EINVAL;
 
-	tp->event = kzalloc(sizeof(struct trace_probe_event) + event_data_size,
-			    GFP_KERNEL);
+	if (alloc_filter)
+		size += sizeof(struct trace_uprobe_filter);
+
+	tp->event = kzalloc(size, GFP_KERNEL);
 	if (!tp->event)
 		return -ENOMEM;
 
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 03e4e180058d..a0ff9e200ef6 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -223,6 +223,12 @@ struct probe_arg {
 	const struct fetch_type	*type;	/* Type of this argument */
 };
 
+struct trace_uprobe_filter {
+	rwlock_t		rwlock;
+	int			nr_systemwide;
+	struct list_head	perf_events;
+};
+
 /* Event call and class holder */
 struct trace_probe_event {
 	unsigned int			flags;	/* For TP_FLAG_* */
@@ -230,7 +236,7 @@ struct trace_probe_event {
 	struct trace_event_call		call;
 	struct list_head 		files;
 	struct list_head		probes;
-	char				data[0];
+	struct trace_uprobe_filter	filter[0];
 };
 
 struct trace_probe {
@@ -323,7 +329,7 @@ static inline bool trace_probe_has_single_file(struct trace_probe *tp)
 }
 
 int trace_probe_init(struct trace_probe *tp, const char *event,
-		     const char *group, size_t event_data_size);
+		     const char *group, bool alloc_filter);
 void trace_probe_cleanup(struct trace_probe *tp);
 int trace_probe_append(struct trace_probe *tp, struct trace_probe *to);
 void trace_probe_unlink(struct trace_probe *tp);
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f66e202fec13..2619bc5ed520 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -34,12 +34,6 @@ struct uprobe_trace_entry_head {
 #define DATAOF_TRACE_ENTRY(entry, is_return)		\
 	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
 
-struct trace_uprobe_filter {
-	rwlock_t		rwlock;
-	int			nr_systemwide;
-	struct list_head	perf_events;
-};
-
 static int trace_uprobe_create(int argc, const char **argv);
 static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
 static int trace_uprobe_release(struct dyn_event *ev);
@@ -263,14 +257,6 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
 }
 NOKPROBE_SYMBOL(process_fetch_insn)
 
-static struct trace_uprobe_filter *
-trace_uprobe_get_filter(struct trace_uprobe *tu)
-{
-	struct trace_probe_event *event = tu->tp.event;
-
-	return (struct trace_uprobe_filter *)&event->data[0];
-}
-
 static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
 {
 	rwlock_init(&filter->rwlock);
@@ -358,8 +344,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	if (!tu)
 		return ERR_PTR(-ENOMEM);
 
-	ret = trace_probe_init(&tu->tp, event, group,
-				sizeof(struct trace_uprobe_filter));
+	ret = trace_probe_init(&tu->tp, event, group, true);
 	if (ret < 0)
 		goto error;
 
@@ -367,7 +352,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	tu->consumer.handler = uprobe_dispatcher;
 	if (is_ret)
 		tu->consumer.ret_handler = uretprobe_dispatcher;
-	init_trace_uprobe_filter(trace_uprobe_get_filter(tu));
+	init_trace_uprobe_filter(tu->tp.event->filter);
 	return tu;
 
 error:
@@ -1076,7 +1061,7 @@ static void __probe_event_disable(struct trace_probe *tp)
 	struct trace_uprobe *tu;
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
+	WARN_ON(!uprobe_filter_is_empty(tu->tp.event->filter));
 
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
 		tu = container_of(pos, struct trace_uprobe, tp);
@@ -1117,7 +1102,7 @@ static int probe_event_enable(struct trace_event_call *call,
 	}
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	WARN_ON(!uprobe_filter_is_empty(trace_uprobe_get_filter(tu)));
+	WARN_ON(!uprobe_filter_is_empty(tu->tp.event->filter));
 
 	if (enabled)
 		return 0;
@@ -1281,7 +1266,7 @@ static int uprobe_perf_close(struct trace_event_call *call,
 		return -ENODEV;
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	if (trace_uprobe_filter_remove(trace_uprobe_get_filter(tu), event))
+	if (trace_uprobe_filter_remove(tu->tp.event->filter, event))
 		return 0;
 
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
@@ -1306,7 +1291,7 @@ static int uprobe_perf_open(struct trace_event_call *call,
 		return -ENODEV;
 
 	tu = container_of(tp, struct trace_uprobe, tp);
-	if (trace_uprobe_filter_add(trace_uprobe_get_filter(tu), event))
+	if (trace_uprobe_filter_add(tu->tp.event->filter, event))
 		return 0;
 
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
@@ -1328,7 +1313,7 @@ static bool uprobe_perf_filter(struct uprobe_consumer *uc,
 	int ret;
 
 	tu = container_of(uc, struct trace_uprobe, consumer);
-	filter = trace_uprobe_get_filter(tu);
+	filter = tu->tp.event->filter;
 
 	read_lock(&filter->rwlock);
 	ret = __uprobe_perf_filter(filter, mm);


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-01-22  3:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10  1:45 [PATCH] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Masami Hiramatsu
2020-01-10  1:47 ` Masami Hiramatsu
2020-01-14 17:35 ` Arnaldo Carvalho de Melo
2020-01-14 17:44   ` Steven Rostedt
2020-01-14 17:45     ` Arnaldo Carvalho de Melo
2020-01-14 18:32       ` Steven Rostedt
2020-01-15  1:44         ` Masami Hiramatsu
2020-01-20 12:40 ` Peter Zijlstra
2020-01-20 15:32   ` Masami Hiramatsu
2020-01-21  7:46   ` [PATCH] tracing/uprobe: Fix to make trace_uprobe_filter alignment safe Masami Hiramatsu
2020-01-21 15:50     ` Steven Rostedt
2020-01-22  3:08       ` Masami Hiramatsu
2020-01-22  3:23         ` [PATCH v2] " Masami Hiramatsu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).