On Tue, Oct 13, 2020 at 11:56:53AM -0700, Dave Hansen wrote:
> > @@ -548,6 +549,11 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
> > (error_code & X86_PF_PK) ? "protection keys violation" :
> > "permissions violation");
> >
> > +#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
> > + if (irq_state && (error_code & X86_PF_PK))
> > + pr_alert("PKRS: 0x%x\n", irq_state->pkrs);
> > +#endif
>
> This means everyone will see 'PKRS: 0x0', even if they're on non-PKS
> hardware. I think I'd rather have this only show PKRS when we're on
> cpu_feature_enabled(PKS) hardware.
Good catch, thanks.
>
> ...
> > @@ -1148,14 +1156,15 @@ static int fault_in_kernel_space(unsigned long address)
> > */
> > static void
> > do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
> > - unsigned long address)
> > + unsigned long address, irqentry_state_t *irq_state)
> > {
> > /*
> > - * 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.
> > + * If protection keys are not enabled for kernel space
> > + * do not expect Pkey errors here.
> > */
>
> Let's fix the double-negative:
>
> /*
> * PF_PK is only expected on kernel addresses whenn
> * supervisor pkeys are enabled:
> */
done. thanks.
>
> > - WARN_ON_ONCE(hw_error_code & X86_PF_PK);
> > + if (!IS_ENABLED(CONFIG_ARCH_HAS_SUPERVISOR_PKEYS) ||
> > + !cpu_feature_enabled(X86_FEATURE_PKS))
> > + WARN_ON_ONCE(hw_error_code & X86_PF_PK);
>
> Yeah, please stick X86_FEATURE_PKS in disabled-features so you can use
> cpu_feature_enabled(X86_FEATURE_PKS) by itself here..
done.
thanks,
Ira
