[PATCH 4/4] tracing: fix splice return too large

[PATCH 4/4] tracing: fix splice return too large

[Lai Jiangshan]

I got these from strace:

splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 12288
splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 12288
splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 12288
splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 16384
splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 8192
splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 8192
splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 8192

I wanted to splice_read 4096 bytes, but it returns 8192 or larger.

It is because the return value of tracing_buffers_splice_read()
is not include "zero out any left over data" bytes.
But tracing_buffers_read() include these bytes, we make them
consistent.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d101c12..a7ea047 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3408,7 +3408,19 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 	int size, i;
 	size_t ret;
 
-	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= size) {
+	if (*ppos & (PAGE_SIZE - 1)) {
+		WARN_ONCE(1, "Ftrace: previous read must page-align\n");
+		return -EINVAL;
+	}
+
+	if (len & (PAGE_SIZE - 1)) {
+		WARN_ONCE(1, "Ftrace: splice_read should page-align\n");
+		if (len < PAGE_SIZE)
+			return -EINVAL;
+		len &= PAGE_MASK;
+	}
+
+	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= PAGE_SIZE) {
 		struct page *page;
 		int r;
 
@@ -3447,7 +3459,7 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 		spd.partial[i].offset = 0;
 		spd.partial[i].private = (unsigned long)ref;
 		spd.nr_pages++;
-		*ppos += size;
+		*ppos += PAGE_SIZE;
 	}
 
 	spd.nr_pages = i;

Re: [PATCH 4/4] tracing: fix splice return too large

[Ingo Molnar]

* Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> -	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= size) {
> +	if (*ppos & (PAGE_SIZE - 1)) {
> +		WARN_ONCE(1, "Ftrace: previous read must page-align\n");
> +		return -EINVAL;
> +	}
> +
> +	if (len & (PAGE_SIZE - 1)) {
> +		WARN_ONCE(1, "Ftrace: splice_read should page-align\n");
> +		if (len < PAGE_SIZE)
> +			return -EINVAL;
> +		len &= PAGE_MASK;
> +	}

Hm, the fix looks good, but is it a good idea to allow the 
triggering of this message from user-space?

	Ingo

Re: [PATCH 4/4] tracing: fix splice return too large

[Lai Jiangshan]

Ingo Molnar wrote:
> * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> 
>> -	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= size) {
>> +	if (*ppos & (PAGE_SIZE - 1)) {
>> +		WARN_ONCE(1, "Ftrace: previous read must page-align\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (len & (PAGE_SIZE - 1)) {
>> +		WARN_ONCE(1, "Ftrace: splice_read should page-align\n");
>> +		if (len < PAGE_SIZE)
>> +			return -EINVAL;
>> +		len &= PAGE_MASK;
>> +	}
> 
> Hm, the fix looks good, but is it a good idea to allow the 
> triggering of this message from user-space?
> 

But it is only triggered once.

If user get content from trace_pipe_raw with non-page-align,
the content is garbage. It's wasting, the kernel disallows it.
This message tell user why he read failed.

These 4 patches make trace_pipe_raw more robustious and consistent
(read by read(2) and splice). I focus on handling raw ftrace data
in userspace, these raw ftrace data should be trustiness.

Thanks.
Lai.

Re: [PATCH 4/4] tracing: fix splice return too large

[Ingo Molnar]

* Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> Ingo Molnar wrote:
> > * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> > 
> >> -	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= size) {
> >> +	if (*ppos & (PAGE_SIZE - 1)) {
> >> +		WARN_ONCE(1, "Ftrace: previous read must page-align\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (len & (PAGE_SIZE - 1)) {
> >> +		WARN_ONCE(1, "Ftrace: splice_read should page-align\n");
> >> +		if (len < PAGE_SIZE)
> >> +			return -EINVAL;
> >> +		len &= PAGE_MASK;
> >> +	}
> > 
> > Hm, the fix looks good, but is it a good idea to allow the 
> > triggering of this message from user-space?
> > 
> 
> But it is only triggered once.
> 
> If user get content from trace_pipe_raw with non-page-align, the 
> content is garbage. It's wasting, the kernel disallows it. This 
> message tell user why he read failed.
> 
> These 4 patches make trace_pipe_raw more robustious and consistent 
> (read by read(2) and splice). I focus on handling raw ftrace data 
> in userspace, these raw ftrace data should be trustiness.

Ok, agreed.

i've applied these four fixes from you to tip:tracing/splice, 
thanks. Steve: i think they are tracing/urgent material - do you 
agree?

	Ingo

[tip:tracing/core] tracing: fix splice return too large

[Lai Jiangshan]

Commit-ID:  93cfb3c9fd83d877a8f1ffad9ff862b617b32828
Gitweb:     http://git.kernel.org/tip/93cfb3c9fd83d877a8f1ffad9ff862b617b32828
Author:     Lai Jiangshan <laijs@cn.fujitsu.com>
AuthorDate: Thu, 2 Apr 2009 15:17:08 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 10 Apr 2009 12:44:46 +0200

tracing: fix splice return too large

I got these from strace:

 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 12288
 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 12288
 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 12288
 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 16384
 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 8192
 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 8192
 splice(0x3, 0, 0x5, 0, 0x1000, 0x1) = 8192

I wanted to splice_read 4096 bytes, but it returns 8192 or larger.

It is because the return value of tracing_buffers_splice_read()
does not include "zero out any left over data" bytes.

But tracing_buffers_read() includes these bytes, we make them
consistent.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <srostedt@redhat.com>
LKML-Reference: <49D46674.9030804@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 9462976..1ce5dc6 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3428,7 +3428,19 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 	int size, i;
 	size_t ret;
 
-	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= size) {
+	if (*ppos & (PAGE_SIZE - 1)) {
+		WARN_ONCE(1, "Ftrace: previous read must page-align\n");
+		return -EINVAL;
+	}
+
+	if (len & (PAGE_SIZE - 1)) {
+		WARN_ONCE(1, "Ftrace: splice_read should page-align\n");
+		if (len < PAGE_SIZE)
+			return -EINVAL;
+		len &= PAGE_MASK;
+	}
+
+	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= PAGE_SIZE) {
 		struct page *page;
 		int r;
 
@@ -3467,7 +3479,7 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 		spd.partial[i].offset = 0;
 		spd.partial[i].private = (unsigned long)ref;
 		spd.nr_pages++;
-		*ppos += size;
+		*ppos += PAGE_SIZE;
 	}
 
 	spd.nr_pages = i;

Re: [PATCH 4/4] tracing: fix splice return too large

[Steven Rostedt]

On Wed, 2009-04-08 at 11:55 +0200, Ingo Molnar wrote:
> * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> 
> > Ingo Molnar wrote:
> > > * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> > > 
> > >> -	for (i = 0; i < PIPE_BUFFERS && len; i++, len -= size) {
> > >> +	if (*ppos & (PAGE_SIZE - 1)) {
> > >> +		WARN_ONCE(1, "Ftrace: previous read must page-align\n");

Small English grammar nit,

  "Ftrace: previous read must be page-aligned"

> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	if (len & (PAGE_SIZE - 1)) {
> > >> +		WARN_ONCE(1, "Ftrace: splice_read should page-align\n");

  "Ftrace: splice_read should be page-aligned"

> > >> +		if (len < PAGE_SIZE)
> > >> +			return -EINVAL;
> > >> +		len &= PAGE_MASK;
> > >> +	}
> > > 
> > > Hm, the fix looks good, but is it a good idea to allow the 
> > > triggering of this message from user-space?
> > > 
> > 
> > But it is only triggered once.
> > 
> > If user get content from trace_pipe_raw with non-page-align, the 
> > content is garbage. It's wasting, the kernel disallows it. This 
> > message tell user why he read failed.
> > 
> > These 4 patches make trace_pipe_raw more robustious and consistent 
> > (read by read(2) and splice). I focus on handling raw ftrace data 
> > in userspace, these raw ftrace data should be trustiness.
> 
> Ok, agreed.
> 
> i've applied these four fixes from you to tip:tracing/splice, 
> thanks. Steve: i think they are tracing/urgent material - do you 
> agree?

Agreed. With my comments above:

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve