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

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:

quoted

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.

quoted

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