linux-kernel - Re: [PATCH v3] powerpc/traps: Enhance readability for trap types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874kgfdmxq.fsf@mpe.ellerman.id.au>
Date:   Sat, 10 Apr 2021 10:04:49 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Christophe Leroy <christophe.leroy@...roup.eu>,
        Xiongwei Song <sxwjean@...com>, benh@...nel.crashing.org,
        paulus@...ba.org, oleg@...hat.com, npiggin@...il.com,
        aneesh.kumar@...ux.ibm.com, ravi.bangoria@...ux.ibm.com,
        mikey@...ling.org, haren@...ux.ibm.com, akpm@...ux-foundation.org,
        rppt@...nel.org, jniethe5@...il.com, atrajeev@...ux.vnet.ibm.com,
        maddy@...ux.ibm.com, peterz@...radead.org, kjain@...ux.ibm.com,
        kan.liang@...ux.intel.com, aik@...abs.ru, alistair@...ple.id.au,
        pmladek@...e.com, john.ogness@...utronix.de
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Xiongwei Song <sxwjean@...il.com>
Subject: Re: [PATCH v3] powerpc/traps: Enhance readability for trap types

Christophe Leroy <christophe.leroy@...roup.eu> writes:
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@...il.com>
>> 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
...
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>   #include <asm/ftrace.h>
>>   #include <asm/kprobes.h>
>>   #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>   
>>   struct interrupt_state {
>>   #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>   		 * CT_WARN_ON comes here via program_check_exception,
>>   		 * so avoid recursion.
>>   		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>   	}
>>   #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>>   #endif
>>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>
> I think too long names hinder readability, see later for suggestions.

I asked for the longer names :)

I think they make it easier for people who are less familiar with the
architecture than us to make sense of the names.

And there's only a couple of cases where it requires splitting a line,
and they could be converted to use switch if we think it's a problem.

>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>   	/* kernel has accessed a bad area */
>>   
>>   	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
>
> It would be better to avoid #ifdefs when none where necessary before.

Yes I agree.

I think these can all be avoided by defining most of the values
regardless of what platform we're building for. Only the values that
overlap need to be kept behind an ifdef.

cheers