LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [2/6] uaccess: add probe_kernel_write()
@ 2008-02-10  7:13 Ingo Molnar
  2008-02-10 18:53 ` Jan Kiszka
  0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2008-02-10  7:13 UTC (permalink / raw)
  To: linux-kernel; +Cc: Linus Torvalds, Andrew Morton, Thomas Gleixner, Jason Wessel

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__ */

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2/6] uaccess: add probe_kernel_write()
  2008-02-10  7:13 [2/6] uaccess: add probe_kernel_write() Ingo Molnar
@ 2008-02-10 18:53 ` Jan Kiszka
  2008-02-10 19:12   ` Linus Torvalds
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Kiszka @ 2008-02-10 18:53 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, Linus Torvalds, Andrew Morton, Thomas Gleixner,
	Jason Wessel

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__ */

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2/6] uaccess: add probe_kernel_write()
  2008-02-10 18:53 ` Jan Kiszka
@ 2008-02-10 19:12   ` Linus Torvalds
  2008-02-10 20:05     ` Ingo Molnar
  0 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2008-02-10 19:12 UTC (permalink / raw)
  To: Jan Kiszka
  Cc: Ingo Molnar, linux-kernel, Andrew Morton, Thomas Gleixner, Jason Wessel



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2/6] uaccess: add probe_kernel_write()
  2008-02-10 19:12   ` Linus Torvalds
@ 2008-02-10 20:05     ` Ingo Molnar
  2008-02-11 16:46       ` Randy Dunlap
  0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2008-02-10 20:05 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jan Kiszka, linux-kernel, Andrew Morton, Thomas Gleixner, Jason Wessel


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2/6] uaccess: add probe_kernel_write()
  2008-02-10 20:05     ` Ingo Molnar
@ 2008-02-11 16:46       ` Randy Dunlap
  0 siblings, 0 replies; 5+ messages in thread
From: Randy Dunlap @ 2008-02-11 16:46 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Jan Kiszka, linux-kernel, Andrew Morton,
	Thomas Gleixner, Jason Wessel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2008-02-11 19:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-10  7:13 [2/6] uaccess: add probe_kernel_write() Ingo Molnar
2008-02-10 18:53 ` Jan Kiszka
2008-02-10 19:12   ` Linus Torvalds
2008-02-10 20:05     ` Ingo Molnar
2008-02-11 16:46       ` Randy Dunlap

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).