| Message ID | 1448491543-17946-1-git-send-email-yang.shi@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
Hi Yang. On Wed, Nov 25, 2015 at 02:45:43PM -0800, Yang Shi wrote: > Check if user address is accessible in atomic version __get_user_pages_fast() > before walking the page table. > And, check if end > start in get_user_pages_fast(), otherwise fallback to slow > path. Two different but related things in one patch is often a bad thing. It would have been better to split it up. > > Signed-off-by: Yang Shi <yang.shi@linaro.org> > --- > Just found slow_irqon label is not defined, added it to avoid compile error. > > arch/sparc/mm/gup.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c > index 2e5c4fc..cf4fb47 100644 > --- a/arch/sparc/mm/gup.c > +++ b/arch/sparc/mm/gup.c > @@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, > addr = start; > len = (unsigned long) nr_pages << PAGE_SHIFT; > end = start + len; > + if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ, > + (void __user *)start, len))) > + return 0; This change is not justified. Why would we take the time to first do the access_ok() stuff. If this had been an expensive operation then we had made this function slower in the normal case ( assuming there were no access violations in the normal case). When I look at the implementation of access_ok() I get the impression that this is not really a check we need. access_ok() always returns 1. > > local_irq_save(flags); > pgdp = pgd_offset(mm, addr); > @@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, > addr = start; > len = (unsigned long) nr_pages << PAGE_SHIFT; > end = start + len; > + if (end < start) > + goto slow_irqon; end can only be smaller than start if there is some overflow. See how end is calculated just the line above. This looks like a highly suspicious change. Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/3/2015 12:38 PM, Sam Ravnborg wrote: > Hi Yang. > > On Wed, Nov 25, 2015 at 02:45:43PM -0800, Yang Shi wrote: >> Check if user address is accessible in atomic version __get_user_pages_fast() >> before walking the page table. >> And, check if end > start in get_user_pages_fast(), otherwise fallback to slow >> path. > > Two different but related things in one patch is often a bad thing. > It would have been better to split it up. > > >> >> Signed-off-by: Yang Shi <yang.shi@linaro.org> >> --- >> Just found slow_irqon label is not defined, added it to avoid compile error. >> >> arch/sparc/mm/gup.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c >> index 2e5c4fc..cf4fb47 100644 >> --- a/arch/sparc/mm/gup.c >> +++ b/arch/sparc/mm/gup.c >> @@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, >> addr = start; >> len = (unsigned long) nr_pages << PAGE_SHIFT; >> end = start + len; >> + if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ, >> + (void __user *)start, len))) >> + return 0; > This change is not justified. > Why would we take the time to first do the access_ok() stuff. > If this had been an expensive operation then we had made this function > slower in the normal case ( assuming there were no access violations in the > normal case). > When I look at the implementation of access_ok() I get the impression that > this is not really a check we need. > > access_ok() always returns 1. Thanks for pointing it out. And, I didn't notice that gup is just built for SPARC64. I though it is built by both 64 bit and 32 bit. A follow-up question, is there any reason to just have sparc specific fast gup for 64 bit not for 32 bit? > > >> >> local_irq_save(flags); >> pgdp = pgd_offset(mm, addr); >> @@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, >> addr = start; >> len = (unsigned long) nr_pages << PAGE_SHIFT; >> end = start + len; >> + if (end < start) >> + goto slow_irqon; > > end can only be smaller than start if there is some overflow. > See how end is calculated just the line above. > > This looks like a highly suspicious change. I'm supposed this is used to protect the overflow. I copied the code from other arch. Actually, every arch has this except sparc. Thanks, Yang > > Sam > -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >access_ok() always returns 1. > > Thanks for pointing it out. And, I didn't notice that gup is just built for > SPARC64. I though it is built by both 64 bit and 32 bit. > > A follow-up question, is there any reason to just have sparc specific fast > gup for 64 bit not for 32 bit? I do not know - sorry. > >>@@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, > >> addr = start; > >> len = (unsigned long) nr_pages << PAGE_SHIFT; > >> end = start + len; > >>+ if (end < start) > >>+ goto slow_irqon; > > > >end can only be smaller than start if there is some overflow. > >See how end is calculated just the line above. > > > >This looks like a highly suspicious change. > > I'm supposed this is used to protect the overflow. I copied the code from > other arch. Actually, every arch has this except sparc. The the other archs are likely confused as well - there is most likely some history behind that can be found if digging a little. The code is not present in the generic version. Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c index 2e5c4fc..cf4fb47 100644 --- a/arch/sparc/mm/gup.c +++ b/arch/sparc/mm/gup.c @@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, addr = start; len = (unsigned long) nr_pages << PAGE_SHIFT; end = start + len; + if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ, + (void __user *)start, len))) + return 0; local_irq_save(flags); pgdp = pgd_offset(mm, addr); @@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, addr = start; len = (unsigned long) nr_pages << PAGE_SHIFT; end = start + len; + if (end < start) + goto slow_irqon; /* * XXX: batch / limit 'nr', to avoid large irq off latency @@ -244,7 +249,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, slow: local_irq_enable(); - +slow_irqon: /* Try to get the remaining pages with get_user_pages */ start += nr << PAGE_SHIFT; pages += nr;
Check if user address is accessible in atomic version __get_user_pages_fast() before walking the page table. And, check if end > start in get_user_pages_fast(), otherwise fallback to slow path. Signed-off-by: Yang Shi <yang.shi@linaro.org> --- Just found slow_irqon label is not defined, added it to avoid compile error. arch/sparc/mm/gup.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)