christophe leroy [off-list ref] writes:

quoted

Le 05/06/2017 à 12:45, Michael Ellerman a écrit :

quoted

Christophe LEROY [off-list ref] writes:

quoted

Le 02/06/2017 à 11:26, Michael Ellerman a écrit :

quoted

Christophe Leroy [off-list ref] writes:

quoted

Only the get_user() in store_updates_sp() has to be done outside
the mm semaphore. All the comparison can be done within the semaphore,
so only when really needed.

As we got a DSI exception, the address pointed by regs->nip is
obviously valid, otherwise we would have had a instruction exception.
So __get_user() can be used instead of get_user()

I don't think that part is true.

You took a DSI so there *was* an instruction at NIP, but since then it
may have been unmapped by another thread.

So I don't think you can assume the get_user() will succeed.

The difference between get_user() and __get_user() is that get_user()
performs an access_ok() in addition.

Doesn't access_ok() only checks whether addr is below TASK_SIZE to
ensure it is a valid user address ?

Yeah more or less, via some gross macros.

I was actually not that worried about the switch from get_user() to
__get_user(), but rather that you removed the check of the return value.
ie.

-	if (get_user(inst, (unsigned int __user *)regs->nip))
-		return 0;

Became:

	if (is_write && user_mode(regs))
-		store_update_sp = store_updates_sp(regs);
+		__get_user(inst, (unsigned int __user *)regs->nip);


I think dropping the access_ok() probably is alright, because the NIP
must (should!) have been in userspace, though as Ben says it's always
good to be paranoid.

But ignoring that the address can fault at all is wrong AFAICS.

I see what you mean now.

Indeed,

-	unsigned int inst;

Became

+	unsigned int inst = 0;

Since __get_user() doesn't modify 'inst' in case of error, 'inst'
remains 0, and store_updates_sp(0) return false. That was the idea behind.

Ugh. OK, my bad. Though it is a little subtle.

How about:

@@ -286,10 +290,13 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
         /*
          * We want to do this outside mmap_sem, because reading code around nip
          * can result in fault, which will cause a deadlock when called with
-        * mmap_sem held
+        * mmap_sem held. We don't need to check if get_user() fails, if it does
+        * it won't modify inst, and an inst of 0 will return false from
+        * store_updates_sp().
          */
+       inst = 0;
         if (is_write && is_user)
-               store_update_sp = store_updates_sp(regs);
+               get_user(inst, (unsigned int __user *)regs->nip);
  
         if (is_user)
                 flags |= FAULT_FLAG_USER;

Then this one can go in.