Messages in this thread | | | Date | Thu, 03 Dec 2015 14:23:16 -0800 | From | "Shi, Yang" <> | Subject | Re: [V2 PATCH] sparc64/gup: check address scope legitimacy |
| |
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 >
| |