Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC][PATCH 2/8] x86/mm: break out kernel address space handling
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20180907194855.74E03836@viggo.jf.intel.com>
Date:   Fri, 7 Sep 2018 15:21:41 -0700
Cc:     linux-kernel@vger.kernel.org, sean.j.christopherson@intel.com,
        peterz@infradead.org, tglx@linutronix.de, x86@kernel.org,
        luto@kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <CD6177E3-BDE5-443F-9A28-351EAE0BE5BA@amacapital.net>
References: <20180907194852.3C351B82@viggo.jf.intel.com> <20180907194855.74E03836@viggo.jf.intel.com>
To:     Dave Hansen <dave.hansen@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Sep 7, 2018, at 12:48 PM, Dave Hansen <dave.hansen@linux.intel.com> wro=
te:
>=20
>=20
> From: Dave Hansen <dave.hansen@linux.intel.com>
>=20
> The page fault handler (__do_page_fault())  basically has two sections:
> one for handling faults in the kernel porttion of the address space
> and another for faults in the user porttion of the address space.
>=20
> But, these two parts don't stick out that well.  Let's make that more
> clear from code separation and naming.  Pull kernel fault
> handling into its own helper, and reflect that naming by renaming
> spurious_fault() -> spurious_kernel_fault().
>=20
> Also, rewrite the vmalloc handling comment a bit.  It was a bit
> stale and also glossed over the reserved bit handling.
>=20
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: x86@kernel.org
> Cc: Andy Lutomirski <luto@kernel.org>
> ---
>=20
> b/arch/x86/mm/fault.c |   98 ++++++++++++++++++++++++++++++---------------=
-----
> 1 file changed, 59 insertions(+), 39 deletions(-)
>=20
> diff -puN arch/x86/mm/fault.c~pkeys-fault-warnings-00 arch/x86/mm/fault.c
> --- a/arch/x86/mm/fault.c~pkeys-fault-warnings-00    2018-09-07 11:21:46.1=
45751902 -0700
> +++ b/arch/x86/mm/fault.c    2018-09-07 11:23:37.643751624 -0700
> @@ -1033,7 +1033,7 @@ mm_fault_error(struct pt_regs *regs, uns
>    }
> }
>=20
> -static int spurious_fault_check(unsigned long error_code, pte_t *pte)
> +static int spurious_kernel_fault_check(unsigned long error_code, pte_t *p=
te)
> {
>    if ((error_code & X86_PF_WRITE) && !pte_write(*pte))
>        return 0;
> @@ -1072,7 +1072,7 @@ static int spurious_fault_check(unsigned
>  * (Optional Invalidation).
>  */
> static noinline int
> -spurious_fault(unsigned long error_code, unsigned long address)
> +spurious_kernel_fault(unsigned long error_code, unsigned long address)
> {
>    pgd_t *pgd;
>    p4d_t *p4d;
> @@ -1103,27 +1103,27 @@ spurious_fault(unsigned long error_code,
>        return 0;
>=20
>    if (p4d_large(*p4d))
> -        return spurious_fault_check(error_code, (pte_t *) p4d);
> +        return spurious_kernel_fault_check(error_code, (pte_t *) p4d);
>=20
>    pud =3D pud_offset(p4d, address);
>    if (!pud_present(*pud))
>        return 0;
>=20
>    if (pud_large(*pud))
> -        return spurious_fault_check(error_code, (pte_t *) pud);
> +        return spurious_kernel_fault_check(error_code, (pte_t *) pud);
>=20
>    pmd =3D pmd_offset(pud, address);
>    if (!pmd_present(*pmd))
>        return 0;
>=20
>    if (pmd_large(*pmd))
> -        return spurious_fault_check(error_code, (pte_t *) pmd);
> +        return spurious_kernel_fault_check(error_code, (pte_t *) pmd);
>=20
>    pte =3D pte_offset_kernel(pmd, address);
>    if (!pte_present(*pte))
>        return 0;
>=20
> -    ret =3D spurious_fault_check(error_code, pte);
> +    ret =3D spurious_kernel_fault_check(error_code, pte);
>    if (!ret)
>        return 0;
>=20
> @@ -1131,12 +1131,12 @@ spurious_fault(unsigned long error_code,
>     * Make sure we have permissions in PMD.
>     * If not, then there's a bug in the page tables:
>     */
> -    ret =3D spurious_fault_check(error_code, (pte_t *) pmd);
> +    ret =3D spurious_kernel_fault_check(error_code, (pte_t *) pmd);
>    WARN_ONCE(!ret, "PMD has incorrect permission bits\n");
>=20
>    return ret;
> }
> -NOKPROBE_SYMBOL(spurious_fault);
> +NOKPROBE_SYMBOL(spurious_kernel_fault);
>=20
> int show_unhandled_signals =3D 1;
>=20
> @@ -1203,6 +1203,55 @@ static inline bool smap_violation(int er
>    return true;
> }
>=20
> +static void
> +do_kern_addr_space_fault(struct pt_regs *regs, unsigned long hw_error_cod=
e,
> +             unsigned long address)
> +{

Can you add a comment above this documenting *when* it=E2=80=99s called?  Is=
 it all faults, !user_mode faults, or !PF_USER?

> +    /*
> +     * We can fault-in kernel-space virtual memory on-demand. The
> +     * 'reference' page table is init_mm.pgd.
> +     *
> +     * NOTE! We MUST NOT take any locks for this case. We may
> +     * be in an interrupt or a critical region, and should
> +     * only copy the information from the master page table,
> +     * nothing more.
> +     *
> +     * Before doing this on-demand faulting, ensure that the
> +     * fault is not any of the following:
> +     * 1. A fault on a PTE with a reserved bit set.
> +     * 2. A fault caused by a user-mode access.  (Do not demand-
> +     *    fault kernel memory due to user-mode accesses).
> +     * 3. A fault caused by a page-level protection violation.
> +     *    (A demand fault would be on a non-present page which
> +     *     would have X86_PF_PROT=3D=3D0).
> +     */
> +    if (!(hw_error_code & (X86_PF_RSVD | X86_PF_USER | X86_PF_PROT))) {
> +        if (vmalloc_fault(address) >=3D 0)
> +            return;
> +    }
> +
> +    /* Was the fault spurious, caused by lazy TLB invalidation? */
> +    if (spurious_kernel_fault(hw_error_code, address))
> +        return;
> +
> +    /* kprobes don't want to hook the spurious faults: */
> +    if (kprobes_fault(regs))
> +        return;
> +
> +    /*
> +     * This is a "bad" fault in the kernel address space.  There
> +     * is no reasonable explanation for it.  We will either kill
> +     * the process for making a bad access, or oops the kernel.
> +     */

Or call an extable handler?

Maybe the wording should be less scary, e.g. =E2=80=9Cthis fault is a genuin=
e error. Send a signal, call an exception handler, or oops, as appropriate.=E2=
=80=9D