Re: [1/2] powerpc/mm: Remove duplicated check in do_page_fault()

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, May 09, 2016 at 08:03:50PM +1000, Michael Ellerman wrote:
>On Fri, 2016-26-02 at 00:26:25 UTC, Gavin Shan wrote:
>> When the page fault happened in user space, we need check it's
>> caused by stack frame pointer update instruction and update
>> local variable @flag with FAULT_FLAG_USER. Currently, the code
>> has two separate check for the same condition. That's unnecessary.
>> 
>> This removes one of the duplicated check. No functinal changes
>> introduced.
>
>It's possible though that store_updates_sp() changes regs, and causes
>user_mode(regs) to change, which would mean the second check is necessary.
>That's not true with the current code, but you should mention that you confirmed
>that in the change log.
>

Thanks for review. Yeah, store_updates_sp() checks the failing instruction
is the one updating stack frame pointer (stdu and the variable). The info
is used to expand the stack later. All of it should have been documented
in the commit log.

>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index a67c6d7..935f386 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -294,11 +294,10 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>>  	 * can result in fault, which will cause a deadlock when called with
>>  	 * mmap_sem held
>>  	 */
>> -	if (user_mode(regs))
>> -		store_update_sp = store_updates_sp(regs);
>> -
>> -	if (user_mode(regs))
>> +	if (user_mode(regs)) {
>>  		flags |= FAULT_FLAG_USER;
>> +		store_update_sp = store_updates_sp(regs);
>> +	}
>
>It doesn't really matter in this case, but it would be better to keep the
>ordering of the two statements the same as before.
>

Yep, It should have two checks in order as before here if store_updates_sp()
can alter MSR[PR] bit in future as you explained above :-)

Thanks,
Gavin

>cheers
>