* [for-linus][PATCH 1/5] tracing: xen: Ordered comparison of function pointers
2020-01-21 14:38 [for-linus][PATCH 0/5] tracing: Fixes for 5.5 Steven Rostedt
@ 2020-01-21 14:38 ` Steven Rostedt
2020-01-21 14:38 ` [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Steven Rostedt
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2020-01-21 14:38 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Changbin Du
From: Changbin Du <changbin.du@gmail.com>
Just as commit 0566e40ce7 ("tracing: initcall: Ordered comparison of
function pointers"), this patch fixes another remaining one in xen.h
found by clang-9.
In file included from arch/x86/xen/trace.c:21:
In file included from ./include/trace/events/xen.h:475:
In file included from ./include/trace/define_trace.h:102:
In file included from ./include/trace/trace_events.h:473:
./include/trace/events/xen.h:69:7: warning: ordered comparison of function \
pointers ('xen_mc_callback_fn_t' (aka 'void (*)(void *)') and 'xen_mc_callback_fn_t') [-Wordered-compare-function-pointers]
__field(xen_mc_callback_fn_t, fn)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/trace/trace_events.h:421:29: note: expanded from macro '__field'
^
./include/trace/trace_events.h:407:6: note: expanded from macro '__field_ext'
is_signed_type(type), filter_type); \
^
./include/linux/trace_events.h:554:44: note: expanded from macro 'is_signed_type'
^
Fixes: c796f213a6934 ("xen/trace: add multicall tracing")
Signed-off-by: Changbin Du <changbin.du@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/trace/events/xen.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/trace/events/xen.h b/include/trace/events/xen.h
index 9a0e8af21310..a5ccfa67bc5c 100644
--- a/include/trace/events/xen.h
+++ b/include/trace/events/xen.h
@@ -66,7 +66,11 @@ TRACE_EVENT(xen_mc_callback,
TP_PROTO(xen_mc_callback_fn_t fn, void *data),
TP_ARGS(fn, data),
TP_STRUCT__entry(
- __field(xen_mc_callback_fn_t, fn)
+ /*
+ * Use field_struct to avoid is_signed_type()
+ * comparison of a function pointer.
+ */
+ __field_struct(xen_mc_callback_fn_t, fn)
__field(void *, data)
),
TP_fast_assign(
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
2020-01-21 14:38 [for-linus][PATCH 0/5] tracing: Fixes for 5.5 Steven Rostedt
2020-01-21 14:38 ` [for-linus][PATCH 1/5] tracing: xen: Ordered comparison of function pointers Steven Rostedt
@ 2020-01-21 14:38 ` Steven Rostedt
2020-01-21 14:50 ` Peter Zijlstra
2020-01-21 14:38 ` [for-linus][PATCH 3/5] tracing: trigger: Replace unneeded RCU-list traversals Steven Rostedt
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2020-01-21 14:38 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Jiri Olsa, Peter Zijlstra,
Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
Jesper Dangaard Brouer, stable, Arnaldo Carvalho de Melo,
Arnaldo Carvalho de Melo, Masami Hiramatsu
From: Masami Hiramatsu <mhiramat@kernel.org>
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.
Link: http://lkml.kernel.org/r/157862073931.1800.3800576241181489174.stgit@devnote2
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Naveen N . Rao" <naveen.n.rao@linux.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: "David S . Miller" <davem@davemloft.net>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: =?utf-8?q?Toke_H=C3=B8iland-J?= =?utf-8?b?w7hyZ2Vuc2Vu?= <thoiland@redhat.com>
Cc: Jean-Tsung Hsiao <jhsiao@redhat.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: stable@vger.kernel.org
Fixes: 60d53e2c3b75 ("tracing/probe: Split trace_event related data from trace_probe")
Link: https://lkml.kernel.org/r/20200108171611.GA8472@kernel.org
Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.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:
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
2020-01-21 14:38 ` [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Steven Rostedt
@ 2020-01-21 14:50 ` Peter Zijlstra
2020-01-21 14:59 ` David Laight
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-01-21 14:50 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Jiri Olsa,
Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
Jesper Dangaard Brouer, stable, Arnaldo Carvalho de Melo,
Arnaldo Carvalho de Melo, Masami Hiramatsu
On Tue, Jan 21, 2020 at 09:38:49AM -0500, Steven Rostedt 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];
> };
Note that this relies on pure 'luck'. If you stick anything <4 bytes in
between the list_head and the data member it'll come unstuck real fast.
> +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];
> +}
That is, the above does not consider the alignment requirements of
struct trace_uprobe_filter.
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
2020-01-21 14:50 ` Peter Zijlstra
@ 2020-01-21 14:59 ` David Laight
2020-01-21 15:06 ` Peter Zijlstra
0 siblings, 1 reply; 11+ messages in thread
From: David Laight @ 2020-01-21 14:59 UTC (permalink / raw)
To: 'Peter Zijlstra', Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Jiri Olsa,
Naveen N . Rao, Anil S Keshavamurthy, David S . Miller,
Namhyung Kim, Toke Høiland-Jørgensen, Jean-Tsung Hsiao,
Jesper Dangaard Brouer, stable, Arnaldo Carvalho de Melo,
Arnaldo Carvalho de Melo, Masami Hiramatsu
From: Peter Zijlstra
> Sent: 21 January 2020 14:50
> On Tue, Jan 21, 2020 at 09:38:49AM -0500, Steven Rostedt 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];
> > };
>
> Note that this relies on pure 'luck'. If you stick anything <4 bytes in
> between the list_head and the data member it'll come unstuck real fast.
Can you fix it by adding an unnamed struct as in:
struct trace_probe_event {
struct {
struct trace_event_call call;
struct list_head files;
struct list_head probes;
};
char data[0];
};
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
2020-01-21 14:59 ` David Laight
@ 2020-01-21 15:06 ` Peter Zijlstra
2020-01-22 22:12 ` Masami Hiramatsu
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-01-21 15:06 UTC (permalink / raw)
To: David Laight
Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Andrew Morton,
Jiri Olsa, Naveen N . Rao, Anil S Keshavamurthy,
David S . Miller, Namhyung Kim, Toke Høiland-Jørgensen,
Jean-Tsung Hsiao, Jesper Dangaard Brouer, stable,
Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo,
Masami Hiramatsu
On Tue, Jan 21, 2020 at 02:59:35PM +0000, David Laight wrote:
> From: Peter Zijlstra
> > Sent: 21 January 2020 14:50
> > On Tue, Jan 21, 2020 at 09:38:49AM -0500, Steven Rostedt 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];
> > > };
> >
> > Note that this relies on pure 'luck'. If you stick anything <4 bytes in
> > between the list_head and the data member it'll come unstuck real fast.
>
> Can you fix it by adding an unnamed struct as in:
The trivial fix is like I suggested in the other thread:
struct trace_uprobe_filter filters[0];
The alternative that Masami-San suggested should also work.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe
2020-01-21 15:06 ` Peter Zijlstra
@ 2020-01-22 22:12 ` Masami Hiramatsu
0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-01-22 22:12 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Laight, Steven Rostedt, linux-kernel, Ingo Molnar,
Andrew Morton, Jiri Olsa, Naveen N . Rao, Anil S Keshavamurthy,
David S . Miller, Namhyung Kim, Toke Høiland-Jørgensen,
Jean-Tsung Hsiao, Jesper Dangaard Brouer, stable,
Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo,
Masami Hiramatsu
On Tue, 21 Jan 2020 16:06:05 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jan 21, 2020 at 02:59:35PM +0000, David Laight wrote:
> > From: Peter Zijlstra
> > > Sent: 21 January 2020 14:50
> > > On Tue, Jan 21, 2020 at 09:38:49AM -0500, Steven Rostedt 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];
> > > > };
> > >
> > > Note that this relies on pure 'luck'. If you stick anything <4 bytes in
> > > between the list_head and the data member it'll come unstuck real fast.
> >
> > Can you fix it by adding an unnamed struct as in:
>
> The trivial fix is like I suggested in the other thread:
>
> struct trace_uprobe_filter filters[0];
>
> The alternative that Masami-San suggested should also work.
I've sent a fix ( https://lkml.org/lkml/2020/1/21/1214 )
as Peter suggested, change the field to
struct trace_uprobe_filter filters[0];
and change the code around that.
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [for-linus][PATCH 3/5] tracing: trigger: Replace unneeded RCU-list traversals
2020-01-21 14:38 [for-linus][PATCH 0/5] tracing: Fixes for 5.5 Steven Rostedt
2020-01-21 14:38 ` [for-linus][PATCH 1/5] tracing: xen: Ordered comparison of function pointers Steven Rostedt
2020-01-21 14:38 ` [for-linus][PATCH 2/5] tracing/uprobe: Fix double perf_event linking on multiprobe uprobe Steven Rostedt
@ 2020-01-21 14:38 ` Steven Rostedt
[not found] ` <20200122022623.20D812465A@mail.kernel.org>
2020-01-21 14:38 ` [for-linus][PATCH 4/5] tracing: Fix histogram code when expression has same var as value Steven Rostedt
2020-01-21 14:38 ` [for-linus][PATCH 5/5] tracing: Do not set trace clock if tracefs lockdown is in effect Steven Rostedt
4 siblings, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2020-01-21 14:38 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, stable, Tom Zanussi, Masami Hiramatsu
From: Masami Hiramatsu <mhiramat@kernel.org>
With CONFIG_PROVE_RCU_LIST, I had many suspicious RCU warnings
when I ran ftracetest trigger testcases.
-----
# dmesg -c > /dev/null
# ./ftracetest test.d/trigger
...
# dmesg | grep "RCU-list traversed" | cut -f 2 -d ] | cut -f 2 -d " "
kernel/trace/trace_events_hist.c:6070
kernel/trace/trace_events_hist.c:1760
kernel/trace/trace_events_hist.c:5911
kernel/trace/trace_events_trigger.c:504
kernel/trace/trace_events_hist.c:1810
kernel/trace/trace_events_hist.c:3158
kernel/trace/trace_events_hist.c:3105
kernel/trace/trace_events_hist.c:5518
kernel/trace/trace_events_hist.c:5998
kernel/trace/trace_events_hist.c:6019
kernel/trace/trace_events_hist.c:6044
kernel/trace/trace_events_trigger.c:1500
kernel/trace/trace_events_trigger.c:1540
kernel/trace/trace_events_trigger.c:539
kernel/trace/trace_events_trigger.c:584
-----
I investigated those warnings and found that the RCU-list
traversals in event trigger and hist didn't need to use
RCU version because those were called only under event_mutex.
I also checked other RCU-list traversals related to event
trigger list, and found that most of them were called from
event_hist_trigger_func() or hist_unregister_trigger() or
register/unregister functions except for a few cases.
Replace these unneeded RCU-list traversals with normal list
traversal macro and lockdep_assert_held() to check the
event_mutex is held.
Link: http://lkml.kernel.org/r/157680910305.11685.15110237954275915782.stgit@devnote2
Cc: stable@vger.kernel.org
Fixes: 30350d65ac567 ("tracing: Add variable support to hist triggers")
Reviewed-by: Tom Zanussi <zanussi@kernel.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 41 +++++++++++++++++++++--------
kernel/trace/trace_events_trigger.c | 20 ++++++++++----
2 files changed, 45 insertions(+), 16 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index f62de5f43e79..d33b046f985a 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1766,11 +1766,13 @@ static struct hist_field *find_var(struct hist_trigger_data *hist_data,
struct event_trigger_data *test;
struct hist_field *hist_field;
+ lockdep_assert_held(&event_mutex);
+
hist_field = find_var_field(hist_data, var_name);
if (hist_field)
return hist_field;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
test_data = test->private_data;
hist_field = find_var_field(test_data, var_name);
@@ -1820,7 +1822,9 @@ static struct hist_field *find_file_var(struct trace_event_file *file,
struct event_trigger_data *test;
struct hist_field *hist_field;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
test_data = test->private_data;
hist_field = find_var_field(test_data, var_name);
@@ -3115,7 +3119,9 @@ static char *find_trigger_filter(struct hist_trigger_data *hist_data,
{
struct event_trigger_data *test;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
if (test->private_data == hist_data)
return test->filter_str;
@@ -3166,9 +3172,11 @@ find_compatible_hist(struct hist_trigger_data *target_hist_data,
struct event_trigger_data *test;
unsigned int n_keys;
+ lockdep_assert_held(&event_mutex);
+
n_keys = target_hist_data->n_fields - target_hist_data->n_vals;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
hist_data = test->private_data;
@@ -5528,7 +5536,7 @@ static int hist_show(struct seq_file *m, void *v)
goto out_unlock;
}
- list_for_each_entry_rcu(data, &event_file->triggers, list) {
+ list_for_each_entry(data, &event_file->triggers, list) {
if (data->cmd_ops->trigger_type == ETT_EVENT_HIST)
hist_trigger_show(m, data, n++);
}
@@ -5921,7 +5929,9 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
if (hist_data->attrs->name && !named_data)
goto new;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
if (!hist_trigger_match(data, test, named_data, false))
continue;
@@ -6005,10 +6015,12 @@ static bool have_hist_trigger_match(struct event_trigger_data *data,
struct event_trigger_data *test, *named_data = NULL;
bool match = false;
+ lockdep_assert_held(&event_mutex);
+
if (hist_data->attrs->name)
named_data = find_named_trigger(hist_data->attrs->name);
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
if (hist_trigger_match(data, test, named_data, false)) {
match = true;
@@ -6026,10 +6038,12 @@ static bool hist_trigger_check_refs(struct event_trigger_data *data,
struct hist_trigger_data *hist_data = data->private_data;
struct event_trigger_data *test, *named_data = NULL;
+ lockdep_assert_held(&event_mutex);
+
if (hist_data->attrs->name)
named_data = find_named_trigger(hist_data->attrs->name);
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
if (!hist_trigger_match(data, test, named_data, false))
continue;
@@ -6051,10 +6065,12 @@ static void hist_unregister_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *test, *named_data = NULL;
bool unregistered = false;
+ lockdep_assert_held(&event_mutex);
+
if (hist_data->attrs->name)
named_data = find_named_trigger(hist_data->attrs->name);
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
if (!hist_trigger_match(data, test, named_data, false))
continue;
@@ -6080,7 +6096,9 @@ static bool hist_file_check_refs(struct trace_event_file *file)
struct hist_trigger_data *hist_data;
struct event_trigger_data *test;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
hist_data = test->private_data;
if (check_var_refs(hist_data))
@@ -6323,7 +6341,8 @@ hist_enable_trigger(struct event_trigger_data *data, void *rec,
struct enable_trigger_data *enable_data = data->private_data;
struct event_trigger_data *test;
- list_for_each_entry_rcu(test, &enable_data->file->triggers, list) {
+ list_for_each_entry_rcu(test, &enable_data->file->triggers, list,
+ lockdep_is_held(&event_mutex)) {
if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
if (enable_data->enable)
test->paused = false;
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index 2cd53ca21b51..40106fff06a4 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -501,7 +501,9 @@ void update_cond_flag(struct trace_event_file *file)
struct event_trigger_data *data;
bool set_cond = false;
- list_for_each_entry_rcu(data, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(data, &file->triggers, list) {
if (data->filter || event_command_post_trigger(data->cmd_ops) ||
event_command_needs_rec(data->cmd_ops)) {
set_cond = true;
@@ -536,7 +538,9 @@ static int register_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *test;
int ret = 0;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(test, &file->triggers, list) {
if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type) {
ret = -EEXIST;
goto out;
@@ -581,7 +585,9 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *data;
bool unregistered = false;
- list_for_each_entry_rcu(data, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(data, &file->triggers, list) {
if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
unregistered = true;
list_del_rcu(&data->list);
@@ -1497,7 +1503,9 @@ int event_enable_register_trigger(char *glob,
struct event_trigger_data *test;
int ret = 0;
- list_for_each_entry_rcu(test, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(test, &file->triggers, list) {
test_enable_data = test->private_data;
if (test_enable_data &&
(test->cmd_ops->trigger_type ==
@@ -1537,7 +1545,9 @@ void event_enable_unregister_trigger(char *glob,
struct event_trigger_data *data;
bool unregistered = false;
- list_for_each_entry_rcu(data, &file->triggers, list) {
+ lockdep_assert_held(&event_mutex);
+
+ list_for_each_entry(data, &file->triggers, list) {
enable_data = data->private_data;
if (enable_data &&
(data->cmd_ops->trigger_type ==
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [for-linus][PATCH 4/5] tracing: Fix histogram code when expression has same var as value
2020-01-21 14:38 [for-linus][PATCH 0/5] tracing: Fixes for 5.5 Steven Rostedt
` (2 preceding siblings ...)
2020-01-21 14:38 ` [for-linus][PATCH 3/5] tracing: trigger: Replace unneeded RCU-list traversals Steven Rostedt
@ 2020-01-21 14:38 ` Steven Rostedt
2020-01-21 14:38 ` [for-linus][PATCH 5/5] tracing: Do not set trace clock if tracefs lockdown is in effect Steven Rostedt
4 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2020-01-21 14:38 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, stable, Tom Zanuss
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
While working on a tool to convert SQL syntex into the histogram language of
the kernel, I discovered the following bug:
# echo 'first u64 start_time u64 end_time pid_t pid u64 delta' >> synthetic_events
# echo 'hist:keys=pid:start=common_timestamp' > events/sched/sched_waking/trigger
# echo 'hist:keys=next_pid:delta=common_timestamp-$start,start2=$start:onmatch(sched.sched_waking).trace(first,$start2,common_timestamp,next_pid,$delta)' > events/sched/sched_switch/trigger
Would not display any histograms in the sched_switch histogram side.
But if I were to swap the location of
"delta=common_timestamp-$start" with "start2=$start"
Such that the last line had:
# echo 'hist:keys=next_pid:start2=$start,delta=common_timestamp-$start:onmatch(sched.sched_waking).trace(first,$start2,common_timestamp,next_pid,$delta)' > events/sched/sched_switch/trigger
The histogram works as expected.
What I found out is that the expressions clear out the value once it is
resolved. As the variables are resolved in the order listed, when
processing:
delta=common_timestamp-$start
The $start is cleared. When it gets to "start2=$start", it errors out with
"unresolved symbol" (which is silent as this happens at the location of the
trace), and the histogram is dropped.
When processing the histogram for variable references, instead of adding a
new reference for a variable used twice, use the same reference. That way,
not only is it more efficient, but the order will no longer matter in
processing of the variables.
From Tom Zanussi:
"Just to clarify some more about what the problem was is that without
your patch, we would have two separate references to the same variable,
and during resolve_var_refs(), they'd both want to be resolved
separately, so in this case, since the first reference to start wasn't
part of an expression, it wouldn't get the read-once flag set, so would
be read normally, and then the second reference would do the read-once
read and also be read but using read-once. So everything worked and
you didn't see a problem:
from: start2=$start,delta=common_timestamp-$start
In the second case, when you switched them around, the first reference
would be resolved by doing the read-once, and following that the second
reference would try to resolve and see that the variable had already
been read, so failed as unset, which caused it to short-circuit out and
not do the trigger action to generate the synthetic event:
to: delta=common_timestamp-$start,start2=$start
With your patch, we only have the single resolution which happens
correctly the one time it's resolved, so this can't happen."
Link: https://lore.kernel.org/r/20200116154216.58ca08eb@gandalf.local.home
Cc: stable@vger.kernel.org
Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to hist triggers")
Reviewed-by: Tom Zanuss <zanussi@kernel.org>
Tested-by: Tom Zanussi <zanussi@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index d33b046f985a..6ac35b9e195d 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -116,6 +116,7 @@ struct hist_field {
struct ftrace_event_field *field;
unsigned long flags;
hist_field_fn_t fn;
+ unsigned int ref;
unsigned int size;
unsigned int offset;
unsigned int is_signed;
@@ -2427,8 +2428,16 @@ static int contains_operator(char *str)
return field_op;
}
+static void get_hist_field(struct hist_field *hist_field)
+{
+ hist_field->ref++;
+}
+
static void __destroy_hist_field(struct hist_field *hist_field)
{
+ if (--hist_field->ref > 1)
+ return;
+
kfree(hist_field->var.name);
kfree(hist_field->name);
kfree(hist_field->type);
@@ -2470,6 +2479,8 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
if (!hist_field)
return NULL;
+ hist_field->ref = 1;
+
hist_field->hist_data = hist_data;
if (flags & HIST_FIELD_FL_EXPR || flags & HIST_FIELD_FL_ALIAS)
@@ -2665,6 +2676,17 @@ static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
{
unsigned long flags = HIST_FIELD_FL_VAR_REF;
struct hist_field *ref_field;
+ int i;
+
+ /* Check if the variable already exists */
+ for (i = 0; i < hist_data->n_var_refs; i++) {
+ ref_field = hist_data->var_refs[i];
+ if (ref_field->var.idx == var_field->var.idx &&
+ ref_field->var.hist_data == var_field->hist_data) {
+ get_hist_field(ref_field);
+ return ref_field;
+ }
+ }
ref_field = create_hist_field(var_field->hist_data, NULL, flags, NULL);
if (ref_field) {
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [for-linus][PATCH 5/5] tracing: Do not set trace clock if tracefs lockdown is in effect
2020-01-21 14:38 [for-linus][PATCH 0/5] tracing: Fixes for 5.5 Steven Rostedt
` (3 preceding siblings ...)
2020-01-21 14:38 ` [for-linus][PATCH 4/5] tracing: Fix histogram code when expression has same var as value Steven Rostedt
@ 2020-01-21 14:38 ` Steven Rostedt
4 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2020-01-21 14:38 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, stable, Masami Ichikawa
From: Masami Ichikawa <masami256@gmail.com>
When trace_clock option is not set and unstable clcok detected,
tracing_set_default_clock() sets trace_clock(ThinkPad A285 is one of
case). In that case, if lockdown is in effect, null pointer
dereference error happens in ring_buffer_set_clock().
Link: http://lkml.kernel.org/r/20200116131236.3866925-1-masami256@gmail.com
Cc: stable@vger.kernel.org
Fixes: 17911ff38aa58 ("tracing: Add locked_down checks to the open calls of files created for tracefs")
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1788488
Signed-off-by: Masami Ichikawa <masami256@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ddb7e7f5fe8d..5b6ee4aadc26 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9420,6 +9420,11 @@ __init static int tracing_set_default_clock(void)
{
/* sched_clock_stable() is determined in late_initcall */
if (!trace_boot_clock && !sched_clock_stable()) {
+ if (security_locked_down(LOCKDOWN_TRACEFS)) {
+ pr_warn("Can not set tracing clock due to lockdown\n");
+ return -EPERM;
+ }
+
printk(KERN_WARNING
"Unstable clock detected, switching default tracing clock to \"global\"\n"
"If you want to keep using the local clock, then add:\n"
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread