Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200421092027.591582014@linutronix.de> <20200421092558.848064318@linutronix.de>
In-Reply-To: <20200421092558.848064318@linutronix.de>
From:   Andy Lutomirski <luto@amacapital.net>
Date:   Tue, 21 Apr 2020 10:04:37 -0700
Message-ID: <CALCETrW-5cR0DFpfKCdmiTxKHpzA+FBep5JuTJv3P--78SiKwA@mail.gmail.com>
Subject: Re: [patch V2 01/16] x86/tlb: Uninline __get_current_cr3_fast()
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        Kees Cook <keescook@chromium.org>,
        Alexandre Chartre <alexandre.chartre@oracle.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Thomas Lendacky <Thomas.Lendacky@amd.com>,
        Juergen Gross <jgross@suse.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 21, 2020 at 2:26 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> cpu_tlbstate is exported because various TLB related functions need access
> to it, but cpu_tlbstate is sensitive information which should only be
> accessed by well contained kernel functions and not be directly exposed to
> modules.
>
> In preparation of unexporting cpu_tlbstate move __get_current_cr3_fast()
> into the x86 TLB management code.
>
> No functional change.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Andy Lutomirski <luto@kernel.org>

> ---
>  arch/x86/include/asm/mmu_context.h |   19 +------------------
>  arch/x86/mm/tlb.c                  |   20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -225,24 +225,7 @@ static inline bool arch_vma_access_permi
>         return __pkru_allows_pkey(vma_pkey(vma), write);
>  }
>
> -/*
> - * This can be used from process context to figure out what the value of
> - * CR3 is without needing to do a (slow) __read_cr3().
> - *
> - * It's intended to be used for code like KVM that sneakily changes CR3
> - * and needs to restore it.  It needs to be used very carefully.
> - */
> -static inline unsigned long __get_current_cr3_fast(void)
> -{
> -       unsigned long cr3 = build_cr3(this_cpu_read(cpu_tlbstate.loaded_mm)->pgd,
> -               this_cpu_read(cpu_tlbstate.loaded_mm_asid));
> -
> -       /* For now, be very restrictive about when this can be called. */
> -       VM_WARN_ON(in_nmi() || preemptible());
> -
> -       VM_BUG_ON(cr3 != __read_cr3());
> -       return cr3;
> -}
> +unsigned long __get_current_cr3_fast(void);
>
>  typedef struct {
>         struct mm_struct *mm;
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -844,6 +844,26 @@ void flush_tlb_kernel_range(unsigned lon
>  }
>
>  /*
> + * This can be used from process context to figure out what the value of
> + * CR3 is without needing to do a (slow) __read_cr3().
> + *
> + * It's intended to be used for code like KVM that sneakily changes CR3
> + * and needs to restore it.  It needs to be used very carefully.
> + */
> +unsigned long __get_current_cr3_fast(void)
> +{
> +       unsigned long cr3 = build_cr3(this_cpu_read(cpu_tlbstate.loaded_mm)->pgd,
> +               this_cpu_read(cpu_tlbstate.loaded_mm_asid));
> +
> +       /* For now, be very restrictive about when this can be called. */
> +       VM_WARN_ON(in_nmi() || preemptible());
> +
> +       VM_BUG_ON(cr3 != __read_cr3());
> +       return cr3;
> +}
> +EXPORT_SYMBOL_GPL(__get_current_cr3_fast);
> +
> +/*
>   * arch_tlbbatch_flush() performs a full TLB flush regardless of the active mm.
>   * This means that the 'struct flush_tlb_info' that describes which mappings to
>   * flush is actually fixed. We therefore set a single fixed struct and use it in
>


-- 
Andy Lutomirski
AMA Capital Management, LLC