[2/6] uaccess: add probe_kernel_write()

[2/6] uaccess: add probe_kernel_write()

[Ingo Molnar]

From: Ingo Molnar <mingo@elte.hu>

add probe_kernel_write() - copy & paste of the existing
probe_kernel_access(), extended to writes.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/uaccess.h |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Index: linux-kgdb.q/include/linux/uaccess.h
===================================================================
--- linux-kgdb.q.orig/include/linux/uaccess.h
+++ linux-kgdb.q/include/linux/uaccess.h
@@ -84,4 +84,26 @@ static inline unsigned long __copy_from_
 		ret;					\
 	})
 
+/**
+ * probe_kernel_write(): safely attempt to write to a location
+ * @addr: address to write to - its type is type typeof(rdval)*
+ * @rdval: write to this variable
+ *
+ * Safely write to address @addr from variable @rdval.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+#define probe_kernel_write(addr, rdval)			\
+	({						\
+		long ret;				\
+		mm_segment_t old_fs = get_fs();		\
+							\
+		set_fs(KERNEL_DS);			\
+		pagefault_disable();			\
+		ret = __put_user(rdval,			\
+	 (__force typeof(rdval) __user *)(addr));	\
+		pagefault_enable();			\
+		set_fs(old_fs);				\
+		ret;					\
+	})
+
 #endif		/* __LINUX_UACCESS_H__ */

Re: [2/6] uaccess: add probe_kernel_write()

[Jan Kiszka]

Ingo Molnar wrote:
> From: Ingo Molnar <mingo@elte.hu>
> 
> add probe_kernel_write() - copy & paste of the existing
> probe_kernel_access(), extended to writes.

Along Linus' suggestion to work on larger chunks in kgdb, here are
improved probe_kernel_read/write helpers that take a size argument.

I'm leaving the original probe_kernel_read as is for now, but maybe it
can be converted to probe_kernel_read (I don't want to dive into this as
well :) ).

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

---
 uaccess.h |   38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 98cfe02..ef75869 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -85,25 +85,49 @@ static inline unsigned long __copy_from_user_nocache(void *to,
 	})
 
 /**
+ * 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.
+ */
+#define probe_kernel_read(dst, src, 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);				\
+		ret ? -EFAULT : 0;			\
+	})
+
+/**
  * probe_kernel_write(): safely attempt to write to a location
- * @addr: address to write to - its type is type typeof(rdval)*
- * @rdval: write to this variable
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
  *
- * Safely write to address @addr from variable @rdval.  If a kernel fault
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
  * happens, handle that and return -EFAULT.
  */
-#define probe_kernel_write(addr, rdval)			\
+#define probe_kernel_write(dst, src, size)		\
 	({						\
 		long ret;				\
 		mm_segment_t old_fs = get_fs();		\
 							\
 		set_fs(KERNEL_DS);			\
 		pagefault_disable();			\
-		ret = __put_user(rdval,			\
-	 (__force typeof(rdval) __user *)(addr));	\
+		ret = __copy_to_user_inatomic(		\
+	 (__force void __user *)(dst), (src), (size));	\
 		pagefault_enable();			\
 		set_fs(old_fs);				\
-		ret;					\
+		ret ? -EFAULT : 0;			\
 	})
 
 #endif		/* __LINUX_UACCESS_H__ */

Re: [2/6] uaccess: add probe_kernel_write()

[Linus Torvalds]

On Sun, 10 Feb 2008, Jan Kiszka wrote:
> 
> Along Linus' suggestion to work on larger chunks in kgdb, here are
> improved probe_kernel_read/write helpers that take a size argument.

I don't think this is good.

Make it
 - a function, not a #define
 - preferably uninlined (this does *not* look performance-critical)
 - get rid of the get_fs/set_fs/set_fs dance

The last point is somewhat debatable, but the fact is, those functions are 
not defined to be safe to be called from interrupt context. And since we 
use the "__copy_xxxx()" functions with two underscores, those really are 
supposed to mean that we've checked the address earlier.

So it's possible that some architecture does need the explicit segment 
munging (S390 comes to mind), but in that case, we really should add 
special support for that, or we should really validate that get_fs/set_fs 
are safe in all contexts.

And regardless, if we *do* end up needing to munge some segment register 
(which on other architectures tends to be an address space identifier, not 
a segment, but whatever), that just makes it even more clear that this 
isn't some macro or inline function, because those things tend to explode 
the code-space.

			Linus

Re: [2/6] uaccess: add probe_kernel_write()

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Along Linus' suggestion to work on larger chunks in kgdb, here are
> > improved probe_kernel_read/write helpers that take a size argument.
> 
> I don't think this is good.
> 
> Make it
>  - a function, not a #define
>  - preferably uninlined (this does *not* look performance-critical)
>  - get rid of the get_fs/set_fs/set_fs dance

yeah. I've done that (see the patch below), and i've just tested that 
kgdb memory accesses work fine with that:

 (gdb) disassemble 0xc0153ed9 0xc0153eff
 Dump of assembler code from 0xc0153ed9 to 0xc0153eff:
 0xc0153ed9:     sfence
 0xc0153edc:     xchg   %ax,%ax
 0xc0153edf:     pop    %ebp
 0xc0153ee0:     movl   $0x0,0xc0a48088
 0xc0153eea:     ret
 0xc0153eeb:     push   %ebp
 0xc0153eec:     mov    %esp,%ebp
 0xc0153eee:     push   $0xc058f11d
 0xc0153ef3:     movl   $0x0,0xc0a4bf4c
 0xc0153efd:     call   0xc0126ec5
 End of assembler dump.

i have to say, it's quite nice that via kgdb i can _see_ what the 
paravirt and alternatives stuff ends up patching into our binary image - 
see the 'sfence' instruction above. Unfortunately looking at the vmlinux 
is not as reliable as it used to be ;-)

( i've added a separate file for it under mm/maccess.c, because these 
  functions will be needed on NOMMU kernel too, so i couldnt move them 
  into their natural place, mm/memory.c. )

	Ingo

---------------->
Subject: uaccess: add probe_kernel_write()
From: Ingo Molnar <mingo@elte.hu>

add probe_kernel_read() and probe_kernel_write().

Uninlined and restricted to kernel range memory only, as suggested
by Linus.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/uaccess.h |   22 ++++++++++++++++++++++
 mm/Makefile             |    2 +-
 mm/maccess.c            |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

Index: linux-kgdb.q/include/linux/uaccess.h
===================================================================
--- linux-kgdb.q.orig/include/linux/uaccess.h
+++ linux-kgdb.q/include/linux/uaccess.h
@@ -84,4 +84,26 @@ static inline unsigned long __copy_from_
 		ret;					\
 	})
 
+/*
+ * 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.
+ */
+extern long probe_kernel_read(void *dst, void *src, size_t size);
+
+/*
+ * 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.
+ */
+extern long probe_kernel_write(void *dst, void *src, size_t size);
+
 #endif		/* __LINUX_UACCESS_H__ */
Index: linux-kgdb.q/mm/Makefile
===================================================================
--- linux-kgdb.q.orig/mm/Makefile
+++ linux-kgdb.q/mm/Makefile
@@ -8,7 +8,7 @@ mmu-$(CONFIG_MMU)	:= fremap.o highmem.o 
 			   vmalloc.o
 
 obj-y			:= bootmem.o filemap.o mempool.o oom_kill.o fadvise.o \
-			   page_alloc.o page-writeback.o pdflush.o \
+			   maccess.o page_alloc.o page-writeback.o pdflush.o \
 			   readahead.o swap.o truncate.o vmscan.o \
 			   prio_tree.o util.o mmzone.o vmstat.o backing-dev.o \
 			   page_isolation.o $(mmu-y)
Index: linux-kgdb.q/mm/maccess.c
===================================================================
--- /dev/null
+++ linux-kgdb.q/mm/maccess.c
@@ -0,0 +1,46 @@
+/*
+ * Access kernel memory without faulting.
+ */
+#include <linux/uaccess.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;
+
+	pagefault_disable();
+	ret = __copy_from_user_inatomic(dst,
+			(__force const void __user *)src, size);
+	pagefault_enable();
+
+	return ret ? -EFAULT : 0;
+}
+
+/**
+ * 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;
+
+	pagefault_disable();
+	ret = __copy_to_user_inatomic((__force void __user *)dst, src, size);
+	pagefault_enable();
+
+	return ret ? -EFAULT : 0;
+}

Re: [2/6] uaccess: add probe_kernel_write()

[Randy Dunlap]

On Sun, 10 Feb 2008 21:05:40 +0100 Ingo Molnar wrote:

> Subject: uaccess: add probe_kernel_write()
> From: Ingo Molnar <mingo@elte.hu>
> 
> add probe_kernel_read() and probe_kernel_write().
> 
> Uninlined and restricted to kernel range memory only, as suggested
> by Linus.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  include/linux/uaccess.h |   22 ++++++++++++++++++++++
>  mm/Makefile             |    2 +-
>  mm/maccess.c            |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+), 1 deletion(-)
> 
> Index: linux-kgdb.q/include/linux/uaccess.h
> ===================================================================
> --- linux-kgdb.q.orig/include/linux/uaccess.h
> +++ linux-kgdb.q/include/linux/uaccess.h
> @@ -84,4 +84,26 @@ static inline unsigned long __copy_from_
>  		ret;					\
>  	})
>  
> +/*
> + * probe_kernel_read(): safely attempt to read from a location

Please insert a hyphen/dash/'-' between function name and its
short description on the line above and on other similar lines.

Documentation/kernel-doc-nano-HOWTO.txt .

> + * @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.
> + */
> +extern long probe_kernel_read(void *dst, void *src, size_t size);
> +
> +/*
> + * 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.
> + */
> +extern long probe_kernel_write(void *dst, void *src, size_t size);
> +
>  #endif		/* __LINUX_UACCESS_H__ */

> Index: linux-kgdb.q/mm/maccess.c
> ===================================================================
> --- /dev/null
> +++ linux-kgdb.q/mm/maccess.c
> @@ -0,0 +1,46 @@
> +/*
> + * Access kernel memory without faulting.
> + */
> +#include <linux/uaccess.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;
> +
> +	pagefault_disable();
> +	ret = __copy_from_user_inatomic(dst,
> +			(__force const void __user *)src, size);
> +	pagefault_enable();
> +
> +	return ret ? -EFAULT : 0;
> +}
> +
> +/**
> + * 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;
> +
> +	pagefault_disable();
> +	ret = __copy_to_user_inatomic((__force void __user *)dst, src, size);
> +	pagefault_enable();
> +
> +	return ret ? -EFAULT : 0;
> +}

---
~Randy