wp_works_ok cleanups again

PATCH In review Andy Lutomirski 4 participants 6 messages Mar 29, 2017 – Mar 30, 2017

Subsystems (3)

x86 x86 MM Xen hypervisor x86

lore

Share & tools

lore

Andy Lutomirski

author maintainer

2017-03-29 16:48 UTC #0 lore raw

v2 cover wp_works_ok cleanups again

Hi Ingo-

Here it is rebased and split up.

--Andy

Andy Lutomirski (2):
x86/boot/32: Delete cpuinfo_x86::wp_works_ok
x86/boot/32: Rewrite test_wp_bit()

 arch/x86/include/asm/processor.h |  2 --
 arch/x86/kernel/cpu/proc.c       |  5 ++---
 arch/x86/kernel/setup.c          |  2 --
 arch/x86/mm/init_32.c            | 44 ++++++++--------------------------------
 arch/x86/xen/enlighten.c         |  1 -
 5 files changed, 11 insertions(+), 43 deletions(-)

Signature · 1 line

2.9.3

Andy Lutomirski

author maintainer

2017-03-29 16:48 UTC #1 lore raw

PATCH v2 1/2 x86/boot/32: Delete cpuinfo_x86::wp_works_ok

Linux refuses to boot if WP doesn't work okay, so tracking whether it works serves no purpose. The only use I can see at all for wp_works_ok is that it lets Xen bypass test_wp_bit(). If this is truly needed, it could be more cleanly handled using X86_FEATURE_XENPV, but it looks like Xen can handle test_wp_bit() correctly without special cases at all.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h |  2 --
 arch/x86/kernel/cpu/proc.c       |  5 ++---
 arch/x86/kernel/setup.c          |  2 --
 arch/x86/mm/init_32.c            | 13 +++++--------
 arch/x86/xen/enlighten.c         |  1 -
 5 files changed, 7 insertions(+), 16 deletions(-)

arch/x86/include/asm/processor.h +0 -2

index 05319c60b3db..1ac08e96411c 100644
@@ -90,8 +90,6 @@ struct cpuinfo_x86 {
 	__u8			x86_model;
 	__u8			x86_mask;
 #ifdef CONFIG_X86_32
-	char			wp_works_ok;	/* It doesn't on 386's */
-
 	/* Problems on some 486Dx4's and old 386's: */
 	char			rfu;
 	char			pad0;

arch/x86/kernel/cpu/proc.c +2 -3

index 18ca99f2798b..6df621ae62a7 100644
@@ -31,14 +31,13 @@ static void show_cpuinfo_misc(struct seq_file *m, struct cpuinfo_x86 *c)
 		   "fpu\t\t: %s\n"
 		   "fpu_exception\t: %s\n"
 		   "cpuid level\t: %d\n"
-		   "wp\t\t: %s\n",
+		   "wp\t\t: yes\n",
 		   static_cpu_has_bug(X86_BUG_FDIV) ? "yes" : "no",
 		   static_cpu_has_bug(X86_BUG_F00F) ? "yes" : "no",
 		   static_cpu_has_bug(X86_BUG_COMA) ? "yes" : "no",
 		   static_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
 		   static_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
-		   c->cpuid_level,
-		   c->wp_works_ok ? "yes" : "no");
+		   c->cpuid_level);
 }
 #else
 static void show_cpuinfo_misc(struct seq_file *m, struct cpuinfo_x86 *c)

arch/x86/kernel/setup.c +0 -2

index 56b1177155db..462b7c69443d 100644
@@ -175,11 +175,9 @@ static struct resource bss_resource = {
 #ifdef CONFIG_X86_32
 /* cpu data as detected by the assembly code in head.S */
 struct cpuinfo_x86 new_cpu_data = {
-	.wp_works_ok = -1,
 };
 /* common cpu data for all cpus */
 struct cpuinfo_x86 boot_cpu_data __read_mostly = {
-	.wp_works_ok = -1,
 };
 EXPORT_SYMBOL(boot_cpu_data);

arch/x86/mm/init_32.c +5 -8

index 5ed3c141bbd5..b30f20951e9e 100644
@@ -731,15 +731,13 @@ static void __init test_wp_bit(void)
 
 	/* Any page-aligned address will do, the test is non-destructive */
 	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_KERNEL_RO);
-	boot_cpu_data.wp_works_ok = do_test_wp_bit();
-	clear_fixmap(FIX_WP_TEST);
-
-	if (!boot_cpu_data.wp_works_ok) {
+	if (!do_test_wp_bit()) {
 		printk(KERN_CONT "No.\n");
 		panic("Linux doesn't support CPUs with broken WP.");
-	} else {
-		printk(KERN_CONT "Ok.\n");
 	}
+	clear_fixmap(FIX_WP_TEST);
+
+	printk(KERN_CONT "Ok.\n");
 }
 
 void __init mem_init(void)

@@ -821,8 +819,7 @@ void __init mem_init(void)
 	BUG_ON(VMALLOC_START				>= VMALLOC_END);
 	BUG_ON((unsigned long)high_memory		> VMALLOC_START);
 
-	if (boot_cpu_data.wp_works_ok < 0)
-		test_wp_bit();
+	test_wp_bit();
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG

arch/x86/xen/enlighten.c +0 -1

index 4951fcf95143..6efa0cc425a2 100644
@@ -1595,7 +1595,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	/* set up basic CPUID stuff */
 	cpu_detect(&new_cpu_data);
 	set_cpu_cap(&new_cpu_data, X86_FEATURE_FPU);
-	new_cpu_data.wp_works_ok = 1;
 	new_cpu_data.x86_capability[CPUID_1_EDX] = cpuid_edx(1);
 #endif

Signature · 1 line

2.9.3

Andy Lutomirski

author maintainer

2017-03-29 16:48 UTC #5 lore raw

PATCH v2 2/2 x86/boot/32: Rewrite test_wp_bit()

In reply to Andy Lutomirski

This code seems to be very old and has gotten only minor updates. It's overcomplicated and has a bunch of comments that are, at best, of purely historical interest. Nowadays we have a shiny function probe_kernel_write() that does more or less exactly what we need. Use it.

I switched the page that we test from swapper_pg_dir to empty_zero_page because writing zero to empty_zero_page is more obviously safe than writing to the paging structures. (It's extremely unlikely that any of this would cause problems in practice because the write will fail on any supported CPU.)

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/mm/init_32.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

arch/x86/mm/init_32.c +6 ~~-29~~

index b30f20951e9e..097089a5e4d5 100644
@@ -56,8 +56,6 @@
 
 unsigned long highstart_pfn, highend_pfn;
 
-static noinline int do_test_wp_bit(void);
-
 bool __read_mostly __vmalloc_start_set = false;
 
 /*

@@ -726,15 +724,18 @@ void __init paging_init(void)
  */
 static void __init test_wp_bit(void)
 {
+	char z = 0;
+
 	printk(KERN_INFO
   "Checking if this processor honours the WP bit even in supervisor mode...");
 
-	/* Any page-aligned address will do, the test is non-destructive */
-	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_KERNEL_RO);
-	if (!do_test_wp_bit()) {
+	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
+
+	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {
 		printk(KERN_CONT "No.\n");
 		panic("Linux doesn't support CPUs with broken WP.");
 	}
+
 	clear_fixmap(FIX_WP_TEST);
 
 	printk(KERN_CONT "Ok.\n");

@@ -847,30 +848,6 @@ int arch_remove_memory(u64 start, u64 size)
 #endif
 #endif
 
-/*
- * This function cannot be __init, since exceptions don't work in that
- * section.  Put this after the callers, so that it cannot be inlined.
- */
-static noinline int do_test_wp_bit(void)
-{
-	char tmp_reg;
-	int flag;
-
-	__asm__ __volatile__(
-		"	movb %0, %1	\n"
-		"1:	movb %1, %0	\n"
-		"	xorl %2, %2	\n"
-		"2:			\n"
-		_ASM_EXTABLE(1b,2b)
-		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
-		 "=q" (tmp_reg),
-		 "=r" (flag)
-		:"2" (1)
-		:"memory");
-
-	return flag;
-}
-
 int kernel_set_to_readonly __read_mostly;
 
 void set_kernel_text_rw(void)

Signature · 1 line

2.9.3

Borislav Petkov

maintainer

2017-03-29 20:21 UTC #2 lore raw

Re: [PATCH v2 1/2] x86/boot/32: Delete cpuinfo_x86::wp_works_ok

In reply to Andy Lutomirski

Quoted 8 lines · Andy Lutomirski

Linux refuses to boot if WP doesn't work okay, so tracking whether it works serves no purpose. The only use I can see at all for wp_works_ok is that it lets Xen bypass test_wp_bit(). If this is truly needed, it could be more cleanly handled using X86_FEATURE_XENPV, but it looks like Xen can handle test_wp_bit() correctly without special cases at all.
Signed-off-by Andy Lutomirski <[email protected]>

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=6415813bae75feba10b8ca3ed6634a72c2a4d313

What's up?

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Andy Lutomirski

2017-03-29 21:07 UTC #3 lore raw

Re: [PATCH v2 1/2] x86/boot/32: Delete cpuinfo_x86::wp_works_ok

Quoted 8 lines · Borislav Petkov

Quoted 7 lines · Andy Lutomirski
Linux refuses to boot if WP doesn't work okay, so tracking whether it works serves no purpose. The only use I can see at all for wp_works_ok is that it lets Xen bypass test_wp_bit(). If this is truly needed, it could be more cleanly handled using X86_FEATURE_XENPV, but it looks like Xen can handle test_wp_bit() correctly without special cases at all.

Quoted 6 lines

Quoted 1 line
Signed-off-by Andy Lutomirski <[email protected]>
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=6415813bae75feba10b8ca3ed6634a72c2a4d313
What's up?

Wow, I based on tip/x86/mm per Ingo's request, but maybe that was the wrong branch, and apparently Mathias did the same thing in the mean time. Whoops. I'll rebase again.

Quoted 5 lines

--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Ingo Molnar

2017-03-30 07:07 UTC #4 lore raw

Re: [PATCH v2 1/2] x86/boot/32: Delete cpuinfo_x86::wp_works_ok

Quoted 11 lines · Andy Lutomirski

Quoted 10 lines · Borislav Petkov
Quoted 8 lines · Andy Lutomirski
Linux refuses to boot if WP doesn't work okay, so tracking whether it works serves no purpose. The only use I can see at all for wp_works_ok is that it lets Xen bypass test_wp_bit(). If this is truly needed, it could be more cleanly handled using X86_FEATURE_XENPV, but it looks like Xen can handle test_wp_bit() correctly without special cases at all.
Signed-off-by Andy Lutomirski <[email protected]>

Quoted 8 lines

Quoted 4 lines
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=6415813bae75feba10b8ca3ed6634a72c2a4d313
What's up?
Wow, I based on tip/x86/mm per Ingo's request, but maybe that was the wrong branch, and apparently Mathias did the same thing in the mean time. Whoops. I'll rebase again.

Oops, I didn't realize the duplication either. The splitting up of the patch that I requested made the merge easier I suspect - albeit that's an unintended side effect.

Thanks,

Ingo