linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6
@ 2020-01-26 19:19 Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 1/7] tracing: Fix very unlikely race of registering two stat tracers Steven Rostedt
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton


Dan Carpenter (1):
      tracing: Remove unneeded NULL check

Hou Pengyang (1):
      tracing: Fix comments about trace/ftrace.h

Josef Bacik (1):
      tracing: Set kernel_stack's caller size properly

Luis Henriques (1):
      tracing: Fix tracing_stat return values in error handling paths

Steven Rostedt (VMware) (3):
      tracing: Fix very unlikely race of registering two stat tracers
      tracing: Decrement trace_array when bootconfig creates an instance
      tracing: Use pr_err() instead of WARN() for memory failures

----
 include/trace/trace_events.h |  9 ++++++---
 kernel/trace/ftrace.c        |  4 ++--
 kernel/trace/trace.c         | 24 ++++++++++++++----------
 kernel/trace/trace.h         | 12 ++++++++++++
 kernel/trace/trace_boot.c    |  1 +
 kernel/trace/trace_entries.h |  2 +-
 kernel/trace/trace_stat.c    | 31 +++++++++++++++++--------------
 7 files changed, 53 insertions(+), 30 deletions(-)

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

* [for-next][PATCH 1/7] tracing: Fix very unlikely race of registering two stat tracers
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 2/7] tracing: Fix tracing_stat return values in error handling paths Steven Rostedt
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Luis Henriques

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Looking through old emails in my INBOX, I came across a patch from Luis
Henriques that attempted to fix a race of two stat tracers registering the
same stat trace (extremely unlikely, as this is done in the kernel, and
probably doesn't even exist). The submitted patch wasn't quite right as it
needed to deal with clean up a bit better (if two stat tracers were the
same, it would have the same files).

But to make the code cleaner, all we needed to do is to keep the
all_stat_sessions_mutex held for most of the registering function.

Link: http://lkml.kernel.org/r/1410299375-20068-1-git-send-email-luis.henriques@canonical.com

Fixes: 002bb86d8d42f ("tracing/ftrace: separate events tracing and stats tracing engine")
Reported-by: Luis Henriques <luis.henriques@canonical.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_stat.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index 874f1274cf99..da8a38c3d5e4 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -304,7 +304,7 @@ static int init_stat_file(struct stat_session *session)
 int register_stat_tracer(struct tracer_stat *trace)
 {
 	struct stat_session *session, *node;
-	int ret;
+	int ret = -EINVAL;
 
 	if (!trace)
 		return -EINVAL;
@@ -315,17 +315,15 @@ int register_stat_tracer(struct tracer_stat *trace)
 	/* Already registered? */
 	mutex_lock(&all_stat_sessions_mutex);
 	list_for_each_entry(node, &all_stat_sessions, session_list) {
-		if (node->ts == trace) {
-			mutex_unlock(&all_stat_sessions_mutex);
-			return -EINVAL;
-		}
+		if (node->ts == trace)
+			goto out;
 	}
-	mutex_unlock(&all_stat_sessions_mutex);
 
+	ret = -ENOMEM;
 	/* Init the session */
 	session = kzalloc(sizeof(*session), GFP_KERNEL);
 	if (!session)
-		return -ENOMEM;
+		goto out;
 
 	session->ts = trace;
 	INIT_LIST_HEAD(&session->session_list);
@@ -334,15 +332,16 @@ int register_stat_tracer(struct tracer_stat *trace)
 	ret = init_stat_file(session);
 	if (ret) {
 		destroy_session(session);
-		return ret;
+		goto out;
 	}
 
+	ret = 0;
 	/* Register */
-	mutex_lock(&all_stat_sessions_mutex);
 	list_add_tail(&session->session_list, &all_stat_sessions);
+ out:
 	mutex_unlock(&all_stat_sessions_mutex);
 
-	return 0;
+	return ret;
 }
 
 void unregister_stat_tracer(struct tracer_stat *trace)
-- 
2.24.1



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

* [for-next][PATCH 2/7] tracing: Fix tracing_stat return values in error handling paths
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 1/7] tracing: Fix very unlikely race of registering two stat tracers Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 3/7] tracing: Set kernel_stacks caller size properly Steven Rostedt
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Luis Henriques

From: Luis Henriques <luis.henriques@canonical.com>

tracing_stat_init() was always returning '0', even on the error paths.  It
now returns -ENODEV if tracing_init_dentry() fails or -ENOMEM if it fails
to created the 'trace_stat' debugfs directory.

Link: http://lkml.kernel.org/r/1410299381-20108-1-git-send-email-luis.henriques@canonical.com

Fixes: ed6f1c996bfe4 ("tracing: Check return value of tracing_init_dentry()")
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
[ Pulled from the archeological digging of my INBOX ]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_stat.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index da8a38c3d5e4..d1fa19773cc8 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -280,18 +280,22 @@ static int tracing_stat_init(void)
 
 	d_tracing = tracing_init_dentry();
 	if (IS_ERR(d_tracing))
-		return 0;
+		return -ENODEV;
 
 	stat_dir = tracefs_create_dir("trace_stat", d_tracing);
-	if (!stat_dir)
+	if (!stat_dir) {
 		pr_warn("Could not create tracefs 'trace_stat' entry\n");
+		return -ENOMEM;
+	}
 	return 0;
 }
 
 static int init_stat_file(struct stat_session *session)
 {
-	if (!stat_dir && tracing_stat_init())
-		return -ENODEV;
+	int ret;
+
+	if (!stat_dir && (ret = tracing_stat_init()))
+		return ret;
 
 	session->file = tracefs_create_file(session->ts->name, 0644,
 					    stat_dir,
-- 
2.24.1



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

* [for-next][PATCH 3/7] tracing: Set kernel_stacks caller size properly
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 1/7] tracing: Fix very unlikely race of registering two stat tracers Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 2/7] tracing: Fix tracing_stat return values in error handling paths Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 4/7] tracing: Remove unneeded NULL check Steven Rostedt
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Josef Bacik

From: Josef Bacik <jbacik@fb.com>

I noticed when trying to use the trace-cmd python interface that reading the raw
buffer wasn't working for kernel_stack events.  This is because it uses a
stubbed version of __dynamic_array that doesn't do the __data_loc trick and
encode the length of the array into the field.  Instead it just shows up as a
size of 0.  So change this to __array and set the len to FTRACE_STACK_ENTRIES
since this is what we actually do in practice and matches how user_stack_trace
works.

Link: http://lkml.kernel.org/r/1411589652-1318-1-git-send-email-jbacik@fb.com

Signed-off-by: Josef Bacik <jbacik@fb.com>
[ Pulled from the archeological digging of my INBOX ]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_entries.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index fc8e97328e54..78c146efb862 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -174,7 +174,7 @@ FTRACE_ENTRY(kernel_stack, stack_entry,
 
 	F_STRUCT(
 		__field(	int,		size	)
-		__dynamic_array(unsigned long,	caller	)
+		__array(	unsigned long,	caller,	FTRACE_STACK_ENTRIES	)
 	),
 
 	F_printk("\t=> %ps\n\t=> %ps\n\t=> %ps\n"
-- 
2.24.1



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

* [for-next][PATCH 4/7] tracing: Remove unneeded NULL check
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
                   ` (2 preceding siblings ...)
  2020-01-26 19:19 ` [for-next][PATCH 3/7] tracing: Set kernel_stacks caller size properly Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 5/7] tracing: Fix comments about trace/ftrace.h Steven Rostedt
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Dan Carpenter

From: Dan Carpenter <dan.carpenter@oracle.com>

We checked "iter->trace" earlier so there is no need to check here.

Link: http://lkml.kernel.org/r/20141122183012.GB6994@mwanda

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
[ Pulled from the archeological digging of my INBOX ]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d1410b4462ac..6fed9b0a8d58 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4224,7 +4224,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	mutex_init(&iter->mutex);
 
 	/* Notify the tracer early; before we stop tracing. */
-	if (iter->trace && iter->trace->open)
+	if (iter->trace->open)
 		iter->trace->open(iter);
 
 	/* Annotate start of buffers if we had overruns */
-- 
2.24.1



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

* [for-next][PATCH 5/7] tracing: Fix comments about trace/ftrace.h
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
                   ` (3 preceding siblings ...)
  2020-01-26 19:19 ` [for-next][PATCH 4/7] tracing: Remove unneeded NULL check Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 6/7] tracing: Decrement trace_array when bootconfig creates an instance Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures Steven Rostedt
  6 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Hou Pengyang

From: Hou Pengyang <houpengyang@huawei.com>

commit f42c85e74faa422cf0bc747ed808681145448f88 moved tracepoint's ftrace
creation into include/trace/ftrace.h and trace/define_trace.h was deleted
as a result. However some comment info does not adapt to the change, which
is such a misguiding when reading related code.

This patch fix this by moving trace/trace_events.h to <trace/events/XXX.h>,
since tracepoint headers have already been moved to tarce/events/.

Link: http://lkml.kernel.org/r/1425419298-61941-1-git-send-email-houpengyang@huawei.com

Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
[ Pulled from the archeological digging of my INBOX ]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/trace/trace_events.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index 13a58d453992..831048507fef 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -2,7 +2,8 @@
 /*
  * Stage 1 of the trace events.
  *
- * Override the macros in <trace/trace_events.h> to include the following:
+ * Override the macros in the event tracepoint header <trace/events/XXX.h>
+ * to include the following:
  *
  * struct trace_event_raw_<call> {
  *	struct trace_entry		ent;
@@ -223,7 +224,8 @@ TRACE_MAKE_SYSTEM_STR();
 /*
  * Stage 3 of the trace events.
  *
- * Override the macros in <trace/trace_events.h> to include the following:
+ * Override the macros in the event tracepoint header <trace/events/XXX.h>
+ * to include the following:
  *
  * enum print_line_t
  * trace_raw_output_<call>(struct trace_iterator *iter, int flags)
@@ -555,7 +557,8 @@ static inline notrace int trace_event_get_offsets_##call(		\
 /*
  * Stage 4 of the trace events.
  *
- * Override the macros in <trace/trace_events.h> to include the following:
+ * Override the macros in the event tracepoint header <trace/events/XXX.h>
+ * to include the following:
  *
  * For those macros defined with TRACE_EVENT:
  *
-- 
2.24.1



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

* [for-next][PATCH 6/7] tracing: Decrement trace_array when bootconfig creates an instance
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
                   ` (4 preceding siblings ...)
  2020-01-26 19:19 ` [for-next][PATCH 5/7] tracing: Fix comments about trace/ftrace.h Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 19:19 ` [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures Steven Rostedt
  6 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The trace_array_get_by_name() creates a ftrace instance and
trace_array_put() is used to remove the reference. Even though the
trace_array_get_by_name() creates the instance, it also adds a reference
count to it, that prevents user space from removing it.

As the bootconfig just creates the instance on boot up, it should still be
used where it can be deleted by user space after boot. A trace_array_put()
is required to let that happen.

Also, change the documentation on trace_array_get_by_name() to make this not
be so confusing.

Link: https://lore.kernel.org/r/20200124205927.76128804@rorschach.local.home

Fixes: 4f712a4d04a4e ("tracing/boot: Add instance node support")
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace.c      | 4 ++++
 kernel/trace/trace_boot.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6fed9b0a8d58..0a5569b1cace 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8602,6 +8602,10 @@ static int instance_mkdir(const char *name)
  * NOTE: This function increments the reference counter associated with the
  * trace array returned. This makes sure it cannot be freed while in use.
  * Use trace_array_put() once the trace array is no longer needed.
+ * If the trace_array is to be freed, trace_array_destroy() needs to
+ * be called after the trace_array_put(), or simply let user space delete
+ * it from the tracefs instances directory. But until the
+ * trace_array_put() is called, user space can not delete it.
  *
  */
 struct trace_array *trace_array_get_by_name(const char *name)
diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index cd541ac1cbc1..2f616cd926b0 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -327,6 +327,7 @@ trace_boot_init_instances(struct xbc_node *node)
 			continue;
 		}
 		trace_boot_init_one_instance(tr, inode);
+		trace_array_put(tr);
 	}
 }
 
-- 
2.24.1



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

* [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures
  2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
                   ` (5 preceding siblings ...)
  2020-01-26 19:19 ` [for-next][PATCH 6/7] tracing: Decrement trace_array when bootconfig creates an instance Steven Rostedt
@ 2020-01-26 19:19 ` Steven Rostedt
  2020-01-26 20:38   ` Joe Perches
  6 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 19:19 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Dmitry Vyukov

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

As warnings can trigger panics, especially when "panic_on_warn" is set,
memory failure warnings can cause panics and fail fuzz testers that are
stressing memory.

Create a MEM_FAIL() macro to use instead of WARN() in the tracing code
(perhaps this should be a kernel wide macro?), and use that for memory
failure issues. This should stop failing fuzz tests due to warnings.

Link: https://lore.kernel.org/r/CACT4Y+ZP-7np20GVRu3p+eZys9GPtbu+JpfV+HtsufAzvTgJrg@mail.gmail.com

Suggested-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |  4 ++--
 kernel/trace/trace.c  | 18 +++++++++---------
 kernel/trace/trace.h  | 12 ++++++++++++
 3 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 5c701765da5b..fdb1a9532420 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5459,7 +5459,7 @@ static void __init set_ftrace_early_graph(char *buf, int enable)
 	struct ftrace_hash *hash;
 
 	hash = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
-	if (WARN_ON(!hash))
+	if (MEM_FAIL(!hash, "Failed to allocate hash\n"))
 		return;
 
 	while (buf) {
@@ -6591,7 +6591,7 @@ static void add_to_clear_hash_list(struct list_head *clear_list,
 
 	func = kmalloc(sizeof(*func), GFP_KERNEL);
 	if (!func) {
-		WARN_ONCE(1, "alloc failure, ftrace filter could be stale\n");
+		MEM_FAIL(1, "alloc failure, ftrace filter could be stale\n");
 		return;
 	}
 
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0a5569b1cace..6a28b1b9bf42 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3126,7 +3126,7 @@ static int alloc_percpu_trace_buffer(void)
 	struct trace_buffer_struct *buffers;
 
 	buffers = alloc_percpu(struct trace_buffer_struct);
-	if (WARN(!buffers, "Could not allocate percpu trace_printk buffer"))
+	if (MEM_FAIL(!buffers, "Could not allocate percpu trace_printk buffer"))
 		return -ENOMEM;
 
 	trace_percpu_buffer = buffers;
@@ -7932,7 +7932,7 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
 
 	tr->percpu_dir = tracefs_create_dir("per_cpu", d_tracer);
 
-	WARN_ONCE(!tr->percpu_dir,
+	MEM_FAIL(!tr->percpu_dir,
 		  "Could not create tracefs directory 'per_cpu/%d'\n", cpu);
 
 	return tr->percpu_dir;
@@ -8253,7 +8253,7 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
 	for (cnt = 0; opts[cnt].name; cnt++) {
 		create_trace_option_file(tr, &topts[cnt], flags,
 					 &opts[cnt]);
-		WARN_ONCE(topts[cnt].entry == NULL,
+		MEM_FAIL(topts[cnt].entry == NULL,
 			  "Failed to create trace option: %s",
 			  opts[cnt].name);
 	}
@@ -8437,7 +8437,7 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
 #ifdef CONFIG_TRACER_MAX_TRACE
 	ret = allocate_trace_buffer(tr, &tr->max_buffer,
 				    allocate_snapshot ? size : 1);
-	if (WARN_ON(ret)) {
+	if (MEM_FAIL(ret, "Failed to allocate trace buffer\n")) {
 		ring_buffer_free(tr->array_buffer.buffer);
 		tr->array_buffer.buffer = NULL;
 		free_percpu(tr->array_buffer.data);
@@ -8726,7 +8726,7 @@ static __init void create_trace_instances(struct dentry *d_tracer)
 	trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
 							 instance_mkdir,
 							 instance_rmdir);
-	if (WARN_ON(!trace_instance_dir))
+	if (MEM_FAIL(!trace_instance_dir, "Failed to create instances directory\n"))
 		return;
 }
 
@@ -8796,7 +8796,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 #endif
 
 	if (ftrace_create_function_files(tr, d_tracer))
-		WARN(1, "Could not allocate function filter files");
+		MEM_FAIL(1, "Could not allocate function filter files");
 
 #ifdef CONFIG_TRACER_SNAPSHOT
 	trace_create_file("snapshot", 0644, d_tracer,
@@ -9348,8 +9348,7 @@ __init static int tracer_alloc_buffers(void)
 
 	/* TODO: make the number of buffers hot pluggable with CPUS */
 	if (allocate_trace_buffers(&global_trace, ring_buf_size) < 0) {
-		printk(KERN_ERR "tracer: failed to allocate ring buffer!\n");
-		WARN_ON(1);
+		MEM_FAIL(1, "tracer: failed to allocate ring buffer!\n");
 		goto out_free_savedcmd;
 	}
 
@@ -9422,7 +9421,8 @@ void __init early_trace_init(void)
 	if (tracepoint_printk) {
 		tracepoint_print_iter =
 			kmalloc(sizeof(*tracepoint_print_iter), GFP_KERNEL);
-		if (WARN_ON(!tracepoint_print_iter))
+		if (MEM_FAIL(!tracepoint_print_iter,
+			     "Failed to allocate trace iterator\n"))
 			tracepoint_printk = 0;
 		else
 			static_key_enable(&tracepoint_printk_key.key);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 4812a36affac..6bb64d89c321 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -94,6 +94,18 @@ enum trace_type {
 
 #include "trace_entries.h"
 
+/* Use this for memory failure errors */
+#define MEM_FAIL(condition, fmt, ...) ({			\
+	static bool __section(.data.once) __warned;		\
+	int __ret_warn_once = !!(condition);			\
+								\
+	if (unlikely(__ret_warn_once && !__warned)) {		\
+		__warned = true;				\
+		pr_err("ERROR: " fmt, ##__VA_ARGS__);		\
+	}							\
+	unlikely(__ret_warn_once);				\
+})
+
 /*
  * syscalls are special, and need special handling, this is why
  * they are not included in trace_entries.h
-- 
2.24.1



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

* Re: [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures
  2020-01-26 19:19 ` [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures Steven Rostedt
@ 2020-01-26 20:38   ` Joe Perches
  2020-01-26 20:50     ` Steven Rostedt
  0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2020-01-26 20:38 UTC (permalink / raw)
  To: Steven Rostedt, linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Dmitry Vyukov

On Sun, 2020-01-26 at 14:19 -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> As warnings can trigger panics, especially when "panic_on_warn" is set,
> memory failure warnings can cause panics and fail fuzz testers that are
> stressing memory.
> 
> Create a MEM_FAIL() macro to use instead of WARN() in the tracing code
> (perhaps this should be a kernel wide macro?), and use that for memory
> failure issues. This should stop failing fuzz tests due to warnings.

It looks as if most of these could just be removed instead
as there is an existing dump_stack() on failure.

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
[]
> @@ -5459,7 +5459,7 @@ static void __init set_ftrace_early_graph(char *buf, int enable)
>  	struct ftrace_hash *hash;
>  
>  	hash = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> -	if (WARN_ON(!hash))
> +	if (MEM_FAIL(!hash, "Failed to allocate hash\n"))
>  		return;

has dump_stack()
 
>  	while (buf) {
> @@ -6591,7 +6591,7 @@ static void add_to_clear_hash_list(struct list_head *clear_list,
>  
>  	func = kmalloc(sizeof(*func), GFP_KERNEL);
>  	if (!func) {
> -		WARN_ONCE(1, "alloc failure, ftrace filter could be stale\n");
> +		MEM_FAIL(1, "alloc failure, ftrace filter could be stale\n");

has dump_stack()

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
[]
> @@ -3126,7 +3126,7 @@ static int alloc_percpu_trace_buffer(void)
>  	struct trace_buffer_struct *buffers;
>  
>  	buffers = alloc_percpu(struct trace_buffer_struct);
> -	if (WARN(!buffers, "Could not allocate percpu trace_printk buffer"))
> +	if (MEM_FAIL(!buffers, "Could not allocate percpu trace_printk buffer"))
>  		return -ENOMEM;

has dump_stack()

>  
>  	trace_percpu_buffer = buffers;
> @@ -7932,7 +7932,7 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
>  
>  	tr->percpu_dir = tracefs_create_dir("per_cpu", d_tracer);
>  
> -	WARN_ONCE(!tr->percpu_dir,
> +	MEM_FAIL(!tr->percpu_dir,
>  		  "Could not create tracefs directory 'per_cpu/%d'\n", cpu);

keep?
 
>  	return tr->percpu_dir;
> @@ -8253,7 +8253,7 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
>  	for (cnt = 0; opts[cnt].name; cnt++) {
>  		create_trace_option_file(tr, &topts[cnt], flags,
>  					 &opts[cnt]);
> -		WARN_ONCE(topts[cnt].entry == NULL,
> +		MEM_FAIL(topts[cnt].entry == NULL,
>  			  "Failed to create trace option: %s",
>  			  opts[cnt].name);
>  	}
> @@ -8437,7 +8437,7 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
>  #ifdef CONFIG_TRACER_MAX_TRACE
>  	ret = allocate_trace_buffer(tr, &tr->max_buffer,
>  				    allocate_snapshot ? size : 1);
> -	if (WARN_ON(ret)) {
> +	if (MEM_FAIL(ret, "Failed to allocate trace buffer\n")) {
>  		ring_buffer_free(tr->array_buffer.buffer);
>  		tr->array_buffer.buffer = NULL;
>  		free_percpu(tr->array_buffer.data);
> @@ -8726,7 +8726,7 @@ static __init void create_trace_instances(struct dentry *d_tracer)
>  	trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
>  							 instance_mkdir,
>  							 instance_rmdir);
> -	if (WARN_ON(!trace_instance_dir))
> +	if (MEM_FAIL(!trace_instance_dir, "Failed to create instances directory\n"))
>  		return;
>  }
>  
> @@ -8796,7 +8796,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
>  #endif
>  
>  	if (ftrace_create_function_files(tr, d_tracer))
> -		WARN(1, "Could not allocate function filter files");
> +		MEM_FAIL(1, "Could not allocate function filter files");
>  
>  #ifdef CONFIG_TRACER_SNAPSHOT
>  	trace_create_file("snapshot", 0644, d_tracer,
> @@ -9348,8 +9348,7 @@ __init static int tracer_alloc_buffers(void)
>  
>  	/* TODO: make the number of buffers hot pluggable with CPUS */
>  	if (allocate_trace_buffers(&global_trace, ring_buf_size) < 0) {
> -		printk(KERN_ERR "tracer: failed to allocate ring buffer!\n");
> -		WARN_ON(1);
> +		MEM_FAIL(1, "tracer: failed to allocate ring buffer!\n");

has dump_stack()

>  		goto out_free_savedcmd;
>  	}
>  
> @@ -9422,7 +9421,8 @@ void __init early_trace_init(void)
>  	if (tracepoint_printk) {
>  		tracepoint_print_iter =
>  			kmalloc(sizeof(*tracepoint_print_iter), GFP_KERNEL);
> -		if (WARN_ON(!tracepoint_print_iter))
> +		if (MEM_FAIL(!tracepoint_print_iter,
> +			     "Failed to allocate trace iterator\n"))

has dump_stack()

>  			tracepoint_printk = 0;
>  		else
>  			static_key_enable(&tracepoint_printk_key.key);
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 4812a36affac..6bb64d89c321 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -94,6 +94,18 @@ enum trace_type {
>  
>  #include "trace_entries.h"
>  
> +/* Use this for memory failure errors */
> +#define MEM_FAIL(condition, fmt, ...) ({			\
> +	static bool __section(.data.once) __warned;		\
> +	int __ret_warn_once = !!(condition);			\
> +								\
> +	if (unlikely(__ret_warn_once && !__warned)) {		\
> +		__warned = true;				\
> +		pr_err("ERROR: " fmt, ##__VA_ARGS__);		\
> +	}							\
> +	unlikely(__ret_warn_once);				\
> +})
> +
>  /*
>   * syscalls are special, and need special handling, this is why
>   * they are not included in trace_entries.h


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

* Re: [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures
  2020-01-26 20:38   ` Joe Perches
@ 2020-01-26 20:50     ` Steven Rostedt
  2020-01-26 21:07       ` Joe Perches
  0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 20:50 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel, Ingo Molnar, Andrew Morton, Dmitry Vyukov

On Sun, 26 Jan 2020 12:38:55 -0800
Joe Perches <joe@perches.com> wrote:

> On Sun, 2020-01-26 at 14:19 -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > As warnings can trigger panics, especially when "panic_on_warn" is set,
> > memory failure warnings can cause panics and fail fuzz testers that are
> > stressing memory.
> > 
> > Create a MEM_FAIL() macro to use instead of WARN() in the tracing code
> > (perhaps this should be a kernel wide macro?), and use that for memory
> > failure issues. This should stop failing fuzz tests due to warnings.  
> 
> It looks as if most of these could just be removed instead
> as there is an existing dump_stack() on failure.

That sounds more generic. This is specific for my own tracing tests to
look for. As the point is, it is *not* to dump_stack, and still report
the error.

-- Steve

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

* Re: [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures
  2020-01-26 20:50     ` Steven Rostedt
@ 2020-01-26 21:07       ` Joe Perches
  2020-01-26 21:40         ` Steven Rostedt
  0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2020-01-26 21:07 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Andrew Morton, Dmitry Vyukov

On Sun, 2020-01-26 at 15:50 -0500, Steven Rostedt wrote:
> On Sun, 26 Jan 2020 12:38:55 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > On Sun, 2020-01-26 at 14:19 -0500, Steven Rostedt wrote:
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > 
> > > As warnings can trigger panics, especially when "panic_on_warn" is set,
> > > memory failure warnings can cause panics and fail fuzz testers that are
> > > stressing memory.
> > > 
> > > Create a MEM_FAIL() macro to use instead of WARN() in the tracing code
> > > (perhaps this should be a kernel wide macro?), and use that for memory
> > > failure issues. This should stop failing fuzz tests due to warnings.  
> > 
> > It looks as if most of these could just be removed instead
> > as there is an existing dump_stack() on failure.
> 
> That sounds more generic. This is specific for my own tracing tests to
> look for. As the point is, it is *not* to dump_stack, and still report
> the error.

__GFP_NOWARN is available too.



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

* Re: [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures
  2020-01-26 21:07       ` Joe Perches
@ 2020-01-26 21:40         ` Steven Rostedt
  0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-01-26 21:40 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel, Ingo Molnar, Andrew Morton, Dmitry Vyukov

On Sun, 26 Jan 2020 13:07:36 -0800
Joe Perches <joe@perches.com> wrote:

> > That sounds more generic. This is specific for my own tracing tests to
> > look for. As the point is, it is *not* to dump_stack, and still report
> > the error.  
> 
> __GFP_NOWARN is available too.

I honestly don't care if there's a dump_stack or not. I just removed the
WARN_ON. If the allocation causes a dump_stack() then that's fine, but
I still like to have a bit more information at what failed to allocate,
than just a offset into a function.

The point of this patch was simply to remove WARN_ON() that caused
fuzzers to fail.

-- Steve

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

end of thread, other threads:[~2020-01-26 21:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-26 19:19 [for-next][PATCH 0/7] tracing: Some very old (and some new) patches for 5.6 Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 1/7] tracing: Fix very unlikely race of registering two stat tracers Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 2/7] tracing: Fix tracing_stat return values in error handling paths Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 3/7] tracing: Set kernel_stacks caller size properly Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 4/7] tracing: Remove unneeded NULL check Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 5/7] tracing: Fix comments about trace/ftrace.h Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 6/7] tracing: Decrement trace_array when bootconfig creates an instance Steven Rostedt
2020-01-26 19:19 ` [for-next][PATCH 7/7] tracing: Use pr_err() instead of WARN() for memory failures Steven Rostedt
2020-01-26 20:38   ` Joe Perches
2020-01-26 20:50     ` Steven Rostedt
2020-01-26 21:07       ` Joe Perches
2020-01-26 21:40         ` Steven Rostedt

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).