Re: [tip:x86/mm] x86/boot/32: Rewrite test_wp_bit()

[Borislav Petkov <bp@alien8.de>]

On Thu, Mar 30, 2017 at 12:29:18AM -0700, tip-bot for Andy Lutomirski wrote:
> Commit-ID:  4af171105144a6475704c1e6024132883d50499e
> Gitweb:     http://git.kernel.org/tip/4af171105144a6475704c1e6024132883d50499e
> Author:     Andy Lutomirski <luto@kernel.org>
> AuthorDate: Wed, 29 Mar 2017 16:47:35 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Thu, 30 Mar 2017 09:08:33 +0200
> 
> x86/boot/32: Rewrite test_wp_bit()
> 
> 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>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Garnier <thgarnie@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/0b9e64ab0236de30e7572213cea77bf95ae2e990.1490831211.git.luto@kernel.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/mm/init_32.c | 41 +++++++----------------------------------
>  1 file changed, 7 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 7116a72..097089a 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,22 +724,21 @@ void __init paging_init(void)
>   */
>  static void __init test_wp_bit(void)
>  {
> -	int wp_works_ok;
> +	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);
> -	wp_works_ok = do_test_wp_bit();
> -	clear_fixmap(FIX_WP_TEST);
> +	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
>  
> -	if (!wp_works_ok) {
> +	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {

Gah, I was just commenting on that but you committed it already...

Oh well, let's flip that logic so that we have the success case - which
we exec most of the time - if not always - not out of line but put the
panic() call at the end instead. A natural "likely" if you will :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Thu, 30 Mar 2017 09:44:05 +0200
Subject: [PATCH] x86/boot/32: Flip the logic in test_wp_bit()

... to have a natural "likely()" in the code flow and thus have the
success case with a branch 99.999% of the times non-taken and function
return code following it instead of jumping to it each time.

This puts the panic() call at the end of the function - it is going to
be practically unreachable anyway.

The C code is a bit more readable too.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/mm/init_32.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 097089a5e4d5..601b8e04e5c6 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -726,19 +726,18 @@ 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...");
+	printk(KERN_INFO "Checking if this processor honours the WP bit even in supervisor mode...");
 
 	__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.");
+	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1)) {
+		clear_fixmap(FIX_WP_TEST);
+		printk(KERN_CONT "Ok.\n");
+		return;
 	}
 
-	clear_fixmap(FIX_WP_TEST);
-
-	printk(KERN_CONT "Ok.\n");
+	printk(KERN_CONT "No.\n");
+	panic("Linux doesn't support CPUs with broken WP.");
 }
 
 void __init mem_init(void)
-- 
2.11.0


-- 
Regards/Gruss,
    Boris.

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