linux-kernel - Re: [PATCH] x86/mm: Rename flush_tlb_single() and flush_tlb

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]

Date:   Wed, 31 Jan 2018 11:41:10 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Andy Lutomirski <luto@...nel.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org
Cc:     linux-mm@...ck.org, Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bpetkov@...e.de>,
        Kees Cook <keescook@...gle.com>,
        Hugh Dickins <hughd@...gle.com>,
        Brian Gerst <brgerst@...il.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Rik van Riel <riel@...hat.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Eduardo Valentin <eduval@...zon.com>,
        Will Deacon <will.deacon@....com>
Subject: Re: [PATCH] x86/mm: Rename flush_tlb_single() and flush_tlb_one()

> @@ -437,18 +437,31 @@ static inline void __flush_tlb_all(void)
>  /*
>   * flush one page in the kernel mapping
>   */
> -static inline void __flush_tlb_one(unsigned long addr)
> +static inline void __flush_tlb_one_kernel(unsigned long addr)
>  {
>  	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ONE);
> -	__flush_tlb_single(addr);
> +
> +	/*
> +	 * If PTI is off, then __flush_tlb_one_user() is just INVLPG or its
> +	 * paravirt equivalent.  Even with PCID, this is sufficient: we only
> +	 * use PCID if we also use global PTEs for the kernel mapping, and
> +	 * INVLPG flushes global translations across all address spaces.

This looks good.

> +	 * If PTI is on, then the kernel is mapped with non-global PTEs, and
> +	 * __flush_tlb_one_user() will flush the given address for the current
> +	 * kernel address space and for its usermode counterpart, but it goes
> +	 * not flush it for other address spaces.
> +	 */
> +	__flush_tlb_one_user(addr);

s/goes/does/

It also goes off and flushes the address out of the user asid.  That
_seems_ a bit goofy, but it is needed for addresses that might be mapped
into the user asid, so it's definitely safe.  Might be worth calling out.

Maybe add a (if one exists) or something like:

... kernel address space and for its usermode counterpart (if one exists).

>  	if (!static_cpu_has(X86_FEATURE_PTI))
>  		return;
>  
>  	/*
> -	 * __flush_tlb_single() will have cleared the TLB entry for this ASID,
> -	 * but since kernel space is replicated across all, we must also
> -	 * invalidate all others.
> +	 * See above.  We need to propagate the flush to all other address
> +	 * spaces.  In principle, we only need to propagate it to kernelmode
> +	 * address spaces, but the extra bookkeeping we would need is not
> +	 * worth it.
>  	 */
>  	invalidate_other_asid();
>  }

That comment is true, except if we were invalidating a user-mapped
address.  Right?

We've just been pretending so far for the purposes of TLB invalidation
that all kernel addresses are potentially user-mapped.

The name change looks really good to me, though.  Thanks for doing this.