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

Message ID 58f17a04cee5726467ef4e283dfbd7da68fa6ab4.1492606298.git.christophe.leroy@c-s.fr
State Superseded
Headers show

Commit Message

Christophe LEROY April 19, 2017, 12:56 p.m.
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()

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/mm/fault.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Aneesh Kumar K.V April 24, 2017, 9:11 a.m. | #1
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()
>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  arch/powerpc/mm/fault.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 67fefb59d40e..9d21e5fd383d 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -73,12 +73,8 @@ static inline int notify_page_fault(struct pt_regs *regs)
>   * Check whether the instruction at regs->nip is a store using
>   * an update addressing form which will update r1.
>   */
> -static int store_updates_sp(struct pt_regs *regs)
> +static int store_updates_sp(unsigned int inst)
>  {
> -	unsigned int inst;
> -
> -	if (get_user(inst, (unsigned int __user *)regs->nip))
> -		return 0;
>  	/* check for 1 in the rA field */
>  	if (((inst >> 16) & 0x1f) != 1)
>  		return 0;
> @@ -207,7 +203,8 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  	int trap = TRAP(regs);
>   	int is_exec = trap == 0x400;
>  	int fault;
> -	int rc = 0, store_update_sp = 0;
> +	int rc = 0;
> +	unsigned int inst = 0;
>  
>  #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
>  	/*
> @@ -288,7 +285,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  	 * mmap_sem held
>  	 */
>  	if (is_write && user_mode(regs))
> -		store_update_sp = store_updates_sp(regs);
> +		__get_user(inst, (unsigned int __user *)regs->nip);
>  
>  	if (user_mode(regs))
>  		flags |= FAULT_FLAG_USER;
> @@ -358,7 +355,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  		 * between the last mapped region and the stack will
>  		 * expand the stack rather than segfaulting.
>  		 */
> -		if (address + 2048 < uregs->gpr[1] && !store_update_sp)
> +		if (address + 2048 < uregs->gpr[1] && !store_updates_sp(inst))
>  			goto bad_area;
>  	}
>  	if (expand_stack(vma, address))
> -- 
> 2.12.0
Michael Ellerman June 2, 2017, 9:26 a.m. | #2
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.

cheers
Christophe LEROY June 2, 2017, 9:39 a.m. | #3
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 ?

Christophe
Benjamin Herrenschmidt June 2, 2017, 12:11 p.m. | #4
On Fri, 2017-06-02 at 11:39 +0200, Christophe LEROY wrote:
> 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 ?

Do you have a measurable improvement by skipping that check ? I agree
with your reasoning but I'm also paranoid and so I wouldn't change it
unless it's really worth it.

Cheers,
Ben.
Christophe LEROY June 2, 2017, 12:31 p.m. | #5
Le 02/06/2017 à 14:11, Benjamin Herrenschmidt a écrit :
> On Fri, 2017-06-02 at 11:39 +0200, Christophe LEROY wrote:
>> 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 ?
> 
> Do you have a measurable improvement by skipping that check ? I agree
> with your reasoning but I'm also paranoid and so I wouldn't change it
> unless it's really worth it.
> 

No I don't have. Taking into account the patch following this serie 
which limits even more the calls to get_user(), it is probably not worth 
it anymore (see https://patchwork.ozlabs.org/patch/757564/)

I will then have to resubmit the entire serie (including that additional 
one), but there is no get_user_inatomic() so will have to either:
- do the access_ok() verification inside the function
- get back to v2 (https://patchwork.ozlabs.org/patch/756234/)
- implement an get_user_inatomic() function

What would be the best ?

Christophe
Michael Ellerman June 5, 2017, 10:45 a.m. | #6
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.

cheers
Michael Ellerman June 5, 2017, 10:49 a.m. | #7
Christophe LEROY <christophe.leroy@c-s.fr> writes:

> Le 02/06/2017 à 14:11, Benjamin Herrenschmidt a écrit :
>> On Fri, 2017-06-02 at 11:39 +0200, Christophe LEROY wrote:
>>> 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 ?
>> 
>> Do you have a measurable improvement by skipping that check ? I agree
>> with your reasoning but I'm also paranoid and so I wouldn't change it
>> unless it's really worth it.
>> 
>
> No I don't have. Taking into account the patch following this serie 
> which limits even more the calls to get_user(), it is probably not worth 
> it anymore (see https://patchwork.ozlabs.org/patch/757564/)
>
> I will then have to resubmit the entire serie (including that additional 
> one), but there is no get_user_inatomic() so will have to either:
> - do the access_ok() verification inside the function

I think open coding the access_ok() check is probably the best option.

cheers
Christophe LEROY June 5, 2017, 5:48 p.m. | #8
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.

Christophe

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
Michael Ellerman June 6, 2017, 11 a.m. | #9
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
Christophe LEROY June 6, 2017, 1:29 p.m. | #10
Le 06/06/2017 à 13:00, Michael Ellerman a écrit :
> 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.
> 

I just submitted v4 version of the patch "powerpc/mm: Only read faulting 
instruction when necessary in do_page_fault()", skipping this step and 
going directly to the final solution.
The new approach has been to keep everything inside store_updates_sp() 
function and just move the call.

Christophe

Patch

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 67fefb59d40e..9d21e5fd383d 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -73,12 +73,8 @@  static inline int notify_page_fault(struct pt_regs *regs)
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
  */
-static int store_updates_sp(struct pt_regs *regs)
+static int store_updates_sp(unsigned int inst)
 {
-	unsigned int inst;
-
-	if (get_user(inst, (unsigned int __user *)regs->nip))
-		return 0;
 	/* check for 1 in the rA field */
 	if (((inst >> 16) & 0x1f) != 1)
 		return 0;
@@ -207,7 +203,8 @@  int do_page_fault(struct pt_regs *regs, unsigned long address,
 	int trap = TRAP(regs);
  	int is_exec = trap == 0x400;
 	int fault;
-	int rc = 0, store_update_sp = 0;
+	int rc = 0;
+	unsigned int inst = 0;
 
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
 	/*
@@ -288,7 +285,7 @@  int do_page_fault(struct pt_regs *regs, unsigned long address,
 	 * mmap_sem held
 	 */
 	if (is_write && user_mode(regs))
-		store_update_sp = store_updates_sp(regs);
+		__get_user(inst, (unsigned int __user *)regs->nip);
 
 	if (user_mode(regs))
 		flags |= FAULT_FLAG_USER;
@@ -358,7 +355,7 @@  int do_page_fault(struct pt_regs *regs, unsigned long address,
 		 * between the last mapped region and the stack will
 		 * expand the stack rather than segfaulting.
 		 */
-		if (address + 2048 < uregs->gpr[1] && !store_update_sp)
+		if (address + 2048 < uregs->gpr[1] && !store_updates_sp(inst))
 			goto bad_area;
 	}
 	if (expand_stack(vma, address))