linux-kernel - Re: [PATCH v5 17/22] powerpc/syscall: Do not check unsupported scv vector on PPC32

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: <82c4abb1-cb52-e856-b2dd-d7c7d48bd292@csgroup.eu>
Date:   Tue, 9 Feb 2021 07:13:17 +0100
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Nicholas Piggin <npiggin@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Michael Ellerman <mpe@...erman.id.au>, msuchanek@...e.de,
        Paul Mackerras <paulus@...ba.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v5 17/22] powerpc/syscall: Do not check unsupported scv
 vector on PPC32



Le 09/02/2021 à 03:00, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
>> Only PPC64 has scv. No need to check the 0x7ff0 trap on PPC32.
>> For that, add a helper trap_is_unsupported_scv() similar to
>> trap_is_scv().
>>
>> And ignore the scv parameter in syscall_exit_prepare (Save 14 cycles
>> 346 => 332 cycles)
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
>> ---
>> v5: Added a helper trap_is_unsupported_scv()
>> ---
>>   arch/powerpc/include/asm/ptrace.h | 5 +++++
>>   arch/powerpc/kernel/entry_32.S    | 1 -
>>   arch/powerpc/kernel/interrupt.c   | 7 +++++--
>>   3 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index 58f9dc060a7b..2c842b11a924 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -229,6 +229,11 @@ static inline bool trap_is_scv(struct pt_regs *regs)
>>   	return (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && TRAP(regs) == 0x3000);
>>   }
>>   
>> +static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>> +{
>> +	return (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && TRAP(regs) == 0x7ff0);
>> +}
> 
> This change is good.
> 
>> +
>>   static inline bool trap_is_syscall(struct pt_regs *regs)
>>   {
>>   	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
>> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
>> index cffe58e63356..7c824e8928d0 100644
>> --- a/arch/powerpc/kernel/entry_32.S
>> +++ b/arch/powerpc/kernel/entry_32.S
>> @@ -344,7 +344,6 @@ transfer_to_syscall:
>>   
>>   ret_from_syscall:
>>   	addi    r4,r1,STACK_FRAME_OVERHEAD
>> -	li	r5,0
>>   	bl	syscall_exit_prepare
> 
> For this one, I think it would be nice to do the "right" thing and make
> the function prototypes different on !64S. They could then declare a
> local const bool scv = 0.
> 
> We could have syscall_exit_prepare and syscall_exit_prepare_maybe_scv
> or something like that, 64s can use the latter one and the former can be
> a wrapper that passes constant 0 for scv. Then we don't have different
> prototypes for the same function, but you just have to make the 32-bit
> version static inline and the 64-bit version exported to asm.

You can't call a static inline function from ASM, I don't understand you.

What is wrong for you really here ? Is that the fact we leave scv random, or is that the below 
IS_ENABLED() ?

I don't mind keeping the 'li r5,0' before calling the function if you find it cleaner, the real 
performance gain is with setting scv to 0 below for PPC32 (and maybe it should be set to zero for 
book3e/64 too ?).

Other solution would be to do replace (!scv) by (!scv || !IS_ENABLED(CONFIG_PPC_BOOK3S_64)) in the 
two places it is used in syscall_exit_prepare().

Any preference ?

Thanks
Christophe

>> @@ -224,6 +224,9 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>   	unsigned long ti_flags;
>>   	unsigned long ret = 0;
>>   
>> +	if (IS_ENABLED(CONFIG_PPC32))
>> +		scv = 0;
>> +
>>   	CT_WARN_ON(ct_state() == CONTEXT_USER);
>>   
>>   #ifdef CONFIG_PPC64
>> -- 
>> 2.25.0
>>
>>