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

Ira Weiny
2022-02-04T20:07:58Z

On Fri, Jan 28, 2022 at 03:10:24PM -0800, Dave Hansen wrote:
> On 1/27/22 09:54, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > 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.

Ok, I'll try and clarify.

>
> > 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.

I'll audit the series to use lower case for consistency.

>
> > If PKS is enabled, avoid the warning in the fault path.
> >
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > 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.
> */

Sounds better,
Ira