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 <[email protected]>
---
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;
* Lai Jiangshan <[email protected]> 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
Ingo Molnar wrote:
> * Lai Jiangshan <[email protected]> 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.
* Lai Jiangshan <[email protected]> wrote:
> Ingo Molnar wrote:
> > * Lai Jiangshan <[email protected]> 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
Commit-ID: 93cfb3c9fd83d877a8f1ffad9ff862b617b32828
Gitweb: http://git.kernel.org/tip/93cfb3c9fd83d877a8f1ffad9ff862b617b32828
Author: Lai Jiangshan <[email protected]>
AuthorDate: Thu, 2 Apr 2009 15:17:08 +0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
On Wed, 2009-04-08 at 11:55 +0200, Ingo Molnar wrote:
> * Lai Jiangshan <[email protected]> wrote:
>
> > Ingo Molnar wrote:
> > > * Lai Jiangshan <[email protected]> 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 <[email protected]>
-- Steve