| Message ID | 58f17a04cee5726467ef4e283dfbd7da68fa6ab4.1492606298.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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.
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
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
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
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
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
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
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))
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(-)