[PATCH v6 16/22] powerpc/book3s64/kuap: Improve error reporting with KUAP

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar at linux.ibm.com> writes:
> Christophe Leroy <christophe.leroy at csgroup.eu> writes:
>
>> Le 25/11/2020 à 06:16, Aneesh Kumar K.V a écrit :
>>> With hash translation use DSISR_KEYFAULT to identify a wrong access.
>>> With Radix we look at the AMR value and type of fault.
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar at linux.ibm.com>
>>> ---
>>>   arch/powerpc/include/asm/book3s/32/kup.h     |  4 +--
>>>   arch/powerpc/include/asm/book3s/64/kup.h     | 27 ++++++++++++++++----
>>>   arch/powerpc/include/asm/kup.h               |  4 +--
>>>   arch/powerpc/include/asm/nohash/32/kup-8xx.h |  4 +--
>>>   arch/powerpc/mm/fault.c                      |  2 +-
>>>   5 files changed, 29 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
>>> index 32fd4452e960..b18cd931e325 100644
>>> --- a/arch/powerpc/include/asm/book3s/32/kup.h
>>> +++ b/arch/powerpc/include/asm/book3s/32/kup.h
>>> @@ -177,8 +177,8 @@ static inline void restore_user_access(unsigned long flags)
>>>   		allow_user_access(to, to, end - addr, KUAP_READ_WRITE);
>>>   }
>>>   
>>> -static inline bool
>>> -bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
>>> +static inline bool bad_kuap_fault(struct pt_regs *regs, unsigned long address,
>>> +				  bool is_write, unsigned long error_code)
>>>   {
>>>   	unsigned long begin = regs->kuap & 0xf0000000;
>>>   	unsigned long end = regs->kuap << 28;
>>> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
>>> index 4a3d0d601745..2922c442a218 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/kup.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
>>> @@ -301,12 +301,29 @@ static inline void set_kuap(unsigned long value)
>>>   	isync();
>>>   }
>>>   
>>> -static inline bool
>>> -bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
>>> +#define RADIX_KUAP_BLOCK_READ	UL(0x4000000000000000)
>>> +#define RADIX_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
>>> +
>>> +static inline bool bad_kuap_fault(struct pt_regs *regs, unsigned long address,
>>> +				  bool is_write, unsigned long error_code)
>>>   {
>>> -	return WARN(mmu_has_feature(MMU_FTR_KUAP) &&
>>> -		    (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
>>> -		    "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
>>> +	if (!mmu_has_feature(MMU_FTR_KUAP))
>>> +		return false;
>>> +
>>> +	if (radix_enabled()) {
>>> +		/*
>>> +		 * Will be a storage protection fault.
>>> +		 * Only check the details of AMR[0]
>>> +		 */
>>> +		return WARN((regs->kuap & (is_write ? RADIX_KUAP_BLOCK_WRITE : RADIX_KUAP_BLOCK_READ)),
>>> +			    "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
>>
>> I think it is pointless to keep the WARN() here.
>>
>> I have a series aiming at removing them. See 
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/cc9129bdda1dbc2f0a09cf45fece7d0b0e690784.1605541983.git.christophe.leroy@csgroup.eu/
>
> Can we do this as a spearate patch as you posted above? We can drop the
> WARN in that while keeping the hash branch to look at DSISR value.

Yeah we can reconcile Christophe's series with yours later.

I'm still not 100% convinced I want to drop that WARN.

cheers