Re: [PATCH V4 06/10] x86/fault: Adjust WARN_ON for PKey fault

From: Sean Christopherson <seanjc@google.com>
To: ira.weiny@intel.com
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	Fenghua Yu <fenghua.yu@intel.com>,
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH V4 06/10] x86/fault: Adjust WARN_ON for PKey fault
Date: Mon, 22 Mar 2021 09:05:43 -0700	[thread overview]
Message-ID: <YFjAV44u7i9t1TDL@google.com> (raw)
In-Reply-To: <20210322053020.2287058-7-ira.weiny@intel.com>

On Sun, Mar 21, 2021, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> PKey faults may now happen on kernel mappings if the feature is enabled.
> Remove the warning in the fault path if PKS is enabled.

When/why can they happen?  I read through all the changelogs, as well as the
cover letters for v1 and the RFC, and didn't see any explicit statement about
why pkey faults on supervisor accesses are now "legal".  Explaining what happens
later in the page fault handler would also be helpful, e.g. is the flag simply
ignored?  Does it lead directly to OOPS?

Documenting what happens on a PKS #PF in the API patch would be nice to have, too.

> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  arch/x86/mm/fault.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index a73347e2cdfc..731ec90ed413 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1141,11 +1141,12 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
>  		   unsigned long address)
>  {
>  	/*
> -	 * Protection keys exceptions only happen on user pages.  We
> -	 * have no user pages in the kernel portion of the address
> -	 * space, so do not expect them here.
> +	 * PF_PK is expected on kernel addresses when supervisor pkeys are

"is expected" can be misinterpreted as "PF is expected on all kernel addresses...".

This ties in with the lack of an explanation in the changelog.

> +	 * enabled.

It'd be helpful to spell out "Protection keys exceptions" so that random readers
don't need to search for PF_PK to understand what's up.  Maybe even use it as an
opportunity to introduce "pkeys", e.g.

	/* Protection keys (pkeys) exceptions are ... */

>  	 */
> -	WARN_ON_ONCE(hw_error_code & X86_PF_PK);
> +	if (!cpu_feature_enabled(X86_FEATURE_PKS))
> +		WARN_ON_ONCE(hw_error_code & X86_PF_PK);

Does this generate the same code if the whole thing is thrown in the WARN?  E.g.

	WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_PKS) &&
		     (hw_error_code & X86_PF_PK));

> +
>  
>  #ifdef CONFIG_X86_32
>  	/*
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 