Re: [PATCH v2] powerpc: Avoid code patching freed init sections

[Tyrel Datwyler <tyreld@linux.vnet.ibm.com>]

On 09/12/2018 05:36 PM, Michael Neuling wrote:
> 
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -23,11 +23,33 @@
>>>   #include <asm/code-patching.h>
>>>   #include <asm/setup.h>
>>>   
>>> +
>>
>> This blank line is not needed
> 
> Ack
> 
>>
>>> +static inline bool in_init_section(unsigned int *patch_addr)
>>> +{
>>> +	if (patch_addr < (unsigned int *)__init_begin)
>>> +		return false;
>>> +	if (patch_addr >= (unsigned int *)__init_end)
>>> +		return false;
>>> +	return true;
>>> +}
>>
>> Can we use the existing function init_section_contains() instead of this 
>> new function ?
> 
> Nice, I was looking for something like that... 
> 
>>> +
>>> +static inline bool init_freed(void)
>>> +{
>>> +	return (system_state >= SYSTEM_RUNNING);
>>> +}
>>> +
>>
>> I would call this function differently, for instance init_is_finished(), 
>> because as you mentionned it doesn't exactly mean that init memory is freed.
> 
> Talking to Nick and mpe offline I think we are going to have to add a flag when
> we free init mem rather than doing what we have now since what we have now has a
> potential race. That change will eliminate the function entirely.
> 
>>>   static int __patch_instruction(unsigned int *exec_addr, unsigned int
>>> instr,
>>>   			       unsigned int *patch_addr)
>>>   {
>>>   	int err;
>>>   
>>> +	/* Make sure we aren't patching a freed init section */
>>> +	if (in_init_section(patch_addr) && init_freed()) {
>>
>> The test must be done on exec_addr, not on patch_addr, as patch_addr is 
>> the address where the instruction as been remapped RW for allowing its 
>> modification.
> 
> Thanks for the catch
> 
>> Also I think it should be tested the other way round, because the 
>> init_freed() is a simpler test which will be false most of the time once 
>> the system is running so it should be checked first.
> 
> ok, I'll change.
> 
>>> +		printk(KERN_DEBUG "Skipping init section patching addr:
>>> 0x%lx\n",
>>
>> Maybe use pr_debug() instead.
> 
> Sure.
> 
>>
>>> +			(unsigned long)patch_addr);
>>
>> Please align second line as per Codying style.
> 
> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
> this out for me...

I suspect that the suggestion is the opening parenthesis of "(unsigned long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty sure Documentation/process/coding-style.rst is very adamant that all identation is always 8 characters and spaces should never be used, but there still seems to be a lot of places/suggestions that argument lists that spill over multiple lines should be space indented to align with the very first argument at the top level. So, I guess I'm not sure what the desire is here. Although moving to pr_debug might fit it to a single line anyways. ;)

-Tyrel

> 
>>
>>> +		return 0;
>>> +	}
>>> +
>>>   	__put_user_size(instr, patch_addr, 4, err);
>>>   	if (err)
>>>   		return err;
>>>
>>
>> I think it would be better to put this verification in 
>> patch_instruction() instead, to avoid RW mapping/unmapping the 
>> instruction to patch when we are not going to do the patching.
> 
> If we do it there then we miss the raw_patch_intruction case.
> 
> IMHO I don't think we need to optimise this rare and non-critical path. 
> 
> Mikey
>