[PATCH 0/4] [git pull] for tip/tracing/ftrace

[PATCH 0/4] [git pull] for tip/tracing/ftrace

[Steven Rostedt]

Ingo,

This patch series implements copy_word_from_user. I found three open
coded locations in ftrace that implemented this. It basically allows
the kernel to read a space delimited word from user space.

This change is a nice clean up to the code. It also fixes a minor
bug that was in one of the implementations. That is, if there was
too much space between the words, it could cause another unneeded
iteration back to userspace. The result was the same, but the
implementation was not clean.

Please pull the latest tip/tracing/ftrace tree, which can be found at:

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


Steven Rostedt (4):
      uaccess: add copy_word_from_user
      tracing: convert event_trace to use copy_word_from_user
      tracing: convert ftrace_regex_write to use copy_word_from_user
      tracing: convert ftrace_graph_write to use copy_word_from_user

----
 include/linux/uaccess.h     |    4 +
 kernel/trace/ftrace.c       |  155 ++++++++++++++++++++++---------------------
 kernel/trace/trace_events.c |   78 ++++++++++------------
 lib/Makefile                |    3 +-
 lib/uaccess.c               |  134 +++++++++++++++++++++++++++++++++++++
 5 files changed, 252 insertions(+), 122 deletions(-)

-- 

[PATCH 1/4] uaccess: add copy_word_from_user

[Steven Rostedt]

[-- Attachment #1: 0001-uaccess-add-copy_word_from_user.patch --]
[-- Type: text/plain, Size: 5574 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The ftrace utility reads space delimited words 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_word_from_user function that can copy
a space delimited word from user space. This puts the code in
a new 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 |    4 ++
 lib/Makefile            |    3 +-
 lib/uaccess.c           |  134 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletions(-)
 create mode 100644 lib/uaccess.c

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 6b58367..2d706d9 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -106,4 +106,8 @@ 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_word_from_user(void *to, const void __user *from,
+			       unsigned int copy, unsigned int read,
+			       unsigned int *copied, int skip);
+
 #endif		/* __LINUX_UACCESS_H__ */
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..5b9a4ac
--- /dev/null
+++ b/lib/uaccess.c
@@ -0,0 +1,134 @@
+/*
+ * lib/uaccess.c
+ * generic user access file.
+ *
+ * started by Steven Rostedt
+ *
+ *  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/ctype.h>
+
+/**
+ * copy_word_from_user - copy a space delimited word 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
+ *
+ * This reads from a user buffer, a space delimited word.
+ * If skip is set, then it will trim all leading white space.
+ * Then it will copy all non white space until @copy bytes have
+ * been copied, @read bytes have been read from the user buffer,
+ * or more white space has been encountered.
+ *
+ * Note, if skip is not set, and white space exists at the beginning
+ *  it will return immediately.
+ *
+ * Returns:
+ *  The number of bytes read from user space
+ *
+ *  -EAGAIN, if we copied a word successfully, but never hit
+ *    ending white space. The number of bytes copied will be the same
+ *    as @read. Note, if skip is set, and all we hit was white space
+ *    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 white space.
+ *    @copy must be bigger than the expected word to read.
+ */
+int copy_word_from_user(void *to, const void __user *from,
+			unsigned int copy, unsigned int read,
+			unsigned int *copied, int skip)
+{
+	unsigned int have_read = 0;
+	unsigned int have_copied = 0;
+	const char __user *user = from;
+	char *kern = to;
+	int ret;
+	char ch;
+
+	/* get the first character */
+	ret = get_user(ch, user++);
+	if (ret)
+		return ret;
+	have_read++;
+
+	/*
+	 * If skip is set, and the first character is white space
+	 * then we will continue to read until we find non white space.
+	 */
+	if (skip) {
+		while (have_read < read && isspace(ch)) {
+			ret = get_user(ch, user++);
+			if (ret)
+				return ret;
+			have_read++;
+		}
+
+		/*
+		 * If ch is still white space, then have_read == read.
+		 * We successfully copied zero bytes. But this is
+		 * still valid. Just let the caller try again.
+		 */
+		if (isspace(ch)) {
+			ret = -EAGAIN;
+			goto out;
+		}
+	} else if (isspace(ch)) {
+		/*
+		 * If skip was not set and the first character was
+		 * white space, then we return immediately.
+		 */
+		ret = have_read;
+		goto out;
+	}
+
+
+	/* Now read the actual word */
+	while (have_read < read &&
+	       have_copied < copy && !isspace(ch)) {
+
+		kern[have_copied++] = ch;
+
+		ret = get_user(ch, user++);
+		if (ret)
+			return ret;
+
+		have_read++;
+	}
+
+	/*
+	 * If we ended with white space then we have successfully
+	 * read in a full word.
+	 *
+	 * If ch is not white space, and we have filled up @from,
+	 * then this was an invalid word.
+	 *
+	 * If ch is not white space, and we still have room in @from
+	 * then we let the caller know we have split a word.
+	 *  (have_read == read)
+	 */
+	if (isspace(ch))
+		ret = have_read;
+	else if (have_copied == copy)
+		ret = -EINVAL;
+	else {
+		WARN_ON(have_read != read);
+		ret = -EAGAIN;
+	}
+
+ out:
+	*copied = have_copied;
+
+	return ret;
+}
-- 
1.5.6.5

-- 

[PATCH 2/4] tracing: convert event_trace to use copy_word_from_user

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-convert-event_trace-to-use-copy_word_from_u.patch --]
[-- Type: text/plain, Size: 2478 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_word_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_events.c |   78 +++++++++++++++++++------------------------
 1 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 3bcb9df..91ed36c 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_word_from_user(buf, ubuf, EVENT_BUF_SIZE,
+				  cnt, &copied, 1);
+
+	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 3/4] tracing: convert ftrace_regex_write to use copy_word_from_user

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-convert-ftrace_regex_write-to-use-copy_word.patch --]
[-- Type: text/plain, Size: 3370 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_word_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..aaa541d 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_word_from_user(buf, ubuf, len, cnt, &copied, skip);
+
+	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 4/4] tracing: convert ftrace_graph_write to use copy_word_from_user

[Steven Rostedt]

[-- Attachment #1: 0004-tracing-convert-ftrace_graph_write-to-use-copy_word.patch --]
[-- Type: text/plain, Size: 2072 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_word_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 aaa541d..a84915b 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_word_from_user(buffer, ubuf, FTRACE_BUFF_MAX,
+				  cnt, &copied, 1);
 
-	/* 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 1/4] uaccess: add copy_word_from_user

[Andrew Morton]

On Wed, 25 Feb 2009 15:30:08 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> The ftrace utility reads space delimited words 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_word_from_user function that can copy
> a space delimited word from user space. This puts the code in
> a new lib/uaccess.c file. This keeps the code in a single location
> and may be optimized in the future.
> 

Does your code actually still need this?  It is unacceptble to just
be more strict about userspace's write()s?

> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 6b58367..2d706d9 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -106,4 +106,8 @@ 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_word_from_user(void *to, const void __user *from,
> +			       unsigned int copy, unsigned int read,
> +			       unsigned int *copied, int skip);
> +
>  #endif		/* __LINUX_UACCESS_H__ */
> 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..5b9a4ac
> --- /dev/null
> +++ b/lib/uaccess.c
> @@ -0,0 +1,134 @@
> +/*
> + * lib/uaccess.c
> + * generic user access file.

That's a good place for it.  I wonder if we have other uaccess
functions which should be moved here sometime.

> + * started by Steven Rostedt
> + *
> + *  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/ctype.h>
> +
> +/**
> + * copy_word_from_user - copy a space delimited word 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
> + *
> + * This reads from a user buffer, a space delimited word.
> + * If skip is set, then it will trim all leading white space.
> + * Then it will copy all non white space until @copy bytes have
> + * been copied, @read bytes have been read from the user buffer,
> + * or more white space has been encountered.
> + *
> + * Note, if skip is not set, and white space exists at the beginning
> + *  it will return immediately.
> + *
> + * Returns:
> + *  The number of bytes read from user space

Confused.

Is this "the number of bytes which I copied into *to", or is it "the
number of userspace bytes over which I advanced"?

Hopefully the latter, because callers of copy_word_from_user() should
be able to call this function multiple times to be able to parse "foo
bar zot\0" into three separate words with three separate calls to
copy_word_from_user().  It might be worth mentioning how callers should
do this in the covering comment?

> + *  -EAGAIN, if we copied a word successfully, but never hit
> + *    ending white space. The number of bytes copied will be the same
> + *    as @read. Note, if skip is set, and all we hit was white space
> + *    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 white space.
> + *    @copy must be bigger than the expected word to read.
> + */
> +int copy_word_from_user(void *to, const void __user *from,
> +			unsigned int copy, unsigned int read,
> +			unsigned int *copied, int skip)
> +{

The uaccess functions are a bit confused about whether the `size' args
are unsigned, unsigned long, etc.  They should be size_t.  unsigned is
OK here.

> +	unsigned int have_read = 0;
> +	unsigned int have_copied = 0;
> +	const char __user *user = from;
> +	char *kern = to;
> +	int ret;
> +	char ch;
> +
> +	/* get the first character */
> +	ret = get_user(ch, user++);
> +	if (ret)
> +		return ret;
> +	have_read++;
> +
> +	/*
> +	 * If skip is set, and the first character is white space
> +	 * then we will continue to read until we find non white space.
> +	 */
> +	if (skip) {
> +		while (have_read < read && isspace(ch)) {
> +			ret = get_user(ch, user++);
> +			if (ret)
> +				return ret;
> +			have_read++;
> +		}
> +
> +		/*
> +		 * If ch is still white space, then have_read == read.
> +		 * We successfully copied zero bytes. But this is
> +		 * still valid. Just let the caller try again.
> +		 */
> +		if (isspace(ch)) {
> +			ret = -EAGAIN;
> +			goto out;
> +		}
> +	} else if (isspace(ch)) {
> +		/*
> +		 * If skip was not set and the first character was
> +		 * white space, then we return immediately.
> +		 */
> +		ret = have_read;
> +		goto out;
> +	}
> +
> +
> +	/* Now read the actual word */
> +	while (have_read < read &&
> +	       have_copied < copy && !isspace(ch)) {
> +
> +		kern[have_copied++] = ch;
> +
> +		ret = get_user(ch, user++);
> +		if (ret)
> +			return ret;
> +
> +		have_read++;
> +	}
> +
> +	/*
> +	 * If we ended with white space then we have successfully
> +	 * read in a full word.
> +	 *
> +	 * If ch is not white space, and we have filled up @from,
> +	 * then this was an invalid word.
> +	 *
> +	 * If ch is not white space, and we still have room in @from
> +	 * then we let the caller know we have split a word.
> +	 *  (have_read == read)
> +	 */
> +	if (isspace(ch))
> +		ret = have_read;
> +	else if (have_copied == copy)
> +		ret = -EINVAL;
> +	else {
> +		WARN_ON(have_read != read);
> +		ret = -EAGAIN;
> +	}
> +
> + out:
> +	*copied = have_copied;
> +
> +	return ret;
> +}

Sheer madness ;)

Someone is going to want to extend the "isspace" to include other
tokens.  We can fall off that bridge when we come to it.

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Frederic Weisbecker]

On Wed, Feb 25, 2009 at 03:30:08PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> The ftrace utility reads space delimited words 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_word_from_user function that can copy
> a space delimited word from user space. This puts the code in
> a new 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 |    4 ++
>  lib/Makefile            |    3 +-
>  lib/uaccess.c           |  134 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 140 insertions(+), 1 deletions(-)
>  create mode 100644 lib/uaccess.c
> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 6b58367..2d706d9 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -106,4 +106,8 @@ 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_word_from_user(void *to, const void __user *from,
> +			       unsigned int copy, unsigned int read,
> +			       unsigned int *copied, int skip);
> +
>  #endif		/* __LINUX_UACCESS_H__ */
> 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..5b9a4ac
> --- /dev/null
> +++ b/lib/uaccess.c
> @@ -0,0 +1,134 @@
> +/*
> + * lib/uaccess.c
> + * generic user access file.
> + *
> + * started by Steven Rostedt
> + *
> + *  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/ctype.h>
> +
> +/**
> + * copy_word_from_user - copy a space delimited word 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
> + *


Why not expand it to a copy_delimited_from_user() ?
This way we could use any separator we want, not just spaces.
We could use one more parameter for the separator and
copy_work_from_user would be a wrapper above.


> + * This reads from a user buffer, a space delimited word.
> + * If skip is set, then it will trim all leading white space.
> + * Then it will copy all non white space until @copy bytes have
> + * been copied, @read bytes have been read from the user buffer,
> + * or more white space has been encountered.
> + *
> + * Note, if skip is not set, and white space exists at the beginning
> + *  it will return immediately.
> + *
> + * Returns:
> + *  The number of bytes read from user space
> + *
> + *  -EAGAIN, if we copied a word successfully, but never hit
> + *    ending white space. The number of bytes copied will be the same
> + *    as @read. Note, if skip is set, and all we hit was white space
> + *    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 white space.
> + *    @copy must be bigger than the expected word to read.
> + */
> +int copy_word_from_user(void *to, const void __user *from,
> +			unsigned int copy, unsigned int read,
> +			unsigned int *copied, int skip)
> +{
> +	unsigned int have_read = 0;
> +	unsigned int have_copied = 0;
> +	const char __user *user = from;
> +	char *kern = to;
> +	int ret;
> +	char ch;
> +
> +	/* get the first character */
> +	ret = get_user(ch, user++);
> +	if (ret)
> +		return ret;
> +	have_read++;
> +
> +	/*
> +	 * If skip is set, and the first character is white space
> +	 * then we will continue to read until we find non white space.
> +	 */
> +	if (skip) {
> +		while (have_read < read && isspace(ch)) {
> +			ret = get_user(ch, user++);
> +			if (ret)
> +				return ret;
> +			have_read++;
> +		}
> +
> +		/*
> +		 * If ch is still white space, then have_read == read.
> +		 * We successfully copied zero bytes. But this is
> +		 * still valid. Just let the caller try again.
> +		 */
> +		if (isspace(ch)) {
> +			ret = -EAGAIN;
> +			goto out;
> +		}
> +	} else if (isspace(ch)) {
> +		/*
> +		 * If skip was not set and the first character was
> +		 * white space, then we return immediately.
> +		 */
> +		ret = have_read;
> +		goto out;
> +	}
> +
> +
> +	/* Now read the actual word */
> +	while (have_read < read &&
> +	       have_copied < copy && !isspace(ch)) {
> +
> +		kern[have_copied++] = ch;
> +
> +		ret = get_user(ch, user++);
> +		if (ret)
> +			return ret;
> +
> +		have_read++;
> +	}
> +
> +	/*
> +	 * If we ended with white space then we have successfully
> +	 * read in a full word.
> +	 *
> +	 * If ch is not white space, and we have filled up @from,
> +	 * then this was an invalid word.
> +	 *
> +	 * If ch is not white space, and we still have room in @from
> +	 * then we let the caller know we have split a word.
> +	 *  (have_read == read)
> +	 */
> +	if (isspace(ch))
> +		ret = have_read;
> +	else if (have_copied == copy)
> +		ret = -EINVAL;
> +	else {
> +		WARN_ON(have_read != read);
> +		ret = -EAGAIN;
> +	}
> +
> + out:
> +	*copied = have_copied;
> +
> +	return ret;
> +}
> -- 
> 1.5.6.5
> 
> -- 

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Andrew Morton]

On Wed, 25 Feb 2009 22:40:19 +0100
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Why not expand it to a copy_delimited_from_user() ?
> This way we could use any separator we want, not just spaces.
> We could use one more parameter for the separator and
> copy_work_from_user would be a wrapper above.

hah!  That was quick.

Yes, you could pass in a delimiter string or a pointer to a user-supplier
function (which defaults to isspace() if NULL?) or whatever.  But we can
do that if and when it is needed.

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Steven Rostedt]

On Wed, 25 Feb 2009, Andrew Morton wrote:

> On Wed, 25 Feb 2009 15:30:08 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > The ftrace utility reads space delimited words 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_word_from_user function that can copy
> > a space delimited word from user space. This puts the code in
> > a new lib/uaccess.c file. This keeps the code in a single location
> > and may be optimized in the future.
> > 
> 
> Does your code actually still need this?  It is unacceptble to just
> be more strict about userspace's write()s?

Well, it does make it easy for cat and grep to work with the interface.

> 
> > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> > index 6b58367..2d706d9 100644
> > --- a/include/linux/uaccess.h
> > +++ b/include/linux/uaccess.h
> > @@ -106,4 +106,8 @@ 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_word_from_user(void *to, const void __user *from,
> > +			       unsigned int copy, unsigned int read,
> > +			       unsigned int *copied, int skip);
> > +
> >  #endif		/* __LINUX_UACCESS_H__ */
> > 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..5b9a4ac
> > --- /dev/null
> > +++ b/lib/uaccess.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * lib/uaccess.c
> > + * generic user access file.
> 
> That's a good place for it.  I wonder if we have other uaccess
> functions which should be moved here sometime.
> 
> > + * started by Steven Rostedt
> > + *
> > + *  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/ctype.h>
> > +
> > +/**
> > + * copy_word_from_user - copy a space delimited word 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
> > + *
> > + * This reads from a user buffer, a space delimited word.
> > + * If skip is set, then it will trim all leading white space.
> > + * Then it will copy all non white space until @copy bytes have
> > + * been copied, @read bytes have been read from the user buffer,
> > + * or more white space has been encountered.
> > + *
> > + * Note, if skip is not set, and white space exists at the beginning
> > + *  it will return immediately.
> > + *
> > + * Returns:
> > + *  The number of bytes read from user space
> 
> Confused.
> 
> Is this "the number of bytes which I copied into *to", or is it "the
> number of userspace bytes over which I advanced"?
> 
> Hopefully the latter, because callers of copy_word_from_user() should
> be able to call this function multiple times to be able to parse "foo
> bar zot\0" into three separate words with three separate calls to
> copy_word_from_user().  It might be worth mentioning how callers should
> do this in the covering comment?

Yes it is the latter. Since I tried to be consistent in using "read" and 
"copy" I thought it was obvious. But like most things technical, 
everything is obvious to the one that wrote the code.


> 
> > + *  -EAGAIN, if we copied a word successfully, but never hit
> > + *    ending white space. The number of bytes copied will be the same
> > + *    as @read. Note, if skip is set, and all we hit was white space
> > + *    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 white space.
> > + *    @copy must be bigger than the expected word to read.
> > + */
> > +int copy_word_from_user(void *to, const void __user *from,
> > +			unsigned int copy, unsigned int read,
> > +			unsigned int *copied, int skip)
> > +{
> 
> The uaccess functions are a bit confused about whether the `size' args
> are unsigned, unsigned long, etc.  They should be size_t.  unsigned is
> OK here.

I'll do a clean up patch.

> 
> > +	unsigned int have_read = 0;
> > +	unsigned int have_copied = 0;
> > +	const char __user *user = from;
> > +	char *kern = to;
> > +	int ret;
> > +	char ch;
> > +
> > +	/* get the first character */
> > +	ret = get_user(ch, user++);
> > +	if (ret)
> > +		return ret;
> > +	have_read++;
> > +
> > +	/*
> > +	 * If skip is set, and the first character is white space
> > +	 * then we will continue to read until we find non white space.
> > +	 */
> > +	if (skip) {
> > +		while (have_read < read && isspace(ch)) {
> > +			ret = get_user(ch, user++);
> > +			if (ret)
> > +				return ret;
> > +			have_read++;
> > +		}
> > +
> > +		/*
> > +		 * If ch is still white space, then have_read == read.
> > +		 * We successfully copied zero bytes. But this is
> > +		 * still valid. Just let the caller try again.
> > +		 */
> > +		if (isspace(ch)) {
> > +			ret = -EAGAIN;
> > +			goto out;
> > +		}
> > +	} else if (isspace(ch)) {
> > +		/*
> > +		 * If skip was not set and the first character was
> > +		 * white space, then we return immediately.
> > +		 */
> > +		ret = have_read;
> > +		goto out;
> > +	}
> > +
> > +
> > +	/* Now read the actual word */
> > +	while (have_read < read &&
> > +	       have_copied < copy && !isspace(ch)) {
> > +
> > +		kern[have_copied++] = ch;
> > +
> > +		ret = get_user(ch, user++);
> > +		if (ret)
> > +			return ret;
> > +
> > +		have_read++;
> > +	}
> > +
> > +	/*
> > +	 * If we ended with white space then we have successfully
> > +	 * read in a full word.
> > +	 *
> > +	 * If ch is not white space, and we have filled up @from,
> > +	 * then this was an invalid word.
> > +	 *
> > +	 * If ch is not white space, and we still have room in @from
> > +	 * then we let the caller know we have split a word.
> > +	 *  (have_read == read)
> > +	 */
> > +	if (isspace(ch))
> > +		ret = have_read;
> > +	else if (have_copied == copy)
> > +		ret = -EINVAL;
> > +	else {
> > +		WARN_ON(have_read != read);
> > +		ret = -EAGAIN;
> > +	}
> > +
> > + out:
> > +	*copied = have_copied;
> > +
> > +	return ret;
> > +}
> 
> Sheer madness ;)
> 
> Someone is going to want to extend the "isspace" to include other
> tokens.  We can fall off that bridge when we come to it.

Hmm, Frederic mentioned this too. I guess adding a "delimiter" field and 
calling it copy_token_from_user would not be to hard to implement. Then we 
can have copy_word_from_user be just a wrapper (as Frederic mentioned).

-- Steve

[PATCH][git pull] uaccess: add example to copy_word_from_user in comment

[Steven Rostedt]

Ingo,

Please pull the latest tip/tracing/ftrace tree, which can be found at:

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


Steven Rostedt (1):
      uaccess: add example to copy_word_from_user in comment

----
 lib/uaccess.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 1 deletions(-)
---------------------------
commit bdec54d5f42a6e702a35687ab110751968c9a4d9
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Wed Feb 25 17:14:40 2009 -0500

    uaccess: add example to copy_word_from_user in comment
    
    Andrew Morton suggested an example of use for copy_word_from_user.
    This patch adds an example in the kernel doc of the function.
    It also clears up what is returned.
    
    Reported-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/lib/uaccess.c b/lib/uaccess.c
index 5b9a4ac..2dda756 100644
--- a/lib/uaccess.c
+++ b/lib/uaccess.c
@@ -30,8 +30,45 @@
  * Note, if skip is not set, and white space 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_word_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 white space.
+ *
+ *  len -= ret;  - 4 bytes was read
+ *  read = ret;
+ *  ret = copy_word_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_word_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_word_from_user above had 0 for @skip, where we
+ * did not want to skip leading white 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
+ *  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 word successfully, but never hit
  *    ending white space. The number of bytes copied will be the same

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Andrew Morton]

btw,

On Wed, 25 Feb 2009 15:30:08 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> +int copy_word_from_user(void *to, const void __user *from,
> +			unsigned int copy, unsigned int read,
> +			unsigned int *copied, int skip)

You presently have this not-exported-to-modules.  Was that deliberate?

There are arguments either way.  Lately we've tended to take the
position that a whole interface either is or is not wholly exported. 
The uaccess functions are exported, so this one should be as well.

We can of course do that later on, when there's a user - I have no
particular preference personally.

<looks at probe_kernel_read and probe_kernel_write>

These are really part of the uaccess interface too.  I don't see a need
for both lib/uaccess.c and mm/maccess.c?

probe_kernel_read() and probe_kernel_write() are EXPORT_SYMBOL_GPL,
whereas the rest of the uaccess interface is EXPORT_SYMBOL.  Ho hum.

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Steven Rostedt]

On Wed, 25 Feb 2009, Andrew Morton wrote:

> btw,
> 
> On Wed, 25 Feb 2009 15:30:08 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > +int copy_word_from_user(void *to, const void __user *from,
> > +			unsigned int copy, unsigned int read,
> > +			unsigned int *copied, int skip)
> 
> You presently have this not-exported-to-modules.  Was that deliberate?

Nope, I didn't even think about it. I usually do the EXPORT_SYMBOL when
I have a need to export to a module. Not saying I left it out on purpose.
I left it out simply because I did not think about using it in a module 
:-/

> 
> There are arguments either way.  Lately we've tended to take the
> position that a whole interface either is or is not wholly exported. 
> The uaccess functions are exported, so this one should be as well.
> 
> We can of course do that later on, when there's a user - I have no
> particular preference personally.
> 
> <looks at probe_kernel_read and probe_kernel_write>
> 
> These are really part of the uaccess interface too.  I don't see a need
> for both lib/uaccess.c and mm/maccess.c?

Ah, I didn't notice that file. I could move this to that file, or move the 
maccess to this file. I just thought lib would be a better location.
What would you suggest?

> 
> probe_kernel_read() and probe_kernel_write() are EXPORT_SYMBOL_GPL,
> whereas the rest of the uaccess interface is EXPORT_SYMBOL.  Ho hum.

Looks like another clean up. I guess probe_kernel is because it is more 
kernel internals and should not be used by binary modules? Although 
nothing prevents a kernel module from using the uaccess code for the 
kernel. Or even make a fault section themselves.

-- Steve

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[H. Peter Anvin]

Steven Rostedt wrote:
> This patch creates a copy_word_from_user function that can copy
> a space delimited word from user space. This puts the code in
> a new lib/uaccess.c file. This keeps the code in a single location
> and may be optimized in the future.

I have a bit of an issue with the naming... at least *I* read this as 
copying a machine word, which made me wonder why you didn't just use 
"get_user".  I would suggest copy_token_from_user or something like that.

	-hpa

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Frederic Weisbecker]

On Wed, Feb 25, 2009 at 06:02:07PM -0800, H. Peter Anvin wrote:
> Steven Rostedt wrote:
>> This patch creates a copy_word_from_user function that can copy
>> a space delimited word from user space. This puts the code in
>> a new lib/uaccess.c file. This keeps the code in a single location
>> and may be optimized in the future.
>
> I have a bit of an issue with the naming... at least *I* read this as  
> copying a machine word, which made me wonder why you didn't just use  
> "get_user".  I would suggest copy_token_from_user or something like that.
>
> 	-hpa

That was the first impression I had too (the sense of a machine word).
But token seems to me imprecise as well, too much generic.

Well, since it is well commented, I guess it's not so much an issue...

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[H. Peter Anvin]

Frederic Weisbecker wrote:
> On Wed, Feb 25, 2009 at 06:02:07PM -0800, H. Peter Anvin wrote:
>> Steven Rostedt wrote:
>>> This patch creates a copy_word_from_user function that can copy
>>> a space delimited word from user space. This puts the code in
>>> a new lib/uaccess.c file. This keeps the code in a single location
>>> and may be optimized in the future.
>> I have a bit of an issue with the naming... at least *I* read this as  
>> copying a machine word, which made me wonder why you didn't just use  
>> "get_user".  I would suggest copy_token_from_user or something like that.
>>
>> 	-hpa
> 
> That was the first impression I had too (the sense of a machine word).
> But token seems to me imprecise as well, too much generic.
> 
> Well, since it is well commented, I guess it's not so much an issue...
> 

Well, "word" has the problem that someone looking at a call site is 
likely to get the wrong idea.  "token" may make them confused, but that 
means they'll try to look it up.  However, I'm sure we could do better.

	-hpa

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Steven Rostedt]

On Wed, 25 Feb 2009, H. Peter Anvin wrote:

> Steven Rostedt wrote:
> > This patch creates a copy_word_from_user function that can copy
> > a space delimited word from user space. This puts the code in
> > a new lib/uaccess.c file. This keeps the code in a single location
> > and may be optimized in the future.
> 
> I have a bit of an issue with the naming... at least *I* read this as copying
> a machine word, which made me wonder why you didn't just use "get_user".  I
> would suggest copy_token_from_user or something like that.

Hehe, I was going to use that name for the function that supplies that 
delimiter. But then we have the question. Should the delimiter be a single 
character. Do we allow any delimiter? Or simply pass in a function that 
will determine the delimiter?

-- Steve

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[H. Peter Anvin]

Steven Rostedt wrote:
> 
> Hehe, I was going to use that name for the function that supplies that 
> delimiter. But then we have the question. Should the delimiter be a single 
> character. Do we allow any delimiter? Or simply pass in a function that 
> will determine the delimiter?
> 

If you allow any delimiter we might call it strtok_from_user().  ;)

	-hpa

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Andrew Morton]

On Wed, 25 Feb 2009 20:46:11 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> > 
> > There are arguments either way.  Lately we've tended to take the
> > position that a whole interface either is or is not wholly exported. 
> > The uaccess functions are exported, so this one should be as well.
> > 
> > We can of course do that later on, when there's a user - I have no
> > particular preference personally.
> > 
> > <looks at probe_kernel_read and probe_kernel_write>
> > 
> > These are really part of the uaccess interface too.  I don't see a need
> > for both lib/uaccess.c and mm/maccess.c?
> 
> Ah, I didn't notice that file. I could move this to that file, or move the 
> maccess to this file. I just thought lib would be a better location.
> What would you suggest?

Don't care much.  There is no rationale for the "maccess" name though -
there's no such term in Linux.  lib/uaccess.c seems a fine place for
such things.

> > 
> > probe_kernel_read() and probe_kernel_write() are EXPORT_SYMBOL_GPL,
> > whereas the rest of the uaccess interface is EXPORT_SYMBOL.  Ho hum.
> 
> Looks like another clean up. I guess probe_kernel is because it is more 
> kernel internals and should not be used by binary modules? Although 
> nothing prevents a kernel module from using the uaccess code for the 
> kernel. Or even make a fault section themselves.

hm, yes.  And I can't find any module which calls probe_kernel_*(). 
Perhaps that export was added for consistency, even though it's
inconsistently licensed.

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Andrew Morton]

On Wed, 25 Feb 2009 18:02:07 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:

> Steven Rostedt wrote:
> > This patch creates a copy_word_from_user function that can copy
> > a space delimited word from user space. This puts the code in
> > a new lib/uaccess.c file. This keeps the code in a single location
> > and may be optimized in the future.
> 
> I have a bit of an issue with the naming... at least *I* read this as 
> copying a machine word, which made me wonder why you didn't just use 
> "get_user".  I would suggest copy_token_from_user or something like that.
> 

Yes, it set me back for a sec.  But anyone who uses the maddeningly
vague concept of a "word" in an interface deserves to get their head
boiled anyway, so there should be no confusion.

Re: [PATCH 1/4] uaccess: add copy_word_from_user

[Steven Rostedt]

On Wed, 25 Feb 2009, Andrew Morton wrote:

> On Wed, 25 Feb 2009 18:02:07 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote:
> 
> > Steven Rostedt wrote:
> > > This patch creates a copy_word_from_user function that can copy
> > > a space delimited word from user space. This puts the code in
> > > a new lib/uaccess.c file. This keeps the code in a single location
> > > and may be optimized in the future.
> > 
> > I have a bit of an issue with the naming... at least *I* read this as 
> > copying a machine word, which made me wonder why you didn't just use 
> > "get_user".  I would suggest copy_token_from_user or something like that.
> > 
> 
> Yes, it set me back for a sec.  But anyone who uses the maddeningly
> vague concept of a "word" in an interface deserves to get their head
> boiled anyway, so there should be no confusion.

I'll remember to stay away from akpm if I see him heading my way with 
anything boiling in his hands.

-- Steve