I have been looking for more places in 2.5 that can be marked
might_sleep() and noticed that all the functions in asm/uaccess.h
are not marked although they sleep if the memory they access
has to be paged in.
After adding might_sleep() in ten places in asm-i386/uaccess.h
and arch/i386/lib/usercopy.c, I have been running this kernel
for about two weeks. So far, I have found only one place where
the kernel actually hits this and that one is trivially
fixable (maintainer cc'd, fix see below).
The question is if we can expect to find more bugs like
that if we have might_sleep() in uaccess.h or if the
extra cycles and the work of changing ~100 places (for all
architectures) are just not worth it.
Arnd <><
===== sound/core/pcm_native.c 1.17 vs edited =====
--- 1.17/sound/core/pcm_native.c Sun Oct 13 21:19:17 2002
+++ edited/sound/core/pcm_native.c Fri Nov 1 12:43:38 2002
@@ -2014,8 +2014,6 @@
n = snd_pcm_playback_hw_avail(runtime);
else
n = snd_pcm_capture_avail(runtime);
- if (put_user(n, res))
- err = -EFAULT;
break;
case SNDRV_PCM_STATE_XRUN:
err = -EPIPE;
@@ -2026,6 +2024,9 @@
break;
}
spin_unlock_irq(&runtime->lock);
+ if (!ret)
+ if (put_user(n, res))
+ err = -EFAULT;
return err;
}
On Fri, 1 Nov 2002, Arnd Bergmann wrote:
> I have been looking for more places in 2.5 that can be marked
> might_sleep() and noticed that all the functions in asm/uaccess.h
> are not marked although they sleep if the memory they access
> has to be paged in.
>
> After adding might_sleep() in ten places in asm-i386/uaccess.h
> and arch/i386/lib/usercopy.c, I have been running this kernel
> for about two weeks. So far, I have found only one place where
> the kernel actually hits this and that one is trivially
> fixable (maintainer cc'd, fix see below).
>
> The question is if we can expect to find more bugs like
> that if we have might_sleep() in uaccess.h or if the
> extra cycles and the work of changing ~100 places (for all
> architectures) are just not worth it.
>
> Arnd <><
>
> ===== sound/core/pcm_native.c 1.17 vs edited =====
> --- 1.17/sound/core/pcm_native.c Sun Oct 13 21:19:17 2002
> +++ edited/sound/core/pcm_native.c Fri Nov 1 12:43:38 2002
> @@ -2014,8 +2014,6 @@
> n = snd_pcm_playback_hw_avail(runtime);
> else
> n = snd_pcm_capture_avail(runtime);
> - if (put_user(n, res))
> - err = -EFAULT;
> break;
> case SNDRV_PCM_STATE_XRUN:
> err = -EPIPE;
> @@ -2026,6 +2024,9 @@
> break;
> }
> spin_unlock_irq(&runtime->lock);
> + if (!ret)
^^^^^^^^^
Shouldn't be here 'if (!err)' ? The fix is in our CVS. Thanks.
> + if (put_user(n, res))
> + err = -EFAULT;
> return err;
> }
Jaroslav
-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project http://www.alsa-project.org
SuSE Linux http://www.suse.com
Arnd Bergmann wrote:
>
> I have been looking for more places in 2.5 that can be marked
> might_sleep() and noticed that all the functions in asm/uaccess.h
> are not marked although they sleep if the memory they access
> has to be paged in.
>
> After adding might_sleep() in ten places in asm-i386/uaccess.h
> and arch/i386/lib/usercopy.c, I have been running this kernel
> for about two weeks.
This is an excellent point. If someone is holding a lock
across a uaccess function and userspace has passed the address
of a valid but not-present page we will hit the "atomic copy_user"
path. Userspace will be returned an EFAULT and will be left
scratching its head, wondering what it did wrong.
Or the kernel will deadlock, of course.
I don't think we need to add the check to anything other than
ia32. That will pick up the great bulk of any problems, and
arch-specific code won't be doing these copies much anyway.
So if you could prepare a patch which adds these checks for
ia32 it would be muchly appreciated.
And if you're feeling really keen, Dave Jones has a patch which
makes the might_sleep check a real config option rather than
overloading CONFIG_DEBUG_KERNEL - would be nice to squeeze that
out of him if poss.
Thanks.
On Friday 01 November 2002 11:51, Andrew Morton wrote:
> I don't think we need to add the check to anything other than
> ia32. That will pick up the great bulk of any problems, and
> arch-specific code won't be doing these copies much anyway.
>
> So if you could prepare a patch which adds these checks for
> ia32 it would be muchly appreciated.
Ok. This is the patch I was using, forward ported to
today's bitkeeper version.
Arnd <><
===== arch/i386/lib/usercopy.c 1.8 vs edited =====
--- 1.8/arch/i386/lib/usercopy.c Tue Oct 29 22:10:51 2002
+++ edited/arch/i386/lib/usercopy.c Fri Nov 1 15:09:23 2002
@@ -25,6 +25,7 @@
#define __do_strncpy_from_user(dst,src,count,res) \
do { \
int __d0, __d1, __d2; \
+ might_sleep(); \
__asm__ __volatile__( \
" testl %1,%1\n" \
" jz 2f\n" \
@@ -75,7 +76,8 @@
#define __do_clear_user(addr,size) \
do { \
int __d0; \
- __asm__ __volatile__( \
+ might_sleep(); \
+ __asm__ __volatile__( \
"0: rep; stosl\n" \
" movl %2,%0\n" \
"1: rep; stosb\n" \
@@ -119,6 +121,7 @@
unsigned long mask = -__addr_ok(s);
unsigned long res, tmp;
+ might_sleep();
__asm__ __volatile__(
" testl %0, %0\n"
" jz 3f\n"
@@ -419,6 +422,7 @@
unsigned long __copy_to_user(void *to, const void *from, unsigned long n)
{
+ might_sleep();
if (movsl_is_ok(to, from, n))
__copy_user(to, from, n);
else
@@ -428,6 +432,7 @@
unsigned long __copy_from_user(void *to, const void *from, unsigned long n)
{
+ might_sleep();
if (movsl_is_ok(to, from, n))
__copy_user_zeroing(to, from, n);
else
===== include/asm-i386/uaccess.h 1.12 vs edited =====
--- 1.12/include/asm-i386/uaccess.h Sat Oct 12 12:18:15 2002
+++ edited/include/asm-i386/uaccess.h Fri Nov 1 14:44:32 2002
@@ -123,6 +123,7 @@
/* Careful: we have to cast the result to the type of the pointer for sign reasons */
#define get_user(x,ptr) \
({ int __ret_gu,__val_gu; \
+ might_sleep(); \
switch(sizeof (*(ptr))) { \
case 1: __get_user_x(1,__ret_gu,__val_gu,ptr); break; \
case 2: __get_user_x(2,__ret_gu,__val_gu,ptr); break; \
@@ -185,6 +186,7 @@
#define __put_user_size(x,ptr,size,retval) \
do { \
retval = 0; \
+ might_sleep(); \
switch (size) { \
case 1: __put_user_asm(x,ptr,retval,"b","b","iq"); break; \
case 2: __put_user_asm(x,ptr,retval,"w","w","ir"); break; \
@@ -231,6 +233,7 @@
#define __get_user_size(x,ptr,size,retval) \
do { \
retval = 0; \
+ might_sleep(); \
switch (size) { \
case 1: __get_user_asm(x,ptr,retval,"b","b","=q"); break; \
case 2: __get_user_asm(x,ptr,retval,"w","w","=r"); break; \
Arnd wrote:
> I have been looking for more places in 2.5 that can be marked
> might_sleep() and noticed that all the functions in asm/uaccess.h
> are not marked although they sleep if the memory they access
> has to be paged in.
>
Good idea.
There is some abuse of __get_user to identify bad pointers:
show_registers in the oops codepath, mm/slab.c in the /proc/slabinfo code.
Could you omit the test from the __ versions?
--
Manfred
On Fri, Nov 01, 2002 at 02:51:17AM -0800, Andrew Morton wrote:
> And if you're feeling really keen, Dave Jones has a patch which
> makes the might_sleep check a real config option rather than
> overloading CONFIG_DEBUG_KERNEL - would be nice to squeeze that
> out of him if poss.
Will update to the new kconfig thingy later, and forward it on..
Dave
--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs
On Friday 01 November 2002 14:47, Manfred Spraul wrote:
> Good idea.
> There is some abuse of __get_user to identify bad pointers:
> show_registers in the oops codepath, mm/slab.c in the /proc/slabinfo code.
>
> Could you omit the test from the __ versions?
That would we the patch below. But shouldn't those abuses
of __get_user rather be changed to use something else?
The name is just wrong there and the first argument is
never used.
We could instead do something like
#define check_pointer(p) ({ \
typeof(*(p)) __t; \
long __gu_err; \
__get_user_size(__t, p, sizeof(__t), __gu_err); \
__gu_err; \
})
Arnd <><
===== arch/i386/lib/usercopy.c 1.8 vs edited =====
--- 1.8/arch/i386/lib/usercopy.c Tue Oct 29 22:10:51 2002
+++ edited/arch/i386/lib/usercopy.c Fri Nov 1 17:10:30 2002
@@ -62,6 +62,7 @@
strncpy_from_user(char *dst, const char *src, long count)
{
long res = -EFAULT;
+ might_sleep();
if (access_ok(VERIFY_READ, src, 1))
__do_strncpy_from_user(dst, src, count, res);
return res;
@@ -96,6 +97,7 @@
unsigned long
clear_user(void *to, unsigned long n)
{
+ might_sleep();
if (access_ok(VERIFY_WRITE, to, n))
__do_clear_user(to, n);
return n;
@@ -119,6 +121,7 @@
unsigned long mask = -__addr_ok(s);
unsigned long res, tmp;
+ might_sleep();
__asm__ __volatile__(
" testl %0, %0\n"
" jz 3f\n"
@@ -438,6 +441,7 @@
unsigned long copy_to_user(void *to, const void *from, unsigned long n)
{
prefetch(from);
+ might_sleep();
if (access_ok(VERIFY_WRITE, to, n))
n = __copy_to_user(to, from, n);
return n;
@@ -446,6 +450,7 @@
unsigned long copy_from_user(void *to, const void *from, unsigned long n)
{
prefetchw(to);
+ might_sleep();
if (access_ok(VERIFY_READ, from, n))
n = __copy_from_user(to, from, n);
return n;
===== include/asm-i386/uaccess.h 1.12 vs edited =====
--- 1.12/include/asm-i386/uaccess.h Sat Oct 12 12:18:15 2002
+++ edited/include/asm-i386/uaccess.h Fri Nov 1 17:07:40 2002
@@ -123,6 +123,7 @@
/* Careful: we have to cast the result to the type of the pointer for sign reasons */
#define get_user(x,ptr) \
({ int __ret_gu,__val_gu; \
+ might_sleep(); \
switch(sizeof (*(ptr))) { \
case 1: __get_user_x(1,__ret_gu,__val_gu,ptr); break; \
case 2: __get_user_x(2,__ret_gu,__val_gu,ptr); break; \
@@ -158,8 +159,9 @@
#define __put_user_check(x,ptr,size) \
({ \
- long __pu_err = -EFAULT; \
+ long __pu_err = -EFAULT; \
__typeof__(*(ptr)) *__pu_addr = (ptr); \
+ might_sleep(); \
if (access_ok(VERIFY_WRITE,__pu_addr,size)) \
__put_user_size((x),__pu_addr,(size),__pu_err); \
__pu_err; \
On Fri, 1 Nov 2002, Andrew Morton wrote:
| Arnd Bergmann wrote:
| >
| > I have been looking for more places in 2.5 that can be marked
| > might_sleep() and noticed that all the functions in asm/uaccess.h
| > are not marked although they sleep if the memory they access
| > has to be paged in.
| >
| > After adding might_sleep() in ten places in asm-i386/uaccess.h
| > and arch/i386/lib/usercopy.c, I have been running this kernel
| > for about two weeks.
|
| This is an excellent point. If someone is holding a lock
| across a uaccess function and userspace has passed the address
| of a valid but not-present page we will hit the "atomic copy_user"
| path. Userspace will be returned an EFAULT and will be left
| scratching its head, wondering what it did wrong.
|
| Or the kernel will deadlock, of course.
|
| I don't think we need to add the check to anything other than
| ia32. That will pick up the great bulk of any problems, and
| arch-specific code won't be doing these copies much anyway.
Another thing to consider is that the rate-limiting in
__might_sleep() hides lots of instances being reported -- or at
least it did when I removed that rate-limiting and had to wait
for 2-3 minutes for all of that scrolling to finish.
I guess that if enough people test it and give feedback, we'll see
and fix all of them eventually...
--
~Randy
"I'm a healthy mushroom."