Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656066 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault() Michael Ellerman <mpe@ellerman.id.au> - 2017-06-02 11:30 +0200
    Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in  do_page_fault() Christophe LEROY <christophe.leroy@c-s.fr> - 2017-06-02 11:50 +0200
      Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts  in do_page_fault() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-06-02 14:20 +0200
        Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in  do_page_fault() Christophe LEROY <christophe.leroy@c-s.fr> - 2017-06-02 14:40 +0200
          Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault() Michael Ellerman <mpe@ellerman.id.au> - 2017-06-05 12:50 +0200
      Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault() Michael Ellerman <mpe@ellerman.id.au> - 2017-06-05 12:50 +0200
        Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in  do_page_fault() christophe leroy <christophe.leroy@c-s.fr> - 2017-06-05 19:50 +0200
          Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault() Michael Ellerman <mpe@ellerman.id.au> - 2017-06-06 13:10 +0200
            Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in  do_page_fault() Christophe LEROY <christophe.leroy@c-s.fr> - 2017-06-06 15:40 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1657507

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

[toc] | [prev] | [next] | [standalone]

#1657508

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1658634

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web