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 <[email protected]>
---
arch/sparc/mm/gup.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
index 2e5c4fc..08b8da5 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
--
2.0.2
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 <[email protected]>
---
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;
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;
--
2.0.2
Hi Yang,
[auto build test ERROR on v4.4-rc2]
[also build test ERROR on next-20151124]
url: https://github.com/0day-ci/linux/commits/Yang-Shi/sparc64-gup-check-address-scope-legitimacy/20151126-065342
config: sparc64-allnoconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All errors (new ones prefixed by >>):
arch/sparc/mm/gup.c: In function 'get_user_pages_fast':
>> arch/sparc/mm/gup.c:210:3: error: label 'slow_irqon' used but not defined
goto slow_irqon;
^
vim +/slow_irqon +210 arch/sparc/mm/gup.c
204
205 start &= PAGE_MASK;
206 addr = start;
207 len = (unsigned long) nr_pages << PAGE_SHIFT;
208 end = start + len;
209 if (end < start)
> 210 goto slow_irqon;
211
212 /*
213 * XXX: batch / limit 'nr', to avoid large irq off latency
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 11/25/2015 4:26 PM, kbuild test robot wrote:
> Hi Yang,
>
> [auto build test ERROR on v4.4-rc2]
> [also build test ERROR on next-20151124]
>
> url: https://github.com/0day-ci/linux/commits/Yang-Shi/sparc64-gup-check-address-scope-legitimacy/20151126-065342
> config: sparc64-allnoconfig (attached as .config)
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=sparc64
>
> All errors (new ones prefixed by >>):
>
> arch/sparc/mm/gup.c: In function 'get_user_pages_fast':
>>> arch/sparc/mm/gup.c:210:3: error: label 'slow_irqon' used but not defined
> goto slow_irqon;
Already fixed in v2.
Thanks,
Yang
> ^
>
> vim +/slow_irqon +210 arch/sparc/mm/gup.c
>
> 204
> 205 start &= PAGE_MASK;
> 206 addr = start;
> 207 len = (unsigned long) nr_pages << PAGE_SHIFT;
> 208 end = start + len;
> 209 if (end < start)
> > 210 goto slow_irqon;
> 211
> 212 /*
> 213 * XXX: batch / limit 'nr', to avoid large irq off latency
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
> >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