* [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily
@ 2017-02-09 23:04 Steven Rostedt
2017-02-10 5:50 ` Masami Hiramatsu
2017-02-10 6:20 ` [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Namhyung Kim
0 siblings, 2 replies; 15+ messages in thread
From: Steven Rostedt @ 2017-02-09 23:04 UTC (permalink / raw)
To: LKML
Cc: Srikar Dronamraju, Namhyung Kim, Masami Hiramatsu, Ingo Molnar,
Andrew Morton
The code in traceprobe_probes_write() reads up to 4096 bytes from userpace
for each line. If userspace passes in several lines to execute, the code
will do a large read for each line, even though, it is highly likely that
the first read from userspace received all of the lines at one.
I changed the logic to do a single read from userspace, and to only read
from userspace again if not all of the read from userspace made it in.
I tested this by adding printk()s and writing files that would test -1, ==,
and +1 the buffer size, to make sure that there's no overflows and that if a
single line is written with +1 the buffer size, that it fails properly.
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_probe.c | 48 ++++++++++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 19 deletions(-)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 8c0553d..2a06f1f 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -647,7 +647,7 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos,
int (*createfn)(int, char **))
{
- char *kbuf, *tmp;
+ char *kbuf, *buf, *tmp;
int ret = 0;
size_t done = 0;
size_t size;
@@ -667,27 +667,37 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
goto out;
}
kbuf[size] = '\0';
- tmp = strchr(kbuf, '\n');
+ buf = kbuf;
+ do {
+ tmp = strchr(buf, '\n');
+ if (tmp) {
+ *tmp = '\0';
+ size = tmp - buf + 1;
+ } else {
+ size = strlen(buf);
+ if (done + size < count) {
+ if (buf != kbuf)
+ break;
+ pr_warn("Line length is too long: Should be less than %d\n",
+ WRITE_BUFSIZE);
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+ done += size;
- if (tmp) {
- *tmp = '\0';
- size = tmp - kbuf + 1;
- } else if (done + size < count) {
- pr_warn("Line length is too long: Should be less than %d\n",
- WRITE_BUFSIZE);
- ret = -EINVAL;
- goto out;
- }
- done += size;
- /* Remove comments */
- tmp = strchr(kbuf, '#');
+ /* Remove comments */
+ tmp = strchr(buf, '#');
- if (tmp)
- *tmp = '\0';
+ if (tmp)
+ *tmp = '\0';
- ret = traceprobe_command(kbuf, createfn);
- if (ret)
- goto out;
+ ret = traceprobe_command(buf, createfn);
+ if (ret)
+ goto out;
+ buf += size;
+
+ } while (done < count);
}
ret = done;
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily
2017-02-09 23:04 [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Steven Rostedt
@ 2017-02-10 5:50 ` Masami Hiramatsu
2017-02-10 7:53 ` Ingo Molnar
2017-02-10 6:20 ` [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Namhyung Kim
1 sibling, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-10 5:50 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: LKML, Srikar Dronamraju, Namhyung Kim, Masami Hiramatsu, Andrew Morton
On Thu, 9 Feb 2017 18:04:58 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
>
> The code in traceprobe_probes_write() reads up to 4096 bytes from userpace
> for each line. If userspace passes in several lines to execute, the code
> will do a large read for each line, even though, it is highly likely that
> the first read from userspace received all of the lines at one.
>
> I changed the logic to do a single read from userspace, and to only read
> from userspace again if not all of the read from userspace made it in.
>
> I tested this by adding printk()s and writing files that would test -1, ==,
> and +1 the buffer size, to make sure that there's no overflows and that if a
> single line is written with +1 the buffer size, that it fails properly.
>
Thanks Steve!
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
BTW, this can conflict with my previous patch.
https://lkml.org/lkml/2017/2/6/1048
https://lkml.org/lkml/2017/2/7/203
I'll update this. Ingo, Can I send these patch to Steve?
Thank you,
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace_probe.c | 48 ++++++++++++++++++++++++++++------------------
> 1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 8c0553d..2a06f1f 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -647,7 +647,7 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos,
> int (*createfn)(int, char **))
> {
> - char *kbuf, *tmp;
> + char *kbuf, *buf, *tmp;
> int ret = 0;
> size_t done = 0;
> size_t size;
> @@ -667,27 +667,37 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> goto out;
> }
> kbuf[size] = '\0';
> - tmp = strchr(kbuf, '\n');
> + buf = kbuf;
> + do {
> + tmp = strchr(buf, '\n');
> + if (tmp) {
> + *tmp = '\0';
> + size = tmp - buf + 1;
> + } else {
> + size = strlen(buf);
> + if (done + size < count) {
> + if (buf != kbuf)
> + break;
> + pr_warn("Line length is too long: Should be less than %d\n",
> + WRITE_BUFSIZE);
> + ret = -EINVAL;
> + goto out;
> + }
> + }
> + done += size;
>
> - if (tmp) {
> - *tmp = '\0';
> - size = tmp - kbuf + 1;
> - } else if (done + size < count) {
> - pr_warn("Line length is too long: Should be less than %d\n",
> - WRITE_BUFSIZE);
> - ret = -EINVAL;
> - goto out;
> - }
> - done += size;
> - /* Remove comments */
> - tmp = strchr(kbuf, '#');
> + /* Remove comments */
> + tmp = strchr(buf, '#');
>
> - if (tmp)
> - *tmp = '\0';
> + if (tmp)
> + *tmp = '\0';
>
> - ret = traceprobe_command(kbuf, createfn);
> - if (ret)
> - goto out;
> + ret = traceprobe_command(buf, createfn);
> + if (ret)
> + goto out;
> + buf += size;
> +
> + } while (done < count);
> }
> ret = done;
>
> --
> 2.9.3
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily
2017-02-09 23:04 [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Steven Rostedt
2017-02-10 5:50 ` Masami Hiramatsu
@ 2017-02-10 6:20 ` Namhyung Kim
1 sibling, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2017-02-10 6:20 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Srikar Dronamraju, Masami Hiramatsu, Ingo Molnar, Andrew Morton
On Thu, Feb 09, 2017 at 06:04:58PM -0500, Steven Rostedt wrote:
>
> The code in traceprobe_probes_write() reads up to 4096 bytes from userpace
> for each line. If userspace passes in several lines to execute, the code
> will do a large read for each line, even though, it is highly likely that
> the first read from userspace received all of the lines at one.
>
> I changed the logic to do a single read from userspace, and to only read
> from userspace again if not all of the read from userspace made it in.
>
> I tested this by adding printk()s and writing files that would test -1, ==,
> and +1 the buffer size, to make sure that there's no overflows and that if a
> single line is written with +1 the buffer size, that it fails properly.
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
> ---
> kernel/trace/trace_probe.c | 48 ++++++++++++++++++++++++++++------------------
> 1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 8c0553d..2a06f1f 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -647,7 +647,7 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos,
> int (*createfn)(int, char **))
> {
> - char *kbuf, *tmp;
> + char *kbuf, *buf, *tmp;
> int ret = 0;
> size_t done = 0;
> size_t size;
> @@ -667,27 +667,37 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> goto out;
> }
> kbuf[size] = '\0';
> - tmp = strchr(kbuf, '\n');
> + buf = kbuf;
> + do {
> + tmp = strchr(buf, '\n');
> + if (tmp) {
> + *tmp = '\0';
> + size = tmp - buf + 1;
> + } else {
> + size = strlen(buf);
> + if (done + size < count) {
> + if (buf != kbuf)
> + break;
> + pr_warn("Line length is too long: Should be less than %d\n",
> + WRITE_BUFSIZE);
> + ret = -EINVAL;
> + goto out;
> + }
> + }
> + done += size;
>
> - if (tmp) {
> - *tmp = '\0';
> - size = tmp - kbuf + 1;
> - } else if (done + size < count) {
> - pr_warn("Line length is too long: Should be less than %d\n",
> - WRITE_BUFSIZE);
> - ret = -EINVAL;
> - goto out;
> - }
> - done += size;
> - /* Remove comments */
> - tmp = strchr(kbuf, '#');
> + /* Remove comments */
> + tmp = strchr(buf, '#');
>
> - if (tmp)
> - *tmp = '\0';
> + if (tmp)
> + *tmp = '\0';
>
> - ret = traceprobe_command(kbuf, createfn);
> - if (ret)
> - goto out;
> + ret = traceprobe_command(buf, createfn);
> + if (ret)
> + goto out;
> + buf += size;
> +
> + } while (done < count);
> }
> ret = done;
>
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily
2017-02-10 5:50 ` Masami Hiramatsu
@ 2017-02-10 7:53 ` Ingo Molnar
2017-02-10 10:37 ` Masami Hiramatsu
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Ingo Molnar @ 2017-02-10 7:53 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Steven Rostedt, LKML, Srikar Dronamraju, Namhyung Kim, Andrew Morton
* Masami Hiramatsu <mhiramat@kernel.org> wrote:
> On Thu, 9 Feb 2017 18:04:58 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> >
> > The code in traceprobe_probes_write() reads up to 4096 bytes from userpace
> > for each line. If userspace passes in several lines to execute, the code
> > will do a large read for each line, even though, it is highly likely that
> > the first read from userspace received all of the lines at one.
> >
> > I changed the logic to do a single read from userspace, and to only read
> > from userspace again if not all of the read from userspace made it in.
> >
> > I tested this by adding printk()s and writing files that would test -1, ==,
> > and +1 the buffer size, to make sure that there's no overflows and that if a
> > single line is written with +1 the buffer size, that it fails properly.
> >
>
> Thanks Steve!
>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>
> BTW, this can conflict with my previous patch.
>
> https://lkml.org/lkml/2017/2/6/1048
> https://lkml.org/lkml/2017/2/7/203
>
> I'll update this. Ingo, Can I send these patch to Steve?
Sure, I've not applied your patch yet - mind sending it to Steve on top of Steve's
patch?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily
2017-02-10 7:53 ` Ingo Molnar
@ 2017-02-10 10:37 ` Masami Hiramatsu
2017-02-10 14:05 ` Steven Rostedt
2017-02-10 13:21 ` [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length Masami Hiramatsu
2017-02-10 13:23 ` [PATCH V2 2/2] tracing/probe: Show subsystem name in messages Masami Hiramatsu
2 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-10 10:37 UTC (permalink / raw)
To: Ingo Molnar
Cc: Steven Rostedt, LKML, Srikar Dronamraju, Namhyung Kim, Andrew Morton
On Fri, 10 Feb 2017 08:53:02 +0100
Ingo Molnar <mingo@kernel.org> wrote:
>
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > On Thu, 9 Feb 2017 18:04:58 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > >
> > > The code in traceprobe_probes_write() reads up to 4096 bytes from userpace
> > > for each line. If userspace passes in several lines to execute, the code
> > > will do a large read for each line, even though, it is highly likely that
> > > the first read from userspace received all of the lines at one.
> > >
> > > I changed the logic to do a single read from userspace, and to only read
> > > from userspace again if not all of the read from userspace made it in.
> > >
> > > I tested this by adding printk()s and writing files that would test -1, ==,
> > > and +1 the buffer size, to make sure that there's no overflows and that if a
> > > single line is written with +1 the buffer size, that it fails properly.
> > >
> >
> > Thanks Steve!
> >
> > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> >
> > BTW, this can conflict with my previous patch.
> >
> > https://lkml.org/lkml/2017/2/6/1048
> > https://lkml.org/lkml/2017/2/7/203
> >
> > I'll update this. Ingo, Can I send these patch to Steve?
>
> Sure, I've not applied your patch yet - mind sending it to Steve on top of Steve's
> patch?
Of course, yes. :)
Thanks!
>
> Thanks,
>
> Ingo
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length
2017-02-10 7:53 ` Ingo Molnar
2017-02-10 10:37 ` Masami Hiramatsu
@ 2017-02-10 13:21 ` Masami Hiramatsu
2017-02-10 16:04 ` Steven Rostedt
2017-02-10 13:23 ` [PATCH V2 2/2] tracing/probe: Show subsystem name in messages Masami Hiramatsu
2 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-10 13:21 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt
Cc: Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Namhyung Kim, Andrew Morton
Since tracing/*probe_events will accept a probe definition
up to 4096 - 2 ('\n' and '\0') bytes, it must show 4094 instead
of 4096 in warning message.
Note that there is one possible case of exceed 4094. If user
prepare 4096 bytes null-terminated string and syscall write
it with the count == 4095, then it can be accepted. However,
if user puts a '\n' after that, it must rejected.
So IMHO, the warning message should indicate shorter one,
since it is safer.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/trace_probe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 2a06f1f..847c1e0 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -678,8 +678,9 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
if (done + size < count) {
if (buf != kbuf)
break;
+ /* This can accept WRITE_BUFSIZE - 2 ('\n' + '\0') */
pr_warn("Line length is too long: Should be less than %d\n",
- WRITE_BUFSIZE);
+ WRITE_BUFSIZE - 2);
ret = -EINVAL;
goto out;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V2 2/2] tracing/probe: Show subsystem name in messages
2017-02-10 7:53 ` Ingo Molnar
2017-02-10 10:37 ` Masami Hiramatsu
2017-02-10 13:21 ` [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length Masami Hiramatsu
@ 2017-02-10 13:23 ` Masami Hiramatsu
2017-02-10 16:01 ` Namhyung Kim
2 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-10 13:23 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt
Cc: Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Namhyung Kim, Andrew Morton
Show "trace_probe:", "trace_kprobe:" and "trace_uprobe:"
headers for each warning/error/info message. This will
help people to notice that kprobe/uprobe events caused
those messages.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/trace_kprobe.c | 1 +
kernel/trace/trace_probe.c | 1 +
kernel/trace/trace_uprobe.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d3729bd..5f688cc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -16,6 +16,7 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#define pr_fmt(fmt) "trace_kprobe: " fmt
#include <linux/module.h>
#include <linux/uaccess.h>
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 847c1e0..52478f0 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -21,6 +21,7 @@
* Copyright (C) IBM Corporation, 2010-2011
* Author: Srikar Dronamraju
*/
+#define pr_fmt(fmt) "trace_probe: " fmt
#include "trace_probe.h"
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index e5445ab..2c6b2d0c 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -17,6 +17,7 @@
* Copyright (C) IBM Corporation, 2010-2012
* Author: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
*/
+#define pr_fmt(fmt) "trace_kprobe: " fmt
#include <linux/module.h>
#include <linux/uaccess.h>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily
2017-02-10 10:37 ` Masami Hiramatsu
@ 2017-02-10 14:05 ` Steven Rostedt
0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2017-02-10 14:05 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ingo Molnar, LKML, Srikar Dronamraju, Namhyung Kim, Andrew Morton
On Fri, 10 Feb 2017 19:37:53 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > Sure, I've not applied your patch yet - mind sending it to Steve on top of Steve's
> > patch?
>
> Of course, yes. :)
>
Thanks! I'll apply them on top of mine then.
-- Steve
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/2] tracing/probe: Show subsystem name in messages
2017-02-10 13:23 ` [PATCH V2 2/2] tracing/probe: Show subsystem name in messages Masami Hiramatsu
@ 2017-02-10 16:01 ` Namhyung Kim
2017-02-10 16:17 ` Steven Rostedt
0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2017-02-10 16:01 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ingo Molnar, Steven Rostedt, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Andrew Morton
On Fri, Feb 10, 2017 at 10:23:06PM +0900, Masami Hiramatsu wrote:
> Show "trace_probe:", "trace_kprobe:" and "trace_uprobe:"
> headers for each warning/error/info message. This will
> help people to notice that kprobe/uprobe events caused
> those messages.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
> kernel/trace/trace_kprobe.c | 1 +
> kernel/trace/trace_probe.c | 1 +
> kernel/trace/trace_uprobe.c | 1 +
> 3 files changed, 3 insertions(+)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index d3729bd..5f688cc 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -16,6 +16,7 @@
> * along with this program; if not, write to the Free Software
> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> */
> +#define pr_fmt(fmt) "trace_kprobe: " fmt
>
> #include <linux/module.h>
> #include <linux/uaccess.h>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 847c1e0..52478f0 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -21,6 +21,7 @@
> * Copyright (C) IBM Corporation, 2010-2011
> * Author: Srikar Dronamraju
> */
> +#define pr_fmt(fmt) "trace_probe: " fmt
>
> #include "trace_probe.h"
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index e5445ab..2c6b2d0c 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -17,6 +17,7 @@
> * Copyright (C) IBM Corporation, 2010-2012
> * Author: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> */
> +#define pr_fmt(fmt) "trace_kprobe: " fmt
s/kprobe/uprobe/ ?
Thanks,
Namhyung
>
> #include <linux/module.h>
> #include <linux/uaccess.h>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length
2017-02-10 13:21 ` [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length Masami Hiramatsu
@ 2017-02-10 16:04 ` Steven Rostedt
2017-02-15 15:10 ` Masami Hiramatsu
0 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2017-02-10 16:04 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ingo Molnar, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Namhyung Kim, Andrew Morton
On Fri, 10 Feb 2017 22:21:55 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Since tracing/*probe_events will accept a probe definition
> up to 4096 - 2 ('\n' and '\0') bytes, it must show 4094 instead
> of 4096 in warning message.
Actually, during the testing I found that we don't need the '\n'.
echo -n 'p:irq do_IRQ a=@jiffies_64' > kprobe_events
works just fine. My tests work with 4095 characters. Before and after
my patch.
-- Steve
>
> Note that there is one possible case of exceed 4094. If user
> prepare 4096 bytes null-terminated string and syscall write
> it with the count == 4095, then it can be accepted. However,
> if user puts a '\n' after that, it must rejected.
> So IMHO, the warning message should indicate shorter one,
> since it is safer.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
> kernel/trace/trace_probe.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 2a06f1f..847c1e0 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -678,8 +678,9 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> if (done + size < count) {
> if (buf != kbuf)
> break;
> + /* This can accept WRITE_BUFSIZE - 2 ('\n' + '\0') */
> pr_warn("Line length is too long: Should be less than %d\n",
> - WRITE_BUFSIZE);
> + WRITE_BUFSIZE - 2);
> ret = -EINVAL;
> goto out;
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/2] tracing/probe: Show subsystem name in messages
2017-02-10 16:01 ` Namhyung Kim
@ 2017-02-10 16:17 ` Steven Rostedt
2017-02-10 22:35 ` Masami Hiramatsu
2017-02-10 22:36 ` [PATCH V3] " Masami Hiramatsu
0 siblings, 2 replies; 15+ messages in thread
From: Steven Rostedt @ 2017-02-10 16:17 UTC (permalink / raw)
To: Namhyung Kim
Cc: Masami Hiramatsu, Ingo Molnar, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Andrew Morton
On Sat, 11 Feb 2017 01:01:55 +0900
Namhyung Kim <namhyung@kernel.org> wrote:
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index e5445ab..2c6b2d0c 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -17,6 +17,7 @@
> > * Copyright (C) IBM Corporation, 2010-2012
> > * Author: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > */
> > +#define pr_fmt(fmt) "trace_kprobe: " fmt
>
> s/kprobe/uprobe/ ?
Masami,
Can you update both patches and resend?
-- Steve
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/2] tracing/probe: Show subsystem name in messages
2017-02-10 16:17 ` Steven Rostedt
@ 2017-02-10 22:35 ` Masami Hiramatsu
2017-02-10 22:36 ` [PATCH V3] " Masami Hiramatsu
1 sibling, 0 replies; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-10 22:35 UTC (permalink / raw)
To: Steven Rostedt
Cc: Namhyung Kim, Masami Hiramatsu, Ingo Molnar, linux-kernel,
Peter Zijlstra, Ananth N Mavinakayanahalli, Thomas Gleixner,
H . Peter Anvin, Andrew Morton
On Fri, 10 Feb 2017 11:17:29 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sat, 11 Feb 2017 01:01:55 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
> > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > > index e5445ab..2c6b2d0c 100644
> > > --- a/kernel/trace/trace_uprobe.c
> > > +++ b/kernel/trace/trace_uprobe.c
> > > @@ -17,6 +17,7 @@
> > > * Copyright (C) IBM Corporation, 2010-2012
> > > * Author: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > > */
> > > +#define pr_fmt(fmt) "trace_kprobe: " fmt
> >
> > s/kprobe/uprobe/ ?
>
> Masami,
>
> Can you update both patches and resend?
Oops! OK.
Thanks,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V3] tracing/probe: Show subsystem name in messages
2017-02-10 16:17 ` Steven Rostedt
2017-02-10 22:35 ` Masami Hiramatsu
@ 2017-02-10 22:36 ` Masami Hiramatsu
1 sibling, 0 replies; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-10 22:36 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt
Cc: Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Namhyung Kim, Andrew Morton
Show "trace_probe:", "trace_kprobe:" and "trace_uprobe:"
headers for each warning/error/info message. This will
help people to notice that kprobe/uprobe events caused
those messages.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
--
Changes in v3:
- Fix a typo.
---
kernel/trace/trace_kprobe.c | 1 +
kernel/trace/trace_probe.c | 1 +
kernel/trace/trace_uprobe.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d3729bd..5f688cc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -16,6 +16,7 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#define pr_fmt(fmt) "trace_kprobe: " fmt
#include <linux/module.h>
#include <linux/uaccess.h>
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 847c1e0..52478f0 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -21,6 +21,7 @@
* Copyright (C) IBM Corporation, 2010-2011
* Author: Srikar Dronamraju
*/
+#define pr_fmt(fmt) "trace_probe: " fmt
#include "trace_probe.h"
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index e5445ab..95dd810 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -17,6 +17,7 @@
* Copyright (C) IBM Corporation, 2010-2012
* Author: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
*/
+#define pr_fmt(fmt) "trace_uprobe: " fmt
#include <linux/module.h>
#include <linux/uaccess.h>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length
2017-02-10 16:04 ` Steven Rostedt
@ 2017-02-15 15:10 ` Masami Hiramatsu
2017-02-15 15:31 ` Steven Rostedt
0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-02-15 15:10 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Namhyung Kim, Andrew Morton
On Fri, 10 Feb 2017 11:04:34 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 10 Feb 2017 22:21:55 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > Since tracing/*probe_events will accept a probe definition
> > up to 4096 - 2 ('\n' and '\0') bytes, it must show 4094 instead
> > of 4096 in warning message.
>
> Actually, during the testing I found that we don't need the '\n'.
>
> echo -n 'p:irq do_IRQ a=@jiffies_64' > kprobe_events
>
> works just fine. My tests work with 4095 characters. Before and after
> my patch.
Yeah, I see. I concider the case that if the writer writes
4095 chars command + '\n' + next command, it will fail, that
is what I pointed out below note.
> >
> > Note that there is one possible case of exceed 4094. If user
> > prepare 4096 bytes null-terminated string and syscall write
> > it with the count == 4095, then it can be accepted. However,
> > if user puts a '\n' after that, it must rejected.
> > So IMHO, the warning message should indicate shorter one,
> > since it is safer.
Thank you,
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> > kernel/trace/trace_probe.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index 2a06f1f..847c1e0 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -678,8 +678,9 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> > if (done + size < count) {
> > if (buf != kbuf)
> > break;
> > + /* This can accept WRITE_BUFSIZE - 2 ('\n' + '\0') */
> > pr_warn("Line length is too long: Should be less than %d\n",
> > - WRITE_BUFSIZE);
> > + WRITE_BUFSIZE - 2);
> > ret = -EINVAL;
> > goto out;
> > }
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length
2017-02-15 15:10 ` Masami Hiramatsu
@ 2017-02-15 15:31 ` Steven Rostedt
0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2017-02-15 15:31 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ingo Molnar, linux-kernel, Peter Zijlstra,
Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
Namhyung Kim, Andrew Morton
On Thu, 16 Feb 2017 00:10:30 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> On Fri, 10 Feb 2017 11:04:34 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Fri, 10 Feb 2017 22:21:55 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > > Since tracing/*probe_events will accept a probe definition
> > > up to 4096 - 2 ('\n' and '\0') bytes, it must show 4094 instead
> > > of 4096 in warning message.
> >
> > Actually, during the testing I found that we don't need the '\n'.
> >
> > echo -n 'p:irq do_IRQ a=@jiffies_64' > kprobe_events
> >
> > works just fine. My tests work with 4095 characters. Before and after
> > my patch.
>
> Yeah, I see. I concider the case that if the writer writes
> 4095 chars command + '\n' + next command, it will fail, that
> is what I pointed out below note.
>
> > >
> > > Note that there is one possible case of exceed 4094. If user
> > > prepare 4096 bytes null-terminated string and syscall write
> > > it with the count == 4095, then it can be accepted. However,
> > > if user puts a '\n' after that, it must rejected.
> > > So IMHO, the warning message should indicate shorter one,
> > > since it is safer.
>
> Thank you,
>
OK, I'll just add the patch as is.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-02-15 15:31 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-09 23:04 [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Steven Rostedt
2017-02-10 5:50 ` Masami Hiramatsu
2017-02-10 7:53 ` Ingo Molnar
2017-02-10 10:37 ` Masami Hiramatsu
2017-02-10 14:05 ` Steven Rostedt
2017-02-10 13:21 ` [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length Masami Hiramatsu
2017-02-10 16:04 ` Steven Rostedt
2017-02-15 15:10 ` Masami Hiramatsu
2017-02-15 15:31 ` Steven Rostedt
2017-02-10 13:23 ` [PATCH V2 2/2] tracing/probe: Show subsystem name in messages Masami Hiramatsu
2017-02-10 16:01 ` Namhyung Kim
2017-02-10 16:17 ` Steven Rostedt
2017-02-10 22:35 ` Masami Hiramatsu
2017-02-10 22:36 ` [PATCH V3] " Masami Hiramatsu
2017-02-10 6:20 ` [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).