Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 02/11/2022 à 10:43, Christophe Leroy a écrit :
> 
> 
> Le 25/10/2022 à 06:44, Benjamin Gray a écrit :
>> Verifies that if the instruction patching did not return an error then
>> the value stored at the given address to patch is now equal to the
>> instruction we patched it to.
> 
> Why do we need that verification ? Until now it wasn't necessary, can 
> you describe a failure that occurs because we don't have this 
> verification ? Or is that until now it was reliable but the new method 
> you are adding will not be as reliable as before ?
> 
> What worries me with that new verification is that you are reading a 
> memory address with is mostly only used for code execution. That means:
> - You will almost always take a DATA TLB Miss, hence performance impact.
> - If one day we want Exec-only text mappings, it will become problematic.
> 
> So really the question is, is that patch really required ?
> 
>>
>> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
>> ---
>>   arch/powerpc/lib/code-patching.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/lib/code-patching.c 
>> b/arch/powerpc/lib/code-patching.c
>> index 3b3b09d5d2e1..b0a12b2d5a9b 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -192,6 +192,8 @@ static int do_patch_instruction(u32 *addr, 
>> ppc_inst_t instr)
>>       err = __do_patch_instruction(addr, instr);
>>       local_irq_restore(flags);
>> +    WARN_ON(!err && !ppc_inst_equal(instr, ppc_inst_read(addr)));
>> +

Another point: you are doing the check outside of IRQ disabled section, 
is that correct ? What if an interrupt changed it in-between ?

Or insn't that possible ? In that case what's the real purpose of 
disabling IRQs here ?

>>       return err;
>>   }
>>   #else /* !CONFIG_STRICT_KERNEL_RWX */