| Message ID | 1448491543-17946-1-git-send-email-yang.shi@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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(-) -- 2.0.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/