[RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[Steven Rostedt]

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

Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[Masami Hiramatsu]

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>

Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[Ingo Molnar]

* 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

Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[Masami Hiramatsu]

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>

Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[Steven Rostedt]

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

[PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length

[Masami Hiramatsu]

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;
 				}

Re: [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length

[Steven Rostedt]

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;
>  				}

Re: [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length

[Masami Hiramatsu]

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>

Re: [PATCH V2 1/2] tracing/probes: Fix a warning message to show correct maximum length

[Steven Rostedt]

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

[PATCH V2 2/2] tracing/probe: Show subsystem name in messages

[Masami Hiramatsu]

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>

Re: [PATCH V2 2/2] tracing/probe: Show subsystem name in messages

[Namhyung Kim]

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>
> 

Re: [PATCH V2 2/2] tracing/probe: Show subsystem name in messages

[Steven Rostedt]

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

Re: [PATCH V2 2/2] tracing/probe: Show subsystem name in messages

[Masami Hiramatsu]

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>

[PATCH V3] tracing/probe: Show subsystem name in messages

[Masami Hiramatsu]

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>

Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily

[Namhyung Kim]

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
>