[patch] x86: test case for the RODATA config option

[patch] x86: test case for the RODATA config option

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: x86: test case for the RODATA config option

This patch adds a test module for the DEBUG_RODATA config
option to make sure change_page_attr() did indeed make
"const" data read only.

This testcase both tests the DEBUG_RODATA code as well as
the change_page_attr() code for correct operation.

When the tests/ patch gets merged, this module should move
to the tests/ directory.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 arch/x86/Kconfig.debug        |    8 +++++
 arch/x86/kernel/Makefile_32   |    1 
 arch/x86/kernel/Makefile_64   |    2 +
 arch/x86/kernel/test_rodata.c |   65 ++++++++++++++++++++++++++++++++++++++++++
 arch/x86/mm/init_32.c         |    3 +
 arch/x86/mm/init_64.c         |    3 +
 6 files changed, 82 insertions(+)

Index: linux-2.6.24-rc8/arch/x86/Kconfig.debug
===================================================================
--- linux-2.6.24-rc8.orig/arch/x86/Kconfig.debug
+++ linux-2.6.24-rc8/arch/x86/Kconfig.debug
@@ -57,6 +57,14 @@ config DEBUG_RODATA
 	  portion of the kernel code won't be covered by a 2MB TLB anymore.
 	  If in doubt, say "N".
 
+config DEBUG_RODATA_TEST
+	tristate "Testcase for the DEBUG_RODATA feature"
+	depends on DEBUG_RODATA && m
+	help
+	  This option enables a testcase for the DEBUG_RODATA
+	  feature as well as for the change_page_attr() infrastructure.
+	  If in doubt, say "N"
+
 config 4KSTACKS
 	bool "Use 4Kb for kernel stacks instead of 8Kb"
 	depends on DEBUG_KERNEL
Index: linux-2.6.24-rc8/arch/x86/mm/init_32.c
===================================================================
--- linux-2.6.24-rc8.orig/arch/x86/mm/init_32.c
+++ linux-2.6.24-rc8/arch/x86/mm/init_32.c
@@ -790,6 +790,9 @@ static int noinline do_test_wp_bit(void)
 
 #ifdef CONFIG_DEBUG_RODATA
 
+const int rodata_test_data;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+
 void mark_rodata_ro(void)
 {
 	unsigned long start = PFN_ALIGN(_text);
Index: linux-2.6.24-rc8/arch/x86/mm/init_64.c
===================================================================
--- linux-2.6.24-rc8.orig/arch/x86/mm/init_64.c
+++ linux-2.6.24-rc8/arch/x86/mm/init_64.c
@@ -590,6 +590,9 @@ void free_initmem(void)
 
 #ifdef CONFIG_DEBUG_RODATA
 
+const int rodata_test_data = 5;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+
 void mark_rodata_ro(void)
 {
 	unsigned long start = (unsigned long)_stext, end;
Index: linux-2.6.24-rc8/arch/x86/kernel/Makefile_64
===================================================================
--- linux-2.6.24-rc8.orig/arch/x86/kernel/Makefile_64
+++ linux-2.6.24-rc8/arch/x86/kernel/Makefile_64
@@ -39,6 +39,8 @@ obj-$(CONFIG_AUDIT)		+= audit_64.o
 obj-$(CONFIG_MODULES)		+= module_64.o
 obj-$(CONFIG_PCI)		+= early-quirks.o
 
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
+
 obj-y				+= topology.o
 obj-y				+= pcspeaker.o
 
Index: linux-2.6.24-rc8/arch/x86/kernel/test_rodata.c
===================================================================
--- /dev/null
+++ linux-2.6.24-rc8/arch/x86/kernel/test_rodata.c
@@ -0,0 +1,65 @@
+#include <linux/module.h>
+
+
+extern int rodata_test_data;
+
+int rodata_test_init(void)
+{
+	unsigned long result;
+	/* test 1: read the value */
+	/* If this test fails, some previous testrun has clobbered the state */
+	if (!rodata_test_data) {
+		printk(KERN_ERR "rodata_test: test 1 fails (start data)\n");
+		return -ENODEV;
+	}
+
+	/* test 2: write to the variable; this should fault */
+	/*
+	 * If this test fails, we managed to overwrite the data
+	 *
+	 * This is written in assembly to be able to catch the
+	 * exception that is supposed to happen in the correct
+	 * case
+	 */
+
+	result = 1;
+	asm volatile(
+		"0:	mov %[zero],(%[rodata_test])\n"
+		"	mov %[zero], %[rslt]\n"
+		"1:\n"
+		".section .fixup,\"ax\"\n"
+		"2:	jmp 1b\n"
+		".previous\n"
+		".section __ex_table,\"a\"\n"
+		"       .align 8\n"
+		"	.quad 0b,2b\n"
+		".previous"
+		: [rslt] "=r" (result)
+		: [rodata_test] "r" (&rodata_test_data), [zero] "r" (0UL)
+	);
+
+
+	if (!result) {
+		printk(KERN_ERR "rodata_test: test data was not read only\n");
+		return -ENODEV;
+	}
+
+	/* test 3: check the value hasn't changed */
+	/* If this test fails, we managed to overwrite the data */
+	if (!rodata_test_data) {
+		printk(KERN_ERR "rodata_test: Test 3 failes (end data)\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+void rodata_test_exit(void)
+{
+}
+
+module_init(rodata_test_init);
+module_exit(rodata_test_exit);
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Testcase for the DEBUG_RODATA infrastructure");
+MODULE_AUTHOR("Arjan van de Ven <arjan@linux.intel.com>");
Index: linux-2.6.24-rc8/arch/x86/kernel/Makefile_32
===================================================================
--- linux-2.6.24-rc8.orig/arch/x86/kernel/Makefile_32
+++ linux-2.6.24-rc8/arch/x86/kernel/Makefile_32
@@ -42,6 +42,7 @@ obj-$(CONFIG_EARLY_PRINTK)	+= early_prin
 obj-$(CONFIG_HPET_TIMER) 	+= hpet.o
 obj-$(CONFIG_K8_NB)		+= k8.o
 obj-$(CONFIG_MGEODE_LX)		+= geode_32.o mfgpt_32.o
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt_32.o

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] x86: test case for the RODATA config option

[Ingo Molnar]

cool!

could you perhaps also do an add-on:

> +	/* test 1: read the value */
> +	/* test 2: write to the variable; this should fault */
> +	/* test 3: check the value hasn't changed */

           test 4: make it writable again
           test 5: make it NX -> check that it's not executable

and perhaps also check that normal kernel allocations (kmalloc(), etc.) are NX as well? (with the same section trick you use in 
this patch - perhaps try to call a kmalloc()-ed buffer that contains a 
'ret' instruction - if that call faults then the test is OK, if the call 
succeeds then the test failed.)

	Ingo

Re: [patch] x86: test case for the RODATA config option

[Nick Piggin]

On Wednesday 23 January 2008 09:44, Arjan van de Ven wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: x86: test case for the RODATA config option
>
> This patch adds a test module for the DEBUG_RODATA config
> option to make sure change_page_attr() did indeed make
> "const" data read only.
>
> This testcase both tests the DEBUG_RODATA code as well as
> the change_page_attr() code for correct operation.
>
> When the tests/ patch gets merged, this module should move
> to the tests/ directory.
>
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  arch/x86/Kconfig.debug        |    8 +++++
>  arch/x86/kernel/Makefile_32   |    1
>  arch/x86/kernel/Makefile_64   |    2 +
>  arch/x86/kernel/test_rodata.c |   65
> ++++++++++++++++++++++++++++++++++++++++++ arch/x86/mm/init_32.c         | 
>   3 +
>  arch/x86/mm/init_64.c         |    3 +
>  6 files changed, 82 insertions(+)
>
> Index: linux-2.6.24-rc8/arch/x86/Kconfig.debug
> ===================================================================
> --- linux-2.6.24-rc8.orig/arch/x86/Kconfig.debug
> +++ linux-2.6.24-rc8/arch/x86/Kconfig.debug
> @@ -57,6 +57,14 @@ config DEBUG_RODATA
>  	  portion of the kernel code won't be covered by a 2MB TLB anymore.
>  	  If in doubt, say "N".
>
> +config DEBUG_RODATA_TEST
> +	tristate "Testcase for the DEBUG_RODATA feature"
> +	depends on DEBUG_RODATA && m
> +	help
> +	  This option enables a testcase for the DEBUG_RODATA
> +	  feature as well as for the change_page_attr() infrastructure.
> +	  If in doubt, say "N"
> +
>  config 4KSTACKS
>  	bool "Use 4Kb for kernel stacks instead of 8Kb"
>  	depends on DEBUG_KERNEL
> Index: linux-2.6.24-rc8/arch/x86/mm/init_32.c
> ===================================================================
> --- linux-2.6.24-rc8.orig/arch/x86/mm/init_32.c
> +++ linux-2.6.24-rc8/arch/x86/mm/init_32.c
> @@ -790,6 +790,9 @@ static int noinline do_test_wp_bit(void)
>
>  #ifdef CONFIG_DEBUG_RODATA
>
> +const int rodata_test_data;
> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +
>  void mark_rodata_ro(void)
>  {
>  	unsigned long start = PFN_ALIGN(_text);
> Index: linux-2.6.24-rc8/arch/x86/mm/init_64.c
> ===================================================================
> --- linux-2.6.24-rc8.orig/arch/x86/mm/init_64.c
> +++ linux-2.6.24-rc8/arch/x86/mm/init_64.c
> @@ -590,6 +590,9 @@ void free_initmem(void)
>
>  #ifdef CONFIG_DEBUG_RODATA
>
> +const int rodata_test_data = 5;

I guess this should match the 32-bit case, and be zero instead of
5?

Can you disallow building as a module, and put this in the test
code? It could be run from the end of mark_rodata_ro()...

Re: [patch] x86: test case for the RODATA config option

[Arjan van de Ven]

On Wed, 23 Jan 2008 12:11:41 +1100
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> >  #ifdef CONFIG_DEBUG_RODATA
> >
> > +const int rodata_test_data = 5;
> 
> I guess this should match the 32-bit case, and be zero instead of
> 5?

actually it should have been 5 for both (well any non-zero number)
> 
> Can you disallow building as a module, and put this in the test
> code? It could be run from the end of mark_rodata_ro()...

fair; I was developing it as a module (just easier) but yeah it makes more sense as part
of mark_rodata_ro(). I'll do that in the next rev


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] x86: test case for the RODATA config option

[Andi Kleen]

Arjan van de Ven <arjan@infradead.org> writes:

> This patch adds a test module for the DEBUG_RODATA config
> option to make sure change_page_attr() did indeed make
> "const" data read only.

The only that this does that is not done pretty much equivalently
by pageattr-test.c (it just checks G instead of W) is testing the kernel 
text mapping on 64bit. On 32bit it is fully redundant I believe.

Testing the kernel mapping might be a good idea, but I would
suggest adding it to pageattr-test.c

Yes I'm sure these basic facts will not stop it from being 
applied anyways; just wanted to point it out for the benefit
of the list readers.

-Andi

Re: [patch] x86: test case for the RODATA config option

[Arjan van de Ven]

On 23 Jan 2008 10:09:58 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> Arjan van de Ven <arjan@infradead.org> writes:
> 
> > This patch adds a test module for the DEBUG_RODATA config
> > option to make sure change_page_attr() did indeed make
> > "const" data read only.
> 
> The only that this does that is not done pretty much equivalently
> by pageattr-test.c (it just checks G instead of W) is testing the
> kernel text mapping on 64bit. On 32bit it is fully redundant I
> believe.

What it does is check if the rodata marking was succesful. That's more than just checking 
change_page_attr() itself, and basically a reasonably self self-check of mark_rodata_const().


> Testing the kernel mapping might be a good idea, but I would
> suggest adding it to pageattr-test.c

where we move the actual test function... I don't really care. Once pageattr-test.c
is merged I have absolutely no problem moving it there if it's the right place...
(one can argue that it should be in or near the mark_rodata_const() function instead
since it tests that one).
Just like the NX test I wrote yesterday (and will post later today) probably wants to live
somewhere close to the NX code.
 
> Yes I'm sure these basic facts will not stop it from being 
> applied anyways; just wanted to point it out for the benefit
> of the list readers.

Please spare me the bitter, undeserved sarcasm; I don't remember doing anything
to you to deserve that.
-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] x86: test case for the RODATA config option

[Andi Kleen]

> What it does is check if the rodata marking was succesful.

The only difference I see is that you check that the TLB flush works,
but for that it looks awfully incomplete. In particular if you really
wanted to test the TLB you would need to do the access test on
all online CPUs. Otherwise you have no guarantee the TLBs are
actually changed everywhere (assuming that was broken) 

Other than it is identical [modulo the kernel mapping bit on 64bit]-- you just 
toggle a different bit in the PTE,  but c_p_a() does not actually care which 
bits you toggle.

>> Testing the kernel mapping might be a good idea, but I would
>> suggest adding it to pageattr-test.c

>where we move the actual test function... 

What I meant using the more extensive test in pageattr-test.c
to test a few changes in the 64bit kernel mapping too, not moving your
code. I don't think moving your code makes sense. Sorry for being unclear.

-Andi

Re: [patch] x86: test case for the RODATA config option

[Arjan van de Ven]

On Wed, 23 Jan 2008 16:27:28 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> 
> > What it does is check if the rodata marking was succesful.
> 
> The only difference I see is that you check that the TLB flush works,
> but for that it looks awfully incomplete.

you think one level too small.
It tests if mark_rodata_ro() operated correctly.
Yes internally that uses c-p-a, but there's more code there, including a set of boundary conditions etc.
And for the page table to work, cr0 needs to be set up correctly etc etc etc.


 
> Other than it is identical [modulo the kernel mapping bit on 64bit]--

it's a test of mark_rodata_ro(), not of c-p-a. Not the same thing.


> What I meant using the more extensive test in pageattr-test.c
> to test a few changes in the 64bit kernel mapping too, not moving your
> code. I don't think moving your code makes sense. Sorry for being
> unclear.

then I just don't get your comment at all; sorry.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch] x86: test case for the RODATA config option

[Andi Kleen]

> Yes internally that uses c-p-a, but there's more code there, including a set of boundary conditions etc.

You mean the ro boundary to the rest of the kernel and the 
rest of the kernel mappings on i386?

It doesn't seem to check for any of those.

> And for the page table to work, cr0 needs to be set up correctly etc etc etc.

If you really want to check that then you should run it on all CPUs
at least.

But ok given that it's already merged I'll shut up now since it's too
late.

-Andi

Re: [patch] x86: test case for the RODATA config option

[Arjan van de Ven]

On Wed, 23 Jan 2008 17:54:01 +0100
Andi Kleen <andi@firstfloor.org> wrote:


> 
> But ok given that it's already merged I'll shut up now since it's too
> late.

I asked you before to cut the unjustified sarcasm; all I can do is ask again.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org