Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Christophe Leroy wrote:
> 
> 
> Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
>> With STRICT_KERNEL_RWX, we are currently ignoring return value from
>> __patch_instruction() in do_patch_instruction(), resulting in the error
>> not being propagated back. Fix the same.
> 
> Good patch.
> 
> Be aware that there is ongoing work which tend to wanting to replace 
> error reporting by BUG_ON() . See 
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003

Hah, I see that you pointed out this exact issue in your review there!

I had noticed this when Russell's series for STRICT_MODULE_RWX started 
causing kretprobe failures, due to one of the early boot-time patching 
failing silently.

I'll defer to Michael on which patch he prefers to take, between this 
one and the series you point out above.

> 
>> 
>> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/lib/code-patching.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>> index 3345f039a876..5c713a6c0bd8 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>>   
>>   static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   {
>> -	int err;
>> +	int err, rc = 0;
>>   	unsigned int *patch_addr = NULL;
>>   	unsigned long flags;
>>   	unsigned long text_poke_addr;
>> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   	patch_addr = (unsigned int *)(text_poke_addr) +
>>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>>   
>> -	__patch_instruction(addr, instr, patch_addr);
>> +	rc = __patch_instruction(addr, instr, patch_addr);
>>   
>>   	err = unmap_patch_area(text_poke_addr);
>>   	if (err)
>> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>>   out:
>>   	local_irq_restore(flags);
>>   
>> -	return err;
>> +	return rc ? rc : err;
> 
> That's not really consistent. __patch_instruction() and 
> unmap_patch_area() return a valid minus errno, while in case of 
> map_patch_area() failure, err has value -1

Not sure I follow -- I'm not changing what would be returned in those 
cases, just also capturing return value from __patch_instruction().

If anything, I've considered the different return codes to be a good 
thing -- return code gives you a clear idea of what exactly failed.


- Naveen