[PATCH v2 0/2] wp_works_ok cleanups again

[PATCH v2 0/2] wp_works_ok cleanups again

[Andy Lutomirski]

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(-)

-- 
2.9.3

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

[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 <luto@kernel.org>
---
 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(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 05319c60b3db..1ac08e96411c 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -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;
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 18ca99f2798b..6df621ae62a7 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -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)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 56b1177155db..462b7c69443d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -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);
 
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 5ed3c141bbd5..b30f20951e9e 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -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
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 4951fcf95143..6efa0cc425a2 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -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
 
-- 
2.9.3

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

[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 <luto@kernel.org>
---
 arch/x86/mm/init_32.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index b30f20951e9e..097089a5e4d5 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -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)
-- 
2.9.3

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

[Borislav Petkov]

On Wed, Mar 29, 2017 at 09:48:41AM -0700, Andy Lutomirski wrote:
> 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 <luto@kernel.org>

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

What's up?

-- 
Regards/Gruss,
    Boris.

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

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

[Andy Lutomirski]

On Wed, Mar 29, 2017 at 1:20 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 29, 2017 at 09:48:41AM -0700, Andy Lutomirski wrote:
>> 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 <luto@kernel.org>
>
> 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.

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



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Mar 29, 2017 at 1:20 PM, Borislav Petkov <bp@alien8.de> wrote:
> > On Wed, Mar 29, 2017 at 09:48:41AM -0700, Andy Lutomirski wrote:
> >> 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 <luto@kernel.org>
> >
> > 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