From: "Steven Rostedt (Red Hat)" <[email protected]>
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]
Reported-by: Petr Mladek <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/trace_seq.h | 20 +++++++++++++++-
kernel/trace/seq_buf.c | 2 +-
kernel/trace/trace.c | 44 ++++++++++++++++++++++++------------
kernel/trace/trace_events.c | 9 +++++---
kernel/trace/trace_functions_graph.c | 5 +++-
kernel/trace/trace_seq.c | 2 +-
6 files changed, 60 insertions(+), 22 deletions(-)
diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index 85d37106be3d..cfaf5a1d4bad 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -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);
}
/**
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index 9ec5305d9da7..ce17f65268ed 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -328,7 +328,7 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
if (s->len <= s->readpos)
return -EBUSY;
- len = s->len - s->readpos;
+ len = seq_buf_used(s) - s->readpos;
if (cnt > len)
cnt = len;
ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 7d7a07e9b9e9..9f1ffc707f3b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
{
int len;
- if (s->seq.len <= s->seq.readpos)
+ if (trace_seq_used(s) <= s->seq.readpos)
return -EBUSY;
- len = s->seq.len - s->seq.readpos;
+ len = trace_seq_used(s) - s->seq.readpos;
if (cnt > len)
cnt = len;
memcpy(buf, s->buffer + s->seq.readpos, cnt);
@@ -4514,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.seq.len;
+ int save_len = iter->seq.seq.len;
ret = print_trace_line(iter);
if (ret == TRACE_TYPE_PARTIAL_LINE) {
/* don't print partial lines */
- iter->seq.seq.len = len;
+ iter->seq.seq.len = save_len;
break;
}
if (ret != TRACE_TYPE_NO_CONSUME)
trace_consume(iter);
- if (iter->seq.seq.len >= cnt)
+ if (trace_seq_used(&iter->seq) >= cnt)
break;
/*
@@ -4541,7 +4541,7 @@ waitagain:
/* Now copy what we have to the user */
sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
- if (iter->seq.seq.readpos >= iter->seq.seq.len)
+ if (iter->seq.seq.readpos >= trace_seq_used(&iter->seq))
trace_seq_init(&iter->seq);
/*
@@ -4575,20 +4575,33 @@ static size_t
tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
{
size_t count;
+ int save_len;
int ret;
/* Seq buffer is page-sized, exactly what we need. */
for (;;) {
- count = iter->seq.seq.len;
+ save_len = iter->seq.seq.len;
ret = print_trace_line(iter);
- count = iter->seq.seq.len - count;
- if (rem < count) {
- rem = 0;
- iter->seq.seq.len -= count;
+
+ if (trace_seq_has_overflowed(&iter->seq)) {
+ iter->seq.seq.len = save_len;
break;
}
+
+ /*
+ * This should not be hit, because it should only
+ * be set if the iter->seq overflowed. But check it
+ * anyway to be safe.
+ */
if (ret == TRACE_TYPE_PARTIAL_LINE) {
- iter->seq.seq.len -= count;
+ iter->seq.seq.len = save_len;
+ break;
+ }
+
+ count = trace_seq_used(&iter->seq) - save_len;
+ if (rem < count) {
+ rem = 0;
+ iter->seq.seq.len = save_len;;
break;
}
@@ -4669,13 +4682,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.seq.len);
+ trace_seq_used(&iter->seq));
if (ret < 0) {
__free_page(spd.pages[i]);
break;
}
spd.partial[i].offset = 0;
- spd.partial[i].len = iter->seq.seq.len;
+ spd.partial[i].len = trace_seq_used(&iter->seq);
trace_seq_init(&iter->seq);
}
@@ -5676,7 +5689,8 @@ 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->seq.len);
+ count = simple_read_from_buffer(ubuf, count, ppos,
+ s->buffer, trace_seq_used(s));
kfree(s);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 4d0067dd7f88..935cbea78532 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -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);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 6d1342ae7a44..ec35468349a7 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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);
}
diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
index 087fa514069d..0c7aab4dd94f 100644
--- a/kernel/trace/trace_seq.c
+++ b/kernel/trace/trace_seq.c
@@ -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.
--
2.1.1
On Fri, 14 Nov 2014 23:59:06 -0500
> /*
> @@ -4575,20 +4575,33 @@ static size_t
> tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
> {
> size_t count;
> + int save_len;
> int ret;
>
> /* Seq buffer is page-sized, exactly what we need. */
> for (;;) {
> - count = iter->seq.seq.len;
> + save_len = iter->seq.seq.len;
> ret = print_trace_line(iter);
> - count = iter->seq.seq.len - count;
> - if (rem < count) {
> - rem = 0;
> - iter->seq.seq.len -= count;
> +
> + if (trace_seq_has_overflowed(&iter->seq)) {
> + iter->seq.seq.len = save_len;
> break;
> }
> +
> + /*
> + * This should not be hit, because it should only
> + * be set if the iter->seq overflowed. But check it
> + * anyway to be safe.
> + */
> if (ret == TRACE_TYPE_PARTIAL_LINE) {
> - iter->seq.seq.len -= count;
> + iter->seq.seq.len = save_len;
> + break;
> + }
> +
> + count = trace_seq_used(&iter->seq) - save_len;
> + if (rem < count) {
> + rem = 0;
> + iter->seq.seq.len = save_len;;
> break;
> }
>
I don't like the fact that I did a code structure change with this
patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before
this patch.
-- Steve
>
> I don't like the fact that I did a code structure change with this
> patch. This patch should be just a simple conversion of len to
> seq_buf_used(). I'm going to strip this change out and put it before
> this patch.
The function tracing_fill_pipe_page() logic is a little confusing with the
use of count saving the seq.len and reusing it.
Instead of subtracting a number that is calculated from the saved
value of the seq.len from seq.len, just save the seq.len at the start
and if we need to reset it, just assign it again.
When the seq_buf overflow is len == size + 1, the current logic will
break. Changing it to use a saved length for resetting back to the
original value is more robust and will work when we change the way
seq_buf sets the overflow.
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/trace/trace.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 7d7a07e9b9e9..2dbc18e5f929 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4575,23 +4575,37 @@ static size_t
tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
{
size_t count;
+ int save_len;
int ret;
/* Seq buffer is page-sized, exactly what we need. */
for (;;) {
- count = iter->seq.seq.len;
+ save_len = iter->seq.seq.len;
ret = print_trace_line(iter);
- count = iter->seq.seq.len - count;
- if (rem < count) {
- rem = 0;
- iter->seq.seq.len -= count;
+
+ if (trace_seq_has_overflowed(&iter->seq)) {
+ iter->seq.seq.len = save_len;
break;
}
+
+ /*
+ * This should not be hit, because it should only
+ * be set if the iter->seq overflowed. But check it
+ * anyway to be safe.
+ */
if (ret == TRACE_TYPE_PARTIAL_LINE) {
- iter->seq.seq.len -= count;
+ iter->seq.seq.len = save_len;
break;
}
+ count = iter->seq.seq.len - save_len;
+ if (rem < count) {
+ rem = 0;
+ iter->seq.seq.len = save_len;
+ break;
+ }
+
+
if (ret != TRACE_TYPE_NO_CONSUME)
trace_consume(iter);
rem -= count;
--
1.8.1.4
> I don't like the fact that I did a code structure change with this
> patch. This patch should be just a simple conversion of len to
> seq_buf_used(). I'm going to strip this change out and put it before
> this patch.
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]
Reported-by: Petr Mladek <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/trace_seq.h | 20 +++++++++++++++++++-
kernel/trace/seq_buf.c | 2 +-
kernel/trace/trace.c | 22 +++++++++++-----------
kernel/trace/trace_events.c | 9 ++++++---
kernel/trace/trace_functions_graph.c | 5 ++++-
kernel/trace/trace_seq.c | 2 +-
6 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index 85d37106be3d..cfaf5a1d4bad 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -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);
}
/**
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index 9ec5305d9da7..ce17f65268ed 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -328,7 +328,7 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
if (s->len <= s->readpos)
return -EBUSY;
- len = s->len - s->readpos;
+ len = seq_buf_used(s) - s->readpos;
if (cnt > len)
cnt = len;
ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2dbc18e5f929..9023446b2c2b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
{
int len;
- if (s->seq.len <= s->seq.readpos)
+ if (trace_seq_used(s) <= s->seq.readpos)
return -EBUSY;
- len = s->seq.len - s->seq.readpos;
+ len = trace_seq_used(s) - s->seq.readpos;
if (cnt > len)
cnt = len;
memcpy(buf, s->buffer + s->seq.readpos, cnt);
@@ -4514,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.seq.len;
+ int save_len = iter->seq.seq.len;
ret = print_trace_line(iter);
if (ret == TRACE_TYPE_PARTIAL_LINE) {
/* don't print partial lines */
- iter->seq.seq.len = len;
+ iter->seq.seq.len = save_len;
break;
}
if (ret != TRACE_TYPE_NO_CONSUME)
trace_consume(iter);
- if (iter->seq.seq.len >= cnt)
+ if (trace_seq_used(&iter->seq) >= cnt)
break;
/*
@@ -4541,7 +4541,7 @@ waitagain:
/* Now copy what we have to the user */
sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
- if (iter->seq.seq.readpos >= iter->seq.seq.len)
+ if (iter->seq.seq.readpos >= trace_seq_used(&iter->seq))
trace_seq_init(&iter->seq);
/*
@@ -4598,14 +4598,13 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
break;
}
- count = iter->seq.seq.len - save_len;
+ count = trace_seq_used(&iter->seq) - save_len;
if (rem < count) {
rem = 0;
iter->seq.seq.len = save_len;
break;
}
-
if (ret != TRACE_TYPE_NO_CONSUME)
trace_consume(iter);
rem -= count;
@@ -4683,13 +4682,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.seq.len);
+ trace_seq_used(&iter->seq));
if (ret < 0) {
__free_page(spd.pages[i]);
break;
}
spd.partial[i].offset = 0;
- spd.partial[i].len = iter->seq.seq.len;
+ spd.partial[i].len = trace_seq_used(&iter->seq);
trace_seq_init(&iter->seq);
}
@@ -5690,7 +5689,8 @@ 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->seq.len);
+ count = simple_read_from_buffer(ubuf, count, ppos,
+ s->buffer, trace_seq_used(s));
kfree(s);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 4d0067dd7f88..935cbea78532 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -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);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 6d1342ae7a44..ec35468349a7 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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);
}
diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
index 087fa514069d..0c7aab4dd94f 100644
--- a/kernel/trace/trace_seq.c
+++ b/kernel/trace/trace_seq.c
@@ -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.
--
1.8.1.4
On Mon 2014-11-17 14:11:08, Steven Rostedt wrote:
> >
> > I don't like the fact that I did a code structure change with this
> > patch. This patch should be just a simple conversion of len to
> > seq_buf_used(). I'm going to strip this change out and put it before
> > this patch.
>
>
> The function tracing_fill_pipe_page() logic is a little confusing with the
> use of count saving the seq.len and reusing it.
>
> Instead of subtracting a number that is calculated from the saved
> value of the seq.len from seq.len, just save the seq.len at the start
> and if we need to reset it, just assign it again.
>
> When the seq_buf overflow is len == size + 1, the current logic will
> break. Changing it to use a saved length for resetting back to the
> original value is more robust and will work when we change the way
> seq_buf sets the overflow.
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> kernel/trace/trace.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 7d7a07e9b9e9..2dbc18e5f929 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4575,23 +4575,37 @@ static size_t
> tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
> {
> size_t count;
> + int save_len;
> int ret;
>
> /* Seq buffer is page-sized, exactly what we need. */
> for (;;) {
> - count = iter->seq.seq.len;
> + save_len = iter->seq.seq.len;
> ret = print_trace_line(iter);
> - count = iter->seq.seq.len - count;
> - if (rem < count) {
> - rem = 0;
> - iter->seq.seq.len -= count;
> +
> + if (trace_seq_has_overflowed(&iter->seq)) {
> + iter->seq.seq.len = save_len;
> break;
> }
> +
> + /*
> + * This should not be hit, because it should only
> + * be set if the iter->seq overflowed. But check it
> + * anyway to be safe.
> + */
> if (ret == TRACE_TYPE_PARTIAL_LINE) {
> - iter->seq.seq.len -= count;
> + iter->seq.seq.len = save_len;
> break;
> }
The two ifs has the same body. Small optimization would be to do:
/*
* The two checks should be equivalent but rather be
* on the safe side.
*/
if (trace_seq_has_overflowed(&iter->seq) ||
ret == TRACE_TYPE_PARTIAL_LINE) {
iter->seq.seq.len = save_len;
break;
}
To be honest, the code seems to be a bit twisted. This function
is called from tracing_splice_read_pipe(). It copies the
trace_seq buffer into spd.page and call trace_seq_init()
in a for cycle.
Therefore if the overflow happens, trace_find_next_entry_inc()
is not called in tracing_fill_pipe_page() and we work with the same
iterator instance next time. It means that the overflow happens most
likely again and we fill all remaining spd.pages with no data and
are stacked on the same iterator instance.
BTW: The trace_seq_to_buffer() in tracing_splice_read_pipe()
is suspicious as well. It passes trace_seq_used(&iter->seq)
as the "cnt" parameter. I guess that it should pass the
size of the "spd.page" instead.
Also we should somehow handle the situation when some data are not
copied. Well, it seems the spd.page has the page size, so it is
the same size as the trace_seq buffer.
Well, this patch does not change the behavior. We could solve the
above problem later if it is there. Maybe I got it wrong.
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
> + count = iter->seq.seq.len - save_len;
> + if (rem < count) {
> + rem = 0;
> + iter->seq.seq.len = save_len;
> + break;
> + }
> +
> +
> if (ret != TRACE_TYPE_NO_CONSUME)
> trace_consume(iter);
> rem -= count;
> --
> 1.8.1.4
>
On Mon 2014-11-17 14:12:15, Steven Rostedt wrote:
>
> > I don't like the fact that I did a code structure change with this
> > patch. This patch should be just a simple conversion of len to
> > seq_buf_used(). I'm going to strip this change out and put it before
> > this patch.
>
>
> 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]
>
> Reported-by: Petr Mladek <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> include/linux/trace_seq.h | 20 +++++++++++++++++++-
> kernel/trace/seq_buf.c | 2 +-
> kernel/trace/trace.c | 22 +++++++++++-----------
> kernel/trace/trace_events.c | 9 ++++++---
> kernel/trace/trace_functions_graph.c | 5 ++++-
> kernel/trace/trace_seq.c | 2 +-
> 6 files changed, 42 insertions(+), 18 deletions(-)
[...]
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
> {
> int len;
>
> - if (s->seq.len <= s->seq.readpos)
> + if (trace_seq_used(s) <= s->seq.readpos)
> return -EBUSY;
>
> - len = s->seq.len - s->seq.readpos;
> + len = trace_seq_used(s) - s->seq.readpos;
> if (cnt > len)
> cnt = len;
> memcpy(buf, s->buffer + s->seq.readpos, cnt);
There is one more dangerous usage in trace_printk_seq(). It is on
three lines there.
The rest looks good.
Best Regards,
Petr
On Tue, 18 Nov 2014 17:33:54 +0100
Petr Mladek <[email protected]> wrote:
> On Mon 2014-11-17 14:12:15, Steven Rostedt wrote:
> >
> > > I don't like the fact that I did a code structure change with this
> > > patch. This patch should be just a simple conversion of len to
> > > seq_buf_used(). I'm going to strip this change out and put it before
> > > this patch.
> >
> >
> > 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]
> >
> > Reported-by: Petr Mladek <[email protected]>
> > Signed-off-by: Steven Rostedt <[email protected]>
> > ---
> > include/linux/trace_seq.h | 20 +++++++++++++++++++-
> > kernel/trace/seq_buf.c | 2 +-
> > kernel/trace/trace.c | 22 +++++++++++-----------
> > kernel/trace/trace_events.c | 9 ++++++---
> > kernel/trace/trace_functions_graph.c | 5 ++++-
> > kernel/trace/trace_seq.c | 2 +-
> > 6 files changed, 42 insertions(+), 18 deletions(-)
>
> [...]
>
>
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
> > {
> > int len;
> >
> > - if (s->seq.len <= s->seq.readpos)
> > + if (trace_seq_used(s) <= s->seq.readpos)
> > return -EBUSY;
> >
> > - len = s->seq.len - s->seq.readpos;
> > + len = trace_seq_used(s) - s->seq.readpos;
> > if (cnt > len)
> > cnt = len;
> > memcpy(buf, s->buffer + s->seq.readpos, cnt);
>
>
> There is one more dangerous usage in trace_printk_seq(). It is on
> three lines there.
You totally confused me. What usage in trace_printk_seq(), and what
three lines?
In this patch, trace_printk_seq() looks like this:
int trace_print_seq(struct seq_file *m, struct trace_seq *s)
{
int ret;
__trace_seq_init(s);
ret = seq_buf_print_seq(m, &s->seq);
/*
* Only reset this buffer if we successfully wrote to the
* seq_file buffer. This lets the caller try again or
* do something else with the contents.
*/
if (!ret)
trace_seq_init(s);
return ret;
}
-- Steve
>
> The rest looks good.
>
> Best Regards,
> Petr
On Tue 2014-11-18 12:37:32, Steven Rostedt wrote:
> On Tue, 18 Nov 2014 17:33:54 +0100
> Petr Mladek <[email protected]> wrote:
>
> > On Mon 2014-11-17 14:12:15, Steven Rostedt wrote:
> > >
> > > > I don't like the fact that I did a code structure change with this
> > > > patch. This patch should be just a simple conversion of len to
> > > > seq_buf_used(). I'm going to strip this change out and put it before
> > > > this patch.
> > >
> > >
> > > 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]
> > >
> > > Reported-by: Petr Mladek <[email protected]>
> > > Signed-off-by: Steven Rostedt <[email protected]>
> > > ---
> > > include/linux/trace_seq.h | 20 +++++++++++++++++++-
> > > kernel/trace/seq_buf.c | 2 +-
> > > kernel/trace/trace.c | 22 +++++++++++-----------
> > > kernel/trace/trace_events.c | 9 ++++++---
> > > kernel/trace/trace_functions_graph.c | 5 ++++-
> > > kernel/trace/trace_seq.c | 2 +-
> > > 6 files changed, 42 insertions(+), 18 deletions(-)
> >
> > [...]
> >
> >
> > > --- a/kernel/trace/trace.c
> > > +++ b/kernel/trace/trace.c
> > > @@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
> > > {
> > > int len;
> > >
> > > - if (s->seq.len <= s->seq.readpos)
> > > + if (trace_seq_used(s) <= s->seq.readpos)
> > > return -EBUSY;
> > >
> > > - len = s->seq.len - s->seq.readpos;
> > > + len = trace_seq_used(s) - s->seq.readpos;
> > > if (cnt > len)
> > > cnt = len;
> > > memcpy(buf, s->buffer + s->seq.readpos, cnt);
> >
> >
> > There is one more dangerous usage in trace_printk_seq(). It is on
> > three lines there.
>
> You totally confused me. What usage in trace_printk_seq(), and what
> three lines?
>
> In this patch, trace_printk_seq() looks like this:
>
> int trace_print_seq(struct seq_file *m, struct trace_seq *s)
> {
> int ret;
>
> __trace_seq_init(s);
>
> ret = seq_buf_print_seq(m, &s->seq);
>
> /*
> * Only reset this buffer if we successfully wrote to the
> * seq_file buffer. This lets the caller try again or
> * do something else with the contents.
> */
> if (!ret)
> trace_seq_init(s);
>
> return ret;
> }
The confusion is caused by the 'k' ("print" vs. "printk") in the
function name. I was talking about the following function from
kernel/trace/trace.c:
void
trace_printk_seq(struct trace_seq *s)
{
/* Probably should print a warning here. */
if (s->seq.len >= TRACE_MAX_PRINT)
s->seq.len = TRACE_MAX_PRINT;
/* should be zero ended, but we are paranoid. */
s->buffer[s->seq.len] = 0;
printk(KERN_TRACE "%s", s->buffer);
trace_seq_init(s);
}
I found it when checking the applied patches in origin/rfc/seq-buf
branch. I hope that it was the correct place.
Best Regards,
Petr
On Wed, 19 Nov 2014 12:40:17 +0100
Petr Mladek <[email protected]> wrote:
> > >
> > > There is one more dangerous usage in trace_printk_seq(). It is on
> > > three lines there.
> >
> > You totally confused me. What usage in trace_printk_seq(), and what
> > three lines?
> >
> > In this patch, trace_printk_seq() looks like this:
> >
> > int trace_print_seq(struct seq_file *m, struct trace_seq *s)
> > {
> > int ret;
> >
> > __trace_seq_init(s);
> >
> > ret = seq_buf_print_seq(m, &s->seq);
> >
> > /*
> > * Only reset this buffer if we successfully wrote to the
> > * seq_file buffer. This lets the caller try again or
> > * do something else with the contents.
> > */
> > if (!ret)
> > trace_seq_init(s);
> >
> > return ret;
> > }
>
> The confusion is caused by the 'k' ("print" vs. "printk") in the
> function name. I was talking about the following function from
> kernel/trace/trace.c:
Silly 'k', Trix are for kids!
>
> void
> trace_printk_seq(struct trace_seq *s)
> {
> /* Probably should print a warning here. */
> if (s->seq.len >= TRACE_MAX_PRINT)
> s->seq.len = TRACE_MAX_PRINT;
>
> /* should be zero ended, but we are paranoid. */
> s->buffer[s->seq.len] = 0;
>
> printk(KERN_TRACE "%s", s->buffer);
>
> trace_seq_init(s);
> }
>
> I found it when checking the applied patches in origin/rfc/seq-buf
> branch. I hope that it was the correct place.
Yes, that's the working branch for this code.
Anyway, I saw this and thought about using trace_seq_used(), but then I
realized that this is trace_seq code which has a hard coded buffer
length of PAGE_SIZE which on all archs is more than 1000
(TRACE_MAX_PRINT).
Regardless of overflow or not (or even if trace_seq is full), that if
statement will prevent this from doing any buffer overflows.
s->seq.len will never be more than s->seq.size after the test against
TRACE_MAX_PRINT. So I see no harm here.
trace_printk_seq() is for dumping the ring buffer to console, which is
usually something done on panic. It's special.
-- Steve
On Wed 2014-11-19 08:48:00, Steven Rostedt wrote:
> On Wed, 19 Nov 2014 12:40:17 +0100
> Petr Mladek <[email protected]> wrote:
>
> > > >
> > > > There is one more dangerous usage in trace_printk_seq(). It is on
> > > > three lines there.
> > >
> > > You totally confused me. What usage in trace_printk_seq(), and what
> > > three lines?
> > >
> > The confusion is caused by the 'k' ("print" vs. "printk") in the
> > function name. I was talking about the following function from
> > kernel/trace/trace.c:
>
> Silly 'k', Trix are for kids!
:-)
> >
> > void
> > trace_printk_seq(struct trace_seq *s)
> > {
> > /* Probably should print a warning here. */
> > if (s->seq.len >= TRACE_MAX_PRINT)
> > s->seq.len = TRACE_MAX_PRINT;
> >
> > /* should be zero ended, but we are paranoid. */
> > s->buffer[s->seq.len] = 0;
> >
> > printk(KERN_TRACE "%s", s->buffer);
> >
> > trace_seq_init(s);
> > }
> >
> > I found it when checking the applied patches in origin/rfc/seq-buf
> > branch. I hope that it was the correct place.
>
> Yes, that's the working branch for this code.
>
> Anyway, I saw this and thought about using trace_seq_used(), but then I
> realized that this is trace_seq code which has a hard coded buffer
> length of PAGE_SIZE which on all archs is more than 1000
> (TRACE_MAX_PRINT).
>
> Regardless of overflow or not (or even if trace_seq is full), that if
> statement will prevent this from doing any buffer overflows.
>
> s->seq.len will never be more than s->seq.size after the test against
> TRACE_MAX_PRINT. So I see no harm here.
Ah, I see. Well, I would feel more comfortable if it uses
trace_seq_used() or if there is some explanation in a comment.
But you are right, it is safe as it is. Feel free to leave it.
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed, 19 Nov 2014 15:40:05 +0100
Petr Mladek <[email protected]> wrote:
> > s->seq.len will never be more than s->seq.size after the test against
> > TRACE_MAX_PRINT. So I see no harm here.
>
> Ah, I see. Well, I would feel more comfortable if it uses
> trace_seq_used() or if there is some explanation in a comment.
> But you are right, it is safe as it is. Feel free to leave it.
Yeah, a comment would be nice. I may add it late.
>
> Reviewed-by: Petr Mladek <[email protected]>
Thanks,
-- Steve
On Wed, 19 Nov 2014 15:40:05 +0100
Petr Mladek <[email protected]> wrote:
> > Regardless of overflow or not (or even if trace_seq is full), that if
> > statement will prevent this from doing any buffer overflows.
> >
> > s->seq.len will never be more than s->seq.size after the test against
> > TRACE_MAX_PRINT. So I see no harm here.
>
> Ah, I see. Well, I would feel more comfortable if it uses
> trace_seq_used() or if there is some explanation in a comment.
> But you are right, it is safe as it is. Feel free to leave it.
>
OK, I added this just for you:
BTW, using trace_seq_used() would not be good enough because it could
return s->seq.size. Which would overflow the buffer on the
s->buffer[s->seq.len] = 0;
-- Steve
>From 1922acc9987d23d0b9c1ad28dc3eaafc1cf2d6b7 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 19 Nov 2014 10:56:41 -0500
Subject: [PATCH] tracing: Add paranoid size check in trace_printk_seq()
To be really paranoid about writing out of bound data in
trace_printk_seq(), add another check of len compared to size.
Link: http://lkml.kernel.org/r/[email protected]
Suggested-by: Petr Mladek <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/trace/trace.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 9023446b2c2b..26facec4625e 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6656,6 +6656,14 @@ trace_printk_seq(struct trace_seq *s)
if (s->seq.len >= TRACE_MAX_PRINT)
s->seq.len = TRACE_MAX_PRINT;
+ /*
+ * More paranoid code. Although the buffer size is set to
+ * PAGE_SIZE, and TRACE_MAX_PRINT is 1000, this is just
+ * an extra layer of protection.
+ */
+ if (WARN_ON_ONCE(s->seq.len >= s->seq.size))
+ s->seq.len = s->seq.size - 1;
+
/* should be zero ended, but we are paranoid. */
s->buffer[s->seq.len] = 0;
--
1.8.1.4
On Tue, 18 Nov 2014 17:15:46 +0100
Petr Mladek <[email protected]> wrote:
> On Mon 2014-11-17 14:11:08, Steven Rostedt wrote:
> > >
> > > I don't like the fact that I did a code structure change with this
> > > patch. This patch should be just a simple conversion of len to
> > > seq_buf_used(). I'm going to strip this change out and put it before
> > > this patch.
> >
> >
> > The function tracing_fill_pipe_page() logic is a little confusing with the
> > use of count saving the seq.len and reusing it.
> >
> > Instead of subtracting a number that is calculated from the saved
> > value of the seq.len from seq.len, just save the seq.len at the start
> > and if we need to reset it, just assign it again.
> >
> > When the seq_buf overflow is len == size + 1, the current logic will
> > break. Changing it to use a saved length for resetting back to the
> > original value is more robust and will work when we change the way
> > seq_buf sets the overflow.
> >
> > Signed-off-by: Steven Rostedt <[email protected]>
> > ---
> > kernel/trace/trace.c | 26 ++++++++++++++++++++------
> > 1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 7d7a07e9b9e9..2dbc18e5f929 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -4575,23 +4575,37 @@ static size_t
> > tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
> > {
> > size_t count;
> > + int save_len;
> > int ret;
> >
> > /* Seq buffer is page-sized, exactly what we need. */
> > for (;;) {
> > - count = iter->seq.seq.len;
> > + save_len = iter->seq.seq.len;
> > ret = print_trace_line(iter);
> > - count = iter->seq.seq.len - count;
> > - if (rem < count) {
> > - rem = 0;
> > - iter->seq.seq.len -= count;
> > +
> > + if (trace_seq_has_overflowed(&iter->seq)) {
> > + iter->seq.seq.len = save_len;
> > break;
> > }
> > +
> > + /*
> > + * This should not be hit, because it should only
> > + * be set if the iter->seq overflowed. But check it
> > + * anyway to be safe.
> > + */
> > if (ret == TRACE_TYPE_PARTIAL_LINE) {
> > - iter->seq.seq.len -= count;
> > + iter->seq.seq.len = save_len;
> > break;
> > }
>
> The two ifs has the same body. Small optimization would be to do:
>
> /*
> * The two checks should be equivalent but rather be
> * on the safe side.
> */
> if (trace_seq_has_overflowed(&iter->seq) ||
> ret == TRACE_TYPE_PARTIAL_LINE) {
> iter->seq.seq.len = save_len;
> break;
> }
Yeah, I originally had something like that, but I wanted to remove that
second check. I left it separate to make it stand out as something that
might be removed in the future. Just a preference I guess.
>
> To be honest, the code seems to be a bit twisted. This function
> is called from tracing_splice_read_pipe(). It copies the
> trace_seq buffer into spd.page and call trace_seq_init()
> in a for cycle.
Yeah, that splice code confused me too. I'll start looking at it some
more and see if it can be fixed up.
>
> Therefore if the overflow happens, trace_find_next_entry_inc()
> is not called in tracing_fill_pipe_page() and we work with the same
> iterator instance next time. It means that the overflow happens most
> likely again and we fill all remaining spd.pages with no data and
> are stacked on the same iterator instance.
Luckily, overflows never happen. But if they do, things might break.
>
> BTW: The trace_seq_to_buffer() in tracing_splice_read_pipe()
> is suspicious as well. It passes trace_seq_used(&iter->seq)
> as the "cnt" parameter. I guess that it should pass the
> size of the "spd.page" instead.
Wow, I should have looked harder at that code when I accepted it. It
just "worked" and I was happy. Oh well, another thing to fix up.
>
> Also we should somehow handle the situation when some data are not
> copied. Well, it seems the spd.page has the page size, so it is
> the same size as the trace_seq buffer.
>
>
> Well, this patch does not change the behavior. We could solve the
> above problem later if it is there. Maybe I got it wrong.
No, that code doesn't look too good. That's some old stuff that got in
when we were still learning, and if it worked, we added it ;-)
That needs to be cleaned up. I'll put it on my ever growing todo list.
Of course if you want to clean it up, feel free to send some patches on
top of this. That is, if we get the OK from Linus or Andrew.
>
> Reviewed-by: Petr Mladek <[email protected]>
>
Thanks,
-- Steve
On Wed 2014-11-19 11:00:37, Steven Rostedt wrote:
> On Wed, 19 Nov 2014 15:40:05 +0100
> Petr Mladek <[email protected]> wrote:
>
> > > Regardless of overflow or not (or even if trace_seq is full), that if
> > > statement will prevent this from doing any buffer overflows.
> > >
> > > s->seq.len will never be more than s->seq.size after the test against
> > > TRACE_MAX_PRINT. So I see no harm here.
> >
> > Ah, I see. Well, I would feel more comfortable if it uses
> > trace_seq_used() or if there is some explanation in a comment.
> > But you are right, it is safe as it is. Feel free to leave it.
> >
>
> OK, I added this just for you:
>
> BTW, using trace_seq_used() would not be good enough because it could
> return s->seq.size. Which would overflow the buffer on the
>
> s->buffer[s->seq.len] = 0;
Great catch!
> -- Steve
>
> From 1922acc9987d23d0b9c1ad28dc3eaafc1cf2d6b7 Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <[email protected]>
> Date: Wed, 19 Nov 2014 10:56:41 -0500
> Subject: [PATCH] tracing: Add paranoid size check in trace_printk_seq()
>
> To be really paranoid about writing out of bound data in
> trace_printk_seq(), add another check of len compared to size.
>
> Link: http://lkml.kernel.org/r/[email protected]
>
> Suggested-by: Petr Mladek <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Thanks a lot for the addition.
Best Regards,
Petr
> ---
> kernel/trace/trace.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 9023446b2c2b..26facec4625e 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6656,6 +6656,14 @@ trace_printk_seq(struct trace_seq *s)
> if (s->seq.len >= TRACE_MAX_PRINT)
> s->seq.len = TRACE_MAX_PRINT;
>
> + /*
> + * More paranoid code. Although the buffer size is set to
> + * PAGE_SIZE, and TRACE_MAX_PRINT is 1000, this is just
> + * an extra layer of protection.
> + */
> + if (WARN_ON_ONCE(s->seq.len >= s->seq.size))
> + s->seq.len = s->seq.size - 1;
> +
> /* should be zero ended, but we are paranoid. */
> s->buffer[s->seq.len] = 0;
>
> --
> 1.8.1.4
>
On Wed 2014-11-19 11:17:18, Steven Rostedt wrote:
> On Tue, 18 Nov 2014 17:15:46 +0100
> Petr Mladek <[email protected]> wrote:
>
> > On Mon 2014-11-17 14:11:08, Steven Rostedt wrote:
> > > >
> > > > I don't like the fact that I did a code structure change with this
> > > > patch. This patch should be just a simple conversion of len to
> > > > seq_buf_used(). I'm going to strip this change out and put it before
> > > > this patch.
> > >
> > >
> > > The function tracing_fill_pipe_page() logic is a little confusing with the
> > > use of count saving the seq.len and reusing it.
> > >
> > > Instead of subtracting a number that is calculated from the saved
> > > value of the seq.len from seq.len, just save the seq.len at the start
> > > and if we need to reset it, just assign it again.
> > >
> > > When the seq_buf overflow is len == size + 1, the current logic will
> > > break. Changing it to use a saved length for resetting back to the
> > > original value is more robust and will work when we change the way
> > > seq_buf sets the overflow.
> > >
> > > Signed-off-by: Steven Rostedt <[email protected]>
> > > ---
> > > kernel/trace/trace.c | 26 ++++++++++++++++++++------
> > > 1 file changed, 20 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > > index 7d7a07e9b9e9..2dbc18e5f929 100644
> > > --- a/kernel/trace/trace.c
> > > +++ b/kernel/trace/trace.c
> > > @@ -4575,23 +4575,37 @@ static size_t
> > > tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
> > > {
> > > size_t count;
> > > + int save_len;
> > > int ret;
> > >
> > > /* Seq buffer is page-sized, exactly what we need. */
> > > for (;;) {
> > > - count = iter->seq.seq.len;
> > > + save_len = iter->seq.seq.len;
> > > ret = print_trace_line(iter);
> > > - count = iter->seq.seq.len - count;
> > > - if (rem < count) {
> > > - rem = 0;
> > > - iter->seq.seq.len -= count;
> > > +
> > > + if (trace_seq_has_overflowed(&iter->seq)) {
> > > + iter->seq.seq.len = save_len;
> > > break;
> > > }
> > > +
> > > + /*
> > > + * This should not be hit, because it should only
> > > + * be set if the iter->seq overflowed. But check it
> > > + * anyway to be safe.
> > > + */
> > > if (ret == TRACE_TYPE_PARTIAL_LINE) {
> > > - iter->seq.seq.len -= count;
> > > + iter->seq.seq.len = save_len;
> > > break;
> > > }
> >
> > The two ifs has the same body. Small optimization would be to do:
> >
> > /*
> > * The two checks should be equivalent but rather be
> > * on the safe side.
> > */
> > if (trace_seq_has_overflowed(&iter->seq) ||
> > ret == TRACE_TYPE_PARTIAL_LINE) {
> > iter->seq.seq.len = save_len;
> > break;
> > }
>
> Yeah, I originally had something like that, but I wanted to remove that
> second check. I left it separate to make it stand out as something that
> might be removed in the future. Just a preference I guess.
Fair enough.
> > To be honest, the code seems to be a bit twisted. This function
> > is called from tracing_splice_read_pipe(). It copies the
> > trace_seq buffer into spd.page and call trace_seq_init()
> > in a for cycle.
>
> Yeah, that splice code confused me too. I'll start looking at it some
> more and see if it can be fixed up.
>
> >
> > Therefore if the overflow happens, trace_find_next_entry_inc()
> > is not called in tracing_fill_pipe_page() and we work with the same
> > iterator instance next time. It means that the overflow happens most
> > likely again and we fill all remaining spd.pages with no data and
> > are stacked on the same iterator instance.
>
> Luckily, overflows never happen. But if they do, things might break.
I thought so. :-)
> >
> > BTW: The trace_seq_to_buffer() in tracing_splice_read_pipe()
> > is suspicious as well. It passes trace_seq_used(&iter->seq)
> > as the "cnt" parameter. I guess that it should pass the
> > size of the "spd.page" instead.
>
> Wow, I should have looked harder at that code when I accepted it. It
> just "worked" and I was happy. Oh well, another thing to fix up.
>
> >
> > Also we should somehow handle the situation when some data are not
> > copied. Well, it seems the spd.page has the page size, so it is
> > the same size as the trace_seq buffer.
> >
> >
> > Well, this patch does not change the behavior. We could solve the
> > above problem later if it is there. Maybe I got it wrong.
>
> No, that code doesn't look too good. That's some old stuff that got in
> when we were still learning, and if it worked, we added it ;-)
>
> That needs to be cleaned up. I'll put it on my ever growing todo
> list.
I believe. I am a bit scared to put it on my todo list because these
kind of working things tend to just fall down.
> Of course if you want to clean it up, feel free to send some patches on
> top of this. That is, if we get the OK from Linus or Andrew.
OK, I'll put it on my todo list. Let's see who is faster ;-) And I
keep my fingers crossed about the OK from Linus and Andrew.
Best Regards,
Petr
On Wed, 19 Nov 2014 17:51:49 +0100
Petr Mladek <[email protected]> wrote:
> OK, I'll put it on my todo list. Let's see who is faster ;-) And I
> keep my fingers crossed about the OK from Linus and Andrew.
I'll assume they are going to be OK'd. I'll place these patches in a
separate branch and after I finish testing (and get acks from my
ftrace/core branch), I'll push them all up to my for-next branch.
Then when the merge window opens, I'll push the normal ftrace/core
code, and then push this one separate, and let Linus decide if he wants
to pull it or not.
Here's the commits that are in this branch and not in ftrace/core:
+ 1293ffff tracing: Create seq_buf layer in trace_seq
+ b5e206f2 tracing: Convert seq_buf_path() to be like seq_path()
+ 79033e01 tracing: Convert seq_buf fields to be like seq_file fields
+ f1698325 tracing: Add a seq_buf_clear() helper and clear len and readpos in init
+ 2b644c7f seq_buf: Create seq_buf_used() to find out how much was written
+ a5c4b79c tracing: Clean up tracing_fill_pipe_page()
+ 359ba4a9 tracing: Use trace_seq_used() and seq_buf_used() instead of len
+ 3ae8769e tracing: Add paranoid size check in trace_printk_seq()
+ e1fb15f1 seq_buf: Add seq_buf_can_fit() helper function
+ 02f09612 tracing: Have seq_buf use full buffer
+ d9c7f4b1 tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions
+ 678a330a seq-buf: Make seq_buf_bprintf() conditional on CONFIG_BINARY_PRINTF
+ a3608c5d seq_buf: Move the seq_buf code to lib/
+ af8f0ef4 printk: Add per_cpu printk func to allow printk to be diverted
+ a066719f x86/nmi: Perform a safe NMI stack trace on all CPUs
-- Steve