trace-seq/seq-buf/x86/printk: Print all stacks from NMI safely

---
 kernel/trace/trace_seq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_seq.c +1 -1

index 1f24ed99dca2..b100994a17fe 100644
@@ -122,7 +122,7 @@ int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 	if (s->full || !len)
 		return 0;
 
-	ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
+	ret = bitmap_scnprintf(s->buffer + s->len, len, maskp, nmaskbits);
 	s->len += ret;
 
 	return 1;

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:20 UTC #2 lore raw

RFC PATCH v4 2/23 tracing: Add trace_seq_has_overflowed() and trace_handle_return()

From: "Steven Rostedt (Red Hat)" <[email protected]>

Adding a trace_seq_has_overflowed() which returns true if the trace_seq had too much written into it allows us to simplify the code.

Instead of checking the return value of every call to trace_seq_printf() and friends, they can all be called normally, and at the end we can return !trace_seq_has_overflowed() instead.

Several functions also return TRACE_TYPE_PARTIAL_LINE when the trace_seq overflowed and TRACE_TYPE_HANDLED otherwise. Another helper function was created called trace_handle_return() which takes a trace_seq and returns these enums. Using this helper function also simplifies the code.

This change also makes it possible to remove the return values of trace_seq_printf() and friends. They should instead just be void functions.

---
 include/linux/ftrace_event.h |  11 ++
 include/linux/trace_seq.h    |  12 ++
 include/trace/ftrace.h       |   6 +-
 kernel/trace/trace.c         |  69 +++----
 kernel/trace/trace.h         |   1 +
 kernel/trace/trace_output.c  | 416 +++++++++++++++++--------------------------
 kernel/trace/trace_output.h  |  16 +-
 7 files changed, 231 insertions(+), 300 deletions(-)

include/linux/ftrace_event.h +11 -0

index 28672e87e910..0bebb5c348b8 100644
@@ -138,6 +138,17 @@ enum print_line_t {
 	TRACE_TYPE_NO_CONSUME	= 3	/* Handled but ask to not consume */
 };
 
+/*
+ * Several functions return TRACE_TYPE_PARTIAL_LINE if the trace_seq
+ * overflowed, and TRACE_TYPE_HANDLED otherwise. This helper function
+ * simplifies those functions and keeps them in sync.
+ */
+static inline enum print_line_t trace_handle_return(struct trace_seq *s)
+{
+	return trace_seq_has_overflowed(s) ?
+		TRACE_TYPE_PARTIAL_LINE : TRACE_TYPE_HANDLED;
+}
+
 void tracing_generic_entry_update(struct trace_entry *entry,
 				  unsigned long flags,
 				  int pc);

include/linux/trace_seq.h +12 -0

index ea6c9dea79e3..07eda413dfcf 100644
@@ -40,6 +40,18 @@ trace_seq_buffer_ptr(struct trace_seq *s)
 	return s->buffer + s->len;
 }
 
+/**
+ * trace_seq_has_overflowed - return true if the trace_seq took too much
+ * @s: trace sequence descriptor
+ *
+ * Returns true if too much data was added to the trace_seq and it is
+ * now full and will not take anymore.
+ */
+static inline bool trace_seq_has_overflowed(struct trace_seq *s)
+{
+	return s->full || s->len > PAGE_SIZE - 1;
+}
+
 /*
  * Currently only defined when tracing is enabled.
  */

include/trace/ftrace.h +2 -4

index 26b4f2e13275..f13471b5d27a 100644
@@ -280,11 +280,9 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags,	\
 	if (ret)							\
 		return ret;						\
 									\
-	ret = trace_seq_printf(s, print);				\
-	if (!ret)							\
-		return TRACE_TYPE_PARTIAL_LINE;				\
+	trace_seq_printf(s, print);					\
 									\
-	return TRACE_TYPE_HANDLED;					\
+	return trace_handle_return(s);					\
 }									\
 static struct trace_event_functions ftrace_event_type_funcs_##call = {	\
 	.trace			= ftrace_raw_output_##call,		\

kernel/trace/trace.c +35 ~~-34~~

118 lines not on this page · raw message

kernel/trace/trace.h +1 -0

index 3376de623ea0..19418221b302 100644
@@ -14,6 +14,7 @@
 #include <linux/trace_seq.h>
 #include <linux/ftrace_event.h>
 #include <linux/compiler.h>
+#include <linux/trace_seq.h>
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 #include <asm/unistd.h>		/* For NR_SYSCALLS	     */

kernel/trace/trace_output.c +165 ~~-251~~

717 lines not on this page · raw message

kernel/trace/trace_output.h +5 ~~-11~~

index 80b25b585a70..8ef2c40efb3c 100644
@@ -35,17 +35,11 @@ trace_print_lat_fmt(struct trace_seq *s, struct trace_entry *entry);
 extern int __unregister_ftrace_event(struct trace_event *event);
 extern struct rw_semaphore trace_event_sem;
 
-#define SEQ_PUT_FIELD_RET(s, x)				\
-do {							\
-	if (!trace_seq_putmem(s, &(x), sizeof(x)))	\
-		return TRACE_TYPE_PARTIAL_LINE;		\
-} while (0)
-
-#define SEQ_PUT_HEX_FIELD_RET(s, x)			\
-do {							\
-	if (!trace_seq_putmem_hex(s, &(x), sizeof(x)))	\
-		return TRACE_TYPE_PARTIAL_LINE;		\
-} while (0)
+#define SEQ_PUT_FIELD(s, x)				\
+	trace_seq_putmem(s, &(x), sizeof(x))
+
+#define SEQ_PUT_HEX_FIELD(s, x)				\
+	trace_seq_putmem_hex(s, &(x), sizeof(x))
 
 #endif

Signature · 1 line

2.1.1

Petr Mladek

Reviewed-by on v4 2/23

2014-11-14 11:25 UTC #3 lore raw

Re: [RFC][PATCH 02/23 v4] tracing: Add trace_seq_has_overflowed() and trace_handle_return()

Quoted 29 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Adding a trace_seq_has_overflowed() which returns true if the trace_seq had too much written into it allows us to simplify the code.
Instead of checking the return value of every call to trace_seq_printf() and friends, they can all be called normally, and at the end we can return !trace_seq_has_overflowed() instead.
Several functions also return TRACE_TYPE_PARTIAL_LINE when the trace_seq overflowed and TRACE_TYPE_HANDLED otherwise. Another helper function was created called trace_handle_return() which takes a trace_seq and returns these enums. Using this helper function also simplifies the code.
This change also makes it possible to remove the return values of trace_seq_printf() and friends. They should instead just be void functions.
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/ftrace_event.h |  11 ++
 include/linux/trace_seq.h    |  12 ++
 include/trace/ftrace.h       |   6 +-
 kernel/trace/trace.c         |  69 +++----
 kernel/trace/trace.h         |   1 +
 kernel/trace/trace_output.c  | 416 +++++++++++++++++--------------------------
 kernel/trace/trace_output.h  |  16 +-
 7 files changed, 231 insertions(+), 300 deletions(-)

[...]

Quoted 4 lines

kernel/trace/trace_output.c +0 -0
index cfa91de22e27..163c11b6b8ff 100644

[...]

Quoted 10 lines

kernel/trace/trace_output.c +0 -1

@@ -193,7 +184,6 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	struct trace_seq *s = &iter->seq;
 	struct trace_seq *p = &iter->tmp_seq;
 	struct trace_entry *entry;
-	int ret;
 
 	event = container_of(trace_event, struct ftrace_event_call, event);
 	entry = iter->ent;

@@ -204,8 +194,9 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	}

kernel/trace/trace_output.c int ftrace_raw_output_prep(struct trace_iterator *iter, · line 202

Quoted 10 lines

kernel/trace/trace_output.c +3 -2

 
 	trace_seq_init(p);
-	ret = trace_seq_printf(s, "%s: ", ftrace_event_name(event));
-	if (!ret)
+	trace_seq_printf(s, "%s: ", ftrace_event_name(event));
+
+	if (trace_seq_has_overflowed(s))
 		return TRACE_TYPE_PARTIAL_LINE;
 
 	return 0;

This looks like a bug in the original code. It returns 0 in each case because TRACE_TYPE_PARTIAL_LINE == 0. I guess that the last three lines should get replaced by

	return trace_handle_return(s);

as it is done in the other functions.

[...]

kernel/trace/trace_output.c int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm, · line 304

Quoted 4 lines

kernel/trace/trace_output.c +0 -0

@@ -320,14 +302,14 @@ int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm,
 		if (file) {
 			ret = trace_seq_path(s, &file->f_path);
 			if (ret)

I think that we could ignore the return value from trace_seq_path() as well. The other trace_seq*() writes will be NOP if there is an overflow.

Or do you have some special plans with trace_seq_path(), please?

kernel/trace/trace_output.c int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm, · line 313

Quoted 13 lines

kernel/trace/trace_output.c +4 -4

-				ret = trace_seq_printf(s, "[+0x%lx]",
-						       ip - vmstart);
+				trace_seq_printf(s, "[+0x%lx]",
+						 ip - vmstart);
 		}
 		up_read(&mm->mmap_sem);
 	}
 	if (ret && ((sym_flags & TRACE_ITER_SYM_ADDR) || !file))
-		ret = trace_seq_printf(s, " <" IP_FMT ">", ip);
-	return ret;
+		trace_seq_printf(s, " <" IP_FMT ">", ip);
+	return !trace_seq_has_overflowed(s);
 }

The wrong return value was there even before. The other problem is just another optimization. Feel free to solve them later in another patch(set).

I really like the simplification.

Best Regards,
Petr

Steven Rostedt

author maintainer

2014-11-14 11:58 UTC #4 lore raw

Re: [RFC][PATCH 02/23 v4] tracing: Add trace_seq_has_overflowed() and trace_handle_return()

Quoted 63 lines · On Fri, 14 Nov 2014 12:25:23 +0100 Petr Mladek <[email protected]> wrote:

Quoted 29 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Adding a trace_seq_has_overflowed() which returns true if the trace_seq had too much written into it allows us to simplify the code.
Instead of checking the return value of every call to trace_seq_printf() and friends, they can all be called normally, and at the end we can return !trace_seq_has_overflowed() instead.
Several functions also return TRACE_TYPE_PARTIAL_LINE when the trace_seq overflowed and TRACE_TYPE_HANDLED otherwise. Another helper function was created called trace_handle_return() which takes a trace_seq and returns these enums. Using this helper function also simplifies the code.
This change also makes it possible to remove the return values of trace_seq_printf() and friends. They should instead just be void functions.
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/ftrace_event.h |  11 ++
 include/linux/trace_seq.h    |  12 ++
 include/trace/ftrace.h       |   6 +-
 kernel/trace/trace.c         |  69 +++----
 kernel/trace/trace.h         |   1 +
 kernel/trace/trace_output.c  | 416 +++++++++++++++++--------------------------
 kernel/trace/trace_output.h  |  16 +-
 7 files changed, 231 insertions(+), 300 deletions(-)
[...]
Quoted 4 lines
kernel/trace/trace_output.c +0 -0
index cfa91de22e27..163c11b6b8ff 100644
[...]
Quoted 10 lines
kernel/trace/trace_output.c +0 -1
@@ -193,7 +184,6 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	struct trace_seq *s = &iter->seq;
 	struct trace_seq *p = &iter->tmp_seq;
 	struct trace_entry *entry;
-	int ret;
 
 	event = container_of(trace_event, struct ftrace_event_call, event);
 	entry = iter->ent;
@@ -204,8 +194,9 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	}
kernel/trace/trace_output.c int ftrace_raw_output_prep(struct trace_iterator *iter, · line 202
Quoted 10 lines
kernel/trace/trace_output.c +3 -2
 
 	trace_seq_init(p);
-	ret = trace_seq_printf(s, "%s: ", ftrace_event_name(event));
-	if (!ret)
+	trace_seq_printf(s, "%s: ", ftrace_event_name(event));
+
+	if (trace_seq_has_overflowed(s))
 		return TRACE_TYPE_PARTIAL_LINE;
 
 	return 0;
This looks like a bug in the original code. It returns 0 in each case because TRACE_TYPE_PARTIAL_LINE == 0. I guess that the last three lines should get replaced by

Quoted 3 lines

	return trace_handle_return(s);
as it is done in the other functions.

Yes, I noticed this too. As it was a bug in the original code, I left it. I do plan on changing it though. Thanks for the reminder.

Quoted 11 lines

[...]
kernel/trace/trace_output.c int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm, · line 304
Quoted 4 lines
kernel/trace/trace_output.c +0 -0
@@ -320,14 +302,14 @@ int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm,
 		if (file) {
 			ret = trace_seq_path(s, &file->f_path);
 			if (ret)
I think that we could ignore the return value from trace_seq_path() as well. The other trace_seq*() writes will be NOP if there is an overflow.

Yes I know this too, and since trace_seq_path() still has a return value, I left it.

Quoted 2 lines

Or do you have some special plans with trace_seq_path(), please?

I want to understand what the seq_path() code does and why Al Viro said it requires having a return value. I'm just not touching that code until I have a full understanding. It's not needed for this patch set.

Quoted 10 lines

kernel/trace/trace_output.c +2 -3

-				ret = trace_seq_printf(s, "[+0x%lx]",
-						       ip - vmstart);
+				trace_seq_printf(s, "[+0x%lx]",
+						 ip - vmstart);
 		}
 		up_read(&mm->mmap_sem);
 	}
 	if (ret && ((sym_flags & TRACE_ITER_SYM_ADDR) || !file))
-		ret = trace_seq_printf(s, " <" IP_FMT ">", ip);

Quoted 8 lines

kernel/trace/trace_output.c int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm, · line 313
Quoted 4 lines
kernel/trace/trace_output.c +2 -1
-	return ret;
+		trace_seq_printf(s, " <" IP_FMT ">", ip);
+	return !trace_seq_has_overflowed(s);
 }
The wrong return value was there even before. The other problem is just another optimization. Feel free to solve them later in another patch(set).

Do you mean your first comment? If so, yeah, that's why I didn't change it. But I forgot to write a patch to fix it, so thanks for the reminder.

Quoted 4 lines

I really like the simplification.
Reviewed-by Petr Mladek <[email protected]>

Thanks,

-- Steve

Steven Rostedt

author maintainer

2014-11-14 18:22 UTC #5 lore raw

Re: [RFC][PATCH 02/23 v4] tracing: Add trace_seq_has_overflowed() and trace_handle_return()

In reply to Petr Mladek

Quoted 25 lines · On Fri, 14 Nov 2014 12:25:23 +0100 Petr Mladek <[email protected]> wrote:

Quoted 10 lines

(quoted diff) +0 -1

@@ -193,7 +184,6 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	struct trace_seq *s = &iter->seq;
 	struct trace_seq *p = &iter->tmp_seq;
 	struct trace_entry *entry;
-	int ret;
 
 	event = container_of(trace_event, struct ftrace_event_call, event);
 	entry = iter->ent;

@@ -204,8 +194,9 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	}

Review int ftrace_raw_output_prep(struct trace_iterator *iter, · line 202

Quoted 10 lines

(quoted diff) +3 -2

 
 	trace_seq_init(p);
-	ret = trace_seq_printf(s, "%s: ", ftrace_event_name(event));
-	if (!ret)
+	trace_seq_printf(s, "%s: ", ftrace_event_name(event));
+
+	if (trace_seq_has_overflowed(s))
 		return TRACE_TYPE_PARTIAL_LINE;
 
 	return 0;

This looks like a bug in the original code. It returns 0 in each case because TRACE_TYPE_PARTIAL_LINE == 0. I guess that the last three lines should get replaced by

Quoted 3 lines

	return trace_handle_return(s);
as it is done in the other functions.

Here's the fix for this:

-- Steve

Quoted 1 line

From 5986fa1aee34cbfe2513d7a20d2590fb2a1fc1f2 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Fri, 14 Nov 2014 11:42:06 -0500
Subject: [PATCH] tracing: Fix return value of ftrace_raw_output_prep()

If the trace_seq of ftrace_raw_output_prep() is full this function returns TRACE_TYPE_PARTIAL_LINE, otherwise it returns zero.

The problem is that TRACE_TYPE_PARTIAL_LINE happens to be zero!

The thing is, the caller of ftrace_raw_output_prep() expects a success to be zero. Change that to expect it to be TRACE_TYPE_HANDLED.

Link http://lkml.kernel.org/r/[email protected] open here

Reminded-by Petr Mladek <[email protected]>

---
 include/trace/ftrace.h      | 2 +-
 kernel/trace/trace_output.c | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

include/trace/ftrace.h +1 -1

index f13471b5d27a..139b5067345b 100644
@@ -277,7 +277,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags,	\
 	field = (typeof(field))iter->ent;				\
 									\
 	ret = ftrace_raw_output_prep(iter, trace_event);		\
-	if (ret)							\
+	if (ret != TRACE_TYPE_HANDLED)					\
 		return ret;						\
 									\
 	trace_seq_printf(s, print);					\

kernel/trace/trace_output.c +1 -4

index 163c11b6b8ff..723818bc83b4 100644
@@ -196,10 +196,7 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
 	trace_seq_init(p);
 	trace_seq_printf(s, "%s: ", ftrace_event_name(event));
 
-	if (trace_seq_has_overflowed(s))
-		return TRACE_TYPE_PARTIAL_LINE;
-
-	return 0;
+	return trace_handle_return(s);
 }
 EXPORT_SYMBOL(ftrace_raw_output_prep);

Signature · 1 line

1.8.1.4

Steven Rostedt

author maintainer

2014-11-14 01:20 UTC #6 lore raw

RFC PATCH v4 3/23 blktrace/tracing: Use trace_seq_has_overflowed() helper function

From: "Steven Rostedt (Red Hat)" <[email protected]>

Checking the return code of every trace_seq_printf() operation and having to return early if it overflowed makes the code messy.

Using the new trace_seq_has_overflowed() and trace_handle_return() functions allows us to clean up the code.

In the future, trace_seq_printf() and friends will be turning into void functions and not returning a value. The trace_seq_has_overflowed() is to be used instead. This cleanup allows that change to take place.

Cc Jens Axboe <[email protected]>

---
 kernel/trace/blktrace.c | 148 +++++++++++++++++++++---------------------------
 1 file changed, 66 insertions(+), 82 deletions(-)

kernel/trace/blktrace.c +66 ~~-82~~

index c1bd4ada2a04..11b9cb36092b 100644
@@ -1142,9 +1142,9 @@ static void get_pdu_remap(const struct trace_entry *ent,
 	r->sector_from = be64_to_cpu(sector_from);
 }
 
-typedef int (blk_log_action_t) (struct trace_iterator *iter, const char *act);
+typedef void (blk_log_action_t) (struct trace_iterator *iter, const char *act);
 
-static int blk_log_action_classic(struct trace_iterator *iter, const char *act)
+static void blk_log_action_classic(struct trace_iterator *iter, const char *act)
 {
 	char rwbs[RWBS_LEN];
 	unsigned long long ts  = iter->ts;

@@ -1154,33 +1154,33 @@ static int blk_log_action_classic(struct trace_iterator *iter, const char *act)
 
 	fill_rwbs(rwbs, t);
 
-	return trace_seq_printf(&iter->seq,
-				"%3d,%-3d %2d %5d.%09lu %5u %2s %3s ",
-				MAJOR(t->device), MINOR(t->device), iter->cpu,
-				secs, nsec_rem, iter->ent->pid, act, rwbs);
+	trace_seq_printf(&iter->seq,
+			 "%3d,%-3d %2d %5d.%09lu %5u %2s %3s ",
+			 MAJOR(t->device), MINOR(t->device), iter->cpu,
+			 secs, nsec_rem, iter->ent->pid, act, rwbs);
 }
 
-static int blk_log_action(struct trace_iterator *iter, const char *act)
+static void blk_log_action(struct trace_iterator *iter, const char *act)
 {
 	char rwbs[RWBS_LEN];
 	const struct blk_io_trace *t = te_blk_io_trace(iter->ent);
 
 	fill_rwbs(rwbs, t);
-	return trace_seq_printf(&iter->seq, "%3d,%-3d %2s %3s ",
-				MAJOR(t->device), MINOR(t->device), act, rwbs);
+	trace_seq_printf(&iter->seq, "%3d,%-3d %2s %3s ",
+			 MAJOR(t->device), MINOR(t->device), act, rwbs);
 }
 
-static int blk_log_dump_pdu(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_dump_pdu(struct trace_seq *s, const struct trace_entry *ent)
 {
 	const unsigned char *pdu_buf;
 	int pdu_len;
-	int i, end, ret;
+	int i, end;
 
 	pdu_buf = pdu_start(ent);
 	pdu_len = te_blk_io_trace(ent)->pdu_len;
 
 	if (!pdu_len)
-		return 1;
+		return;
 
 	/* find the last zero that needs to be printed */
 	for (end = pdu_len - 1; end >= 0; end--)

@@ -1188,119 +1188,107 @@ static int blk_log_dump_pdu(struct trace_seq *s, const struct trace_entry *ent)
 			break;
 	end++;
 
-	if (!trace_seq_putc(s, '('))
-		return 0;
+	trace_seq_putc(s, '(');
 
 	for (i = 0; i < pdu_len; i++) {
 
-		ret = trace_seq_printf(s, "%s%02x",
-				       i == 0 ? "" : " ", pdu_buf[i]);
-		if (!ret)
-			return ret;
+		trace_seq_printf(s, "%s%02x",
+				 i == 0 ? "" : " ", pdu_buf[i]);
 
 		/*
 		 * stop when the rest is just zeroes and indicate so
 		 * with a ".." appended
 		 */
-		if (i == end && end != pdu_len - 1)
-			return trace_seq_puts(s, " ..) ");
+		if (i == end && end != pdu_len - 1) {
+			trace_seq_puts(s, " ..) ");
+			return;
+		}
 	}
 
-	return trace_seq_puts(s, ") ");
+	trace_seq_puts(s, ") ");
 }
 
-static int blk_log_generic(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_generic(struct trace_seq *s, const struct trace_entry *ent)
 {
 	char cmd[TASK_COMM_LEN];
 
 	trace_find_cmdline(ent->pid, cmd);
 
 	if (t_action(ent) & BLK_TC_ACT(BLK_TC_PC)) {
-		int ret;
-
-		ret = trace_seq_printf(s, "%u ", t_bytes(ent));
-		if (!ret)
-			return 0;
-		ret = blk_log_dump_pdu(s, ent);
-		if (!ret)
-			return 0;
-		return trace_seq_printf(s, "[%s]\n", cmd);
+		trace_seq_printf(s, "%u ", t_bytes(ent));
+		blk_log_dump_pdu(s, ent);
+		trace_seq_printf(s, "[%s]\n", cmd);
 	} else {
 		if (t_sec(ent))
-			return trace_seq_printf(s, "%llu + %u [%s]\n",
+			trace_seq_printf(s, "%llu + %u [%s]\n",
 						t_sector(ent), t_sec(ent), cmd);
-		return trace_seq_printf(s, "[%s]\n", cmd);
+		else
+			trace_seq_printf(s, "[%s]\n", cmd);
 	}
 }
 
-static int blk_log_with_error(struct trace_seq *s,
+static void blk_log_with_error(struct trace_seq *s,
 			      const struct trace_entry *ent)
 {
 	if (t_action(ent) & BLK_TC_ACT(BLK_TC_PC)) {
-		int ret;
-
-		ret = blk_log_dump_pdu(s, ent);
-		if (ret)
-			return trace_seq_printf(s, "[%d]\n", t_error(ent));
-		return 0;
+		blk_log_dump_pdu(s, ent);
+		trace_seq_printf(s, "[%d]\n", t_error(ent));
 	} else {
 		if (t_sec(ent))
-			return trace_seq_printf(s, "%llu + %u [%d]\n",
-						t_sector(ent),
-						t_sec(ent), t_error(ent));
-		return trace_seq_printf(s, "%llu [%d]\n",
-					t_sector(ent), t_error(ent));
+			trace_seq_printf(s, "%llu + %u [%d]\n",
+					 t_sector(ent),
+					 t_sec(ent), t_error(ent));
+		else
+			trace_seq_printf(s, "%llu [%d]\n",
+					 t_sector(ent), t_error(ent));
 	}
 }
 
-static int blk_log_remap(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_remap(struct trace_seq *s, const struct trace_entry *ent)
 {
 	struct blk_io_trace_remap r = { .device_from = 0, };
 
 	get_pdu_remap(ent, &r);
-	return trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
-				t_sector(ent), t_sec(ent),
-				MAJOR(r.device_from), MINOR(r.device_from),
-				(unsigned long long)r.sector_from);
+	trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
+			 t_sector(ent), t_sec(ent),
+			 MAJOR(r.device_from), MINOR(r.device_from),
+			 (unsigned long long)r.sector_from);
 }
 
-static int blk_log_plug(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_plug(struct trace_seq *s, const struct trace_entry *ent)
 {
 	char cmd[TASK_COMM_LEN];
 
 	trace_find_cmdline(ent->pid, cmd);
 
-	return trace_seq_printf(s, "[%s]\n", cmd);
+	trace_seq_printf(s, "[%s]\n", cmd);
 }
 
-static int blk_log_unplug(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_unplug(struct trace_seq *s, const struct trace_entry *ent)
 {
 	char cmd[TASK_COMM_LEN];
 
 	trace_find_cmdline(ent->pid, cmd);
 
-	return trace_seq_printf(s, "[%s] %llu\n", cmd, get_pdu_int(ent));
+	trace_seq_printf(s, "[%s] %llu\n", cmd, get_pdu_int(ent));
 }
 
-static int blk_log_split(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_split(struct trace_seq *s, const struct trace_entry *ent)
 {
 	char cmd[TASK_COMM_LEN];
 
 	trace_find_cmdline(ent->pid, cmd);
 
-	return trace_seq_printf(s, "%llu / %llu [%s]\n", t_sector(ent),
-				get_pdu_int(ent), cmd);
+	trace_seq_printf(s, "%llu / %llu [%s]\n", t_sector(ent),
+			 get_pdu_int(ent), cmd);
 }
 
-static int blk_log_msg(struct trace_seq *s, const struct trace_entry *ent)
+static void blk_log_msg(struct trace_seq *s, const struct trace_entry *ent)
 {
-	int ret;
 	const struct blk_io_trace *t = te_blk_io_trace(ent);
 
-	ret = trace_seq_putmem(s, t + 1, t->pdu_len);
-	if (ret)
-		return trace_seq_putc(s, '\n');
-	return ret;
+	trace_seq_putmem(s, t + 1, t->pdu_len);
+	trace_seq_putc(s, '\n');
 }
 
 /*

@@ -1339,7 +1327,7 @@ static void blk_tracer_reset(struct trace_array *tr)
 
 static const struct {
 	const char *act[2];
-	int	   (*print)(struct trace_seq *s, const struct trace_entry *ent);
+	void	   (*print)(struct trace_seq *s, const struct trace_entry *ent);
 } what2act[] = {
 	[__BLK_TA_QUEUE]	= {{  "Q", "queue" },	   blk_log_generic },
 	[__BLK_TA_BACKMERGE]	= {{  "M", "backmerge" },  blk_log_generic },

@@ -1364,7 +1352,6 @@ static enum print_line_t print_one_line(struct trace_iterator *iter,
 	struct trace_seq *s = &iter->seq;
 	const struct blk_io_trace *t;
 	u16 what;
-	int ret;
 	bool long_act;
 	blk_log_action_t *log_action;

@@ -1374,21 +1361,18 @@ static enum print_line_t print_one_line(struct trace_iterator *iter,
 	log_action = classic ? &blk_log_action_classic : &blk_log_action;
 
 	if (t->action == BLK_TN_MESSAGE) {
-		ret = log_action(iter, long_act ? "message" : "m");
-		if (ret)
-			ret = blk_log_msg(s, iter->ent);
-		goto out;
+		log_action(iter, long_act ? "message" : "m");
+		blk_log_msg(s, iter->ent);
 	}
 
 	if (unlikely(what == 0 || what >= ARRAY_SIZE(what2act)))
-		ret = trace_seq_printf(s, "Unknown action %x\n", what);
+		trace_seq_printf(s, "Unknown action %x\n", what);
 	else {
-		ret = log_action(iter, what2act[what].act[long_act]);
-		if (ret)
-			ret = what2act[what].print(s, iter->ent);
+		log_action(iter, what2act[what].act[long_act]);
+		what2act[what].print(s, iter->ent);
 	}
-out:
-	return ret ? TRACE_TYPE_HANDLED : TRACE_TYPE_PARTIAL_LINE;
+
+	return trace_handle_return(s);
 }
 
 static enum print_line_t blk_trace_event_print(struct trace_iterator *iter,

@@ -1397,7 +1381,7 @@ static enum print_line_t blk_trace_event_print(struct trace_iterator *iter,
 	return print_one_line(iter, false);
 }
 
-static int blk_trace_synthesize_old_trace(struct trace_iterator *iter)
+static void blk_trace_synthesize_old_trace(struct trace_iterator *iter)
 {
 	struct trace_seq *s = &iter->seq;
 	struct blk_io_trace *t = (struct blk_io_trace *)iter->ent;

@@ -1407,18 +1391,18 @@ static int blk_trace_synthesize_old_trace(struct trace_iterator *iter)
 		.time     = iter->ts,
 	};
 
-	if (!trace_seq_putmem(s, &old, offset))
-		return 0;
-	return trace_seq_putmem(s, &t->sector,
-				sizeof(old) - offset + t->pdu_len);
+	trace_seq_putmem(s, &old, offset);
+	trace_seq_putmem(s, &t->sector,
+			 sizeof(old) - offset + t->pdu_len);
 }
 
 static enum print_line_t
 blk_trace_event_print_binary(struct trace_iterator *iter, int flags,
 			     struct trace_event *event)
 {
-	return blk_trace_synthesize_old_trace(iter) ?
-			TRACE_TYPE_HANDLED : TRACE_TYPE_PARTIAL_LINE;
+	blk_trace_synthesize_old_trace(iter);
+
+	return trace_handle_return(&iter->seq);
 }
 
 static enum print_line_t blk_tracer_print_line(struct trace_iterator *iter)

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:14 UTC #7 lore raw

RFC PATCH v4 4/23 ring-buffer: Remove check of trace_seq_{puts,printf}() return values

From: "Steven Rostedt (Red Hat)" <[email protected]>

Remove checking the return value of all trace_seq_puts(). It was wrong anyway as only the last return value mattered. But as the trace_seq_puts() is going to be a void function in the future, we should not be checking the return value of it anyway.

Just return !trace_seq_has_overflowed() instead.

---
 kernel/trace/ring_buffer.c | 75 ++++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 39 deletions(-)

kernel/trace/ring_buffer.c +36 ~~-39~~

index 2d75c94ae87d..a28bdd17c853 100644
@@ -34,21 +34,19 @@ static void update_pages_handler(struct work_struct *work);
  */
 int ring_buffer_print_entry_header(struct trace_seq *s)
 {
-	int ret;
-
-	ret = trace_seq_puts(s, "# compressed entry header\n");
-	ret = trace_seq_puts(s, "\ttype_len    :    5 bits\n");
-	ret = trace_seq_puts(s, "\ttime_delta  :   27 bits\n");
-	ret = trace_seq_puts(s, "\tarray       :   32 bits\n");
-	ret = trace_seq_putc(s, '\n');
-	ret = trace_seq_printf(s, "\tpadding     : type == %d\n",
-			       RINGBUF_TYPE_PADDING);
-	ret = trace_seq_printf(s, "\ttime_extend : type == %d\n",
-			       RINGBUF_TYPE_TIME_EXTEND);
-	ret = trace_seq_printf(s, "\tdata max type_len  == %d\n",
-			       RINGBUF_TYPE_DATA_TYPE_LEN_MAX);
+	trace_seq_puts(s, "# compressed entry header\n");
+	trace_seq_puts(s, "\ttype_len    :    5 bits\n");
+	trace_seq_puts(s, "\ttime_delta  :   27 bits\n");
+	trace_seq_puts(s, "\tarray       :   32 bits\n");
+	trace_seq_putc(s, '\n');
+	trace_seq_printf(s, "\tpadding     : type == %d\n",
+			 RINGBUF_TYPE_PADDING);
+	trace_seq_printf(s, "\ttime_extend : type == %d\n",
+			 RINGBUF_TYPE_TIME_EXTEND);
+	trace_seq_printf(s, "\tdata max type_len  == %d\n",
+			 RINGBUF_TYPE_DATA_TYPE_LEN_MAX);
 
-	return ret;
+	return !trace_seq_has_overflowed(s);
 }
 
 /*

@@ -419,32 +417,31 @@ static inline int test_time_stamp(u64 delta)
 int ring_buffer_print_page_header(struct trace_seq *s)
 {
 	struct buffer_data_page field;
-	int ret;
-
-	ret = trace_seq_printf(s, "\tfield: u64 timestamp;\t"
-			       "offset:0;\tsize:%u;\tsigned:%u;\n",
-			       (unsigned int)sizeof(field.time_stamp),
-			       (unsigned int)is_signed_type(u64));
-
-	ret = trace_seq_printf(s, "\tfield: local_t commit;\t"
-			       "offset:%u;\tsize:%u;\tsigned:%u;\n",
-			       (unsigned int)offsetof(typeof(field), commit),
-			       (unsigned int)sizeof(field.commit),
-			       (unsigned int)is_signed_type(long));
-
-	ret = trace_seq_printf(s, "\tfield: int overwrite;\t"
-			       "offset:%u;\tsize:%u;\tsigned:%u;\n",
-			       (unsigned int)offsetof(typeof(field), commit),
-			       1,
-			       (unsigned int)is_signed_type(long));
-
-	ret = trace_seq_printf(s, "\tfield: char data;\t"
-			       "offset:%u;\tsize:%u;\tsigned:%u;\n",
-			       (unsigned int)offsetof(typeof(field), data),
-			       (unsigned int)BUF_PAGE_SIZE,
-			       (unsigned int)is_signed_type(char));
 
-	return ret;
+	trace_seq_printf(s, "\tfield: u64 timestamp;\t"
+			 "offset:0;\tsize:%u;\tsigned:%u;\n",
+			 (unsigned int)sizeof(field.time_stamp),
+			 (unsigned int)is_signed_type(u64));
+
+	trace_seq_printf(s, "\tfield: local_t commit;\t"
+			 "offset:%u;\tsize:%u;\tsigned:%u;\n",
+			 (unsigned int)offsetof(typeof(field), commit),
+			 (unsigned int)sizeof(field.commit),
+			 (unsigned int)is_signed_type(long));
+
+	trace_seq_printf(s, "\tfield: int overwrite;\t"
+			 "offset:%u;\tsize:%u;\tsigned:%u;\n",
+			 (unsigned int)offsetof(typeof(field), commit),
+			 1,
+			 (unsigned int)is_signed_type(long));
+
+	trace_seq_printf(s, "\tfield: char data;\t"
+			 "offset:%u;\tsize:%u;\tsigned:%u;\n",
+			 (unsigned int)offsetof(typeof(field), data),
+			 (unsigned int)BUF_PAGE_SIZE,
+			 (unsigned int)is_signed_type(char));
+
+	return !trace_seq_has_overflowed(s);
 }
 
 struct rb_irq_work {

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:14 UTC #8 lore raw

RFC PATCH v4 5/23 tracing: Have branch tracer use trace_handle_return() helper function

From: "Steven Rostedt (Red Hat)" <[email protected]>

The branch tracer should not be checking the trace_seq_printf() return value as that will soon be void. There's a new trace_handle_return() helper function that will return TRACE_TYPE_PARTIAL_LINE if the trace_seq overflowed and TRACE_TYPE_HANDLED otherwise.

---
 kernel/trace/trace_branch.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

kernel/trace/trace_branch.c +7 -8

index 697fb9bac8f0..42c85036fc3f 100644
@@ -151,14 +151,13 @@ static enum print_line_t trace_branch_print(struct trace_iterator *iter,
 
 	trace_assign_type(field, iter->ent);
 
-	if (trace_seq_printf(&iter->seq, "[%s] %s:%s:%d\n",
-			     field->correct ? "  ok  " : " MISS ",
-			     field->func,
-			     field->file,
-			     field->line))
-		return TRACE_TYPE_PARTIAL_LINE;
-
-	return TRACE_TYPE_HANDLED;
+	trace_seq_printf(&iter->seq, "[%s] %s:%s:%d\n",
+			 field->correct ? "  ok  " : " MISS ",
+			 field->func,
+			 field->file,
+			 field->line);
+
+	return trace_handle_return(&iter->seq);
 }
 
 static void branch_print_header(struct seq_file *s)

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:19 UTC #9 lore raw

RFC PATCH v4 6/23 tracing: Have function_graph use trace_seq_has_overflowed()

From: "Steven Rostedt (Red Hat)" <[email protected]>

Instead of doing individual checks all over the place that makes the code very messy. Just check trace_seq_has_overflowed() at the end or in strategic places.

This makes the code much cleaner and also helps with getting closer to removing the return values of trace_seq_printf() and friends.

---
 kernel/trace/trace.h                 |   2 +-
 kernel/trace/trace_functions_graph.c | 374 +++++++++++------------------------
 2 files changed, 116 insertions(+), 260 deletions(-)

kernel/trace/trace.h +1 -1

index 19418221b302..c3a37e55ec8b 100644
@@ -726,7 +726,7 @@ extern unsigned long trace_flags;
 extern enum print_line_t
 print_graph_function_flags(struct trace_iterator *iter, u32 flags);
 extern void print_graph_headers_flags(struct seq_file *s, u32 flags);
-extern enum print_line_t
+extern void
 trace_print_graph_duration(unsigned long long duration, struct trace_seq *s);
 extern void graph_trace_open(struct trace_iterator *iter);
 extern void graph_trace_close(struct trace_iterator *iter);

kernel/trace/trace_functions_graph.c +115 ~~-259~~

636 lines not on this page · raw message

Signature · 1 line

2.1.1

Petr Mladek

Reviewed-by on v4 6/23

2014-11-14 12:41 UTC #10 lore raw

Re: [RFC][PATCH 06/23 v4] tracing: Have function_graph use trace_seq_has_overflowed()

Quoted 32 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Instead of doing individual checks all over the place that makes the code very messy. Just check trace_seq_has_overflowed() at the end or in strategic places.
This makes the code much cleaner and also helps with getting closer to removing the return values of trace_seq_printf() and friends.
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace.h                 |   2 +-
 kernel/trace/trace_functions_graph.c | 374 +++++++++++------------------------
 2 files changed, 116 insertions(+), 260 deletions(-)
kernel/trace/trace.h +1 -1
index 19418221b302..c3a37e55ec8b 100644
@@ -726,7 +726,7 @@ extern unsigned long trace_flags;
 extern enum print_line_t
 print_graph_function_flags(struct trace_iterator *iter, u32 flags);
 extern void print_graph_headers_flags(struct seq_file *s, u32 flags);
-extern enum print_line_t
+extern void
 trace_print_graph_duration(unsigned long long duration, struct trace_seq *s);
 extern void graph_trace_open(struct trace_iterator *iter);
 extern void graph_trace_close(struct trace_iterator *iter);
kernel/trace/trace_functions_graph.c +0 -0
index f0a0c982cde3..477a7c65cf08 100644

[...]

Quoted 20 lines

kernel/trace/trace_functions_graph.c +4 -4

@@ -682,94 +639,69 @@ get_return_for_leaf(struct trace_iterator *iter,
 	return next;
 }
 
-static int print_graph_abs_time(u64 t, struct trace_seq *s)
+static void print_graph_abs_time(u64 t, struct trace_seq *s)
 {
 	unsigned long usecs_rem;
 
 	usecs_rem = do_div(t, NSEC_PER_SEC);
 	usecs_rem /= 1000;
 
-	return trace_seq_printf(s, "%5lu.%06lu |  ",
-			(unsigned long)t, usecs_rem);
+	trace_seq_printf(s, "%5lu.%06lu |  ",
+			 (unsigned long)t, usecs_rem);
 }
 
-static enum print_line_t
+static void

kernel/trace/trace_functions_graph.c get_return_for_leaf(struct trace_iterator *iter, · line 661

Quoted 10 lines

kernel/trace/trace_functions_graph.c +1 -2

 print_graph_irq(struct trace_iterator *iter, unsigned long addr,
 		enum trace_type type, int cpu, pid_t pid, u32 flags)
 {
-	int ret;
 	struct trace_seq *s = &iter->seq;
 
 	if (addr < (unsigned long)__irqentry_text_start ||
 		addr >= (unsigned long)__irqentry_text_end)
-		return TRACE_TYPE_UNHANDLED;
+		return;

I was curious if the TRACE_TYPE_UNHANDLED return value has any special effect. But it seems to be ignored. There are only two callers and they take care only of the PARTIAL_LINE return value.

[...]

Quoted 64 lines

kernel/trace/trace_functions_graph.c +11 ~~-27~~

@@ -953,62 +856,43 @@ print_graph_entry_nested(struct trace_iterator *iter,
 	return TRACE_TYPE_NO_CONSUME;
 }
 
-static enum print_line_t
+static void
 print_graph_prologue(struct trace_iterator *iter, struct trace_seq *s,
 		     int type, unsigned long addr, u32 flags)
 {
 	struct fgraph_data *data = iter->private;
 	struct trace_entry *ent = iter->ent;
 	int cpu = iter->cpu;
-	int ret;
 
 	/* Pid */
-	if (verif_pid(s, ent->pid, cpu, data) == TRACE_TYPE_PARTIAL_LINE)
-		return TRACE_TYPE_PARTIAL_LINE;
+	verif_pid(s, ent->pid, cpu, data);
 
-	if (type) {
+	if (type)
 		/* Interrupt */
-		ret = print_graph_irq(iter, addr, type, cpu, ent->pid, flags);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+		print_graph_irq(iter, addr, type, cpu, ent->pid, flags);
 
 	if (!(trace_flags & TRACE_ITER_CONTEXT_INFO))
-		return 0;
+		return;
 
 	/* Absolute time */
-	if (flags & TRACE_GRAPH_PRINT_ABS_TIME) {
-		ret = print_graph_abs_time(iter->ts, s);
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+	if (flags & TRACE_GRAPH_PRINT_ABS_TIME)
+		print_graph_abs_time(iter->ts, s);
 
 	/* Cpu */
-	if (flags & TRACE_GRAPH_PRINT_CPU) {
-		ret = print_graph_cpu(s, cpu);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+	if (flags & TRACE_GRAPH_PRINT_CPU)
+		print_graph_cpu(s, cpu);
 
 	/* Proc */
 	if (flags & TRACE_GRAPH_PRINT_PROC) {
-		ret = print_graph_proc(s, ent->pid);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-
-		ret = trace_seq_puts(s, " | ");
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
+		print_graph_proc(s, ent->pid);
+		trace_seq_puts(s, " | ");
 	}
 
 	/* Latency format */

kernel/trace/trace_functions_graph.c print_graph_entry_nested(struct trace_iterator *iter, · line 895

Quoted 10 lines

kernel/trace/trace_functions_graph.c +3 -6

-	if (trace_flags & TRACE_ITER_LATENCY_FMT) {
-		ret = print_graph_lat_fmt(s, ent);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+	if (trace_flags & TRACE_ITER_LATENCY_FMT)
+		print_graph_lat_fmt(s, ent);
 
-	return 0;
+	return;

This probably even fixed a bug. The function returned TRACE_TYPE_PARTIAL_LINE (0) even when it did not print anything.

The simplification is really cool.

Best Regards,
Petr Mladek

Steven Rostedt

author maintainer

2014-11-14 12:52 UTC #11 lore raw

Re: [RFC][PATCH 06/23 v4] tracing: Have function_graph use trace_seq_has_overflowed()

Quoted 8 lines · On Fri, 14 Nov 2014 13:41:26 +0100 Petr Mladek <[email protected]> wrote:

Quoted 8 lines

kernel/trace/trace_functions_graph.c +1 -2

-static enum print_line_t
+static void
 print_graph_irq(struct trace_iterator *iter, unsigned long addr,
 		enum trace_type type, int cpu, pid_t pid, u32 flags)
 {
-	int ret;
 	struct trace_seq *s = &iter->seq;

Quoted 8 lines

kernel/trace/trace_functions_graph.c get_return_for_leaf(struct trace_iterator *iter, · line 661
Quoted 4 lines
kernel/trace/trace_functions_graph.c +1 -1
 	if (addr < (unsigned long)__irqentry_text_start ||
 		addr >= (unsigned long)__irqentry_text_end)
-		return TRACE_TYPE_UNHANDLED;
+		return;
I was curious if the TRACE_TYPE_UNHANDLED return value has any special effect. But it seems to be ignored. There are only two callers and they take care only of the PARTIAL_LINE return value.

Yep, I noticed that too, and did the same check before I made the change. IIRC, that UNHANDLED was written in case anyone did care.

Quoted 75 lines

[...]

Quoted 72 lines

kernel/trace/trace_functions_graph.c +13 ~~-32~~

@@ -953,62 +856,43 @@ print_graph_entry_nested(struct trace_iterator *iter,
 	return TRACE_TYPE_NO_CONSUME;
 }
 
-static enum print_line_t
+static void
 print_graph_prologue(struct trace_iterator *iter, struct trace_seq *s,
 		     int type, unsigned long addr, u32 flags)
 {
 	struct fgraph_data *data = iter->private;
 	struct trace_entry *ent = iter->ent;
 	int cpu = iter->cpu;
-	int ret;
 
 	/* Pid */
-	if (verif_pid(s, ent->pid, cpu, data) == TRACE_TYPE_PARTIAL_LINE)
-		return TRACE_TYPE_PARTIAL_LINE;
+	verif_pid(s, ent->pid, cpu, data);
 
-	if (type) {
+	if (type)
 		/* Interrupt */
-		ret = print_graph_irq(iter, addr, type, cpu, ent->pid, flags);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+		print_graph_irq(iter, addr, type, cpu, ent->pid, flags);
 
 	if (!(trace_flags & TRACE_ITER_CONTEXT_INFO))
-		return 0;
+		return;
 
 	/* Absolute time */
-	if (flags & TRACE_GRAPH_PRINT_ABS_TIME) {
-		ret = print_graph_abs_time(iter->ts, s);
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+	if (flags & TRACE_GRAPH_PRINT_ABS_TIME)
+		print_graph_abs_time(iter->ts, s);
 
 	/* Cpu */
-	if (flags & TRACE_GRAPH_PRINT_CPU) {
-		ret = print_graph_cpu(s, cpu);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+	if (flags & TRACE_GRAPH_PRINT_CPU)
+		print_graph_cpu(s, cpu);
 
 	/* Proc */
 	if (flags & TRACE_GRAPH_PRINT_PROC) {
-		ret = print_graph_proc(s, ent->pid);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-
-		ret = trace_seq_puts(s, " | ");
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
+		print_graph_proc(s, ent->pid);
+		trace_seq_puts(s, " | ");
 	}
 
 	/* Latency format */
-	if (trace_flags & TRACE_ITER_LATENCY_FMT) {
-		ret = print_graph_lat_fmt(s, ent);
-		if (ret == TRACE_TYPE_PARTIAL_LINE)
-			return TRACE_TYPE_PARTIAL_LINE;
-	}
+	if (trace_flags & TRACE_ITER_LATENCY_FMT)
+		print_graph_lat_fmt(s, ent);

Quoted 7 lines

Review print_graph_entry_nested(struct trace_iterator *iter, · line 895
Quoted 2 lines
(quoted diff) +1 -1
-	return 0;
+	return;
This probably even fixed a bug. The function returned TRACE_TYPE_PARTIAL_LINE (0) even when it did not print anything.
The simplification is really cool.

Thanks,

-- Steve

Quoted 5 lines

Reviewed-by Petr Mladek <[email protected]>
Best Regards,
Petr Mladek

Steven Rostedt

author maintainer

2014-11-14 01:20 UTC #12 lore raw

RFC PATCH v4 7/23 kprobes/tracing: Use trace_seq_has_overflowed() for overflow checks

From: "Steven Rostedt (Red Hat)" <[email protected]>

Instead of checking the return value of trace_seq_printf() and friends for overflowing of the buffer, use the trace_seq_has_overflowed() helper function.

This cleans up the code quite a bit and also takes us a step closer to changing the return values of trace_seq_printf() and friends to void.

Cc Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace_kprobe.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

kernel/trace/trace_kprobe.c +16 ~~-26~~

index 282f6e4e5539..be14b2f091c8 100644
@@ -1024,27 +1024,22 @@ print_kprobe_event(struct trace_iterator *iter, int flags,
 	field = (struct kprobe_trace_entry_head *)iter->ent;
 	tp = container_of(event, struct trace_probe, call.event);
 
-	if (!trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call)))
-		goto partial;
+	trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call));
 
 	if (!seq_print_ip_sym(s, field->ip, flags | TRACE_ITER_SYM_OFFSET))
-		goto partial;
+		goto out;
 
-	if (!trace_seq_puts(s, ")"))
-		goto partial;
+	trace_seq_puts(s, ")");
 
 	data = (u8 *)&field[1];
 	for (i = 0; i < tp->nr_args; i++)
 		if (!tp->args[i].type->print(s, tp->args[i].name,
 					     data + tp->args[i].offset, field))
-			goto partial;
-
-	if (!trace_seq_puts(s, "\n"))
-		goto partial;
+			goto out;
 
-	return TRACE_TYPE_HANDLED;
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_puts(s, "\n");
+ out:
+	return trace_handle_return(s);
 }
 
 static enum print_line_t

@@ -1060,33 +1055,28 @@ print_kretprobe_event(struct trace_iterator *iter, int flags,
 	field = (struct kretprobe_trace_entry_head *)iter->ent;
 	tp = container_of(event, struct trace_probe, call.event);
 
-	if (!trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call)))
-		goto partial;
+	trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call));
 
 	if (!seq_print_ip_sym(s, field->ret_ip, flags | TRACE_ITER_SYM_OFFSET))
-		goto partial;
+		goto out;
 
-	if (!trace_seq_puts(s, " <- "))
-		goto partial;
+	trace_seq_puts(s, " <- ");
 
 	if (!seq_print_ip_sym(s, field->func, flags & ~TRACE_ITER_SYM_OFFSET))
-		goto partial;
+		goto out;
 
-	if (!trace_seq_puts(s, ")"))
-		goto partial;
+	trace_seq_puts(s, ")");
 
 	data = (u8 *)&field[1];
 	for (i = 0; i < tp->nr_args; i++)
 		if (!tp->args[i].type->print(s, tp->args[i].name,
 					     data + tp->args[i].offset, field))
-			goto partial;
+			goto out;
 
-	if (!trace_seq_puts(s, "\n"))
-		goto partial;
+	trace_seq_puts(s, "\n");
 
-	return TRACE_TYPE_HANDLED;
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+ out:
+	return trace_handle_return(s);
 }

Signature · 1 line

2.1.1

Srikar Dronamraju

Reviewed-by on v4 7/23

2014-11-14 05:24 UTC #13 lore raw

Re: [RFC][PATCH 07/23 v4] kprobes/tracing: Use trace_seq_has_overflowed() for overflow checks

Quoted 11 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Instead of checking the return value of trace_seq_printf() and friends for overflowing of the buffer, use the trace_seq_has_overflowed() helper function.
This cleans up the code quite a bit and also takes us a step closer to changing the return values of trace_seq_printf() and friends to void.
Cc Masami Hiramatsu <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Looks good me to me.

Reviewed-by Srikar Dronamraju <[email protected]>

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Masami Hiramatsu

Acked-by on v4 7/23

2014-11-14 14:52 UTC #14 lore raw

Re: [RFC][PATCH 07/23 v4] kprobes/tracing: Use trace_seq_has_overflowed() for overflow checks

Quoted 8 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Instead of checking the return value of trace_seq_printf() and friends for overflowing of the buffer, use the trace_seq_has_overflowed() helper function.
This cleans up the code quite a bit and also takes us a step closer to changing the return values of trace_seq_printf() and friends to void.

Looks good to me :)

Acked-by Masami Hiramatsu <[email protected]>

Thank you,

Quoted 89 lines

Cc Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace_kprobe.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

kernel/trace/trace_kprobe.c +16 ~~-26~~

index 282f6e4e5539..be14b2f091c8 100644
@@ -1024,27 +1024,22 @@ print_kprobe_event(struct trace_iterator *iter, int flags,
 	field = (struct kprobe_trace_entry_head *)iter->ent;
 	tp = container_of(event, struct trace_probe, call.event);
 
-	if (!trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call)))
-		goto partial;
+	trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call));
 
 	if (!seq_print_ip_sym(s, field->ip, flags | TRACE_ITER_SYM_OFFSET))
-		goto partial;
+		goto out;
 
-	if (!trace_seq_puts(s, ")"))
-		goto partial;
+	trace_seq_puts(s, ")");
 
 	data = (u8 *)&field[1];
 	for (i = 0; i < tp->nr_args; i++)
 		if (!tp->args[i].type->print(s, tp->args[i].name,
 					     data + tp->args[i].offset, field))
-			goto partial;
-
-	if (!trace_seq_puts(s, "\n"))
-		goto partial;
+			goto out;
 
-	return TRACE_TYPE_HANDLED;
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_puts(s, "\n");
+ out:
+	return trace_handle_return(s);
 }
 
 static enum print_line_t

@@ -1060,33 +1055,28 @@ print_kretprobe_event(struct trace_iterator *iter, int flags,
 	field = (struct kretprobe_trace_entry_head *)iter->ent;
 	tp = container_of(event, struct trace_probe, call.event);
 
-	if (!trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call)))
-		goto partial;
+	trace_seq_printf(s, "%s: (", ftrace_event_name(&tp->call));
 
 	if (!seq_print_ip_sym(s, field->ret_ip, flags | TRACE_ITER_SYM_OFFSET))
-		goto partial;
+		goto out;
 
-	if (!trace_seq_puts(s, " <- "))
-		goto partial;
+	trace_seq_puts(s, " <- ");
 
 	if (!seq_print_ip_sym(s, field->func, flags & ~TRACE_ITER_SYM_OFFSET))
-		goto partial;
+		goto out;
 
-	if (!trace_seq_puts(s, ")"))
-		goto partial;
+	trace_seq_puts(s, ")");
 
 	data = (u8 *)&field[1];
 	for (i = 0; i < tp->nr_args; i++)
 		if (!tp->args[i].type->print(s, tp->args[i].name,
 					     data + tp->args[i].offset, field))
-			goto partial;
+			goto out;
 
-	if (!trace_seq_puts(s, "\n"))
-		goto partial;
+	trace_seq_puts(s, "\n");
 
-	return TRACE_TYPE_HANDLED;
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+ out:
+	return trace_handle_return(s);
 }

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Steven Rostedt

author maintainer

2014-11-14 01:20 UTC #15 lore raw

RFC PATCH v4 8/23 tracing: Do not check return values of trace_seq_p*() for mmio tracer

From: "Steven Rostedt (Red Hat)" <[email protected]>

The return values for trace_seq_printf() and friends are going to be removed and they will become void functions. The mmio tracer checked their return and even did so incorrectly.

Some of the funtions which returned the values were never checked themselves. Removing all the checks simplifies the code.

Use trace_seq_has_overflowed() and trace_handle_return() where necessary instead.

---
 kernel/trace/trace_mmiotrace.c | 52 +++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

kernel/trace/trace_mmiotrace.c +21 ~~-31~~

index 0abd9b863474..7a9ba62e9fef 100644
@@ -59,17 +59,15 @@ static void mmio_trace_start(struct trace_array *tr)
 	mmio_reset_data(tr);
 }
 
-static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)
+static void mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)
 {
-	int ret = 0;
 	int i;
 	resource_size_t start, end;
 	const struct pci_driver *drv = pci_dev_driver(dev);
 
-	/* XXX: incomplete checks for trace_seq_printf() return value */
-	ret += trace_seq_printf(s, "PCIDEV %02x%02x %04x%04x %x",
-				dev->bus->number, dev->devfn,
-				dev->vendor, dev->device, dev->irq);
+	trace_seq_printf(s, "PCIDEV %02x%02x %04x%04x %x",
+			 dev->bus->number, dev->devfn,
+			 dev->vendor, dev->device, dev->irq);
 	/*
 	 * XXX: is pci_resource_to_user() appropriate, since we are
 	 * supposed to interpret the __ioremap() phys_addr argument based on

@@ -77,21 +75,20 @@ static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)
 	 */
 	for (i = 0; i < 7; i++) {
 		pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
-		ret += trace_seq_printf(s, " %llx",
+		trace_seq_printf(s, " %llx",
 			(unsigned long long)(start |
 			(dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
 	}
 	for (i = 0; i < 7; i++) {
 		pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
-		ret += trace_seq_printf(s, " %llx",
+		trace_seq_printf(s, " %llx",
 			dev->resource[i].start < dev->resource[i].end ?
 			(unsigned long long)(end - start) + 1 : 0);
 	}
 	if (drv)
-		ret += trace_seq_printf(s, " %s\n", drv->name);
+		trace_seq_printf(s, " %s\n", drv->name);
 	else
-		ret += trace_seq_puts(s, " \n");
-	return ret;
+		trace_seq_puts(s, " \n");
 }
 
 static void destroy_header_iter(struct header_iter *hiter)

@@ -179,28 +176,27 @@ static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
 	unsigned long long t	= ns2usecs(iter->ts);
 	unsigned long usec_rem	= do_div(t, USEC_PER_SEC);
 	unsigned secs		= (unsigned long)t;
-	int ret = 1;
 
 	trace_assign_type(field, entry);
 	rw = &field->rw;
 
 	switch (rw->opcode) {
 	case MMIO_READ:
-		ret = trace_seq_printf(s,
+		trace_seq_printf(s,
 			"R %d %u.%06lu %d 0x%llx 0x%lx 0x%lx %d\n",
 			rw->width, secs, usec_rem, rw->map_id,
 			(unsigned long long)rw->phys,
 			rw->value, rw->pc, 0);
 		break;
 	case MMIO_WRITE:
-		ret = trace_seq_printf(s,
+		trace_seq_printf(s,
 			"W %d %u.%06lu %d 0x%llx 0x%lx 0x%lx %d\n",
 			rw->width, secs, usec_rem, rw->map_id,
 			(unsigned long long)rw->phys,
 			rw->value, rw->pc, 0);
 		break;
 	case MMIO_UNKNOWN_OP:
-		ret = trace_seq_printf(s,
+		trace_seq_printf(s,
 			"UNKNOWN %u.%06lu %d 0x%llx %02lx,%02lx,"
 			"%02lx 0x%lx %d\n",
 			secs, usec_rem, rw->map_id,

@@ -209,12 +205,11 @@ static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
 			(rw->value >> 0) & 0xff, rw->pc, 0);
 		break;
 	default:
-		ret = trace_seq_puts(s, "rw what?\n");
+		trace_seq_puts(s, "rw what?\n");
 		break;
 	}
-	if (ret)
-		return TRACE_TYPE_HANDLED;
-	return TRACE_TYPE_PARTIAL_LINE;
+
+	return trace_handle_return(s);
 }
 
 static enum print_line_t mmio_print_map(struct trace_iterator *iter)

@@ -226,31 +221,29 @@ static enum print_line_t mmio_print_map(struct trace_iterator *iter)
 	unsigned long long t	= ns2usecs(iter->ts);
 	unsigned long usec_rem	= do_div(t, USEC_PER_SEC);
 	unsigned secs		= (unsigned long)t;
-	int ret;
 
 	trace_assign_type(field, entry);
 	m = &field->map;
 
 	switch (m->opcode) {
 	case MMIO_PROBE:
-		ret = trace_seq_printf(s,
+		trace_seq_printf(s,
 			"MAP %u.%06lu %d 0x%llx 0x%lx 0x%lx 0x%lx %d\n",
 			secs, usec_rem, m->map_id,
 			(unsigned long long)m->phys, m->virt, m->len,
 			0UL, 0);
 		break;
 	case MMIO_UNPROBE:
-		ret = trace_seq_printf(s,
+		trace_seq_printf(s,
 			"UNMAP %u.%06lu %d 0x%lx %d\n",
 			secs, usec_rem, m->map_id, 0UL, 0);
 		break;
 	default:
-		ret = trace_seq_puts(s, "map what?\n");
+		trace_seq_puts(s, "map what?\n");
 		break;
 	}
-	if (ret)
-		return TRACE_TYPE_HANDLED;
-	return TRACE_TYPE_PARTIAL_LINE;
+
+	return trace_handle_return(s);
 }
 
 static enum print_line_t mmio_print_mark(struct trace_iterator *iter)

@@ -262,14 +255,11 @@ static enum print_line_t mmio_print_mark(struct trace_iterator *iter)
 	unsigned long long t	= ns2usecs(iter->ts);
 	unsigned long usec_rem	= do_div(t, USEC_PER_SEC);
 	unsigned secs		= (unsigned long)t;
-	int ret;
 
 	/* The trailing newline must be in the message. */
-	ret = trace_seq_printf(s, "MARK %u.%06lu %s", secs, usec_rem, msg);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_printf(s, "MARK %u.%06lu %s", secs, usec_rem, msg);
 
-	return TRACE_TYPE_HANDLED;
+	return trace_handle_return(s);
 }
 
 static enum print_line_t mmio_print_line(struct trace_iterator *iter)

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:18 UTC #16 lore raw

RFC PATCH v4 9/23 tracing/probes: Do not use return value of trace_seq_printf()

From: "Steven Rostedt (Red Hat)" <[email protected]>

The functions trace_seq_printf() and friends will soon not have a return value and will only be a void function. Use trace_seq_has_overflowed() instead to know if the trace_seq operations succeeded or not.

Cc Masami Hiramatsu <[email protected]>

Cc Namhyung Kim <[email protected]>

---
 kernel/trace/trace_probe.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

kernel/trace/trace_probe.c +6 -4

index d4b9fc22cd27..b983b2fd2ca1 100644
@@ -40,7 +40,8 @@ const char *reserved_field_names[] = {
 int PRINT_TYPE_FUNC_NAME(type)(struct trace_seq *s, const char *name,	\
 				void *data, void *ent)			\
 {									\
-	return trace_seq_printf(s, " %s=" fmt, name, *(type *)data);	\
+	trace_seq_printf(s, " %s=" fmt, name, *(type *)data);		\
+	return !trace_seq_has_overflowed(s);				\
 }									\
 const char PRINT_TYPE_FMT_NAME(type)[] = fmt;				\
 NOKPROBE_SYMBOL(PRINT_TYPE_FUNC_NAME(type));

@@ -61,10 +62,11 @@ int PRINT_TYPE_FUNC_NAME(string)(struct trace_seq *s, const char *name,
 	int len = *(u32 *)data >> 16;
 
 	if (!len)
-		return trace_seq_printf(s, " %s=(fault)", name);
+		trace_seq_printf(s, " %s=(fault)", name);
 	else
-		return trace_seq_printf(s, " %s=\"%s\"", name,
-					(const char *)get_loc_data(data, ent));
+		trace_seq_printf(s, " %s=\"%s\"", name,
+				 (const char *)get_loc_data(data, ent));
+	return !trace_seq_has_overflowed(s);
 }
 NOKPROBE_SYMBOL(PRINT_TYPE_FUNC_NAME(string));

Signature · 1 line

2.1.1

Masami Hiramatsu

Acked-by on v4 9/23

2014-11-14 15:31 UTC #17 lore raw

Re: [RFC][PATCH 09/23 v4] tracing/probes: Do not use return value of trace_seq_printf()

Quoted 6 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will soon not have a return value and will only be a void function. Use trace_seq_has_overflowed() instead to know if the trace_seq operations succeeded or not.

OK, Looks good to me.

Acked-by Masami Hiramatsu <[email protected]>

Quoted 36 lines

Cc Masami Hiramatsu <[email protected]>

Cc Namhyung Kim <[email protected]>

---
 kernel/trace/trace_probe.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

kernel/trace/trace_probe.c +6 -4

index d4b9fc22cd27..b983b2fd2ca1 100644
@@ -40,7 +40,8 @@ const char *reserved_field_names[] = {
 int PRINT_TYPE_FUNC_NAME(type)(struct trace_seq *s, const char *name,	\
 				void *data, void *ent)			\
 {									\
-	return trace_seq_printf(s, " %s=" fmt, name, *(type *)data);	\
+	trace_seq_printf(s, " %s=" fmt, name, *(type *)data);		\
+	return !trace_seq_has_overflowed(s);				\
 }									\
 const char PRINT_TYPE_FMT_NAME(type)[] = fmt;				\
 NOKPROBE_SYMBOL(PRINT_TYPE_FUNC_NAME(type));

@@ -61,10 +62,11 @@ int PRINT_TYPE_FUNC_NAME(string)(struct trace_seq *s, const char *name,
 	int len = *(u32 *)data >> 16;
 
 	if (!len)
-		return trace_seq_printf(s, " %s=(fault)", name);
+		trace_seq_printf(s, " %s=(fault)", name);
 	else
-		return trace_seq_printf(s, " %s=\"%s\"", name,
-					(const char *)get_loc_data(data, ent));
+		trace_seq_printf(s, " %s=\"%s\"", name,
+				 (const char *)get_loc_data(data, ent));
+	return !trace_seq_has_overflowed(s);
 }
 NOKPROBE_SYMBOL(PRINT_TYPE_FUNC_NAME(string));

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Namhyung Kim

Acked-by on v4 9/23

2014-11-18 00:31 UTC #18 lore raw

Re: [RFC][PATCH 09/23 v4] tracing/probes: Do not use return value of trace_seq_printf()

Hi Steve,

Quoted 18 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will soon not have a return value and will only be a void function. Use trace_seq_has_overflowed() instead to know if the trace_seq operations succeeded or not.
Cc Masami Hiramatsu <[email protected]>
Cc Namhyung Kim <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_probe.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/trace/trace_probe.c +0 -0
index d4b9fc22cd27..b983b2fd2ca1 100644

kernel/trace/trace_probe.c const char *reserved_field_names[] = { · line 44

Quoted 6 lines

kernel/trace/trace_probe.c +2 -1

@@ -40,7 +40,8 @@ const char *reserved_field_names[] = {
 int PRINT_TYPE_FUNC_NAME(type)(struct trace_seq *s, const char *name,	\
 				void *data, void *ent)			\
 {									\
-	return trace_seq_printf(s, " %s=" fmt, name, *(type *)data);	\
+	trace_seq_printf(s, " %s=" fmt, name, *(type *)data);		\
+	return !trace_seq_has_overflowed(s);				\

At first, I wondered why you used this instead of trace_handle_return() but I found out that it's only used for checking loop break so I'm okay with this.

Acked-by Namhyung Kim <[email protected]>

Thanks,
Namhyung

Quoted 17 lines

kernel/trace/trace_probe.c +4 -3

 }									\
 const char PRINT_TYPE_FMT_NAME(type)[] = fmt;				\
 NOKPROBE_SYMBOL(PRINT_TYPE_FUNC_NAME(type));

@@ -61,10 +62,11 @@ int PRINT_TYPE_FUNC_NAME(string)(struct trace_seq *s, const char *name,
 	int len = *(u32 *)data >> 16;
 
 	if (!len)
-		return trace_seq_printf(s, " %s=(fault)", name);
+		trace_seq_printf(s, " %s=(fault)", name);
 	else
-		return trace_seq_printf(s, " %s=\"%s\"", name,
-					(const char *)get_loc_data(data, ent));
+		trace_seq_printf(s, " %s=\"%s\"", name,
+				 (const char *)get_loc_data(data, ent));
+	return !trace_seq_has_overflowed(s);
 }
 NOKPROBE_SYMBOL(PRINT_TYPE_FUNC_NAME(string));

Steven Rostedt

author maintainer

2014-11-14 01:19 UTC #19 lore raw

RFC PATCH v4 10/23 tracing/uprobes: Do not use return values of trace_seq_printf()

From: "Steven Rostedt (Red Hat)" <[email protected]>

The functions trace_seq_printf() and friends will soon no longer have return values. Using trace_seq_has_overflowed() and trace_handle_return() should be used instead.

Cc Masami Hiramatsu <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

kernel/trace/trace_uprobe.c +9 ~~-14~~

index 33ff6a24b802..bd6007b13b52 100644
@@ -852,31 +852,26 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e
 	tu = container_of(event, struct trace_uprobe, tp.call.event);
 
 	if (is_ret_probe(tu)) {
-		if (!trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[1], entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[1], entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, true);
 	} else {
-		if (!trace_seq_printf(s, "%s: (0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, false);
 	}
 
 	for (i = 0; i < tu->tp.nr_args; i++) {
 		struct probe_arg *parg = &tu->tp.args[i];
 
-		if (!parg->type->print(s, parg->name, data + parg->offset, entry))
-			goto partial;
+		parg->type->print(s, parg->name, data + parg->offset, entry);
 	}
 
-	if (trace_seq_puts(s, "\n"))
-		return TRACE_TYPE_HANDLED;
+	trace_seq_puts(s, "\n");
 
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+	return trace_handle_return(s);
 }
 
 typedef bool (*filter_func_t)(struct uprobe_consumer *self,

Signature · 1 line

2.1.1

Srikar Dronamraju

Reviewed-by on v4 10/23

2014-11-14 05:24 UTC #20 lore raw

Re: [RFC][PATCH 10/23 v4] tracing/uprobes: Do not use return values of trace_seq_printf()

Quoted 10 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will soon no longer have return values. Using trace_seq_has_overflowed() and trace_handle_return() should be used instead.
Cc Masami Hiramatsu <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Srikar Dronamraju <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Looks good to me.

Reviewed-by Srikar Dronamraju <[email protected]>

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Masami Hiramatsu

2014-11-14 15:35 UTC #21 lore raw

Re: [RFC][PATCH 10/23 v4] tracing/uprobes: Do not use return values of trace_seq_printf()

Quoted 37 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

The functions trace_seq_printf() and friends will soon no longer have return values. Using trace_seq_has_overflowed() and trace_handle_return() should be used instead.

Cc Masami Hiramatsu <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

kernel/trace/trace_uprobe.c +5 -8

index 33ff6a24b802..bd6007b13b52 100644
@@ -852,31 +852,26 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e
 	tu = container_of(event, struct trace_uprobe, tp.call.event);
 
 	if (is_ret_probe(tu)) {
-		if (!trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[1], entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[1], entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, true);
 	} else {
-		if (!trace_seq_printf(s, "%s: (0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx)",
+				 ftrace_event_name(&tu->tp.call),

kernel/trace/trace_uprobe.c print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e · line 869

Quoted 10 lines

kernel/trace/trace_uprobe.c +2 -2

+				 entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, false);
 	}
 
 	for (i = 0; i < tu->tp.nr_args; i++) {
 		struct probe_arg *parg = &tu->tp.args[i];
 
-		if (!parg->type->print(s, parg->name, data + parg->offset, entry))
-			goto partial;
+		parg->type->print(s, parg->name, data + parg->offset, entry);

In 7/23 you've left loop canceling path, why don't you do same thing here?

Thank you,

Quoted 12 lines

kernel/trace/trace_uprobe.c +2 -4

 	}
 
-	if (trace_seq_puts(s, "\n"))
-		return TRACE_TYPE_HANDLED;
+	trace_seq_puts(s, "\n");
 
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+	return trace_handle_return(s);
 }
 
 typedef bool (*filter_func_t)(struct uprobe_consumer *self,

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Steven Rostedt

author maintainer

2014-11-14 15:37 UTC #22 lore raw

Re: [RFC][PATCH 10/23 v4] tracing/uprobes: Do not use return values of trace_seq_printf()

Quoted 8 lines · On Sat, 15 Nov 2014 00:35:15 +0900 Masami Hiramatsu…

kernel/trace/trace_uprobe.c print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e · line 869
Quoted 6 lines
kernel/trace/trace_uprobe.c +1 -2
 	for (i = 0; i < tu->tp.nr_args; i++) {
 		struct probe_arg *parg = &tu->tp.args[i];
 
-		if (!parg->type->print(s, parg->name, data + parg->offset, entry))
-			goto partial;
+		parg->type->print(s, parg->name, data + parg->offset, entry);
In 7/23 you've left loop canceling path, why don't you do same thing here?

While rebasing this series on my latest code, this patch conflicted. I notice the difference too (just two minutes ago!).

I agree, it should break out still.

Srikar, can I still have your Reviewed-by if I do that?

-- Steve

Quoted 18 lines

Thank you,

Quoted 12 lines

kernel/trace/trace_uprobe.c +2 -4

 	}
 
-	if (trace_seq_puts(s, "\n"))
-		return TRACE_TYPE_HANDLED;
+	trace_seq_puts(s, "\n");
 
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+	return trace_handle_return(s);
 }
 
 typedef bool (*filter_func_t)(struct uprobe_consumer *self,

Steven Rostedt

author maintainer

2014-11-14 18:14 UTC #23 lore raw

Re: [RFC][PATCH 10/23 v4] tracing/uprobes: Do not use return values of trace_seq_printf()

Quoted 12 lines · On Fri, 14 Nov 2014 10:37:33 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines · On Sat, 15 Nov 2014 00:35:15 +0900 Masami Hiramatsu…
Review
Quoted 6 lines
(quoted diff) +1 -2
 	for (i = 0; i < tu->tp.nr_args; i++) {
 		struct probe_arg *parg = &tu->tp.args[i];
 
-		if (!parg->type->print(s, parg->name, data + parg->offset, entry))
-			goto partial;
+		parg->type->print(s, parg->name, data + parg->offset, entry);
In 7/23 you've left loop canceling path, why don't you do same thing here?

Quoted 7 lines

While rebasing this series on my latest code, this patch conflicted. I notice the difference too (just two minutes ago!).
I agree, it should break out still.
Srikar, can I still have your Reviewed-by if I do that?

Here's the new patch:

-- Steve

Quoted 1 line

From cd883b4c2d5d68ec598d6c6a3b5dce4dd99a4cc1 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 12 Nov 2014 17:26:57 -0500
Subject: [PATCH] tracing/uprobes: Do not use return values of
trace_seq_printf()

The functions trace_seq_printf() and friends will soon no longer have return values. Using trace_seq_has_overflowed() and trace_handle_return() should be used instead.

Link http://lkml.kernel.org/r/[email protected] open here

Cc Masami Hiramatsu <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

kernel/trace/trace_uprobe.c +10 ~~-13~~

index e35327c787f7..fd76f8e108ef 100644
@@ -852,16 +852,14 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e
 	tu = container_of(event, struct trace_uprobe, tp.call.event);
 
 	if (is_ret_probe(tu)) {
-		if (!trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[1], entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[1], entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, true);
 	} else {
-		if (!trace_seq_printf(s, "%s: (0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, false);
 	}

@@ -869,14 +867,13 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e
 		struct probe_arg *parg = &tu->tp.args[i];
 
 		if (!parg->type->print(s, parg->name, data + parg->offset, entry))
-			goto partial;
+			goto out;
 	}
 
-	if (trace_seq_putc(s, '\n'))
-		return TRACE_TYPE_HANDLED;
+	trace_seq_putc(s, '\n');
 
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+ out:
+	return trace_handle_return(s);
 }
 
 typedef bool (*filter_func_t)(struct uprobe_consumer *self,

Signature · 1 line

1.8.1.4

Namhyung Kim

Acked-by

2014-11-18 00:32 UTC #24 lore raw

Re: [RFC][PATCH 10/23 v4] tracing/uprobes: Do not use return values of trace_seq_printf()

Quoted 14 lines · Steven Rostedt

Here's the new patch:
-- Steve
From cd883b4c2d5d68ec598d6c6a3b5dce4dd99a4cc1 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 12 Nov 2014 17:26:57 -0500
Subject: [PATCH] tracing/uprobes: Do not use return values of
trace_seq_printf()
The functions trace_seq_printf() and friends will soon no longer have return values. Using trace_seq_has_overflowed() and trace_handle_return() should be used instead.

Quoted 5 lines

Link http://lkml.kernel.org/r/[email protected] open here
Cc Masami Hiramatsu <[email protected]>
Cc Srikar Dronamraju <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Acked-by Namhyung Kim <[email protected]>

Thanks,
Namhyung

Quoted 50 lines

---
 kernel/trace/trace_uprobe.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

kernel/trace/trace_uprobe.c +10 ~~-13~~

index e35327c787f7..fd76f8e108ef 100644
@@ -852,16 +852,14 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e
 	tu = container_of(event, struct trace_uprobe, tp.call.event);
 
 	if (is_ret_probe(tu)) {
-		if (!trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[1], entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx <- 0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[1], entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, true);
 	} else {
-		if (!trace_seq_printf(s, "%s: (0x%lx)",
-					ftrace_event_name(&tu->tp.call),
-					entry->vaddr[0]))
-			goto partial;
+		trace_seq_printf(s, "%s: (0x%lx)",
+				 ftrace_event_name(&tu->tp.call),
+				 entry->vaddr[0]);
 		data = DATAOF_TRACE_ENTRY(entry, false);
 	}

@@ -869,14 +867,13 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e
 		struct probe_arg *parg = &tu->tp.args[i];
 
 		if (!parg->type->print(s, parg->name, data + parg->offset, entry))
-			goto partial;
+			goto out;
 	}
 
-	if (trace_seq_putc(s, '\n'))
-		return TRACE_TYPE_HANDLED;
+	trace_seq_putc(s, '\n');
 
-partial:
-	return TRACE_TYPE_PARTIAL_LINE;
+ out:
+	return trace_handle_return(s);
 }
 
 typedef bool (*filter_func_t)(struct uprobe_consumer *self,

Steven Rostedt

author maintainer

2014-11-14 01:17 UTC #25 lore raw

RFC PATCH v4 11/23 tracing: Do not use return values of trace_seq_printf() in syscall tracing

From: "Steven Rostedt (Red Hat)" <[email protected]>

The functions trace_seq_printf() and friends will not be returning values soon and will be void functions. To know if they succeeded or not, the functions trace_seq_has_overflowed() and trace_handle_return() should be used instead.

---
 kernel/trace/trace_syscalls.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

kernel/trace/trace_syscalls.c +15 ~~-28~~

index 4dc8b79c5f75..2193e8038681 100644
@@ -114,7 +114,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 	struct trace_entry *ent = iter->ent;
 	struct syscall_trace_enter *trace;
 	struct syscall_metadata *entry;
-	int i, ret, syscall;
+	int i, syscall;
 
 	trace = (typeof(trace))ent;
 	syscall = trace->nr;

@@ -128,35 +128,24 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 		goto end;
 	}
 
-	ret = trace_seq_printf(s, "%s(", entry->name);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_printf(s, "%s(", entry->name);
 
 	for (i = 0; i < entry->nb_args; i++) {
 		/* parameter types */
-		if (trace_flags & TRACE_ITER_VERBOSE) {
-			ret = trace_seq_printf(s, "%s ", entry->types[i]);
-			if (!ret)
-				return TRACE_TYPE_PARTIAL_LINE;
-		}
+		if (trace_flags & TRACE_ITER_VERBOSE)
+			trace_seq_printf(s, "%s ", entry->types[i]);
+
 		/* parameter values */
-		ret = trace_seq_printf(s, "%s: %lx%s", entry->args[i],
-				       trace->args[i],
-				       i == entry->nb_args - 1 ? "" : ", ");
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
+		trace_seq_printf(s, "%s: %lx%s", entry->args[i],
+				 trace->args[i],
+				 i == entry->nb_args - 1 ? "" : ", ");
 	}
 
-	ret = trace_seq_putc(s, ')');
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
-
+	trace_seq_putc(s, ')');
 end:
-	ret =  trace_seq_putc(s, '\n');
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_putc(s, '\n');
 
-	return TRACE_TYPE_HANDLED;
+	return trace_handle_return(s);
 }
 
 static enum print_line_t

@@ -168,7 +157,6 @@ print_syscall_exit(struct trace_iterator *iter, int flags,
 	struct syscall_trace_exit *trace;
 	int syscall;
 	struct syscall_metadata *entry;
-	int ret;
 
 	trace = (typeof(trace))ent;
 	syscall = trace->nr;

@@ -176,7 +164,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags,
 
 	if (!entry) {
 		trace_seq_putc(s, '\n');
-		return TRACE_TYPE_HANDLED;
+		goto out;
 	}
 
 	if (entry->exit_event->event.type != ent->type) {

@@ -184,12 +172,11 @@ print_syscall_exit(struct trace_iterator *iter, int flags,
 		return TRACE_TYPE_UNHANDLED;
 	}
 
-	ret = trace_seq_printf(s, "%s -> 0x%lx\n", entry->name,
+	trace_seq_printf(s, "%s -> 0x%lx\n", entry->name,
 				trace->ret);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
 
-	return TRACE_TYPE_HANDLED;
+ out:
+	return trace_handle_return(s);
 }
 
 extern char *__bad_type_size(void);

Signature · 1 line

2.1.1

Namhyung Kim

2014-11-18 00:49 UTC #26 lore raw

Re: [RFC][PATCH 11/23 v4] tracing: Do not use return values of trace_seq_printf() in syscall tracing

Quoted 26 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will not be returning values soon and will be void functions. To know if they succeeded or not, the functions trace_seq_has_overflowed() and trace_handle_return() should be used instead.
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_syscalls.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)
kernel/trace/trace_syscalls.c +1 -1
index 4dc8b79c5f75..2193e8038681 100644
@@ -114,7 +114,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 	struct trace_entry *ent = iter->ent;
 	struct syscall_trace_enter *trace;
 	struct syscall_metadata *entry;
-	int i, ret, syscall;
+	int i, syscall;
 
 	trace = (typeof(trace))ent;
 	syscall = trace->nr;

kernel/trace/trace_syscalls.c print_syscall_enter(struct trace_iterator *iter, int flags, · line 133

Quoted 9 lines

kernel/trace/trace_syscalls.c +1 -3

@@ -128,35 +128,24 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 		goto end;
 	}
 
-	ret = trace_seq_printf(s, "%s(", entry->name);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_printf(s, "%s(", entry->name);
 
 	for (i = 0; i < entry->nb_args; i++) {

		if (trace_seq_has_overflowed())
			goto end;

Thanks,
Namhyung

Quoted 34 lines

kernel/trace/trace_syscalls.c +9 ~~-18~~

 		/* parameter types */
-		if (trace_flags & TRACE_ITER_VERBOSE) {
-			ret = trace_seq_printf(s, "%s ", entry->types[i]);
-			if (!ret)
-				return TRACE_TYPE_PARTIAL_LINE;
-		}
+		if (trace_flags & TRACE_ITER_VERBOSE)
+			trace_seq_printf(s, "%s ", entry->types[i]);
+
 		/* parameter values */
-		ret = trace_seq_printf(s, "%s: %lx%s", entry->args[i],
-				       trace->args[i],
-				       i == entry->nb_args - 1 ? "" : ", ");
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
+		trace_seq_printf(s, "%s: %lx%s", entry->args[i],
+				 trace->args[i],
+				 i == entry->nb_args - 1 ? "" : ", ");
 	}
 
-	ret = trace_seq_putc(s, ')');
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
-
+	trace_seq_putc(s, ')');
 end:
-	ret =  trace_seq_putc(s, '\n');
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_putc(s, '\n');
 
-	return TRACE_TYPE_HANDLED;
+	return trace_handle_return(s);
 }

Steven Rostedt

author maintainer

2014-11-18 02:44 UTC #27 lore raw

Re: [RFC][PATCH 11/23 v4] tracing: Do not use return values of trace_seq_printf() in syscall tracing

Quoted 35 lines · On Tue, 18 Nov 2014 09:49:12 +0900 Namhyung Kim <[email protected]> wrote:

Quoted 34 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will not be returning values soon and will be void functions. To know if they succeeded or not, the functions trace_seq_has_overflowed() and trace_handle_return() should be used instead.
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_syscalls.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)
kernel/trace/trace_syscalls.c +2 -4
index 4dc8b79c5f75..2193e8038681 100644
@@ -114,7 +114,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 	struct trace_entry *ent = iter->ent;
 	struct syscall_trace_enter *trace;
 	struct syscall_metadata *entry;
-	int i, ret, syscall;
+	int i, syscall;
 
 	trace = (typeof(trace))ent;
 	syscall = trace->nr;
@@ -128,35 +128,24 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 		goto end;
 	}
 
-	ret = trace_seq_printf(s, "%s(", entry->name);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_printf(s, "%s(", entry->name);
 

Quoted 5 lines

kernel/trace/trace_syscalls.c print_syscall_enter(struct trace_iterator *iter, int flags, · line 133
Quoted 1 line
kernel/trace/trace_syscalls.c +0 -0
 	for (i = 0; i < entry->nb_args; i++) {
		if (trace_seq_has_overflowed())
			goto end;
?

Yeah, I guess I can do that.

Oh well, so much for not needing to post the first 13 patches again ;-)

-- Steve

Steven Rostedt

author maintainer Reviewed-by Petr Mladek

2014-11-18 03:04 UTC #28 lore raw

Re: [RFC][PATCH 11/23 v4] tracing: Do not use return values of trace_seq_printf() in syscall tracing

Quoted 9 lines · On Mon, 17 Nov 2014 21:44:11 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 5 lines · On Tue, 18 Nov 2014 09:49:12 +0900 Namhyung Kim <[email protected]> wrote:
Quoted 5 lines
(quoted diff) +1 -3
-	ret = trace_seq_printf(s, "%s(", entry->name);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_printf(s, "%s(", entry->name);

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
for (i = 0; i < entry->nb_args; i++) {
		if (trace_seq_has_overflowed())
			goto end;
?
Yeah, I guess I can do that.

Here's the update:

-- Steve

Quoted 1 line

From 17845179b46f3e8b849d06a9a0069fd0a085e666 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 12 Nov 2014 17:41:33 -0500
Subject: [PATCH] tracing: Do not use return values of trace_seq_printf() in
syscall tracing

---
 kernel/trace/trace_syscalls.c | 47 +++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

kernel/trace/trace_syscalls.c +19 ~~-28~~

index 4dc8b79c5f75..a72f3d8d813e 100644
@@ -114,7 +114,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 	struct trace_entry *ent = iter->ent;
 	struct syscall_trace_enter *trace;
 	struct syscall_metadata *entry;
-	int i, ret, syscall;
+	int i, syscall;
 
 	trace = (typeof(trace))ent;
 	syscall = trace->nr;

@@ -128,35 +128,28 @@ print_syscall_enter(struct trace_iterator *iter, int flags,
 		goto end;
 	}
 
-	ret = trace_seq_printf(s, "%s(", entry->name);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_printf(s, "%s(", entry->name);
 
 	for (i = 0; i < entry->nb_args; i++) {
+
+		if (trace_seq_has_overflowed(s))
+			goto end;
+
 		/* parameter types */
-		if (trace_flags & TRACE_ITER_VERBOSE) {
-			ret = trace_seq_printf(s, "%s ", entry->types[i]);
-			if (!ret)
-				return TRACE_TYPE_PARTIAL_LINE;
-		}
+		if (trace_flags & TRACE_ITER_VERBOSE)
+			trace_seq_printf(s, "%s ", entry->types[i]);
+
 		/* parameter values */
-		ret = trace_seq_printf(s, "%s: %lx%s", entry->args[i],
-				       trace->args[i],
-				       i == entry->nb_args - 1 ? "" : ", ");
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
+		trace_seq_printf(s, "%s: %lx%s", entry->args[i],
+				 trace->args[i],
+				 i == entry->nb_args - 1 ? "" : ", ");
 	}
 
-	ret = trace_seq_putc(s, ')');
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
-
+	trace_seq_putc(s, ')');
 end:
-	ret =  trace_seq_putc(s, '\n');
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
+	trace_seq_putc(s, '\n');
 
-	return TRACE_TYPE_HANDLED;
+	return trace_handle_return(s);
 }
 
 static enum print_line_t

@@ -168,7 +161,6 @@ print_syscall_exit(struct trace_iterator *iter, int flags,
 	struct syscall_trace_exit *trace;
 	int syscall;
 	struct syscall_metadata *entry;
-	int ret;
 
 	trace = (typeof(trace))ent;
 	syscall = trace->nr;

@@ -176,7 +168,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags,
 
 	if (!entry) {
 		trace_seq_putc(s, '\n');
-		return TRACE_TYPE_HANDLED;
+		goto out;
 	}
 
 	if (entry->exit_event->event.type != ent->type) {

@@ -184,12 +176,11 @@ print_syscall_exit(struct trace_iterator *iter, int flags,
 		return TRACE_TYPE_UNHANDLED;
 	}
 
-	ret = trace_seq_printf(s, "%s -> 0x%lx\n", entry->name,
+	trace_seq_printf(s, "%s -> 0x%lx\n", entry->name,
 				trace->ret);
-	if (!ret)
-		return TRACE_TYPE_PARTIAL_LINE;
 
-	return TRACE_TYPE_HANDLED;
+ out:
+	return trace_handle_return(s);
 }
 
 extern char *__bad_type_size(void);

Signature · 1 line

1.8.1.4

Steven Rostedt

author maintainer

2014-11-14 01:19 UTC #29 lore raw

RFC PATCH v4 12/23 tracing: Remove return values of most trace_seq_*() functions

From: "Steven Rostedt (Red Hat)" <[email protected]>

The trace_seq_printf() and friends are used to store strings into a buffer that can be passed around from function to function. If the trace_seq buffer fills up, it will not print any more. The return values were somewhat inconsistant and using trace_seq_has_overflowed() was a better way to know if the write to the trace_seq buffer succeeded or not.

Now that all users have removed reading the return value of the printf() type functions, they can safely return void and keep future users of them from reading the inconsistent values as well.

---
 include/linux/trace_seq.h | 37 +++++++++------------
 kernel/trace/trace_seq.c  | 84 +++++++++++++----------------------------------
 2 files changed, 37 insertions(+), 84 deletions(-)

include/linux/trace_seq.h +15 ~~-22~~

index 07eda413dfcf..db8a73224f1a 100644
@@ -57,40 +57,37 @@ static inline bool trace_seq_has_overflowed(struct trace_seq *s)
  */
 #ifdef CONFIG_TRACING
 extern __printf(2, 3)
-int trace_seq_printf(struct trace_seq *s, const char *fmt, ...);
+void trace_seq_printf(struct trace_seq *s, const char *fmt, ...);
 extern __printf(2, 0)
-int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args);
-extern int
+void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args);
+extern void
 trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary);
 extern int trace_print_seq(struct seq_file *m, struct trace_seq *s);
 extern int trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
 			     int cnt);
-extern int trace_seq_puts(struct trace_seq *s, const char *str);
-extern int trace_seq_putc(struct trace_seq *s, unsigned char c);
-extern int trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len);
-extern int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
+extern void trace_seq_puts(struct trace_seq *s, const char *str);
+extern void trace_seq_putc(struct trace_seq *s, unsigned char c);
+extern void trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len);
+extern void trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 				unsigned int len);
 extern int trace_seq_path(struct trace_seq *s, const struct path *path);
 
-extern int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
+extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 			     int nmaskbits);
 
 #else /* CONFIG_TRACING */
-static inline int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
+static inline void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
-	return 0;
 }
-static inline int
+static inline void
 trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
 {
-	return 0;
 }
 
-static inline int
+static inline void
 trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 		  int nmaskbits)
 {
-	return 0;
 }
 
 static inline int trace_print_seq(struct seq_file *m, struct trace_seq *s)

@@ -102,23 +99,19 @@ static inline int trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
 {
 	return 0;
 }
-static inline int trace_seq_puts(struct trace_seq *s, const char *str)
+static inline void trace_seq_puts(struct trace_seq *s, const char *str)
 {
-	return 0;
 }
-static inline int trace_seq_putc(struct trace_seq *s, unsigned char c)
+static inline void trace_seq_putc(struct trace_seq *s, unsigned char c)
 {
-	return 0;
 }
-static inline int
+static inline void
 trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len)
 {
-	return 0;
 }
-static inline int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
+static inline void trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 				       unsigned int len)
 {
-	return 0;
 }
 static inline int trace_seq_path(struct trace_seq *s, const struct path *path)
 {

kernel/trace/trace_seq.c +22 ~~-62~~

235 lines not on this page · raw message

Signature · 1 line

2.1.1

Petr Mladek

2014-11-14 13:17 UTC #30 lore raw

Re: [RFC][PATCH 12/23 v4] tracing: Remove return values of most trace_seq_*() functions

Quoted 17 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
The trace_seq_printf() and friends are used to store strings into a buffer that can be passed around from function to function. If the trace_seq buffer fills up, it will not print any more. The return values were somewhat inconsistant and using trace_seq_has_overflowed() was a better way to know if the write to the trace_seq buffer succeeded or not.
Now that all users have removed reading the return value of the printf() type functions, they can safely return void and keep future users of them from reading the inconsistent values as well.
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/trace_seq.h | 37 +++++++++------------
 kernel/trace/trace_seq.c  | 84 +++++++++++++----------------------------------
 2 files changed, 37 insertions(+), 84 deletions(-)

[...]

Quoted 4 lines

kernel/trace/trace_seq.c +0 -0
index b100994a17fe..e54c0a1fb3f0 100644

[...]

Quoted 24 lines

kernel/trace/trace_seq.c +2 -5

@@ -303,20 +269,17 @@ EXPORT_SYMBOL_GPL(trace_seq_putmem);
  * This is similar to trace_seq_putmem() except instead of just copying the
  * raw memory into the buffer it writes its ASCII representation of it
  * in hex characters.
- *
- * Returns how much it wrote to the buffer.
  */
-int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
+void trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 			 unsigned int len)
 {
 	unsigned char hex[HEX_CHARS];
 	const unsigned char *data = mem;
 	unsigned int start_len;
 	int i, j;
-	int cnt = 0;
 
 	if (s->full)
-		return 0;
+		return;
 
 	while (len) {
 		start_len = min(len, HEX_CHARS - 1);

kernel/trace/trace_seq.c int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, · line 338

Quoted 5 lines

kernel/trace/trace_seq.c +0 -2

@@ -334,10 +297,7 @@ int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 		/* j increments twice per loop */
 		len -= j / 2;
 		hex[j++] = ' ';
-
-		cnt += trace_seq_putmem(s, hex, j);

trace_seq_putmem(s, hex, j); should stay ;-)

Best Regards,
Petr

Quoted 9 lines

kernel/trace/trace_seq.c +0 -1
 	}
-	return cnt;
 }
 EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
 
Signature · 1 line
2.1.1

Steven Rostedt

author maintainer

2014-11-14 14:53 UTC #31 lore raw

Re: [RFC][PATCH 12/23 v4] tracing: Remove return values of most trace_seq_*() functions

Quoted 14 lines · On Fri, 14 Nov 2014 14:17:28 +0100 Petr Mladek <[email protected]> wrote:

Quoted 14 lines

kernel/trace/trace_seq.c +2 -3

-int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
+void trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 			 unsigned int len)
 {
 	unsigned char hex[HEX_CHARS];
 	const unsigned char *data = mem;
 	unsigned int start_len;
 	int i, j;
-	int cnt = 0;
 
 	if (s->full)
-		return 0;
+		return;

Quoted 10 lines

kernel/trace/trace_seq.c int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, · line 338
Quoted 8 lines
kernel/trace/trace_seq.c +0 -2
 	while (len) {
 		start_len = min(len, HEX_CHARS - 1);
@@ -334,10 +297,7 @@ int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 		/* j increments twice per loop */
 		len -= j / 2;
 		hex[j++] = ' ';
-
-		cnt += trace_seq_putmem(s, hex, j);
trace_seq_putmem(s, hex, j); should stay ;-)

Ouch! Looks like I got a little happy with the delete line key stroke.

Good catch!

Thanks,

-- Steve

Quoted 13 lines

Best Regards,
Petr
Quoted 9 lines
kernel/trace/trace_seq.c +0 -1
 	}
-	return cnt;
 }
 EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
 
Signature · 1 line
2.1.1

Steven Rostedt

author maintainer

2014-11-14 16:22 UTC #32 lore raw

Re: [RFC][PATCH 12/23 v4] tracing: Remove return values of most trace_seq_*() functions

Quoted 9 lines · On Fri, 14 Nov 2014 09:53:39 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines
kernel/trace/trace_seq.c int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, · line 338
Quoted 6 lines
kernel/trace/trace_seq.c +0 -2
@@ -334,10 +297,7 @@ int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 		/* j increments twice per loop */
 		len -= j / 2;
 		hex[j++] = ' ';
-
-		cnt += trace_seq_putmem(s, hex, j);
trace_seq_putmem(s, hex, j); should stay ;-)

Quoted 4 lines

Ouch! Looks like I got a little happy with the delete line key stroke.
Good catch!

And in the end it really didn't matter, as the patch "tracing: Create seq_buf layer in trace_seq" took all this out.

I'll still update the patch for correctness.

Thanks,

-- Steve

Steven Rostedt

author maintainer

2014-11-14 17:09 UTC #33 lore raw

Re: [RFC][PATCH 12/23 v4] tracing: Remove return values of most trace_seq_*() functions

In reply to Petr Mladek

Quoted 8 lines · On Fri, 14 Nov 2014 14:17:28 +0100 Petr Mladek <[email protected]> wrote:

Review int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, · line 338
Quoted 6 lines
(quoted diff) +0 -2
@@ -334,10 +297,7 @@ int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 		/* j increments twice per loop */
 		len -= j / 2;
 		hex[j++] = ' ';
-
-		cnt += trace_seq_putmem(s, hex, j);
trace_seq_putmem(s, hex, j); should stay ;-)

Here's the new patch:

Quoted 1 line

From 08223449645f482ee41333d414228da1d46e2095 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 12 Nov 2014 18:07:22 -0500
Subject: [PATCH] tracing: Remove return values of most trace_seq_*() functions

Now that all users have removed reading the return value of the printf() type functions, they can safely return void and keep future users of them from reading the inconsistent values as well.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/trace_seq.h | 37 +++++++++------------
 kernel/trace/trace_seq.c  | 84 +++++++++++++----------------------------------
 2 files changed, 38 insertions(+), 83 deletions(-)

include/linux/trace_seq.h +15 ~~-22~~

index 07eda413dfcf..db8a73224f1a 100644
@@ -57,40 +57,37 @@ static inline bool trace_seq_has_overflowed(struct trace_seq *s)
  */
 #ifdef CONFIG_TRACING
 extern __printf(2, 3)
-int trace_seq_printf(struct trace_seq *s, const char *fmt, ...);
+void trace_seq_printf(struct trace_seq *s, const char *fmt, ...);
 extern __printf(2, 0)
-int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args);
-extern int
+void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args);
+extern void
 trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary);
 extern int trace_print_seq(struct seq_file *m, struct trace_seq *s);
 extern int trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
 			     int cnt);
-extern int trace_seq_puts(struct trace_seq *s, const char *str);
-extern int trace_seq_putc(struct trace_seq *s, unsigned char c);
-extern int trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len);
-extern int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
+extern void trace_seq_puts(struct trace_seq *s, const char *str);
+extern void trace_seq_putc(struct trace_seq *s, unsigned char c);
+extern void trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len);
+extern void trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 				unsigned int len);
 extern int trace_seq_path(struct trace_seq *s, const struct path *path);
 
-extern int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
+extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 			     int nmaskbits);
 
 #else /* CONFIG_TRACING */
-static inline int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
+static inline void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
-	return 0;
 }
-static inline int
+static inline void
 trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
 {
-	return 0;
 }
 
-static inline int
+static inline void
 trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 		  int nmaskbits)
 {
-	return 0;
 }
 
 static inline int trace_print_seq(struct seq_file *m, struct trace_seq *s)

@@ -102,23 +99,19 @@ static inline int trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
 {
 	return 0;
 }
-static inline int trace_seq_puts(struct trace_seq *s, const char *str)
+static inline void trace_seq_puts(struct trace_seq *s, const char *str)
 {
-	return 0;
 }
-static inline int trace_seq_putc(struct trace_seq *s, unsigned char c)
+static inline void trace_seq_putc(struct trace_seq *s, unsigned char c)
 {
-	return 0;
 }
-static inline int
+static inline void
 trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len)
 {
-	return 0;
 }
-static inline int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
+static inline void trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 				       unsigned int len)
 {
-	return 0;
 }
 static inline int trace_seq_path(struct trace_seq *s, const struct path *path)
 {

kernel/trace/trace_seq.c +23 ~~-61~~

235 lines not on this page · raw message

Signature · 1 line

1.8.1.4

Steven Rostedt

author maintainer

2014-11-14 01:17 UTC #34 lore raw

RFC PATCH v4 13/23 tracing: Create seq_buf layer in trace_seq

From: "Steven Rostedt (Red Hat)" <[email protected]>

Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h              |  81 +++++++++
 include/linux/trace_seq.h            |  12 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 172 +++++++++---------
 8 files changed, 538 insertions(+), 120 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c

include/linux/seq_buf.h new file +81 -0

new file mode 100644
index 000000000000..4f7a96a9d71a
@@ -0,0 +1,81 @@
+#ifndef _LINUX_SEQ_BUF_H
+#define _LINUX_SEQ_BUF_H
+
+#include <linux/fs.h>
+
+/*
+ * Trace sequences are used to allow a function to call several other functions
+ * to create a string of data to use.
+ */
+
+/**
+ * seq_buf - seq buffer structure
+ * @buffer:	pointer to the buffer
+ * @size:	size of the buffer
+ * @len:	the amount of data inside the buffer
+ * @readpos:	The next position to read in the buffer.
+ */
+struct seq_buf {
+	unsigned char		*buffer;
+	unsigned int		size;
+	unsigned int		len;
+	unsigned int		readpos;
+};
+
+static inline void
+seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
+{
+	s->buffer = buf;
+	s->size = size;
+	s->len = 0;
+	s->readpos = 0;
+}
+
+/*
+ * seq_buf have a buffer that might overflow. When this happens
+ * the len and size are set to be equal.
+ */
+static inline bool
+seq_buf_has_overflowed(struct seq_buf *s)
+{
+	return s->len == s->size;
+}
+
+static inline void
+seq_buf_set_overflow(struct seq_buf *s)
+{
+	s->len = s->size;
+}
+
+/*
+ * How much buffer is left on the seq_buf?
+ */
+static inline unsigned int
+seq_buf_buffer_left(struct seq_buf *s)
+{
+	if (seq_buf_has_overflowed(s))
+		return 0;
+
+	return (s->size - 1) - s->len;
+}
+
+extern __printf(2, 3)
+int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
+extern __printf(2, 0)
+int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args);
+extern int
+seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s);
+extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,
+			   int cnt);
+extern int seq_buf_puts(struct seq_buf *s, const char *str);
+extern int seq_buf_putc(struct seq_buf *s, unsigned char c);
+extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
+extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+			      unsigned int len);
+extern int seq_buf_path(struct seq_buf *s, const struct path *path);
+
+extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+			   int nmaskbits);
+
+#endif /* _LINUX_SEQ_BUF_H */

include/linux/trace_seq.h +5 -7

index db8a73224f1a..85d37106be3d 100644
@@ -1,7 +1,7 @@
 #ifndef _LINUX_TRACE_SEQ_H
 #define _LINUX_TRACE_SEQ_H
 
-#include <linux/fs.h>
+#include <linux/seq_buf.h>
 
 #include <asm/page.h>

@@ -12,16 +12,14 @@
 
 struct trace_seq {
 	unsigned char		buffer[PAGE_SIZE];
-	unsigned int		len;
-	unsigned int		readpos;
+	struct seq_buf		seq;
 	int			full;
 };
 
 static inline void
 trace_seq_init(struct trace_seq *s)
 {
-	s->len = 0;
-	s->readpos = 0;
+	seq_buf_init(&s->seq, s->buffer, PAGE_SIZE);
 	s->full = 0;
 }

@@ -37,7 +35,7 @@ trace_seq_init(struct trace_seq *s)
 static inline unsigned char *
 trace_seq_buffer_ptr(struct trace_seq *s)
 {
-	return s->buffer + s->len;
+	return s->buffer + s->seq.len;
 }
 
 /**

@@ -49,7 +47,7 @@ trace_seq_buffer_ptr(struct trace_seq *s)
  */
 static inline bool trace_seq_has_overflowed(struct trace_seq *s)
 {
-	return s->full || s->len > PAGE_SIZE - 1;
+	return s->full || seq_buf_has_overflowed(&s->seq);
 }
 
 /*

kernel/trace/Makefile +1 -0

index 67d6369ddf83..edc98c72a634 100644
@@ -29,6 +29,7 @@ obj-$(CONFIG_RING_BUFFER_BENCHMARK) += ring_buffer_benchmark.o
 obj-$(CONFIG_TRACING) += trace.o
 obj-$(CONFIG_TRACING) += trace_output.o
 obj-$(CONFIG_TRACING) += trace_seq.o
+obj-$(CONFIG_TRACING) += seq_buf.o
 obj-$(CONFIG_TRACING) += trace_stat.o
 obj-$(CONFIG_TRACING) += trace_printk.o
 obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o

kernel/trace/seq_buf.c new file +341 -0

341 lines not on this page · raw message

kernel/trace/trace.c +21 ~~-18~~

index f5a435a6e8fb..dd43a0d3843a 100644
@@ -938,19 +938,20 @@ out:
 	return ret;
 }
 
+/* TODO add a seq_buf_to_buffer() */
 static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
 {
 	int len;
 
-	if (s->len <= s->readpos)
+	if (s->seq.len <= s->seq.readpos)
 		return -EBUSY;
 
-	len = s->len - s->readpos;
+	len = s->seq.len - s->seq.readpos;
 	if (cnt > len)
 		cnt = len;
-	memcpy(buf, s->buffer + s->readpos, cnt);
+	memcpy(buf, s->buffer + s->seq.readpos, cnt);
 
-	s->readpos += cnt;
+	s->seq.readpos += cnt;
 	return cnt;
 }

@@ -4314,6 +4315,8 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
 		goto out;
 	}
 
+	trace_seq_init(&iter->seq);
+
 	/*
 	 * We make a copy of the current tracer to avoid concurrent
 	 * changes on it while we are reading.

@@ -4510,18 +4513,18 @@ waitagain:
 	trace_access_lock(iter->cpu_file);
 	while (trace_find_next_entry_inc(iter) != NULL) {
 		enum print_line_t ret;
-		int len = iter->seq.len;
+		int len = iter->seq.seq.len;
 
 		ret = print_trace_line(iter);
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
 			/* don't print partial lines */
-			iter->seq.len = len;
+			iter->seq.seq.len = len;
 			break;
 		}
 		if (ret != TRACE_TYPE_NO_CONSUME)
 			trace_consume(iter);
 
-		if (iter->seq.len >= cnt)
+		if (iter->seq.seq.len >= cnt)
 			break;
 
 		/*

@@ -4537,7 +4540,7 @@ waitagain:
 
 	/* Now copy what we have to the user */
 	sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
-	if (iter->seq.readpos >= iter->seq.len)
+	if (iter->seq.seq.readpos >= iter->seq.seq.len)
 		trace_seq_init(&iter->seq);
 
 	/*

@@ -4575,16 +4578,16 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
 
 	/* Seq buffer is page-sized, exactly what we need. */
 	for (;;) {
-		count = iter->seq.len;
+		count = iter->seq.seq.len;
 		ret = print_trace_line(iter);
-		count = iter->seq.len - count;
+		count = iter->seq.seq.len - count;
 		if (rem < count) {
 			rem = 0;
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}

@@ -4665,13 +4668,13 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		/* Copy the data into the page, so we can start over. */
 		ret = trace_seq_to_buffer(&iter->seq,
 					  page_address(spd.pages[i]),
-					  iter->seq.len);
+					  iter->seq.seq.len);
 		if (ret < 0) {
 			__free_page(spd.pages[i]);
 			break;
 		}
 		spd.partial[i].offset = 0;
-		spd.partial[i].len = iter->seq.len;
+		spd.partial[i].len = iter->seq.seq.len;
 
 		trace_seq_init(&iter->seq);
 	}

@@ -5672,7 +5675,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 	cnt = ring_buffer_read_events_cpu(trace_buf->buffer, cpu);
 	trace_seq_printf(s, "read events: %ld\n", cnt);
 
-	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->len);
+	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

@@ -6635,11 +6638,11 @@ void
 trace_printk_seq(struct trace_seq *s)
 {
 	/* Probably should print a warning here. */
-	if (s->len >= TRACE_MAX_PRINT)
-		s->len = TRACE_MAX_PRINT;
+	if (s->seq.len >= TRACE_MAX_PRINT)
+		s->seq.len = TRACE_MAX_PRINT;
 
 	/* should be zero ended, but we are paranoid. */
-	s->buffer[s->len] = 0;
+	s->buffer[s->seq.len] = 0;
 
 	printk(KERN_TRACE "%s", s->buffer);

kernel/trace/trace_events.c +3 -3

index 0cc51edde3a8..33525bf6cbf5 100644
@@ -1044,7 +1044,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	mutex_unlock(&event_mutex);
 
 	if (file)
-		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

@@ -1210,7 +1210,7 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	trace_seq_init(s);
 
 	print_subsystem_event_filter(system, s);
-	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

@@ -1265,7 +1265,7 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)
 	trace_seq_init(s);
 
 	func(s);
-	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

kernel/trace/trace_functions_graph.c +3 -3

index 477a7c65cf08..19d6145f73ed 100644
@@ -1149,9 +1149,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,
 	}
 
 	/* Strip ending newline */
-	if (s->buffer[s->len - 1] == '\n') {
-		s->buffer[s->len - 1] = '\0';
-		s->len--;
+	if (s->buffer[s->seq.len - 1] == '\n') {
+		s->buffer[s->seq.len - 1] = '\0';
+		s->seq.len--;
 	}
 
 	trace_seq_puts(s, " */\n");

kernel/trace/trace_seq.c +83 ~~-89~~

312 lines not on this page · raw message

Signature · 1 line

2.1.1

Petr Mladek

Reviewed-by on v4 13/23

2014-11-14 16:27 UTC #35 lore raw

Re: [RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq

Quoted 29 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.
Link http://lkml.kernel.org/r/[email protected] open here
Tested-by Jiri Kosina <[email protected]>
Acked-by Jiri Kosina <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/seq_buf.h              |  81 +++++++++
 include/linux/trace_seq.h            |  12 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 172 +++++++++---------
 8 files changed, 538 insertions(+), 120 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c
kernel/trace/seq_buf.c new file +0 -0
new file mode 100644
index 000000000000..e9a7861595d2

[...]

Quoted 20 lines

kernel/trace/seq_buf.c new file +20 -0

+/**
+ * seq_buf_to_user - copy the squence buffer to user space
+ * @s: seq_buf descriptor
+ * @ubuf: The userspace memory location to copy to
+ * @cnt: The amount to copy
+ *
+ * Copies the sequence buffer into the userspace memory pointed to
+ * by @ubuf. It starts from the last read position (@s->readpos)
+ * and writes up to @cnt characters or till it reaches the end of
+ * the content in the buffer (@s->len), which ever comes first.
+ *
+ * On success, it returns a positive number of the number of bytes
+ * it copied.
+ *
+ * On failure it returns -EBUSY if all of the content in the
+ * sequence has been already read, which includes nothing in the
+ * sequence (@s->len == @s->readpos).
+ *
+ * Returns -EFAULT if the copy to userspace fails.
+ */

kernel/trace/seq_buf.c line 328

Quoted 10 lines

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
+{
+	int len;
+	int ret;
+
+	if (!cnt)
+		return 0;
+
+	if (s->len <= s->readpos)
+		return -EBUSY;

Ah, we should add here:

	if (seq_buf_has_overflowed(s))
		return -EINVAL;

It will be especially important after appyling "[RFC][PATCH 17/23 v4]
tracing: Have seq_buf use full buffer".

The patch will make "seq.len = seq.size + 1" when there is an
overflow. It could cause overflow in the following copy_to_user().

It is pity that I have not realized this in the earlier review. Ach, it was OK in this patch.

Quoted 21 lines

kernel/trace/seq_buf.c new file +12 -0

+	len = s->len - s->readpos;
+	if (cnt > len)
+		cnt = len;
+	ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
+	if (ret == cnt)
+		return -EFAULT;
+
+	cnt -= ret;
+
+	s->readpos += cnt;
+	return cnt;
+}

kernel/trace/trace.c +1 -0

index f5a435a6e8fb..dd43a0d3843a 100644
@@ -938,19 +938,20 @@ out:
 	return ret;
 }
 
+/* TODO add a seq_buf_to_buffer() */

kernel/trace/trace.c out: · line 949

Quoted 10 lines

kernel/trace/trace.c +2 -2

 static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
 {
 	int len;
 
-	if (s->len <= s->readpos)
+	if (s->seq.len <= s->seq.readpos)
 		return -EBUSY;
 
-	len = s->len - s->readpos;
+	len = s->seq.len - s->seq.readpos;

Similar problem is here. if (seq.len = seq.size + 1) the following memcpy might access outside of the buffer.

I am afraid that we need to get rid of all direct uses of seq.len outside of the seq_buf implemetation.

Quoted 21 lines

kernel/trace/trace.c +4 -2

 	if (cnt > len)
 		cnt = len;
-	memcpy(buf, s->buffer + s->readpos, cnt);
+	memcpy(buf, s->buffer + s->seq.readpos, cnt);
 
-	s->readpos += cnt;
+	s->seq.readpos += cnt;
 	return cnt;
 }

@@ -4314,6 +4315,8 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
 		goto out;
 	}
 
+	trace_seq_init(&iter->seq);
+
 	/*
 	 * We make a copy of the current tracer to avoid concurrent
 	 * changes on it while we are reading.

@@ -4510,18 +4513,18 @@ waitagain:
 	trace_access_lock(iter->cpu_file);

kernel/trace/trace.c waitagain: · line 4521

Quoted 10 lines

kernel/trace/trace.c +2 -2

 	while (trace_find_next_entry_inc(iter) != NULL) {
 		enum print_line_t ret;
-		int len = iter->seq.len;
+		int len = iter->seq.seq.len;
 
 		ret = print_trace_line(iter);
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
 			/* don't print partial lines */
-			iter->seq.len = len;
+			iter->seq.seq.len = len;

It is fine here because it just restore the original value but...

Quoted 20 lines

kernel/trace/trace.c +2 -2

 			break;
 		}
 		if (ret != TRACE_TYPE_NO_CONSUME)
 			trace_consume(iter);
 
-		if (iter->seq.len >= cnt)
+		if (iter->seq.seq.len >= cnt)
 			break;
 
 		/*

@@ -4537,7 +4540,7 @@ waitagain:
 
 	/* Now copy what we have to the user */
 	sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
-	if (iter->seq.readpos >= iter->seq.len)
+	if (iter->seq.seq.readpos >= iter->seq.seq.len)
 		trace_seq_init(&iter->seq);
 
 	/*

kernel/trace/trace.c tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) · line 4583

Quoted 8 lines

kernel/trace/trace.c +2 -2

@@ -4575,16 +4578,16 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
 
 	/* Seq buffer is page-sized, exactly what we need. */
 	for (;;) {
-		count = iter->seq.len;
+		count = iter->seq.seq.len;
 		ret = print_trace_line(iter);
-		count = iter->seq.len - count;
+		count = iter->seq.seq.len - count;

this looks safe as well;

Quoted 29 lines

kernel/trace/trace.c +4 -4

 		if (rem < count) {
 			rem = 0;
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}

@@ -4665,13 +4668,13 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		/* Copy the data into the page, so we can start over. */
 		ret = trace_seq_to_buffer(&iter->seq,
 					  page_address(spd.pages[i]),
-					  iter->seq.len);
+					  iter->seq.seq.len);
 		if (ret < 0) {
 			__free_page(spd.pages[i]);
 			break;
 		}
 		spd.partial[i].offset = 0;
-		spd.partial[i].len = iter->seq.len;
+		spd.partial[i].len = iter->seq.seq.len;
 
 		trace_seq_init(&iter->seq);
 	}

kernel/trace/trace.c tracing_stats_read(struct file *filp, char __user *ubuf, · line 5678

Quoted 5 lines

kernel/trace/trace.c +1 -1

@@ -5672,7 +5675,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 	cnt = ring_buffer_read_events_cpu(trace_buf->buffer, cpu);
 	trace_seq_printf(s, "read events: %ld\n", cnt);
 
-	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->len);
+	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->seq.len);

but this looks dangerous.

kernel/trace/trace.c void · line 6642

Quoted 10 lines

kernel/trace/trace.c +2 -2

 	kfree(s);

@@ -6635,11 +6638,11 @@ void
 trace_printk_seq(struct trace_seq *s)
 {
 	/* Probably should print a warning here. */
-	if (s->len >= TRACE_MAX_PRINT)
-		s->len = TRACE_MAX_PRINT;
+	if (s->seq.len >= TRACE_MAX_PRINT)
+		s->seq.len = TRACE_MAX_PRINT;

looks safe

Quoted 11 lines

kernel/trace/trace.c +1 -1

 	/* should be zero ended, but we are paranoid. */
-	s->buffer[s->len] = 0;
+	s->buffer[s->seq.len] = 0;
 
 	printk(KERN_TRACE "%s", s->buffer);

kernel/trace/trace_events.c +0 -0

index 0cc51edde3a8..33525bf6cbf5 100644

kernel/trace/trace_events.c event_filter_read(struct file *filp, char __user *ubuf, size_t cnt, · line 1047

Quoted 5 lines

kernel/trace/trace_events.c +1 -1

@@ -1044,7 +1044,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	mutex_unlock(&event_mutex);
 
 	if (file)
-		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);

dangerous...

[...]

Quoted 4 lines

kernel/trace/trace_seq.c +0 -0
index e54c0a1fb3f0..3c63b619d6b7 100644

[...]

Quoted 16 lines

kernel/trace/trace_seq.c +4 -3

@@ -72,24 +82,24 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s)
  */
 void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;
 	va_list ap;
-	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return;
 
+	__trace_seq_init(s);
+
 	va_start(ap, fmt);

kernel/trace/trace_seq.c int trace_print_seq(struct seq_file *m, struct trace_seq *s) · line 101

Quoted 10 lines

kernel/trace/trace_seq.c +3 -2

-	ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
+	seq_buf_vprintf(&s->seq, fmt, ap);
 	va_end(ap);
 
 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return;

The return is redundant here.

Quoted 37 lines

kernel/trace/trace_seq.c +11 -8

 	}
-
-	s->len += ret;
 }
 EXPORT_SYMBOL_GPL(trace_seq_printf);

@@ -104,14 +114,19 @@ EXPORT_SYMBOL_GPL(trace_seq_printf);
 void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 		      int nmaskbits)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
-	int ret;
+	unsigned int save_len = s->seq.len;
 
-	if (s->full || !len)
+	if (s->full)
 		return;
 
-	ret = bitmap_scnprintf(s->buffer + s->len, len, maskp, nmaskbits);
-	s->len += ret;
+	__trace_seq_init(s);
+
+	seq_buf_bitmask(&s->seq, maskp, nmaskbits);
+
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
+		s->full = 1;
+	}
 }
 EXPORT_SYMBOL_GPL(trace_seq_bitmask);

@@ -128,21 +143,22 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);
  */
 void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 154

Quoted 10 lines

kernel/trace/trace_seq.c +4 -2

 	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return;
 
-	ret = vsnprintf(s->buffer + s->len, len, fmt, args);
+	__trace_seq_init(s);
+
+	ret = seq_buf_vprintf(&s->seq, fmt, args);

The ret value is not used.

kernel/trace/trace_seq.c int trace_print_seq(struct seq_file *m, struct trace_seq *s) · line 101

Quoted 6 lines

kernel/trace/trace_seq.c +2 -1

 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return;

The return is redundant.

The above mentioned potential overflows happen only if we apply "[RFC][PATCH 17/23 v4]
tracing: Have seq_buf use full buffer". The code is safe at this stage. The other problems are minor.

If you decide to address the potential overflows in another patch, feel free to add to this one:

Best Regards,
Petr

Steven Rostedt

author maintainer

2014-11-14 17:19 UTC #36 lore raw

Re: [RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq

Quoted 67 lines · On Fri, 14 Nov 2014 17:26:52 +0100 Petr Mladek <[email protected]> wrote:

Quoted 29 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.
Link http://lkml.kernel.org/r/[email protected] open here
Tested-by Jiri Kosina <[email protected]>
Acked-by Jiri Kosina <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/seq_buf.h              |  81 +++++++++
 include/linux/trace_seq.h            |  12 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 172 +++++++++---------
 8 files changed, 538 insertions(+), 120 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c
kernel/trace/seq_buf.c new file +0 -0
new file mode 100644
index 000000000000..e9a7861595d2

[...]

Quoted 20 lines

kernel/trace/seq_buf.c new file +20 -0

+/**
+ * seq_buf_to_user - copy the squence buffer to user space
+ * @s: seq_buf descriptor
+ * @ubuf: The userspace memory location to copy to
+ * @cnt: The amount to copy
+ *
+ * Copies the sequence buffer into the userspace memory pointed to
+ * by @ubuf. It starts from the last read position (@s->readpos)
+ * and writes up to @cnt characters or till it reaches the end of
+ * the content in the buffer (@s->len), which ever comes first.
+ *
+ * On success, it returns a positive number of the number of bytes
+ * it copied.
+ *
+ * On failure it returns -EBUSY if all of the content in the
+ * sequence has been already read, which includes nothing in the
+ * sequence (@s->len == @s->readpos).
+ *
+ * Returns -EFAULT if the copy to userspace fails.
+ */

kernel/trace/seq_buf.c line 328

Quoted 10 lines

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
+{
+	int len;
+	int ret;
+
+	if (!cnt)
+		return 0;
+
+	if (s->len <= s->readpos)
+		return -EBUSY;

Ah, we should add here:

	if (seq_buf_has_overflowed(s))
		return -EINVAL;

Quoted 8 lines

It will be especially important after appyling "[RFC][PATCH 17/23 v4]
tracing: Have seq_buf use full buffer".
The patch will make "seq.len = seq.size + 1" when there is an
overflow. It could cause overflow in the following copy_to_user().
It is pity that I have not realized this in the earlier review. Ach, it was OK in this patch.

Actually, I wouldn't use -EINVAL but instead use the seq_buf_used() helper function:

	len = seq_buf_used(s);

Quoted 30 lines

kernel/trace/seq_buf.c new file +12 -0

+	len = s->len - s->readpos;
+	if (cnt > len)
+		cnt = len;
+	ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
+	if (ret == cnt)
+		return -EFAULT;
+
+	cnt -= ret;
+
+	s->readpos += cnt;
+	return cnt;
+}

kernel/trace/trace.c +2 -1

index f5a435a6e8fb..dd43a0d3843a 100644
@@ -938,19 +938,20 @@ out:
 	return ret;
 }
 
+/* TODO add a seq_buf_to_buffer() */
 static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
 {
 	int len;
 
-	if (s->len <= s->readpos)
+	if (s->seq.len <= s->seq.readpos)
 		return -EBUSY;

Quoted 8 lines

kernel/trace/trace.c out: · line 949
Quoted 2 lines
kernel/trace/trace.c +1 -1
-	len = s->len - s->readpos;
+	len = s->seq.len - s->seq.readpos;
Similar problem is here. if (seq.len = seq.size + 1) the following memcpy might access outside of the buffer.
I am afraid that we need to get rid of all direct uses of seq.len outside of the seq_buf implemetation.

Yep, I agree. The seq_buf_used() should be used instead.

Quoted 27 lines

kernel/trace/trace.c +5 -3

 	if (cnt > len)
 		cnt = len;
-	memcpy(buf, s->buffer + s->readpos, cnt);
+	memcpy(buf, s->buffer + s->seq.readpos, cnt);
 
-	s->readpos += cnt;
+	s->seq.readpos += cnt;
 	return cnt;
 }

@@ -4314,6 +4315,8 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
 		goto out;
 	}
 
+	trace_seq_init(&iter->seq);
+
 	/*
 	 * We make a copy of the current tracer to avoid concurrent
 	 * changes on it while we are reading.

@@ -4510,18 +4513,18 @@ waitagain:
 	trace_access_lock(iter->cpu_file);
 	while (trace_find_next_entry_inc(iter) != NULL) {
 		enum print_line_t ret;
-		int len = iter->seq.len;
+		int len = iter->seq.seq.len;

Quoted 7 lines

kernel/trace/trace.c waitagain: · line 4521
Quoted 5 lines
kernel/trace/trace.c +1 -1
 		ret = print_trace_line(iter);
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
 			/* don't print partial lines */
-			iter->seq.len = len;
+			iter->seq.seq.len = len;
It is fine here because it just restore the original value but...

but?

Quoted 22 lines

kernel/trace/trace.c +2 -2

 			break;
 		}
 		if (ret != TRACE_TYPE_NO_CONSUME)
 			trace_consume(iter);
 
-		if (iter->seq.len >= cnt)
+		if (iter->seq.seq.len >= cnt)
 			break;
 
 		/*

@@ -4537,7 +4540,7 @@ waitagain:
 
 	/* Now copy what we have to the user */
 	sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
-	if (iter->seq.readpos >= iter->seq.len)
+	if (iter->seq.seq.readpos >= iter->seq.seq.len)
 		trace_seq_init(&iter->seq);
 
 	/*

@@ -4575,16 +4578,16 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)

Quoted 9 lines

kernel/trace/trace.c tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) · line 4583
Quoted 7 lines
kernel/trace/trace.c +2 -2
 	/* Seq buffer is page-sized, exactly what we need. */
 	for (;;) {
-		count = iter->seq.len;
+		count = iter->seq.seq.len;
 		ret = print_trace_line(iter);
-		count = iter->seq.len - count;
+		count = iter->seq.seq.len - count;
this looks safe as well;

This should use the seq_buf_used() as well.

Quoted 33 lines

kernel/trace/trace.c +4 -4

 		if (rem < count) {
 			rem = 0;
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}

@@ -4665,13 +4668,13 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		/* Copy the data into the page, so we can start over. */
 		ret = trace_seq_to_buffer(&iter->seq,
 					  page_address(spd.pages[i]),
-					  iter->seq.len);
+					  iter->seq.seq.len);
 		if (ret < 0) {
 			__free_page(spd.pages[i]);
 			break;
 		}
 		spd.partial[i].offset = 0;
-		spd.partial[i].len = iter->seq.len;
+		spd.partial[i].len = iter->seq.seq.len;
 
 		trace_seq_init(&iter->seq);
 	}

@@ -5672,7 +5675,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 	cnt = ring_buffer_read_events_cpu(trace_buf->buffer, cpu);
 	trace_seq_printf(s, "read events: %ld\n", cnt);

Quoted 4 lines

kernel/trace/trace.c tracing_stats_read(struct file *filp, char __user *ubuf, · line 5678
Quoted 2 lines
kernel/trace/trace.c +1 -1
-	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->len);
+	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->seq.len);
but this looks dangerous.

Agreed.

Quoted 13 lines

kernel/trace/trace.c void · line 6642

Quoted 11 lines

kernel/trace/trace.c +2 -2

 	kfree(s);

@@ -6635,11 +6638,11 @@ void
 trace_printk_seq(struct trace_seq *s)
 {
 	/* Probably should print a warning here. */
-	if (s->len >= TRACE_MAX_PRINT)
-		s->len = TRACE_MAX_PRINT;
+	if (s->seq.len >= TRACE_MAX_PRINT)
+		s->seq.len = TRACE_MAX_PRINT;

looks safe

Yeah.

Quoted 15 lines

kernel/trace/trace.c +1 -1

 	/* should be zero ended, but we are paranoid. */
-	s->buffer[s->len] = 0;
+	s->buffer[s->seq.len] = 0;
 
 	printk(KERN_TRACE "%s", s->buffer);

kernel/trace/trace_events.c +0 -0

index 0cc51edde3a8..33525bf6cbf5 100644
@@ -1044,7 +1044,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	mutex_unlock(&event_mutex);

Quoted 5 lines

kernel/trace/trace_events.c event_filter_read(struct file *filp, char __user *ubuf, size_t cnt, · line 1047
Quoted 3 lines
kernel/trace/trace_events.c +1 -1
 	if (file)
-		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
dangerous...

Agreed.

Quoted 28 lines

[...]

Quoted 4 lines

kernel/trace/trace_seq.c +0 -0
index e54c0a1fb3f0..3c63b619d6b7 100644

[...]

Quoted 20 lines

kernel/trace/trace_seq.c +5 -4

@@ -72,24 +82,24 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s)
  */
 void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;
 	va_list ap;
-	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return;
 
+	__trace_seq_init(s);
+
 	va_start(ap, fmt);
-	ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
+	seq_buf_vprintf(&s->seq, fmt, ap);
 	va_end(ap);

Quoted 8 lines

kernel/trace/trace_seq.c int trace_print_seq(struct seq_file *m, struct trace_seq *s) · line 101
Quoted 6 lines
kernel/trace/trace_seq.c +2 -1
 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return;
The return is redundant here.

Heh, yep. Doing a lot of rather trivial updates makes one do stupid things like this.

Quoted 44 lines

kernel/trace/trace_seq.c +12 -9

 	}
-
-	s->len += ret;
 }
 EXPORT_SYMBOL_GPL(trace_seq_printf);

@@ -104,14 +114,19 @@ EXPORT_SYMBOL_GPL(trace_seq_printf);
 void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 		      int nmaskbits)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
-	int ret;
+	unsigned int save_len = s->seq.len;
 
-	if (s->full || !len)
+	if (s->full)
 		return;
 
-	ret = bitmap_scnprintf(s->buffer + s->len, len, maskp, nmaskbits);
-	s->len += ret;
+	__trace_seq_init(s);
+
+	seq_buf_bitmask(&s->seq, maskp, nmaskbits);
+
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
+		s->full = 1;
+	}
 }
 EXPORT_SYMBOL_GPL(trace_seq_bitmask);

@@ -128,21 +143,22 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);
  */
 void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;
 	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return;

Quoted 6 lines

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 154
Quoted 4 lines
kernel/trace/trace_seq.c +3 -1
-	ret = vsnprintf(s->buffer + s->len, len, fmt, args);
+	__trace_seq_init(s);
+
+	ret = seq_buf_vprintf(&s->seq, fmt, args);
The ret value is not used.

Yep, I'll nuke it.

Quoted 9 lines

kernel/trace/trace_seq.c int trace_print_seq(struct seq_file *m, struct trace_seq *s) · line 101
Quoted 7 lines
kernel/trace/trace_seq.c +2 -1
 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return;
The return is redundant.

Yep, Just missed it.

Quoted 8 lines

The above mentioned potential overflows happen only if we apply "[RFC][PATCH 17/23 v4]
tracing: Have seq_buf use full buffer". The code is safe at this stage. The other problems are minor.
If you decide to address the potential overflows in another patch, feel free to add to this one:

Yeah, I'll do that after the seq_buf_used() is introduced.

Quoted 2 lines

Reviewed-by Petr Mladek <[email protected]>

Thanks!

-- Steve

Quoted 3 lines

Best Regards,
Petr

Steven Rostedt

author maintainer Tested-by Jiri Kosina Acked-by Jiri Kosina Reviewed-by Petr Mladek

2014-11-14 17:23 UTC #37 lore raw

Re: [RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq

In reply to Petr Mladek

Quoted 11 lines · On Fri, 14 Nov 2014 17:26:52 +0100 Petr Mladek <[email protected]> wrote:

The return is redundant.
The above mentioned potential overflows happen only if we apply "[RFC][PATCH 17/23 v4]
tracing: Have seq_buf use full buffer". The code is safe at this stage. The other problems are minor.
If you decide to address the potential overflows in another patch, feel free to add to this one:
Reviewed-by Petr Mladek <[email protected]>

Thanks.

I removed the redundant returns and the unused ret variable. I left the dangerous copies in as I plan on addressing that after I introduce seq_buf_used().

-- Steve

Here's the new patch:

Quoted 1 line

From 17dc0a77777556c998849dabe27d367a5702a967 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 25 Jun 2014 15:54:42 -0400
Subject: [PATCH] tracing: Create seq_buf layer in trace_seq

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h              |  81 +++++++++
 include/linux/trace_seq.h            |  12 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 177 +++++++++---------
 8 files changed, 538 insertions(+), 125 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c

include/linux/seq_buf.h new file +81 -0

new file mode 100644
index 000000000000..4f7a96a9d71a
@@ -0,0 +1,81 @@
+#ifndef _LINUX_SEQ_BUF_H
+#define _LINUX_SEQ_BUF_H
+
+#include <linux/fs.h>
+
+/*
+ * Trace sequences are used to allow a function to call several other functions
+ * to create a string of data to use.
+ */
+
+/**
+ * seq_buf - seq buffer structure
+ * @buffer:	pointer to the buffer
+ * @size:	size of the buffer
+ * @len:	the amount of data inside the buffer
+ * @readpos:	The next position to read in the buffer.
+ */
+struct seq_buf {
+	unsigned char		*buffer;
+	unsigned int		size;
+	unsigned int		len;
+	unsigned int		readpos;
+};
+
+static inline void
+seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
+{
+	s->buffer = buf;
+	s->size = size;
+	s->len = 0;
+	s->readpos = 0;
+}
+
+/*
+ * seq_buf have a buffer that might overflow. When this happens
+ * the len and size are set to be equal.
+ */
+static inline bool
+seq_buf_has_overflowed(struct seq_buf *s)
+{
+	return s->len == s->size;
+}
+
+static inline void
+seq_buf_set_overflow(struct seq_buf *s)
+{
+	s->len = s->size;
+}
+
+/*
+ * How much buffer is left on the seq_buf?
+ */
+static inline unsigned int
+seq_buf_buffer_left(struct seq_buf *s)
+{
+	if (seq_buf_has_overflowed(s))
+		return 0;
+
+	return (s->size - 1) - s->len;
+}
+
+extern __printf(2, 3)
+int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
+extern __printf(2, 0)
+int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args);
+extern int
+seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s);
+extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,
+			   int cnt);
+extern int seq_buf_puts(struct seq_buf *s, const char *str);
+extern int seq_buf_putc(struct seq_buf *s, unsigned char c);
+extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
+extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+			      unsigned int len);
+extern int seq_buf_path(struct seq_buf *s, const struct path *path);
+
+extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+			   int nmaskbits);
+
+#endif /* _LINUX_SEQ_BUF_H */

include/linux/trace_seq.h +5 -7

index db8a73224f1a..85d37106be3d 100644
@@ -1,7 +1,7 @@
 #ifndef _LINUX_TRACE_SEQ_H
 #define _LINUX_TRACE_SEQ_H
 
-#include <linux/fs.h>
+#include <linux/seq_buf.h>
 
 #include <asm/page.h>

@@ -12,16 +12,14 @@
 
 struct trace_seq {
 	unsigned char		buffer[PAGE_SIZE];
-	unsigned int		len;
-	unsigned int		readpos;
+	struct seq_buf		seq;
 	int			full;
 };
 
 static inline void
 trace_seq_init(struct trace_seq *s)
 {
-	s->len = 0;
-	s->readpos = 0;
+	seq_buf_init(&s->seq, s->buffer, PAGE_SIZE);
 	s->full = 0;
 }

@@ -37,7 +35,7 @@ trace_seq_init(struct trace_seq *s)
 static inline unsigned char *
 trace_seq_buffer_ptr(struct trace_seq *s)
 {
-	return s->buffer + s->len;
+	return s->buffer + s->seq.len;
 }
 
 /**

@@ -49,7 +47,7 @@ trace_seq_buffer_ptr(struct trace_seq *s)
  */
 static inline bool trace_seq_has_overflowed(struct trace_seq *s)
 {
-	return s->full || s->len > PAGE_SIZE - 1;
+	return s->full || seq_buf_has_overflowed(&s->seq);
 }
 
 /*

kernel/trace/Makefile +1 -0

index 67d6369ddf83..edc98c72a634 100644
@@ -29,6 +29,7 @@ obj-$(CONFIG_RING_BUFFER_BENCHMARK) += ring_buffer_benchmark.o
 obj-$(CONFIG_TRACING) += trace.o
 obj-$(CONFIG_TRACING) += trace_output.o
 obj-$(CONFIG_TRACING) += trace_seq.o
+obj-$(CONFIG_TRACING) += seq_buf.o
 obj-$(CONFIG_TRACING) += trace_stat.o
 obj-$(CONFIG_TRACING) += trace_printk.o
 obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o

kernel/trace/seq_buf.c new file +341 -0

341 lines not on this page · raw message

kernel/trace/trace.c +21 ~~-18~~

index 3ce3c4ccfc94..7d7a07e9b9e9 100644
@@ -939,19 +939,20 @@ out:
 	return ret;
 }
 
+/* TODO add a seq_buf_to_buffer() */
 static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
 {
 	int len;
 
-	if (s->len <= s->readpos)
+	if (s->seq.len <= s->seq.readpos)
 		return -EBUSY;
 
-	len = s->len - s->readpos;
+	len = s->seq.len - s->seq.readpos;
 	if (cnt > len)
 		cnt = len;
-	memcpy(buf, s->buffer + s->readpos, cnt);
+	memcpy(buf, s->buffer + s->seq.readpos, cnt);
 
-	s->readpos += cnt;
+	s->seq.readpos += cnt;
 	return cnt;
 }

@@ -4315,6 +4316,8 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp)
 		goto out;
 	}
 
+	trace_seq_init(&iter->seq);
+
 	/*
 	 * We make a copy of the current tracer to avoid concurrent
 	 * changes on it while we are reading.

@@ -4511,18 +4514,18 @@ waitagain:
 	trace_access_lock(iter->cpu_file);
 	while (trace_find_next_entry_inc(iter) != NULL) {
 		enum print_line_t ret;
-		int len = iter->seq.len;
+		int len = iter->seq.seq.len;
 
 		ret = print_trace_line(iter);
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
 			/* don't print partial lines */
-			iter->seq.len = len;
+			iter->seq.seq.len = len;
 			break;
 		}
 		if (ret != TRACE_TYPE_NO_CONSUME)
 			trace_consume(iter);
 
-		if (iter->seq.len >= cnt)
+		if (iter->seq.seq.len >= cnt)
 			break;
 
 		/*

@@ -4538,7 +4541,7 @@ waitagain:
 
 	/* Now copy what we have to the user */
 	sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
-	if (iter->seq.readpos >= iter->seq.len)
+	if (iter->seq.seq.readpos >= iter->seq.seq.len)
 		trace_seq_init(&iter->seq);
 
 	/*

@@ -4576,16 +4579,16 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
 
 	/* Seq buffer is page-sized, exactly what we need. */
 	for (;;) {
-		count = iter->seq.len;
+		count = iter->seq.seq.len;
 		ret = print_trace_line(iter);
-		count = iter->seq.len - count;
+		count = iter->seq.seq.len - count;
 		if (rem < count) {
 			rem = 0;
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}
 		if (ret == TRACE_TYPE_PARTIAL_LINE) {
-			iter->seq.len -= count;
+			iter->seq.seq.len -= count;
 			break;
 		}

@@ -4666,13 +4669,13 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		/* Copy the data into the page, so we can start over. */
 		ret = trace_seq_to_buffer(&iter->seq,
 					  page_address(spd.pages[i]),
-					  iter->seq.len);
+					  iter->seq.seq.len);
 		if (ret < 0) {
 			__free_page(spd.pages[i]);
 			break;
 		}
 		spd.partial[i].offset = 0;
-		spd.partial[i].len = iter->seq.len;
+		spd.partial[i].len = iter->seq.seq.len;
 
 		trace_seq_init(&iter->seq);
 	}

@@ -5673,7 +5676,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 	cnt = ring_buffer_read_events_cpu(trace_buf->buffer, cpu);
 	trace_seq_printf(s, "read events: %ld\n", cnt);
 
-	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->len);
+	count = simple_read_from_buffer(ubuf, count, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

@@ -6636,11 +6639,11 @@ void
 trace_printk_seq(struct trace_seq *s)
 {
 	/* Probably should print a warning here. */
-	if (s->len >= TRACE_MAX_PRINT)
-		s->len = TRACE_MAX_PRINT;
+	if (s->seq.len >= TRACE_MAX_PRINT)
+		s->seq.len = TRACE_MAX_PRINT;
 
 	/* should be zero ended, but we are paranoid. */
-	s->buffer[s->len] = 0;
+	s->buffer[s->seq.len] = 0;
 
 	printk(KERN_TRACE "%s", s->buffer);

kernel/trace/trace_events.c +3 -3

index f9d0cbe014b7..4d0067dd7f88 100644
@@ -1044,7 +1044,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	mutex_unlock(&event_mutex);
 
 	if (file)
-		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

@@ -1210,7 +1210,7 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	trace_seq_init(s);
 
 	print_subsystem_event_filter(system, s);
-	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

@@ -1265,7 +1265,7 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)
 	trace_seq_init(s);
 
 	func(s);
-	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
+	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
 
 	kfree(s);

kernel/trace/trace_functions_graph.c +3 -3

index 100288d10e1f..6d1342ae7a44 100644
@@ -1154,9 +1154,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,
 	}
 
 	/* Strip ending newline */
-	if (s->buffer[s->len - 1] == '\n') {
-		s->buffer[s->len - 1] = '\0';
-		s->len--;
+	if (s->buffer[s->seq.len - 1] == '\n') {
+		s->buffer[s->seq.len - 1] = '\0';
+		s->seq.len--;
 	}
 
 	trace_seq_puts(s, " */\n");

kernel/trace/trace_seq.c +83 ~~-94~~

314 lines not on this page · raw message

Signature · 1 line

1.8.1.4

Steven Rostedt

author maintainer

2014-11-14 01:18 UTC #38 lore raw

RFC PATCH v4 14/23 tracing: Convert seq_buf_path() to be like seq_path()

From: "Steven Rostedt (Red Hat)" <[email protected]>

Rewrite seq_buf_path() like it is done in seq_path() and allow it to accept any escape character instead of just "\n".

Making seq_buf_path() like seq_path() will help prevent problems when converting seq_file to use the seq_buf logic.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h  |  2 +-
 kernel/trace/seq_buf.c   | 28 ++++++++++++++++------------
 kernel/trace/trace_seq.c |  4 ++--
 3 files changed, 19 insertions(+), 15 deletions(-)

include/linux/seq_buf.h +1 -1

index 4f7a96a9d71a..38770688a627 100644
@@ -73,7 +73,7 @@ extern int seq_buf_putc(struct seq_buf *s, unsigned char c);
 extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
 extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
 			      unsigned int len);
-extern int seq_buf_path(struct seq_buf *s, const struct path *path);
+extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc);
 
 extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
 			   int nmaskbits);

kernel/trace/seq_buf.c +16 ~~-12~~

index e9a7861595d2..7dac34d1235b 100644
@@ -272,28 +272,32 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
  * seq_buf_path - copy a path into the sequence buffer
  * @s: seq_buf descriptor
  * @path: path to write into the sequence buffer.
+ * @esc: set of characters to escape in the output
  *
  * Write a path name into the sequence buffer.
  *
- * Returns zero on success, -1 on overflow
+ * Returns the number of written bytes on success, -1 on overflow
  */
-int seq_buf_path(struct seq_buf *s, const struct path *path)
+int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 {
-	unsigned int len = seq_buf_buffer_left(s);
-	unsigned char *p;
+	char *buf = s->buffer + s->len;
+	size_t size = seq_buf_buffer_left(s);
+	int res = -1;
 
 	WARN_ON(s->size == 0);
 
-	p = d_path(path, s->buffer + s->len, len);
-	if (!IS_ERR(p)) {
-		p = mangle_path(s->buffer + s->len, p, "\n");
-		if (p) {
-			s->len = p - s->buffer;
-			return 0;
+	if (size) {
+		char *p = d_path(path, buf, size);
+		if (!IS_ERR(p)) {
+			char *end = mangle_path(buf, p, esc);
+			if (end)
+				res = end - buf;
 		}
 	}
-	seq_buf_set_overflow(s);
-	return -1;
+	if (res > 0)
+		s->len += res;
+
+	return res;
 }
 
 /**

kernel/trace/trace_seq.c +2 -2

index 3c63b619d6b7..475412e31de5 100644
@@ -342,7 +342,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)
 		return 0;
 	}
 
-	ret = seq_buf_path(&s->seq, path);
+	ret = seq_buf_path(&s->seq, path, "\n");
 
 	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
 		s->seq.len = save_len;

@@ -350,7 +350,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)
 		return 0;
 	}
 
-	return ret;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(trace_seq_path);

Signature · 1 line

2.1.1

Petr Mladek

2014-11-14 16:53 UTC #39 lore raw

Re: [RFC][PATCH 14/23 v4] tracing: Convert seq_buf_path() to be like seq_path()

Quoted 96 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

Rewrite seq_buf_path() like it is done in seq_path() and allow it to accept any escape character instead of just "\n".

Making seq_buf_path() like seq_path() will help prevent problems when converting seq_file to use the seq_buf logic.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h  |  2 +-
 kernel/trace/seq_buf.c   | 28 ++++++++++++++++------------
 kernel/trace/trace_seq.c |  4 ++--
 3 files changed, 19 insertions(+), 15 deletions(-)

include/linux/seq_buf.h +1 -1

index 4f7a96a9d71a..38770688a627 100644
@@ -73,7 +73,7 @@ extern int seq_buf_putc(struct seq_buf *s, unsigned char c);
 extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
 extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
 			      unsigned int len);
-extern int seq_buf_path(struct seq_buf *s, const struct path *path);
+extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc);
 
 extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
 			   int nmaskbits);

kernel/trace/seq_buf.c +16 ~~-12~~

index e9a7861595d2..7dac34d1235b 100644
@@ -272,28 +272,32 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
  * seq_buf_path - copy a path into the sequence buffer
  * @s: seq_buf descriptor
  * @path: path to write into the sequence buffer.
+ * @esc: set of characters to escape in the output
  *
  * Write a path name into the sequence buffer.
  *
- * Returns zero on success, -1 on overflow
+ * Returns the number of written bytes on success, -1 on overflow
  */
-int seq_buf_path(struct seq_buf *s, const struct path *path)
+int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 {
-	unsigned int len = seq_buf_buffer_left(s);
-	unsigned char *p;
+	char *buf = s->buffer + s->len;
+	size_t size = seq_buf_buffer_left(s);
+	int res = -1;
 
 	WARN_ON(s->size == 0);
 
-	p = d_path(path, s->buffer + s->len, len);
-	if (!IS_ERR(p)) {
-		p = mangle_path(s->buffer + s->len, p, "\n");
-		if (p) {
-			s->len = p - s->buffer;
-			return 0;
+	if (size) {
+		char *p = d_path(path, buf, size);
+		if (!IS_ERR(p)) {
+			char *end = mangle_path(buf, p, esc);
+			if (end)
+				res = end - buf;
 		}
 	}
-	seq_buf_set_overflow(s);
-	return -1;
+	if (res > 0)
+		s->len += res;
+
+	return res;
 }
 
 /**

kernel/trace/trace_seq.c +1 -1

index 3c63b619d6b7..475412e31de5 100644
@@ -342,7 +342,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)
 		return 0;
 	}
 
-	ret = seq_buf_path(&s->seq, path);
+	ret = seq_buf_path(&s->seq, path, "\n");
 
 	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
 		s->seq.len = save_len;

kernel/trace/trace_seq.c int trace_seq_path(struct trace_seq *s, const struct path *path) · line 353

Quoted 5 lines

kernel/trace/trace_seq.c +1 -1

@@ -350,7 +350,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)
 		return 0;
 	}
 
-	return ret;
+	return 1;

Ah, I have just realized that this should return -1 when seq_buf_path() returns -1.

Note that set_buf_path() does not longer calls seq_buf_set_overflow(s) when the path handling fails. Therefore the above check for overflow will probably never happen.

Best Regards,
Petr

Quoted 7 lines

kernel/trace/trace_seq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(trace_seq_path);
 
Signature · 1 line
2.1.1

Steven Rostedt

author maintainer

2014-11-14 17:47 UTC #40 lore raw

Re: [RFC][PATCH 14/23 v4] tracing: Convert seq_buf_path() to be like seq_path()

Quoted 18 lines · On Fri, 14 Nov 2014 17:53:08 +0100 Petr Mladek <[email protected]> wrote:

Quoted 18 lines

/**

kernel/trace/trace_seq.c +1 -1

index 3c63b619d6b7..475412e31de5 100644
@@ -342,7 +342,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)
 		return 0;
 	}
 
-	ret = seq_buf_path(&s->seq, path);
+	ret = seq_buf_path(&s->seq, path, "\n");
 
 	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
 		s->seq.len = save_len;

@@ -350,7 +350,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)
 		return 0;
 	}

Quoted 5 lines

kernel/trace/trace_seq.c int trace_seq_path(struct trace_seq *s, const struct path *path) · line 353
Quoted 2 lines
kernel/trace/trace_seq.c +1 -1
-	return ret;
+	return 1;
Ah, I have just realized that this should return -1 when seq_buf_path() returns -1.

It really doesn't matter as long as it's not zero. The one user just tests if the return is zero or not.

trace_seq() doesn't have to follow all the same rules as seq_buf() does, as seq_buf() is more in line with seq_file().

Quoted 4 lines

Note that set_buf_path() does not longer calls seq_buf_set_overflow(s) when the path handling fails. Therefore the above check for overflow will probably never happen.

This is in a transitional state of the patch series. The final version of seq_buf_path() will call seq_buf_commit() which will put the seq_buf in an overflow state if it isn't correct. Trying to fix this version here will probably just cause me to make another stupid mistake and ruin the final version ;-)

-- Steve

Quoted 11 lines

Best Regards,
Petr
Quoted 7 lines
kernel/trace/trace_seq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(trace_seq_path);
 
Signature · 1 line
2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:16 UTC #41 lore raw

RFC PATCH v4 15/23 tracing: Convert seq_buf fields to be like seq_file fields

From: "Steven Rostedt (Red Hat)" <[email protected]>

In facilitating the conversion of seq_file to use seq_buf, have the seq_buf fields match the types used by seq_file.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

include/linux/seq_buf.h +4 -4

index 38770688a627..d14dc9023dde 100644
@@ -16,10 +16,10 @@
  * @readpos:	The next position to read in the buffer.
  */
 struct seq_buf {
-	unsigned char		*buffer;
-	unsigned int		size;
-	unsigned int		len;
-	unsigned int		readpos;
+	char			*buffer;
+	size_t			size;
+	size_t			len;
+	loff_t			readpos;
 };
 
 static inline void

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:17 UTC #42 lore raw

RFC PATCH v4 16/23 tracing: Add a seq_buf_clear() helper and clear len and readpos in init

From: "Steven Rostedt (Red Hat)" <[email protected]>

Add a helper function seq_buf_clear() that resets the len and readpos fields of a seq_buf. Currently it is only used in the seq_buf_init() but will be used later when updating the seq_file code.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

include/linux/seq_buf.h +7 -2

index d14dc9023dde..5d91262433e2 100644
@@ -22,13 +22,18 @@ struct seq_buf {
 	loff_t			readpos;
 };
 
+static inline void seq_buf_clear(struct seq_buf *s)
+{
+	s->len = 0;
+	s->readpos = 0;
+}
+
 static inline void
 seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
 {
 	s->buffer = buf;
 	s->size = size;
-	s->len = 0;
-	s->readpos = 0;
+	seq_buf_clear(s);
 }
 
 /*

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:16 UTC #43 lore raw

RFC PATCH v4 17/23 tracing: Have seq_buf use full buffer

From: "Steven Rostedt (Red Hat)" <[email protected]>

Currently seq_buf is full when all but one byte of the buffer is filled. Change it so that the seq_buf is full when all of the buffer is filled.

Some of the functions would fill the buffer completely and report everything was fine. This was inconsistent with the max of size - 1. Changing this to be max of size makes all functions consistent.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h |  6 +++---
 kernel/trace/seq_buf.c  | 19 +++++++++++--------
 2 files changed, 14 insertions(+), 11 deletions(-)

include/linux/seq_buf.h +3 -3

index 5d91262433e2..581c1fc733c3 100644
@@ -43,13 +43,13 @@ seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
 static inline bool
 seq_buf_has_overflowed(struct seq_buf *s)
 {
-	return s->len == s->size;
+	return s->len > s->size;
 }
 
 static inline void
 seq_buf_set_overflow(struct seq_buf *s)
 {
-	s->len = s->size;
+	s->len = s->size + 1;
 }
 
 /*

@@ -61,7 +61,7 @@ seq_buf_buffer_left(struct seq_buf *s)
 	if (seq_buf_has_overflowed(s))
 		return 0;
 
-	return (s->size - 1) - s->len;
+	return s->size - s->len;
 }
 
 extern __printf(2, 3)

kernel/trace/seq_buf.c +11 -8

index 7dac34d1235b..9d3bb64dca31 100644
@@ -17,7 +17,7 @@
 #include <linux/seq_buf.h>
 
 /* How much buffer is written? */
-#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
+#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
 
 /**
  * seq_buf_print_seq - move the contents of seq_buf into a seq_file

@@ -51,7 +51,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
 
 	if (s->len < s->size) {
 		len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
-		if (s->len + len < s->size) {
+		if (s->len + len <= s->size) {
 			s->len += len;
 			return 0;
 		}

@@ -100,8 +100,11 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
 	WARN_ON(s->size == 0);
 
 	/*
-	 * The last byte of the buffer is used to determine if we
-	 * overflowed or not.
+	 * Note, because bitmap_scnprintf() only returns the number of bytes
+	 * written and not the number that would be written, we use the last
+	 * byte of the buffer to let us know if we overflowed. There's a small
+	 * chance that the bitmap could have fit exactly inside the buffer, but
+	 * it's not that critical if that does happen.
 	 */
 	if (len > 1) {
 		ret = bitmap_scnprintf(s->buffer + s->len, len, maskp, nmaskbits);

@@ -140,7 +143,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
 
 	if (s->len < s->size) {
 		ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
-		if (s->len + ret < s->size) {
+		if (s->len + ret <= s->size) {
 			s->len += ret;
 			return 0;
 		}

@@ -164,7 +167,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
 
 	WARN_ON(s->size == 0);
 
-	if (s->len + len < s->size) {
+	if (s->len + len <= s->size) {
 		memcpy(s->buffer + s->len, str, len);
 		s->len += len;
 		return 0;

@@ -186,7 +189,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c)
 {
 	WARN_ON(s->size == 0);
 
-	if (s->len + 1 < s->size) {
+	if (s->len + 1 <= s->size) {
 		s->buffer[s->len++] = c;
 		return 0;
 	}

@@ -210,7 +213,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
 {
 	WARN_ON(s->size == 0);
 
-	if (s->len + len < s->size) {
+	if (s->len + len <= s->size) {
 		memcpy(s->buffer + s->len, mem, len);
 		s->len += len;
 		return 0;

Signature · 1 line

2.1.1

Petr Mladek

2014-11-14 17:07 UTC #44 lore raw

Re: [RFC][PATCH 17/23 v4] tracing: Have seq_buf use full buffer

Quoted 10 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Currently seq_buf is full when all but one byte of the buffer is filled. Change it so that the seq_buf is full when all of the buffer is filled.
Some of the functions would fill the buffer completely and report everything was fine. This was inconsistent with the max of size - 1. Changing this to be max of size makes all functions consistent.

Quoted 6 lines

Link http://lkml.kernel.org/r/[email protected] open here
Tested-by Jiri Kosina <[email protected]>
Acked-by Jiri Kosina <[email protected]>
Reviewed-by Petr Mladek <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Hmm, we should not apply this patch before we fix all other locations accessing seq.len. We need to make sure that they do not access outside of the buffer when seq.len = seq.size + 1.

See my comments for "[RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq"

BTW: Are these patches applied in some public branch, please? The patch series is getting long. I would like to see it applied but I do not want to copy all the patches and apply manually :-)

Best Regards,
Petr

PS: I will need to go in a while. I am not sure that I will be able to review the whole patchset before the weekend. I do the review in the order of patches. I sent reply only when I had something to add. The non-commented other patches (< 17) looks fine to me.

Quoted 110 lines

---
 include/linux/seq_buf.h |  6 +++---
 kernel/trace/seq_buf.c  | 19 +++++++++++--------
 2 files changed, 14 insertions(+), 11 deletions(-)

include/linux/seq_buf.h +3 -3

index 5d91262433e2..581c1fc733c3 100644
@@ -43,13 +43,13 @@ seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
 static inline bool
 seq_buf_has_overflowed(struct seq_buf *s)
 {
-	return s->len == s->size;
+	return s->len > s->size;
 }
 
 static inline void
 seq_buf_set_overflow(struct seq_buf *s)
 {
-	s->len = s->size;
+	s->len = s->size + 1;
 }
 
 /*

@@ -61,7 +61,7 @@ seq_buf_buffer_left(struct seq_buf *s)
 	if (seq_buf_has_overflowed(s))
 		return 0;
 
-	return (s->size - 1) - s->len;
+	return s->size - s->len;
 }
 
 extern __printf(2, 3)

kernel/trace/seq_buf.c +11 -8

index 7dac34d1235b..9d3bb64dca31 100644
@@ -17,7 +17,7 @@
 #include <linux/seq_buf.h>
 
 /* How much buffer is written? */
-#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
+#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
 
 /**
  * seq_buf_print_seq - move the contents of seq_buf into a seq_file

@@ -51,7 +51,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
 
 	if (s->len < s->size) {
 		len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
-		if (s->len + len < s->size) {
+		if (s->len + len <= s->size) {
 			s->len += len;
 			return 0;
 		}

@@ -100,8 +100,11 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
 	WARN_ON(s->size == 0);
 
 	/*
-	 * The last byte of the buffer is used to determine if we
-	 * overflowed or not.
+	 * Note, because bitmap_scnprintf() only returns the number of bytes
+	 * written and not the number that would be written, we use the last
+	 * byte of the buffer to let us know if we overflowed. There's a small
+	 * chance that the bitmap could have fit exactly inside the buffer, but
+	 * it's not that critical if that does happen.
 	 */
 	if (len > 1) {
 		ret = bitmap_scnprintf(s->buffer + s->len, len, maskp, nmaskbits);

@@ -140,7 +143,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
 
 	if (s->len < s->size) {
 		ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
-		if (s->len + ret < s->size) {
+		if (s->len + ret <= s->size) {
 			s->len += ret;
 			return 0;
 		}

@@ -164,7 +167,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
 
 	WARN_ON(s->size == 0);
 
-	if (s->len + len < s->size) {
+	if (s->len + len <= s->size) {
 		memcpy(s->buffer + s->len, str, len);
 		s->len += len;
 		return 0;

@@ -186,7 +189,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c)
 {
 	WARN_ON(s->size == 0);
 
-	if (s->len + 1 < s->size) {
+	if (s->len + 1 <= s->size) {
 		s->buffer[s->len++] = c;
 		return 0;
 	}

@@ -210,7 +213,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
 {
 	WARN_ON(s->size == 0);
 
-	if (s->len + len < s->size) {
+	if (s->len + len <= s->size) {
 		memcpy(s->buffer + s->len, mem, len);
 		s->len += len;
 		return 0;

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 17:30 UTC #45 lore raw

Re: [RFC][PATCH 17/23 v4] tracing: Have seq_buf use full buffer

Quoted 18 lines · On Fri, 14 Nov 2014 18:07:16 +0100 Petr Mladek <[email protected]> wrote:

Quoted 10 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Currently seq_buf is full when all but one byte of the buffer is filled. Change it so that the seq_buf is full when all of the buffer is filled.
Some of the functions would fill the buffer completely and report everything was fine. This was inconsistent with the max of size - 1. Changing this to be max of size makes all functions consistent.
Quoted 6 lines
Link http://lkml.kernel.org/r/[email protected] open here
Tested-by Jiri Kosina <[email protected]>
Acked-by Jiri Kosina <[email protected]>
Reviewed-by Petr Mladek <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Quoted 6 lines

Hmm, we should not apply this patch before we fix all other locations accessing seq.len. We need to make sure that they do not access outside of the buffer when seq.len = seq.size + 1.
See my comments for "[RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq"

I agree. As I replied there, I'll add a patch before this gets applied (right after seq_buf_left() is introduced), that will fix those issues.

Quoted 4 lines

BTW: Are these patches applied in some public branch, please? The patch series is getting long. I would like to see it applied but I do not want to copy all the patches and apply manually :-)

I'll start pushing them up to my repo under rfc/seq-buf

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git

That branch will always be rebasing. I have it applied on top of my ftrace/core branch that will become my next for-next soon (after it passes all my tests).

Quoted 8 lines

Best Regards,
Petr
PS: I will need to go in a while. I am not sure that I will be able to review the whole patchset before the weekend. I do the review in the order of patches. I sent reply only when I had something to add. The non-commented other patches (< 17) looks fine to me.

Thanks, I'll add your Reviewed-by tags on them.

To ease the pain of review, I'll reply to your email comments with the patches as I fix them up (as I've already done). I'll keep the commit ids as well so that you can verify them. I'll try to remember to constantly update my rfc/seq-buf branch.

Thanks a lot for your reviews. I really do appreciate it.

-- Steve

Steven Rostedt

author maintainer

2014-11-14 20:56 UTC #46 lore raw

Re: [RFC][PATCH 17/23 v4] tracing: Have seq_buf use full buffer

Quoted 9 lines · On Fri, 14 Nov 2014 12:30:01 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 6 lines
Hmm, we should not apply this patch before we fix all other locations accessing seq.len. We need to make sure that they do not access outside of the buffer when seq.len = seq.size + 1.
See my comments for "[RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq"
I agree. As I replied there, I'll add a patch before this gets applied (right after seq_buf_left() is introduced), that will fix those issues.

I made the move of seq_buf_used() before the full buffer usage patch, and here's a patch I added.

I'll be posting a v5 with all these changes too, but this may make it easier to review:

-- Steve

Quoted 1 line

From a43da42e939ba41a32c7ea83793cef9c2360ec1a Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Fri, 14 Nov 2014 15:49:41 -0500
Subject: [PATCH] tracing: Use trace_seq_used() and seq_buf_used() instead of
len

As the seq_buf->len will soon be +1 size when there's an overflow, we must use trace_seq_used() or seq_buf_used() methods to get the real length. This will prevent buffer overflow issues if just the len of the seq_buf descriptor is used to copy memory.

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Petr Mladek <[email protected]>

---
 include/linux/trace_seq.h            | 20 +++++++++++++++-
 kernel/trace/trace.c                 | 44 ++++++++++++++++++++++++------------
 kernel/trace/trace_events.c          |  9 +++++---
 kernel/trace/trace_functions_graph.c |  5 +++-
 kernel/trace/trace_seq.c             |  2 +-
 5 files changed, 59 insertions(+), 21 deletions(-)

include/linux/trace_seq.h +19 -1

index 85d37106be3d..cfaf5a1d4bad 100644
@@ -24,6 +24,24 @@ trace_seq_init(struct trace_seq *s)
 }
 
 /**
+ * trace_seq_used - amount of actual data written to buffer
+ * @s: trace sequence descriptor
+ *
+ * Returns the amount of data written to the buffer.
+ *
+ * IMPORTANT!
+ *
+ * Use this instead of @s->seq.len if you need to pass the amount
+ * of data from the buffer to another buffer (userspace, or what not).
+ * The @s->seq.len on overflow is bigger than the buffer size and
+ * using it can cause access to undefined memory.
+ */
+static inline int trace_seq_used(struct trace_seq *s)
+{
+	return seq_buf_used(&s->seq);
+}
+
+/**
  * trace_seq_buffer_ptr - return pointer to next location in buffer
  * @s: trace sequence descriptor
  *

@@ -35,7 +53,7 @@ trace_seq_init(struct trace_seq *s)
 static inline unsigned char *
 trace_seq_buffer_ptr(struct trace_seq *s)
 {
-	return s->buffer + s->seq.len;
+	return s->buffer + seq_buf_used(&s->seq);
 }
 
 /**

kernel/trace/trace.c +29 ~~-15~~

104 lines not on this page · raw message

kernel/trace/trace_events.c +6 -3

index 4d0067dd7f88..935cbea78532 100644
@@ -1044,7 +1044,8 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	mutex_unlock(&event_mutex);
 
 	if (file)
-		r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
+		r = simple_read_from_buffer(ubuf, cnt, ppos,
+					    s->buffer, trace_seq_used(s));
 
 	kfree(s);

@@ -1210,7 +1211,8 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 	trace_seq_init(s);
 
 	print_subsystem_event_filter(system, s);
-	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
+	r = simple_read_from_buffer(ubuf, cnt, ppos,
+				    s->buffer, trace_seq_used(s));
 
 	kfree(s);

@@ -1265,7 +1267,8 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)
 	trace_seq_init(s);
 
 	func(s);
-	r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->seq.len);
+	r = simple_read_from_buffer(ubuf, cnt, ppos,
+				    s->buffer, trace_seq_used(s));
 
 	kfree(s);

kernel/trace/trace_functions_graph.c +4 -1

index 6d1342ae7a44..ec35468349a7 100644
@@ -1153,6 +1153,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,
 			return ret;
 	}
 
+	if (trace_seq_has_overflowed(s))
+		goto out;
+
 	/* Strip ending newline */
 	if (s->buffer[s->seq.len - 1] == '\n') {
 		s->buffer[s->seq.len - 1] = '\0';

@@ -1160,7 +1163,7 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,
 	}
 
 	trace_seq_puts(s, " */\n");
-
+ out:
 	return trace_handle_return(s);
 }

kernel/trace/trace_seq.c +1 -1

index 087fa514069d..0c7aab4dd94f 100644
@@ -30,7 +30,7 @@
 #define TRACE_SEQ_BUF_LEFT(s) seq_buf_buffer_left(&(s)->seq)
 
 /* How much buffer is written? */
-#define TRACE_SEQ_BUF_USED(s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1))
+#define TRACE_SEQ_BUF_USED(s) seq_buf_used(&(s)->seq)
 
 /*
  * trace_seq should work with being initialized with 0s.

Signature · 1 line

1.8.1.4

Steven Rostedt

author maintainer

2014-11-14 01:16 UTC #47 lore raw

RFC PATCH v4 18/23 tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions

From: "Steven Rostedt (Red Hat)" <[email protected]>

Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that are used by seq_buf_path(). This makes the code similar to the
seq_file: seq_path() function, and will help to be able to consolidate the functions between seq_file and trace_seq.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
 kernel/trace/seq_buf.c  |  7 +++----
 2 files changed, 43 insertions(+), 4 deletions(-)

include/linux/seq_buf.h +40 -0

index 581c1fc733c3..3e550d214187 100644
@@ -64,6 +64,46 @@ seq_buf_buffer_left(struct seq_buf *s)
 	return s->size - s->len;
 }
 
+/**
+ * seq_buf_get_buf - get buffer to write arbitrary data to
+ * @s: the seq_buf handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
+{
+	WARN_ON(s->len > s->size + 1);
+
+	if (s->len < s->size) {
+		*bufp = s->buffer + s->len;
+		return s->size - s->len;
+	}
+
+	*bufp = NULL;
+	return 0;
+}
+
+/**
+ * seq_buf_commit - commit data to the buffer
+ * @s: the seq_buf handle
+ * @num: the number of bytes to commit
+ *
+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get.  To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_buf_commit(struct seq_buf *s, int num)
+{
+	if (num < 0) {
+		seq_buf_set_overflow(s);
+	} else {
+		BUG_ON(s->len + num > s->size + 1);
+		s->len += num;
+	}
+}
+
 extern __printf(2, 3)
 int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
 extern __printf(2, 0)

kernel/trace/seq_buf.c +3 -4

index 9d3bb64dca31..4f35a783b82f 100644
@@ -283,8 +283,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
  */
 int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 {
-	char *buf = s->buffer + s->len;
-	size_t size = seq_buf_buffer_left(s);
+	char *buf;
+	size_t size = seq_buf_get_buf(s, &buf);
 	int res = -1;
 
 	WARN_ON(s->size == 0);

@@ -297,8 +297,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 				res = end - buf;
 		}
 	}
-	if (res > 0)
-		s->len += res;
+	seq_buf_commit(s, res);
 
 	return res;
 }

Signature · 1 line

2.1.1

Petr Mladek

2014-11-14 17:18 UTC #48 lore raw

Re: [RFC][PATCH 18/23 v4] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions

Quoted 52 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that are used by seq_buf_path(). This makes the code similar to the
seq_file: seq_path() function, and will help to be able to consolidate the functions between seq_file and trace_seq.
Link http://lkml.kernel.org/r/[email protected] open here
Tested-by Jiri Kosina <[email protected]>
Acked-by Jiri Kosina <[email protected]>
Reviewed-by Petr Mladek <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
 kernel/trace/seq_buf.c  |  7 +++----
 2 files changed, 43 insertions(+), 4 deletions(-)
include/linux/seq_buf.h +26 -0
index 581c1fc733c3..3e550d214187 100644
@@ -64,6 +64,46 @@ seq_buf_buffer_left(struct seq_buf *s)
 	return s->size - s->len;
 }
 
+/**
+ * seq_buf_get_buf - get buffer to write arbitrary data to
+ * @s: the seq_buf handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
+{
+	WARN_ON(s->len > s->size + 1);
+
+	if (s->len < s->size) {
+		*bufp = s->buffer + s->len;
+		return s->size - s->len;
+	}
+
+	*bufp = NULL;
+	return 0;
+}
+
+/**
+ * seq_buf_commit - commit data to the buffer
+ * @s: the seq_buf handle
+ * @num: the number of bytes to commit
+ *

include/linux/seq_buf.h seq_buf_buffer_left(struct seq_buf *s) · line 102

Quoted 10 lines

include/linux/seq_buf.h +10 -0

+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get.  To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_buf_commit(struct seq_buf *s, int num)
+{
+	if (num < 0) {
+		seq_buf_set_overflow(s);
+	} else {
+		BUG_ON(s->len + num > s->size + 1);

I thought about it more and we should probably do

		BUG_ON(s->len + num > s->size);

"size + 1" signalizes that there was an overflow. It is a valid value for the "len" variable. But it is not valid to commit "size + 1" bytes into the buffer. It would mean access outside of the buffer.

What do you think, please?

Best Regards,
Petr

PS: Sigh, I wish, I saw all this problems during the first review and not triggering another patchset version.

Quoted 36 lines

include/linux/seq_buf.h +4 -0

+		s->len += num;
+	}
+}
+
 extern __printf(2, 3)
 int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
 extern __printf(2, 0)

kernel/trace/seq_buf.c +3 -4

index 9d3bb64dca31..4f35a783b82f 100644
@@ -283,8 +283,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
  */
 int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 {
-	char *buf = s->buffer + s->len;
-	size_t size = seq_buf_buffer_left(s);
+	char *buf;
+	size_t size = seq_buf_get_buf(s, &buf);
 	int res = -1;
 
 	WARN_ON(s->size == 0);

@@ -297,8 +297,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 				res = end - buf;
 		}
 	}
-	if (res > 0)
-		s->len += res;
+	seq_buf_commit(s, res);
 
 	return res;
 }

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 20:31 UTC #49 lore raw

Re: [RFC][PATCH 18/23 v4] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions

Quoted 66 lines · On Fri, 14 Nov 2014 18:18:28 +0100 Petr Mladek <[email protected]> wrote:

Quoted 52 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that are used by seq_buf_path(). This makes the code similar to the
seq_file: seq_path() function, and will help to be able to consolidate the functions between seq_file and trace_seq.
Link http://lkml.kernel.org/r/[email protected] open here
Tested-by Jiri Kosina <[email protected]>
Acked-by Jiri Kosina <[email protected]>
Reviewed-by Petr Mladek <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
 kernel/trace/seq_buf.c  |  7 +++----
 2 files changed, 43 insertions(+), 4 deletions(-)
include/linux/seq_buf.h +26 -0
index 581c1fc733c3..3e550d214187 100644
@@ -64,6 +64,46 @@ seq_buf_buffer_left(struct seq_buf *s)
 	return s->size - s->len;
 }
 
+/**
+ * seq_buf_get_buf - get buffer to write arbitrary data to
+ * @s: the seq_buf handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
+{
+	WARN_ON(s->len > s->size + 1);
+
+	if (s->len < s->size) {
+		*bufp = s->buffer + s->len;
+		return s->size - s->len;
+	}
+
+	*bufp = NULL;
+	return 0;
+}
+
+/**
+ * seq_buf_commit - commit data to the buffer
+ * @s: the seq_buf handle
+ * @num: the number of bytes to commit
+ *
include/linux/seq_buf.h seq_buf_buffer_left(struct seq_buf *s) · line 102
Quoted 10 lines
include/linux/seq_buf.h +10 -0
+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get.  To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_buf_commit(struct seq_buf *s, int num)
+{
+	if (num < 0) {
+		seq_buf_set_overflow(s);
+	} else {
+		BUG_ON(s->len + num > s->size + 1);
I thought about it more and we should probably do

Quoted 7 lines

		BUG_ON(s->len + num > s->size);
"size + 1" signalizes that there was an overflow. It is a valid value for the "len" variable. But it is not valid to commit "size + 1" bytes into the buffer. It would mean access outside of the buffer.
What do you think, please?

I don't see anything wrong with committing an overflowed state.

Note, seq_commit() allows an overflow number to go in too. But of course that's where overflow is still on the buffer.

Hmm, I guess I can still make this change. The seq_commit() comment states that @num needs to be < 0 if the data did not fit, and if it is in overflow state, then it should be -1 not size + 1 (or size for the old way).

-- Steve

Quoted 44 lines

Best Regards,
Petr

PS: Sigh, I wish, I saw all this problems during the first review and not triggering another patchset version.

Quoted 36 lines

include/linux/seq_buf.h +4 -0

+		s->len += num;
+	}
+}
+
 extern __printf(2, 3)
 int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
 extern __printf(2, 0)

kernel/trace/seq_buf.c +3 -4

index 9d3bb64dca31..4f35a783b82f 100644
@@ -283,8 +283,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
  */
 int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 {
-	char *buf = s->buffer + s->len;
-	size_t size = seq_buf_buffer_left(s);
+	char *buf;
+	size_t size = seq_buf_get_buf(s, &buf);
 	int res = -1;
 
 	WARN_ON(s->size == 0);

@@ -297,8 +297,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 				res = end - buf;
 		}
 	}
-	if (res > 0)
-		s->len += res;
+	seq_buf_commit(s, res);
 
 	return res;
 }

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer Tested-by Jiri Kosina Acked-by Jiri Kosina Reviewed-by Petr Mladek

2014-11-14 20:58 UTC #50 lore raw

Re: [RFC][PATCH 18/23 v4] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions

Quoted 4 lines · On Fri, 14 Nov 2014 15:31:23 -0500 Steven Rostedt <[email protected]> wrote:

Hmm, I guess I can still make this change. The seq_commit() comment states that @num needs to be < 0 if the data did not fit, and if it is in overflow state, then it should be -1 not size + 1 (or size for the old way).

I made the change, and here it is:

-- Steve

Quoted 1 line

From 69488bad76c4af9f9b5948c8990b5ed1196b2bc1 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 29 Oct 2014 17:30:50 -0400
Subject: [PATCH] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper
functions

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h | 41 +++++++++++++++++++++++++++++++++++++++++
 kernel/trace/seq_buf.c  |  7 +++----
 2 files changed, 44 insertions(+), 4 deletions(-)

include/linux/seq_buf.h +41 -0

index 0800a24b4348..12c64282aa98 100644
@@ -70,6 +70,47 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
 	return min(s->len, s->size);
 }
 
+/**
+ * seq_buf_get_buf - get buffer to write arbitrary data to
+ * @s: the seq_buf handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
+{
+	WARN_ON(s->len > s->size + 1);
+
+	if (s->len < s->size) {
+		*bufp = s->buffer + s->len;
+		return s->size - s->len;
+	}
+
+	*bufp = NULL;
+	return 0;
+}
+
+/**
+ * seq_buf_commit - commit data to the buffer
+ * @s: the seq_buf handle
+ * @num: the number of bytes to commit
+ *
+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get.  To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_buf_commit(struct seq_buf *s, int num)
+{
+	if (num < 0) {
+		seq_buf_set_overflow(s);
+	} else {
+		/* num must be negative on overflow */
+		BUG_ON(s->len + num > s->size);
+		s->len += num;
+	}
+}
+
 extern __printf(2, 3)
 int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
 extern __printf(2, 0)

kernel/trace/seq_buf.c +3 -4

index 86e50f47894b..3657af4b5f53 100644
@@ -280,8 +280,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
  */
 int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 {
-	char *buf = s->buffer + s->len;
-	size_t size = seq_buf_buffer_left(s);
+	char *buf;
+	size_t size = seq_buf_get_buf(s, &buf);
 	int res = -1;
 
 	WARN_ON(s->size == 0);

@@ -294,8 +294,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 				res = end - buf;
 		}
 	}
-	if (res > 0)
-		s->len += res;
+	seq_buf_commit(s, res);
 
 	return res;
 }

Signature · 1 line

1.8.1.4

Steven Rostedt

author maintainer

2014-11-14 01:15 UTC #51 lore raw

RFC PATCH v4 19/23 seq_buf: Create seq_buf_used() to find out how much was written

From: "Steven Rostedt (Red Hat)" <[email protected]>

Add a helper function seq_buf_used() that replaces the SEQ_BUF_USED() private macro to let callers have a method to know how much of the seq_buf was written to.

---
 include/linux/seq_buf.h | 6 ++++++
 kernel/trace/seq_buf.c  | 5 +----
 2 files changed, 7 insertions(+), 4 deletions(-)

include/linux/seq_buf.h +6 -0

index 3e550d214187..8fda502ee630 100644
@@ -64,6 +64,12 @@ seq_buf_buffer_left(struct seq_buf *s)
 	return s->size - s->len;
 }
 
+/* How much buffer was written? */
+static inline unsigned int seq_buf_used(struct seq_buf *s)
+{
+	return min((s)->len, (s)->size);
+}
+
 /**
  * seq_buf_get_buf - get buffer to write arbitrary data to
  * @s: the seq_buf handle

kernel/trace/seq_buf.c +1 -4

index 4f35a783b82f..3657af4b5f53 100644
@@ -16,9 +16,6 @@
 #include <linux/seq_file.h>
 #include <linux/seq_buf.h>
 
-/* How much buffer is written? */
-#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
-
 /**
  * seq_buf_print_seq - move the contents of seq_buf into a seq_file
  * @m: the seq_file descriptor that is the destination

@@ -28,7 +25,7 @@
  */
 int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s)
 {
-	unsigned int len = SEQ_BUF_USED(s);
+	unsigned int len = seq_buf_used(s);
 
 	return seq_write(m, s->buffer, len);
 }

Signature · 1 line

2.1.1

Petr Mladek

Reviewed-by on v4 19/23

2014-11-14 17:23 UTC #52 lore raw

Re: [RFC][PATCH 19/23 v4] seq_buf: Create seq_buf_used() to find out how much was written

Quoted 7 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function seq_buf_used() that replaces the SEQ_BUF_USED() private macro to let callers have a method to know how much of the seq_buf was written to.
Signed-off-by Steven Rostedt <[email protected]>

The patch is correct. But we should move it before "[RFC][PATCH 17/23 v4] tracing: Have seq_buf use full buffer" and use it everywhere any code is accessing the internal buffer using the seq.len value. I mean to solve the potential buffer overflows mentioned for "[RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq"

Best Regards,
Petr

Quoted 49 lines

---
 include/linux/seq_buf.h | 6 ++++++
 kernel/trace/seq_buf.c  | 5 +----
 2 files changed, 7 insertions(+), 4 deletions(-)

include/linux/seq_buf.h +6 -0

index 3e550d214187..8fda502ee630 100644
@@ -64,6 +64,12 @@ seq_buf_buffer_left(struct seq_buf *s)
 	return s->size - s->len;
 }
 
+/* How much buffer was written? */
+static inline unsigned int seq_buf_used(struct seq_buf *s)
+{
+	return min((s)->len, (s)->size);
+}
+
 /**
  * seq_buf_get_buf - get buffer to write arbitrary data to
  * @s: the seq_buf handle

kernel/trace/seq_buf.c +1 -4

index 4f35a783b82f..3657af4b5f53 100644
@@ -16,9 +16,6 @@
 #include <linux/seq_file.h>
 #include <linux/seq_buf.h>
 
-/* How much buffer is written? */
-#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
-
 /**
  * seq_buf_print_seq - move the contents of seq_buf into a seq_file
  * @m: the seq_file descriptor that is the destination

@@ -28,7 +25,7 @@
  */
 int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s)
 {
-	unsigned int len = SEQ_BUF_USED(s);
+	unsigned int len = seq_buf_used(s);
 
 	return seq_write(m, s->buffer, len);
 }

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 17:34 UTC #53 lore raw

Re: [RFC][PATCH 19/23 v4] seq_buf: Create seq_buf_used() to find out how much was written

Quoted 11 lines · On Fri, 14 Nov 2014 18:23:06 +0100 Petr Mladek <[email protected]> wrote:

Quoted 7 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function seq_buf_used() that replaces the SEQ_BUF_USED() private macro to let callers have a method to know how much of the seq_buf was written to.
Signed-off-by Steven Rostedt <[email protected]>
Reviewed-by Petr Mladek <[email protected]>

Quoted 7 lines

The patch is correct. But we should move it before "[RFC][PATCH 17/23 v4] tracing: Have seq_buf use full buffer" and use it everywhere any code is accessing the internal buffer using the seq.len value. I mean to solve the potential buffer overflows mentioned for "[RFC][PATCH 13/23 v4] tracing: Create seq_buf layer in trace_seq"
Best Regards,

Bah, I'm getting seq_buf_buffer_left() confused with seq_buf_used(). Yes, this should be moved before hand. I thought it was by looking at:

git show <patch-17-SHA1>:include/linux/seq_buf.h

I saw the seq_buf_buffer_left() and confused that with seq_buf_used() and said to myself "oh good, it has the function I need". Not!

Thanks,

-- Steve

Steven Rostedt

author maintainer

2014-11-14 01:15 UTC #54 lore raw

RFC PATCH v4 20/23 seq-buf: Make seq_buf_bprintf() conditional on CONFIG_BINARY_PRINTF

From: "Steven Rostedt (Red Hat)" <[email protected]>

The function bstr_printf() from lib/vsprnintf.c is only available if CONFIG_BINARY_PRINTF is defined. This is due to the only user currently being the tracing infrastructure, which needs to select this config when tracing is configured. Until there is another user of the binary printf formats, this will continue to be the case.

Since seq_buf.c is now lives in lib/ and is compiled even without tracing, it must encompass its use of bstr_printf() which is used by seq_buf_printf(). This too is only used by the tracing infrastructure and is still encapsulated by the CONFIG_BINARY_PRINTF.

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/linux/seq_buf.h | 7 +++++--
 kernel/trace/seq_buf.c  | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

include/linux/seq_buf.h +5 -2

index 8fda502ee630..da211dfbcebe 100644
@@ -114,8 +114,6 @@ extern __printf(2, 3)
 int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
 extern __printf(2, 0)
 int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args);
-extern int
-seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
 extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s);
 extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,
 			   int cnt);

@@ -129,4 +127,9 @@ extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *
 extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
 			   int nmaskbits);
 
+#ifdef CONFIG_BINARY_PRINTF
+extern int
+seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+#endif
+
 #endif /* _LINUX_SEQ_BUF_H */

kernel/trace/seq_buf.c +2 -0

index 3657af4b5f53..84b6377df827 100644
@@ -114,6 +114,7 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
 	return -1;
 }
 
+#ifdef CONFIG_BINARY_PRINTF
 /**
  * seq_buf_bprintf - Write the printf string from binary arguments
  * @s: seq_buf descriptor

@@ -148,6 +149,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
 	seq_buf_set_overflow(s);
 	return -1;
 }
+#endif /* CONFIG_BINARY_PRINTF */
 
 /**
  * seq_buf_puts - sequence printing of simple string

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:15 UTC #55 lore raw

RFC PATCH v4 21/23 seq_buf: Move the seq_buf code to lib/

From: "Steven Rostedt (Red Hat)" <[email protected]>

The seq_buf functions are rather useful outside of tracing. Instead of having it be dependent on CONFIG_TRACING, move the code into lib/ and allow other users to have access to it even when tracing is not configured.

The seq_buf utility is similar to the seq_file utility, but instead of writing sending data back up to userland, it writes it into a buffer defined at seq_buf_init(). This allows us to send a descriptor around that writes printf() formatted strings into it that can be retrieved later.

It is currently used by the tracing facility for such things like trace events to convert its binary saved data in the ring buffer into an ASCII human readable context to be displayed in /sys/kernel/debug/trace.

It can also be used for doing NMI prints safely from NMI context into the seq_buf and retrieved later and dumped to printk() safely. Doing printk() from an NMI context is dangerous because an NMI can preempt a current printk() and deadlock on it.

Link http://lkml.kernel.org/p/[email protected] open here

---
 kernel/trace/Makefile  |   1 -
 kernel/trace/seq_buf.c | 346 -------------------------------------------------
 lib/Makefile           |   2 +-
 lib/seq_buf.c          | 346 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 347 insertions(+), 348 deletions(-)
 delete mode 100644 kernel/trace/seq_buf.c
 create mode 100644 lib/seq_buf.c

kernel/trace/Makefile +0 -1

index edc98c72a634..67d6369ddf83 100644
@@ -29,7 +29,6 @@ obj-$(CONFIG_RING_BUFFER_BENCHMARK) += ring_buffer_benchmark.o
 obj-$(CONFIG_TRACING) += trace.o
 obj-$(CONFIG_TRACING) += trace_output.o
 obj-$(CONFIG_TRACING) += trace_seq.o
-obj-$(CONFIG_TRACING) += seq_buf.o
 obj-$(CONFIG_TRACING) += trace_stat.o
 obj-$(CONFIG_TRACING) += trace_printk.o
 obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o

kernel/trace/seq_buf.c deleted +0 ~~-346~~

346 lines not on this page · raw message

lib/Makefile +1 -1

index 7512dc978f18..a1aa1e81ed36 100644
@@ -13,7 +13,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 sha1.o md5.o irq_regs.o reciprocal_div.o argv_split.o \
 	 proportions.o flex_proportions.o ratelimit.o show_mem.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
-	 earlycpio.o
+	 earlycpio.o seq_buf.o
 
 obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
 lib-$(CONFIG_MMU) += ioremap.o

lib/seq_buf.c new file +346 -0

346 lines not on this page · raw message

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:14 UTC #56 lore raw

RFC PATCH v4 22/23 printk: Add per_cpu printk func to allow printk to be diverted

From: "Steven Rostedt (Red Hat)" <[email protected]>

Being able to divert printk to call another function besides the normal logging is useful for such things like NMI handling. If some functions are to be called from NMI that does printk() it is possible to lock up the box if the nmi handler triggers when another printk is happening.

One example of this use is to perform a stack trace on all CPUs via NMI. But if the NMI is to do the printk() it can cause the system to lock up. By allowing the printk to be diverted to another function that can safely record the printk output and then print it when it in a safe context then NMIs will be safe to call these functions like show_regs().

Link http://lkml.kernel.org/p/[email protected] open here

Acked-by Paul E. McKenney <[email protected]>

---
 include/linux/percpu.h |  3 +++
 include/linux/printk.h |  2 ++
 kernel/printk/printk.c | 38 +++++++++++++++++++++++++++++---------
 3 files changed, 34 insertions(+), 9 deletions(-)

include/linux/percpu.h +3 -0

index a3aa63e47637..ba2e85a0ff5b 100644
@@ -134,4 +134,7 @@ extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
 	(typeof(type) __percpu *)__alloc_percpu(sizeof(type),		\
 						__alignof__(type))
 
+/* To avoid include hell, as printk can not declare this, we declare it here */
+DECLARE_PER_CPU(printk_func_t, printk_func);
+
 #endif /* __LINUX_PERCPU_H */

include/linux/printk.h +2 -0

index d78125f73ac4..3bbd979d32fb 100644
@@ -162,6 +162,8 @@ extern int kptr_restrict;
 
 extern void wake_up_klogd(void);
 
+typedef int(*printk_func_t)(const char *fmt, va_list args);
+
 void log_buf_kexec_setup(void);
 void __init setup_log_buf(int early);
 void dump_stack_set_arch_desc(const char *fmt, ...);

kernel/printk/printk.c +29 -9

index ced2b84b1cb7..f7b723f98cb9 100644
@@ -1807,6 +1807,30 @@ asmlinkage int printk_emit(int facility, int level,
 }
 EXPORT_SYMBOL(printk_emit);
 
+int vprintk_default(const char *fmt, va_list args)
+{
+	int r;
+
+#ifdef CONFIG_KGDB_KDB
+	if (unlikely(kdb_trap_printk)) {
+		r = vkdb_printf(fmt, args);
+		return r;
+	}
+#endif
+	r = vprintk_emit(0, -1, NULL, 0, fmt, args);
+
+	return r;
+}
+EXPORT_SYMBOL_GPL(vprintk_default);
+
+/*
+ * This allows printk to be diverted to another function per cpu.
+ * This is useful for calling printk functions from within NMI
+ * without worrying about race conditions that can lock up the
+ * box.
+ */
+DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default;
+
 /**
  * printk - print a kernel message
  * @fmt: format string

@@ -1830,19 +1854,15 @@ EXPORT_SYMBOL(printk_emit);
  */
 asmlinkage __visible int printk(const char *fmt, ...)
 {
+	printk_func_t vprintk_func;
 	va_list args;
 	int r;
 
-#ifdef CONFIG_KGDB_KDB
-	if (unlikely(kdb_trap_printk)) {
-		va_start(args, fmt);
-		r = vkdb_printf(fmt, args);
-		va_end(args);
-		return r;
-	}
-#endif
 	va_start(args, fmt);
-	r = vprintk_emit(0, -1, NULL, 0, fmt, args);
+	preempt_disable();
+	vprintk_func = this_cpu_read(printk_func);
+	r = vprintk_func(fmt, args);
+	preempt_enable();
 	va_end(args);
 
 	return r;

Signature · 1 line

2.1.1

Steven Rostedt

author maintainer

2014-11-14 01:14 UTC #57 lore raw

RFC PATCH v4 23/23 x86/nmi: Perform a safe NMI stack trace on all CPUs

From: "Steven Rostedt (Red Hat)" <[email protected]>

When trigger_all_cpu_backtrace() is called on x86, it will trigger an NMI on each CPU and call show_regs(). But this can lead to a hard lock up if the NMI comes in on another printk().

In order to avoid this, when the NMI triggers, it switches the printk routine for that CPU to call a NMI safe printk function that records the printk in a per_cpu seq_buf descriptor. After all NMIs have finished recording its data, the seq_bufs are printed in a safe context.

Link http://lkml.kernel.org/p/[email protected] open here

Acked-by Paul E. McKenney <[email protected]>