Re: [PATCH V8 08/44] x86/fault: Adjust WARN_ON for PKey fault

[Dave Hansen]

On 1/27/22 09:54, ira.weiny@xxxxxxxxx wrote:
> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> Previously if a Protection key fault occurred it indicated something
> very wrong because user page mappings are not supposed to be in the
> kernel address space.

This is missing a key point.  The problem is PK faults on "*kernel*
addresses.

> Now PKey faults may happen on kernel mappings if the feature is enabled.

One nit: I've been using "pkeys" and "pkey" as the terms.  I usually
don't capitalize them except at the beginning of a sentence.

> If PKS is enabled, avoid the warning in the fault path.
> 
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> ---
>  arch/x86/mm/fault.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index d0074c6ed31a..6ed91b632eac 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1148,11 +1148,15 @@ 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.
> +	 * X86_PF_PK (Protection key exceptions) may occur on kernel addresses
> +	 * when PKS (PKeys Supervisor) is enabled.
> +	 *
> +	 * However, if PKS is not enabled WARN if this exception is seen
> +	 * because there are no user pages in the kernel portion of the address
> +	 * space.
>  	 */
> -	WARN_ON_ONCE(hw_error_code & X86_PF_PK);
> +	WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_PKS) &&
> +		     (hw_error_code & X86_PF_PK));
>  
>  #ifdef CONFIG_X86_32
>  	/*

I'm wondering if this warning is even doing us any good.  I'm pretty
sure it's never triggered on me at least.  Either way, let's not get too
carried away with the comment.  I think this should do:

	/*
	 * PF_PF faults should only occur on kernel
	 * addresses when supervisor pkeys are enabled.
	 */