Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault()

[Michael Ellerman <mpe@ellerman.id.au>]

christophe leroy <christophe.leroy@c-s.fr> writes:

> Le 05/06/2017 à 12:45, Michael Ellerman a écrit :
>> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>>
>>> Le 02/06/2017 à 11:26, Michael Ellerman a écrit :
>>>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>>>
>>>>> 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.

cheers