[PATCH v2 0/5] [RFC] copy_strtok_from_user

[PATCH v2 0/5] [RFC] copy_strtok_from_user

[Steven Rostedt]

This is the second series of the uaccess code.

Changes in v2:

 - moved probe_kernel_* functions to lib/uaccess.c

 - renamed copy_word_from_user to copy_strtok_from_user.

 - changed copy_strtok_from_user to pass in a delimiter string.
   ftrace defines SPACE to be ' \t\r\n'.

Ingo,
I added your copy right to lib/uaccess.c since git blame shows you
as the author of the probe_kernel_* code. Also, is it OK that I
added the "GPL v2" line in that file as well?

Andrew,
Since you are, in essence, the memory maintainer, could you give
your Acked-by: to the copy_strtok_from_user code.

The probe_kernel code is still EXPORT_SYMBOL_GPL, and I added
that too to copy_strtok_from_user. Are there any objections to that?

-- Steve



The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: tip/tracing/uaccess


Steven Rostedt (5):
      uaccess: move probe_kernel_* functions to lib/uaccess.c
      uaccess: add copy_strtok_from_user
      tracing: convert event_trace to use copy_strtok_from_user
      tracing: convert ftrace_regex_write to use copy_strtok_from_user
      tracing: convert ftrace_graph_write to use copy_strtok_from_user

----
 include/linux/uaccess.h     |    5 +
 kernel/trace/ftrace.c       |  155 +++++++++++++++---------------
 kernel/trace/trace.h        |    2 +
 kernel/trace/trace_events.c |   78 +++++++--------
 lib/Makefile                |    3 +-
 lib/uaccess.c               |  229 +++++++++++++++++++++++++++++++++++++++++++
 mm/Makefile                 |    2 +-
 mm/maccess.c                |   55 ----------
 8 files changed, 351 insertions(+), 178 deletions(-)
-- 

[PATCH v2 1/5] uaccess: move probe_kernel_* functions to lib/uaccess.c

[Steven Rostedt]

[-- Attachment #1: 0001-uaccess-move-probe_kernel_-functions-to-lib-uacces.patch --]
[-- Type: text/plain, Size: 4955 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

This patch moves the file mm/maccess.c to lib/uaccess.c. This
is a more generic location to find lib utils. Since probe_kernel_*
are declared in linux/uaccess.h, having them in a file named uaccess.c
seems to be a better place.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 lib/Makefile  |    3 ++-
 lib/uaccess.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/Makefile   |    2 +-
 mm/maccess.c  |   55 -------------------------------------------------------
 4 files changed, 58 insertions(+), 57 deletions(-)
 create mode 100644 lib/uaccess.c
 delete mode 100644 mm/maccess.c

diff --git a/lib/Makefile b/lib/Makefile
index 32b0e64..46ce28c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -11,7 +11,8 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 rbtree.o radix-tree.o dump_stack.o \
 	 idr.o int_sqrt.o extable.o prio_tree.o \
 	 sha1.o irq_regs.o reciprocal_div.o argv_split.o \
-	 proportions.o prio_heap.o ratelimit.o show_mem.o is_single_threaded.o
+	 proportions.o prio_heap.o ratelimit.o show_mem.o is_single_threaded.o \
+	 uaccess.o
 
 lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o
diff --git a/lib/uaccess.c b/lib/uaccess.c
new file mode 100644
index 0000000..ac40796
--- /dev/null
+++ b/lib/uaccess.c
@@ -0,0 +1,55 @@
+/*
+ * Access kernel memory without faulting.
+ */
+#include <linux/uaccess.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+
+/**
+ * probe_kernel_read(): safely attempt to read from a location
+ * @dst: pointer to the buffer that shall take the data
+ * @src: address to read from
+ * @size: size of the data chunk
+ *
+ * Safely read from address @src to the buffer at @dst.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+long probe_kernel_read(void *dst, void *src, size_t size)
+{
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	pagefault_disable();
+	ret = __copy_from_user_inatomic(dst,
+			(__force const void __user *)src, size);
+	pagefault_enable();
+	set_fs(old_fs);
+
+	return ret ? -EFAULT : 0;
+}
+EXPORT_SYMBOL_GPL(probe_kernel_read);
+
+/**
+ * probe_kernel_write(): safely attempt to write to a location
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
+ *
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+long probe_kernel_write(void *dst, void *src, size_t size)
+{
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	pagefault_disable();
+	ret = __copy_to_user_inatomic((__force void __user *)dst, src, size);
+	pagefault_enable();
+	set_fs(old_fs);
+
+	return ret ? -EFAULT : 0;
+}
+EXPORT_SYMBOL_GPL(probe_kernel_write);
diff --git a/mm/Makefile b/mm/Makefile
index 72255be..90323d1 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -8,7 +8,7 @@ mmu-$(CONFIG_MMU)	:= fremap.o highmem.o madvise.o memory.o mincore.o \
 			   vmalloc.o
 
 obj-y			:= bootmem.o filemap.o mempool.o oom_kill.o fadvise.o \
-			   maccess.o page_alloc.o page-writeback.o pdflush.o \
+			   page_alloc.o page-writeback.o pdflush.o \
 			   readahead.o swap.o truncate.o vmscan.o shmem.o \
 			   prio_tree.o util.o mmzone.o vmstat.o backing-dev.o \
 			   page_isolation.o mm_init.o $(mmu-y)
diff --git a/mm/maccess.c b/mm/maccess.c
deleted file mode 100644
index ac40796..0000000
--- a/mm/maccess.c
+++ /dev/null
@@ -1,55 +0,0 @@
-/*
- * Access kernel memory without faulting.
- */
-#include <linux/uaccess.h>
-#include <linux/module.h>
-#include <linux/mm.h>
-
-/**
- * probe_kernel_read(): safely attempt to read from a location
- * @dst: pointer to the buffer that shall take the data
- * @src: address to read from
- * @size: size of the data chunk
- *
- * Safely read from address @src to the buffer at @dst.  If a kernel fault
- * happens, handle that and return -EFAULT.
- */
-long probe_kernel_read(void *dst, void *src, size_t size)
-{
-	long ret;
-	mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	pagefault_disable();
-	ret = __copy_from_user_inatomic(dst,
-			(__force const void __user *)src, size);
-	pagefault_enable();
-	set_fs(old_fs);
-
-	return ret ? -EFAULT : 0;
-}
-EXPORT_SYMBOL_GPL(probe_kernel_read);
-
-/**
- * probe_kernel_write(): safely attempt to write to a location
- * @dst: address to write to
- * @src: pointer to the data that shall be written
- * @size: size of the data chunk
- *
- * Safely write to address @dst from the buffer at @src.  If a kernel fault
- * happens, handle that and return -EFAULT.
- */
-long probe_kernel_write(void *dst, void *src, size_t size)
-{
-	long ret;
-	mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	pagefault_disable();
-	ret = __copy_to_user_inatomic((__force void __user *)dst, src, size);
-	pagefault_enable();
-	set_fs(old_fs);
-
-	return ret ? -EFAULT : 0;
-}
-EXPORT_SYMBOL_GPL(probe_kernel_write);
-- 
1.5.6.5

-- 

[PATCH v2 2/5] uaccess: add copy_strtok_from_user

[Steven Rostedt]

[-- Attachment #1: 0002-uaccess-add-copy_strtok_from_user.patch --]
[-- Type: text/plain, Size: 7012 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The ftrace utility reads delimited tokens from user space.
Andrew Morton did not like how ftrace open coded this. He had
a good point since more than one location performed this feature.

This patch creates a copy_strtok_from_user function that can copy
a delimited token from user space. This puts the code in  the
lib/uaccess.c file. This keeps the code in a single location
and may be optimized in the future.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 include/linux/uaccess.h |    5 ++
 lib/uaccess.c           |  176 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 180 insertions(+), 1 deletions(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 6b58367..08769ad 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -106,4 +106,9 @@ extern long probe_kernel_read(void *dst, void *src, size_t size);
  */
 extern long probe_kernel_write(void *dst, void *src, size_t size);
 
+extern int copy_strtok_from_user(void *to, const void __user *from,
+				 unsigned int copy, unsigned int read,
+				 unsigned int *copied, int skip,
+				 const char *delim);
+
 #endif		/* __LINUX_UACCESS_H__ */
diff --git a/lib/uaccess.c b/lib/uaccess.c
index ac40796..0c12360 100644
--- a/lib/uaccess.c
+++ b/lib/uaccess.c
@@ -1,8 +1,19 @@
 /*
- * Access kernel memory without faulting.
+ * lib/uaccess.c
+ *   Generic memory access without faulting.
+ *
+ *  Copyright (C) 2008 Red Hat, Inc., Ingo Molnar <mingo@redhat.com>
+ *
+ *  Added copy_strtok_from_user -
+ *     Copyright (C) 2009 Red Hat, Inc., Steven Rostedt <srostedt@redhat.com>
+ *
+ * This source code is licensed under the GNU General Public License,
+ * Version 2.  See the file COPYING for more details.
+ *
  */
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/ctype.h>
 #include <linux/mm.h>
 
 /**
@@ -53,3 +64,166 @@ long probe_kernel_write(void *dst, void *src, size_t size)
 	return ret ? -EFAULT : 0;
 }
 EXPORT_SYMBOL_GPL(probe_kernel_write);
+
+/**
+ * copy_strtok_from_user - copy a delimited token from user space
+ * @to:	     The location to copy to
+ * @from:    The location to copy from
+ * @copy:    The number of bytes to copy
+ * @read:    The number of bytes to read
+ * @copied:  The number of bytes actually copied to @to
+ * @skip:    If other than zero, will skip leading white space
+ * @delim:   NULL terminated string of character delimiters
+ *
+ * This reads from a user buffer, a delimited toke.
+ * If skip is set, then it will trim all leading delimiters.
+ * Then it will copy thet token (non delimiter characters) until
+ * @copy bytes have been copied, @read bytes have been read from
+ * the user buffer, or more delimiters have been encountered.
+ *
+ * Note, if skip is not set, and a dilmiter exists at the beginning
+ *  it will return immediately.
+ *
+ * Example of use:
+ *
+ *  in user space a write(fd, "foo  bar zot", 12) is done. We want to
+ *  read three words.
+ *
+ *  len = 12;  - length of user buffer
+ *  ret = copy_strtok_from_user(buf, ubuf, 100, len, @copied, 1, " ");
+ *    ret will equal 4 ("foo " read)
+ *    buf will contain "foo"
+ *    copied will equal 3 ("foo" copied)
+ *    Note, @skip could be 1 or zero and the same would have happened
+ *          since there was no leading space.
+ *
+ *  len -= ret;  - 4 bytes was read
+ *  read = ret;
+ *  ret = copy_strtok_from_user(buf, ubuf+read, 100, len, @copied, 1, " ");
+ *    ret will equal 5 (" bar " read, notice the double space between
+ *                       foo and bar in the original write.)
+ *    buf will contain "bar"
+ *    copied will equal 3 ("bar" copied)
+ *    Note, @skip is 1, if it was zero the results would be different.
+ *          (see below)
+ *
+ *  len -= ret; - 5 bytes read
+ *  read += ret;
+ *  ret = copy_strtok_from_user(buf, ubuf+read, 100, len, @copied, 1, " ");
+ *   ret = -EAGAIN (no space after "zot")
+ *   buf will contain "zot"
+ *   copied will equal 3 ("zot" copied)
+ *
+ * If the second copy_strtok_from_user above had 0 for @skip, where we
+ * did not want to skip leading space (" bar zot")
+ *   ret will equal 1 (" " read)
+ *   buf will not be modified
+ *   copied will equal 0 (nothing copied).
+ *
+ * Returns:
+ *  The number of bytes read from user space (@from). This may or may not
+ *  be the same as what was copied into @to.
+ *
+ *  -EAGAIN, if we copied a token successfully, but never hit an
+ *    ending delimiter. The number of bytes copied will be the same
+ *    as @read. Note, if skip is set, and all we hit were delimiters
+ *    then we will also returne -EAGAIN with @copied = 0.
+ *
+ *  @copied will contain the number of bytes copied into @to
+ *
+ *  -EFAULT, if we faulted during any part of the copy.
+ *     @copied will be undefined.
+ *
+ *  -EINVAL, if we fill up @from before hitting a single delimiter.
+ *    @copy must be bigger than the expected token to read.
+ */
+int copy_strtok_from_user(void *to, const void __user *from,
+			  unsigned int copy, unsigned int read,
+			  unsigned int *copied, int skip,
+			  const char *delim)
+{
+	unsigned int have_read = 0;
+	unsigned int have_copied = 0;
+	const char __user *user = from;
+	char *kern = to;
+	int ret, len;
+	char ch;
+
+	/* get the first character */
+	ret = get_user(ch, user++);
+	if (ret)
+		return ret;
+	have_read++;
+
+	len = strlen(delim);
+
+	/*
+	 * If skip is set, and the first character is a delimiter
+	 * then we will continue to read until we find a non delimiter.
+	 */
+	if (skip) {
+		while (have_read < read && memchr(delim, ch, len)) {
+			ret = get_user(ch, user++);
+			if (ret)
+				return ret;
+			have_read++;
+		}
+
+		/*
+		 * If ch is still a delimiter, then have_read == read.
+		 * We successfully copied zero bytes. But this is
+		 * still valid. Just let the caller try again.
+		 */
+		if (memchr(delim, ch, len)) {
+			ret = -EAGAIN;
+			goto out;
+		}
+	} else if (memchr(delim, ch, len)) {
+		/*
+		 * If skip was not set and the first character was
+		 * a delimiter, then we return immediately.
+		 */
+		ret = have_read;
+		goto out;
+	}
+
+
+	/* Now read the actual token */
+	while (have_read < read &&
+	       have_copied < copy && !memchr(delim, ch, len)) {
+
+		kern[have_copied++] = ch;
+
+		ret = get_user(ch, user++);
+		if (ret)
+			return ret;
+
+		have_read++;
+	}
+
+	/*
+	 * If we ended with a delimiter then we have successfully
+	 * read in a full token.
+	 *
+	 * If ch is not a delimiter, and we have filled up @from,
+	 * then this was an invalid token.
+	 *
+	 * If ch is not white space, and we still have room in @from
+	 * then we let the caller know we have split a token.
+	 *  (have_read == read)
+	 */
+	if (memchr(delim, ch, len))
+		ret = have_read;
+	else if (have_copied == copy)
+		ret = -EINVAL;
+	else {
+		WARN_ON(have_read != read);
+		ret = -EAGAIN;
+	}
+
+ out:
+	*copied = have_copied;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(copy_strtok_from_user);
-- 
1.5.6.5

-- 

[PATCH v2 3/5] tracing: convert event_trace to use copy_strtok_from_user

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-convert-event_trace-to-use-copy_strtok_from.patch --]
[-- Type: text/plain, Size: 2841 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

This patch converts the open coded retrieving of a word from user
space to use copy_strtok_from_user.

Also removed a cnt < 0 check that Andrew Morton pointed out saying
that it was irrelevant since cnt is unsigned.

Also changed file->pos += to (*ppos) += which is the proper way
to modify positions of the file.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.h        |    2 +
 kernel/trace/trace_events.c |   78 +++++++++++++++++++------------------------
 2 files changed, 36 insertions(+), 44 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 6321917..e75b673 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -720,4 +720,6 @@ static inline void trace_branch_disable(void)
 }
 #endif /* CONFIG_BRANCH_TRACER */
 
+#define SPACES	" \t\r\n"
+
 #endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 3bcb9df..ab4da24 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -80,62 +80,52 @@ static ssize_t
 ftrace_event_write(struct file *file, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)
 {
-	size_t read = 0;
-	int i, set = 1;
+	unsigned int copied;
+	size_t read;
 	ssize_t ret;
+	int set = 1;
 	char *buf;
-	char ch;
 
-	if (!cnt || cnt < 0)
+	if (!cnt)
 		return 0;
 
-	ret = get_user(ch, ubuf++);
-	if (ret)
-		return ret;
-	read++;
-	cnt--;
-
-	/* skip white space */
-	while (cnt && isspace(ch)) {
-		ret = get_user(ch, ubuf++);
-		if (ret)
-			return ret;
-		read++;
-		cnt--;
-	}
-
-	/* Only white space found? */
-	if (isspace(ch)) {
-		file->f_pos += read;
-		ret = read;
-		return ret;
-	}
-
 	buf = kmalloc(EVENT_BUF_SIZE+1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	if (cnt > EVENT_BUF_SIZE)
-		cnt = EVENT_BUF_SIZE;
-
-	i = 0;
-	while (cnt && !isspace(ch)) {
-		if (!i && ch == '!')
-			set = 0;
-		else
-			buf[i++] = ch;
-
-		ret = get_user(ch, ubuf++);
-		if (ret)
-			goto out_free;
-		read++;
-		cnt--;
+	ret = copy_strtok_from_user(buf, ubuf, EVENT_BUF_SIZE,
+				    cnt, &copied, 1, SPACES);
+
+	if (ret == -EAGAIN) {
+		/*
+		 * echo > set_event
+		 *  This will cause copy_word_from_user to return -EAGAIN
+		 *  but copied will be 0.
+		 */
+		if (!copied) {
+			ret = cnt;
+			(*ppos) += cnt;
+		}
+		/* TODO, handle split words */
+		goto out_free;
 	}
-	buf[i] = 0;
 
-	file->f_pos += read;
+	if (ret < 0)
+		goto out_free;
+
+	buf[copied] = 0;
+
+	if (buf[0] == '!')
+		set = 0;
+
+	(*ppos) += ret;
+	read = ret;
 
-	ret = ftrace_set_clr_event(buf, set);
+	/*
+	 * A little hack here. If set is true, we want to use buf.
+	 * Otherwise, we want to use buf+1 (to skip the '!').
+	 */
+	ret = ftrace_set_clr_event(buf + !set, set);
 	if (ret)
 		goto out_free;
 
-- 
1.5.6.5

-- 

[PATCH v2 4/5] tracing: convert ftrace_regex_write to use copy_strtok_from_user

[Steven Rostedt]

[-- Attachment #1: 0004-tracing-convert-ftrace_regex_write-to-use-copy_strt.patch --]
[-- Type: text/plain, Size: 3382 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

This removes the open coded parsing of a word sent in by the user
and replaces it with copy_strtok_from_user.

Also removes cnt < 0 check since cnt is unsigned.

Also uses (*ppos) += cnt, instead of file->pos += cnt.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |  103 ++++++++++++++++++++++++++++---------------------
 1 files changed, 59 insertions(+), 44 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 5a3a06b..4ecce15 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1690,11 +1690,13 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos, int enable)
 {
 	struct ftrace_iterator *iter;
-	char ch;
+	unsigned int copied;
 	size_t read = 0;
+	int len, skip;
 	ssize_t ret;
+	char *buf;
 
-	if (!cnt || cnt < 0)
+	if (!cnt)
 		return 0;
 
 	mutex_lock(&ftrace_regex_lock);
@@ -1710,58 +1712,71 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 		iter->buffer_idx = 0;
 	}
 
-	ret = get_user(ch, ubuf++);
-	if (ret)
-		goto out;
-	read++;
-	cnt--;
+	if (iter->flags & FTRACE_ITER_CONT) {
+		/*
+		 * We split on a word, continue from where
+		 * we left off.
+		*/
+		buf = &iter->buffer[iter->buffer_idx];
+		len = FTRACE_BUFF_MAX - iter->buffer_idx;
+		/* Stop on leading spaces */
+		skip = 0;
+	} else {
+		buf = iter->buffer;
+		/* iter->buffer is size of FTRACE_BUF_MAX + 1 */
+		len = FTRACE_BUFF_MAX;
+		iter->buffer_idx = 0;
+		/* Skip over any leading spaces */
+		skip = 1;
+	}
 
-	if (!(iter->flags & ~FTRACE_ITER_CONT)) {
-		/* skip white space */
-		while (cnt && isspace(ch)) {
-			ret = get_user(ch, ubuf++);
-			if (ret)
+	ret = copy_strtok_from_user(buf, ubuf, len, cnt, &copied, skip, SPACES);
+
+	if (ret == -EAGAIN) {
+		if (copied) {
+			/* we split on a word */
+			iter->buffer_idx += copied;
+			if (iter->buffer_idx >= FTRACE_BUFF_MAX) {
+				/* too big of a word */
+				iter->buffer_idx = 0;
+				iter->flags &= ~FTRACE_ITER_CONT;
+				ret = -EINVAL;
 				goto out;
-			read++;
-			cnt--;
-		}
+			}
 
-		if (isspace(ch)) {
-			file->f_pos += read;
-			ret = read;
+			(*ppos) += cnt;
+			ret = cnt;
+			iter->flags |= FTRACE_ITER_CONT;
 			goto out;
 		}
-
-		iter->buffer_idx = 0;
-	}
-
-	while (cnt && !isspace(ch)) {
-		if (iter->buffer_idx < FTRACE_BUFF_MAX)
-			iter->buffer[iter->buffer_idx++] = ch;
-		else {
-			ret = -EINVAL;
+		/* We only read white space. */
+		if (!(iter->flags & FTRACE_ITER_CONT)) {
+			(*ppos) += cnt;
+			ret = cnt;
 			goto out;
 		}
-		ret = get_user(ch, ubuf++);
-		if (ret)
-			goto out;
-		read++;
-		cnt--;
+
+		/*
+		 * We read white space but we are also at the end
+		 * of a word in the buffer. Continue processing.
+		 */
+		ret = cnt;
 	}
 
-	if (isspace(ch)) {
-		iter->filtered++;
-		iter->buffer[iter->buffer_idx] = 0;
-		ret = ftrace_process_regex(iter->buffer,
-					   iter->buffer_idx, enable);
-		if (ret)
-			goto out;
-		iter->buffer_idx = 0;
-	} else
-		iter->flags |= FTRACE_ITER_CONT;
+	if (ret < 0)
+		goto out;
 
+	/* NULL terminate the string */
+	buf[copied] = 0;
 
-	file->f_pos += read;
+	read = ret;
+
+	ret = ftrace_process_regex(iter->buffer, iter->buffer_idx, enable);
+	if (ret)
+		goto out;
+	iter->buffer_idx = 0;
+
+	(*ppos) += read;
 
 	ret = read;
  out:
-- 
1.5.6.5

-- 

[PATCH v2 5/5] tracing: convert ftrace_graph_write to use copy_strtok_from_user

[Steven Rostedt]

[-- Attachment #1: 0005-tracing-convert-ftrace_graph_write-to-use-copy_strt.patch --]
[-- Type: text/plain, Size: 2086 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

This removes the open coded parsing of a word sent in by the user
and replaces it with copy_strtok_from_user.

Also removes cnt < 0 check since cnt is unsigned.

Also uses (*ppos) += cnt, instead of file->pos += cnt.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |   52 +++++++++++++++++-------------------------------
 1 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4ecce15..6204b3c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2079,10 +2079,9 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,
 	unsigned long *array;
 	size_t read = 0;
 	ssize_t ret;
-	int index = 0;
-	char ch;
+	int copied;
 
-	if (!cnt || cnt < 0)
+	if (!cnt)
 		return 0;
 
 	mutex_lock(&graph_lock);
@@ -2098,48 +2097,35 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,
 	} else
 		array = file->private_data;
 
-	ret = get_user(ch, ubuf++);
-	if (ret)
-		goto out;
-	read++;
-	cnt--;
+	ret = copy_strtok_from_user(buffer, ubuf, FTRACE_BUFF_MAX,
+				    cnt, &copied, 1, SPACES);
 
-	/* skip white space */
-	while (cnt && isspace(ch)) {
-		ret = get_user(ch, ubuf++);
-		if (ret)
+	if (ret == -EAGAIN) {
+		if (copied) {
+			/* This do not deal with split words */
+			ret = -EINVAL;
 			goto out;
-		read++;
-		cnt--;
-	}
+		}
 
-	if (isspace(ch)) {
-		*ppos += read;
-		ret = read;
+		/* Just white space */
+		(*ppos) += cnt;
+		ret = cnt;
 		goto out;
 	}
 
-	while (cnt && !isspace(ch)) {
-		if (index < FTRACE_BUFF_MAX)
-			buffer[index++] = ch;
-		else {
-			ret = -EINVAL;
-			goto out;
-		}
-		ret = get_user(ch, ubuf++);
-		if (ret)
-			goto out;
-		read++;
-		cnt--;
-	}
-	buffer[index] = 0;
+	if (ret < 0)
+		goto out;
+
+	read = ret;
+
+	buffer[copied] = 0;
 
 	/* we allow only one expression at a time */
 	ret = ftrace_set_func(array, &ftrace_graph_count, buffer);
 	if (ret)
 		goto out;
 
-	file->f_pos += read;
+	(*ppos) += read;
 
 	ret = read;
  out:
-- 
1.5.6.5

-- 

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Peter Zijlstra]

On Thu, 2009-02-26 at 00:32 -0500, Steven Rostedt wrote:
> This is the second series of the uaccess code.
> 
> Changes in v2:
> 
>  - moved probe_kernel_* functions to lib/uaccess.c
> 
>  - renamed copy_word_from_user to copy_strtok_from_user.
> 
>  - changed copy_strtok_from_user to pass in a delimiter string.
>    ftrace defines SPACE to be ' \t\r\n'.
> 
> Ingo,
> I added your copy right to lib/uaccess.c since git blame shows you
> as the author of the probe_kernel_* code. Also, is it OK that I
> added the "GPL v2" line in that file as well?
> 
> Andrew,
> Since you are, in essence, the memory maintainer, could you give
> your Acked-by: to the copy_strtok_from_user code.
> 
> The probe_kernel code is still EXPORT_SYMBOL_GPL, and I added
> that too to copy_strtok_from_user. Are there any objections to that?

I have to ask,..

cant this be done with a regular copy_from_user() followed by a regular
strtok()? Do we really have to combine all that?

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, 2009-02-26 at 00:32 -0500, Steven Rostedt wrote:
> > This is the second series of the uaccess code.
> > 
> > Changes in v2:
> > 
> >  - moved probe_kernel_* functions to lib/uaccess.c
> > 
> >  - renamed copy_word_from_user to copy_strtok_from_user.
> > 
> >  - changed copy_strtok_from_user to pass in a delimiter string.
> >    ftrace defines SPACE to be ' \t\r\n'.
> > 
> > Ingo,
> > I added your copy right to lib/uaccess.c since git blame shows you
> > as the author of the probe_kernel_* code. Also, is it OK that I
> > added the "GPL v2" line in that file as well?
> > 
> > Andrew,
> > Since you are, in essence, the memory maintainer, could you give
> > your Acked-by: to the copy_strtok_from_user code.
> > 
> > The probe_kernel code is still EXPORT_SYMBOL_GPL, and I added
> > that too to copy_strtok_from_user. Are there any objections to that?
> 
> I have to ask,..
> 
> cant this be done with a regular copy_from_user() followed by 
> a regular strtok()? Do we really have to combine all that?

yes. Note that strsep() is the preferred API. (in fact it's the 
only such string API that is in the kernel)

	Ingo

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, 2009-02-26 at 00:32 -0500, Steven Rostedt wrote:
> > > This is the second series of the uaccess code.
> > > 
> > > Changes in v2:
> > > 
> > >  - moved probe_kernel_* functions to lib/uaccess.c
> > > 
> > >  - renamed copy_word_from_user to copy_strtok_from_user.
> > > 
> > >  - changed copy_strtok_from_user to pass in a delimiter string.
> > >    ftrace defines SPACE to be ' \t\r\n'.
> > > 
> > > Ingo,
> > > I added your copy right to lib/uaccess.c since git blame shows you
> > > as the author of the probe_kernel_* code. Also, is it OK that I
> > > added the "GPL v2" line in that file as well?
> > > 
> > > Andrew,
> > > Since you are, in essence, the memory maintainer, could you give
> > > your Acked-by: to the copy_strtok_from_user code.
> > > 
> > > The probe_kernel code is still EXPORT_SYMBOL_GPL, and I added
> > > that too to copy_strtok_from_user. Are there any objections to that?
> > 
> > I have to ask,..
> > 
> > cant this be done with a regular copy_from_user() followed 
> > by a regular strtok()? Do we really have to combine all 
> > that?
> 
> yes. Note that strsep() is the preferred API. (in fact it's 
> the only such string API that is in the kernel)

btw., for larger strings we cannot really copy into the kernel i 
suspect. We'd have to kmalloc() and that adds overhead, etc.

Nevertheless, copy_strsep_from_user() would be the symmetric API 
here.

	Ingo

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Steven Rostedt]

On Thu, 26 Feb 2009, Ingo Molnar wrote:

> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Thu, 2009-02-26 at 00:32 -0500, Steven Rostedt wrote:
> > > > This is the second series of the uaccess code.
> > > > 
> > > > Changes in v2:
> > > > 
> > > >  - moved probe_kernel_* functions to lib/uaccess.c
> > > > 
> > > >  - renamed copy_word_from_user to copy_strtok_from_user.
> > > > 
> > > >  - changed copy_strtok_from_user to pass in a delimiter string.
> > > >    ftrace defines SPACE to be ' \t\r\n'.
> > > > 
> > > > Ingo,
> > > > I added your copy right to lib/uaccess.c since git blame shows you
> > > > as the author of the probe_kernel_* code. Also, is it OK that I
> > > > added the "GPL v2" line in that file as well?
> > > > 
> > > > Andrew,
> > > > Since you are, in essence, the memory maintainer, could you give
> > > > your Acked-by: to the copy_strtok_from_user code.
> > > > 
> > > > The probe_kernel code is still EXPORT_SYMBOL_GPL, and I added
> > > > that too to copy_strtok_from_user. Are there any objections to that?
> > > 
> > > I have to ask,..
> > > 
> > > cant this be done with a regular copy_from_user() followed 
> > > by a regular strtok()? Do we really have to combine all 
> > > that?
> > 
> > yes. Note that strsep() is the preferred API. (in fact it's 
> > the only such string API that is in the kernel)
> 
> btw., for larger strings we cannot really copy into the kernel i 
> suspect. We'd have to kmalloc() and that adds overhead, etc.
> 
> Nevertheless, copy_strsep_from_user() would be the symmetric API 
> here.

not really. strsep stops at the first delimiter. strtok skips multiple 
delimiters. I could implement the copy_strtok_from_user with a 
copy_from_user (all of read), and then use strtok_r (reentrant version) to 
find the next token. This would require implementing strtok_r for the 
kernel.

Having the copy_strtok_from_user will handle the copying and the caller 
would not need to worry about having a big enough buffer to hold 
delimiters and tokens. The copy_strtok_from_user would copy first to an 
internal buffer, then scan for tokens to copy into the callers buffer. 
Then repeat until read or copy limits have been hit.

-- Steve

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Steven Rostedt]

On Thu, 26 Feb 2009, Steven Rostedt wrote:
> 
> not really. strsep stops at the first delimiter. strtok skips multiple 

I may have read the man page wrong. It does stop at the token not the 
delimiter.

Never mind :-p

-- Steve

> delimiters. I could implement the copy_strtok_from_user with a 
> copy_from_user (all of read), and then use strtok_r (reentrant version) to 
> find the next token. This would require implementing strtok_r for the 
> kernel.
> 
> Having the copy_strtok_from_user will handle the copying and the caller 
> would not need to worry about having a big enough buffer to hold 
> delimiters and tokens. The copy_strtok_from_user would copy first to an 
> internal buffer, then scan for tokens to copy into the callers buffer. 
> Then repeat until read or copy limits have been hit.
> 
> -- Steve
> 
> 

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Steven Rostedt]

On Thu, 26 Feb 2009, Ingo Molnar wrote:
> > 
> > yes. Note that strsep() is the preferred API. (in fact it's 
> > the only such string API that is in the kernel)
> 
> btw., for larger strings we cannot really copy into the kernel i 
> suspect. We'd have to kmalloc() and that adds overhead, etc.
> 
> Nevertheless, copy_strsep_from_user() would be the symmetric API 
> here.

OK, I'll rebase again calling it copy_strsep_from_user. And I'll change 
the implementation to use copy_from_user/strsep

-- Steve

Re: [PATCH v2 0/5] [RFC] copy_strtok_from_user

[Steven Rostedt]

On Thu, 26 Feb 2009, Steven Rostedt wrote:

> 
> On Thu, 26 Feb 2009, Ingo Molnar wrote:
> > > 
> > > yes. Note that strsep() is the preferred API. (in fact it's 
> > > the only such string API that is in the kernel)
> > 
> > btw., for larger strings we cannot really copy into the kernel i 
> > suspect. We'd have to kmalloc() and that adds overhead, etc.
> > 
> > Nevertheless, copy_strsep_from_user() would be the symmetric API 
> > here.
> 
> OK, I'll rebase again calling it copy_strsep_from_user. And I'll change 
> the implementation to use copy_from_user/strsep

I need to have my first cup of coffee _before_ replying :-p

My first understanding of strsep was correct.  strsep stops at the first 
delimiter. It may be time to add strtok_r into the kernel.

-- Steve