Re: [PATCH v8] powerpc/mm: Only read faulting instruction when necessary in do_page_fault()

On Wed, 23 May 2018 09:31:33 +0200
Christophe LEROY [off-list ref] wrote:

Le 23/05/2018 =C3=A0 09:17, Nicholas Piggin a =C3=A9crit=C2=A0:

quoted

On Wed, 23 May 2018 09:01:19 +0200 (CEST)
Christophe Leroy [off-list ref] wrote:
 =20

quoted

quoted

@@ -264,8 +266,30 @@ static bool bad_stack_expansion(struct pt_regs *r=

egs, unsigned long address,

quoted

quoted

  		 * between the last mapped region and the stack will
  		 * expand the stack rather than segfaulting.
  		 */
-		if (address + 2048 < uregs->gpr[1] && !store_update_sp)
-			return true;
+		if (address + 2048 >=3D uregs->gpr[1])
+			return false;
+		if (is_retry)
+			return false;
+
+		if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
+		    access_ok(VERIFY_READ, nip, sizeof(inst))) {
+			int res;
+
+			pagefault_disable();
+			res =3D __get_user_inatomic(inst, nip);
+			pagefault_enable();
+			if (res) {
+				up_read(&mm->mmap_sem);
+				res =3D __get_user(inst, nip);
+				if (!res && store_updates_sp(inst))
+					return -1;
+				return true;
+			}
+			if (store_updates_sp(inst))
+				return false;
+		}
+		up_read(&mm->mmap_sem); =20

=20
Starting to look pretty good... I think probably I prefer the mmap_sem
drop going into the caller so we don't don't drop in the child function=

. =20

=20
Yes I can do that. I though it was ok as the drop is already done in=20
children functions like bad_area(), bad_access(), ...

That's true, all exit functions though. I think it may end up being a
bit nicer with the up_read in the caller, but see what you think.

quoted

I thought the retry logic was a little bit complex too, what do you
think of using fault_in_pages_readable and just doing a full retry to
avoid some of this complexity? =20

=20
Yes lets try that way, allthough fault_in_pages_readable() is nothing=20
else than a get_user().
Should we take any precaution to avoid retrying forever or is it just=20
not worth it ?

generic_perform_write() the core of the data copying for write(2)
syscall does this retry, so I think it's okay... Although I think I
wrote that so maybe that's a circular justification.

I think if we end up thrashing on this type of loop for a long time,
the system will already be basically dead.

quoted

quoted

  	/* The stack is being expanded, check if it's valid */
-	if (unlikely(bad_stack_expansion(regs, address, vma, store_update_sp=

)))

quoted

quoted

-		return bad_area(regs, address);
+	is_bad =3D bad_stack_expansion(regs, address, vma, flags, is_retry);
+	if (unlikely(is_bad =3D=3D -1)) {
+		is_retry =3D true;
+		goto retry;
+	}
+	if (unlikely(is_bad))
+		return bad_area_nosemaphore(regs, address); =20

=20
Suggest making the return so that you can do a single unlikely test for
the retry or bad case, and then distinguish the retry in there. Code
generation should be better. =20

=20
Ok. I'll try and come with v9 during this morning.

Thanks,
Nick