linux-kernel - Re: [PATCH 2/7] x86/mm/tlb: Restructure switch_mm_irqs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181002073254.GP3439@hirez.programming.kicks-ass.net>
Date:   Tue, 2 Oct 2018 09:32:54 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Rik van Riel <riel@...riel.com>
Cc:     linux-kernel@...r.kernel.org, kernel-team@...com,
        will.deacon@....com, songliubraving@...com, mingo@...nel.org,
        luto@...nel.org, hpa@...or.com, npiggin@...il.com,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>, efault@....de
Subject: Re: [PATCH 2/7] x86/mm/tlb: Restructure switch_mm_irqs_off()

On Tue, Sep 25, 2018 at 11:58:39PM -0400, Rik van Riel wrote:
> Move some code that will be needed for the lazy -> !lazy state
> transition when a lazy TLB CPU has gotten out of date.
> 
> No functional changes, since the if (real_prev == next) branch
> always returns.
> 
> Suggested-by: Andy Lutomirski <luto@...nel.org>
> Signed-off-by: Rik van Riel <riel@...riel.com>
> Acked-by: Dave Hansen <dave.hansen@...el.com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: efault@....de
> Cc: kernel-team@...com
> Link: http://lkml.kernel.org/r/20180716190337.26133-4-riel@surriel.com
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> (cherry picked from commit 61d0beb5796ab11f7f3bf38cb2eccc6579aaa70b)
> ---
>  arch/x86/mm/tlb.c | 64 ++++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 54a5870190a6..1224f7fb1311 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -187,6 +187,8 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>  	u16 prev_asid = this_cpu_read(cpu_tlbstate.loaded_mm_asid);
>  	unsigned cpu = smp_processor_id();
>  	u64 next_tlb_gen;
> +	bool need_flush;
> +	u16 new_asid;
>  
>  	/*
>  	 * NB: The scheduler will call us with prev == next when switching
> @@ -308,44 +310,44 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>  		/* Let nmi_uaccess_okay() know that we're changing CR3. */
>  		this_cpu_write(cpu_tlbstate.loaded_mm, LOADED_MM_SWITCHING);
>  		barrier();
> +	}

Compiling this gives me:

CC      arch/x86/mm/tlb.o
../arch/x86/mm/tlb.c: In function ‘switch_mm_irqs_off’:
../arch/x86/mm/tlb.c:315:5: warning: ‘need_flush’ may be used uninitialized in this function [-Wmaybe-uninitialized]
if (need_flush) {
^
../arch/x86/mm/tlb.c:316:45: warning: ‘new_asid’ may be used uninitialized in this function [-Wmaybe-uninitialized]
this_cpu_write(cpu_tlbstate.ctxs[new_asid].ctx_id, next->context.ctx_id);
^

Because you shadow need_flush and new_asid in a branch. I need the below
delta to make it happy again.

---
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -254,8 +254,6 @@ void switch_mm_irqs_off(struct mm_struct
 
 		return;
 	} else {
-		u16 new_asid;
-		bool need_flush;
 		u64 last_ctx_id = this_cpu_read(cpu_tlbstate.last_ctx_id);
 
 		/*