might_sleep() in copy_{from,to}_user and friends?

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 =====

@@ -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;
 }
 		

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.

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 =====

@@ -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 =====

@@ -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;	\

Will update to the new kconfig thingy later, and forward it on..

Dave

| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

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."