Christophe Leroy <christophe.le...@csgroup.eu> writes:
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
>> For powerpc64, redefine the ppc_inst type so both word and prefixed
>> instructions can be represented. On powerpc32 the type will remain the
>> same. Update places which had assumed instructions to be 4 bytes long.
...
>> diff --git a/arch/powerpc/include/asm/uaccess.h
>> b/arch/powerpc/include/asm/uaccess.h
>> index c0a35e4586a5..217897927926 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -105,11 +105,49 @@ static inline int __access_ok(unsigned long addr,
>> unsigned long size,
>> #define __put_user_inatomic(x, ptr) \
>> __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>
>> +#ifdef __powerpc64__
>
> Replace by CONFIG_PPC64
>
>> +#define __get_user_instr(x, ptr) \
>> +({ \
>> + long __gui_ret = 0; \
>> + unsigned long __gui_ptr = (unsigned long)ptr; \
>> + struct ppc_inst __gui_inst; \
>> + unsigned int prefix, suffix; \
>> + __gui_ret = __get_user(prefix, (unsigned int __user *)__gui_ptr);
>> \
>
> __get_user() can be costly especially with KUAP. I think you should
> perform a 64 bits read and fallback on a 32 bits read only if the 64
> bits read failed.
I worry that might break something.
It _should_ be safe, but I'm paranoid.
If we think the KUAP overhead is a problem then I think we'd be better
off pulling the KUAP disable/enable into this macro.
>> diff --git a/arch/powerpc/lib/feature-fixups.c
>> b/arch/powerpc/lib/feature-fixups.c
>> index 2bd2b752de4f..a8238eff3a31 100644
>> --- a/arch/powerpc/lib/feature-fixups.c
>> +++ b/arch/powerpc/lib/feature-fixups.c
>> @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value,
>> struct fixup_entry *fcur)
>> src = alt_start;
>> dest = start;
>>
>> - for (; src < alt_end; src++, dest++) {
>> + for (; src < alt_end; src = (void *)src +
>> ppc_inst_len(ppc_inst_read(src)),
>> + (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) {
>
> Can we do this outside the for() for readability ?
I have an idea for cleaning these up, will post it as a follow-up to the series.
>> diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
>> index 08dedd927268..eb6f9ee28ac6 100644
>> --- a/arch/powerpc/lib/inst.c
>> +++ b/arch/powerpc/lib/inst.c
>> @@ -3,9 +3,49 @@
>> * Copyright 2020, IBM Corporation.
>> */
>>
>> +#include <asm/ppc-opcode.h>
>> #include <linux/uaccess.h>
>> #include <asm/inst.h>
>>
>> +#ifdef __powerpc64__
>> +int probe_user_read_inst(struct ppc_inst *inst,
>> + struct ppc_inst *nip)
>> +{
>> + unsigned int val, suffix;
>> + int err;
>> +
>> + err = probe_user_read(&val, nip, sizeof(val));
>
> A user read is costly with KUAP. Can we do a 64 bits read and perform a
> 32 bits read only when 64 bits read fails ?
Same comment as above.
cheers
