Nasty 2.6 sendfile() bug / regression; affects vsftpd

Hi Andrew, linux-kernel,

Large-file download support with vsftpd and the 2.6 kernel is a broken combination. After careful consideration, I'm pretty sure the bug is with the 2.6 kernel. At the very least, behaviour has changed from 2.4. An initial fix is included below.

vsftpd makes extensive use of sendfile to achieve high performance. When vsftpd was first written, sendfile64 did not exist. So, in order to transfer >2Gb files, multiple sendfile() calls are used [1]. The main issue with sendfile() and >2Gb files is of course that the "offset" in/out parameter cannot be used, because it is not 64-bit. The solution is simple; use NULL for the offset parameter and keep track of the file position in a purely user-space variable.
The above scheme works well with 2.4, but unfortunately fails with EOVERFLOW in 2.6 at the 2Gb mark.
This failure seems to be a bug. I could be wrong here, but it seems the intent of EOVERFLOW is to indicate to the user that a large 64-bit kernel value cannot be stuffed into a 32-bit userspace return value. In the case of using NULL as the offset paramter, userspace does not care about the 64-bit file offset and no truncation will ever occur.
The below patch fixes this up to restore 2.4 behaviour in the event of a NULL offset parameter. It's currently under testing and looking good.

Any chance you can review this and sneak into 2.6.soon?

@@ -545,6 +545,7 @@
 	loff_t pos;
 	ssize_t retval;
 	int fput_needed_in, fput_needed_out;
+	loff_t *orig_ppos = ppos;
 
 	/*
 	 * Get input file, and verify that it is ok..

@@ -599,7 +600,7 @@
 	retval = -EINVAL;
 	if (unlikely(pos < 0))
 		goto fput_out;
-	if (unlikely(pos + count > max)) {
+	if (unlikely(pos + count > max && orig_ppos != NULL)) {
 		retval = -EOVERFLOW;
 		if (pos >= max)
 			goto fput_out;

@@ -608,7 +609,7 @@
 
 	retval = in_file->f_op->sendfile(in_file, ppos, count, file_send_actor, out_file);
 
-	if (*ppos > max)
+	if (*ppos > max && orig_ppos != NULL)
 		retval = -EOVERFLOW;
 
 fput_out:

Cheers
Chris

[1] Note that I'm not inspired to use sendfile64, as it will STILL need multiple sendfile64 calls - the "count" variabe is still 32-bit. The only change from sendfile is that the offset parameter becomes 64-bit.

Your patch is horribly ugly. How about this (much simpler) patch instead?

It just sets the "max" to zero if pos in NULL in the caller. That just seems a much better/saner approach.

Can you test that this one-liner fixes the issue for you?

Linus

----

@@ -635,7 +635,7 @@
 		return ret;
 	}
 
-	return do_sendfile(out_fd, in_fd, NULL, count, MAX_NON_LFS);
+	return do_sendfile(out_fd, in_fd, NULL, count, 0);
 }
 
 asmlinkage ssize_t sys_sendfile64(int out_fd, int in_fd, loff_t __user *offset, size_t count)

Hey, it's a guaranteed way to get your attention ;-)

It certainly does.

Cheers
Chris

Looking at related code, sys_sendfile64 a few lines down.

	if (unlikely(copy_from_user(&pos, offset, sizeof(loff_t))))
		return -EFAULT;

	if (unlikely(put_user(pos, offset)))
		return -EFAULT;

It seems odd that put_user() is used to write an 8-byte value, but get_user() cannot be used read one. I looked in <asm-i386/uaccess.h> and indeed the asymmetry is there.

Is there a reason why put_user() supports 1/2/4/8 bytes and get_user() supports only 1/2/4 bytes?

-- Jamie

It's a bit more complicated to do get_user, mainly because we use a 64-bit value to pass the data around already on x86 - the "real data" in %eax, and the error code in %edx. So you'd need to have a slightly different calling convention for the 8-byte case, so it was more than just "duplicate the other cases".

I agree that it's an ugly special case. get/put_user should really accept all the normal cases, and that includes 'u64'.

Not a lot of code cares, though, so for now we've just had the special case. You are the first one to notice, I think.

Linus

Enjoy,
-- Jamie

Subject: [PATCH] Add 64-bit get_user and __get_user for i386
Patch: uaccess64-2.6.5

Add 64-bit get_user and __get_user for i386. Don't ask me how, but this shrinks the kernel too.

@@ -169,16 +169,19 @@
  * On error, the variable @x is set to zero.
  */
 #define get_user(x,ptr)							\
-({	int __ret_gu,__val_gu;						\
+(sizeof (*(ptr)) == 8							\
+ ? (__copy_from_user_ll((void *)&(x), (ptr), 8) ? -EFAULT : 0) :	\
+({									\
+  	int __ret_gu,__val_gu;						\
 	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;		\
 	case 4:  __get_user_x(4,__ret_gu,__val_gu,ptr); break;		\
-	default: __get_user_x(X,__ret_gu,__val_gu,ptr); break;		\
+	default: __get_user_bad(); break;				\
 	}								\
 	(x) = (__typeof__(*(ptr)))__val_gu;				\
 	__ret_gu;							\
-})
+}))
 
 extern void __put_user_bad(void);
 

@@ -333,13 +336,14 @@
 		: ltype (x), "m"(__m(addr)), "i"(errret), "0"(err))
 
 
-#define __get_user_nocheck(x,ptr,size)				\
-({								\
-	long __gu_err, __gu_val;				\
-	__get_user_size(__gu_val,(ptr),(size),__gu_err,-EFAULT);\
-	(x) = (__typeof__(*(ptr)))__gu_val;			\
-	__gu_err;						\
-})
+#define __get_user_nocheck(x,ptr,size)					     \
+((size) == 8 ? (__copy_from_user_ll((void *)&(x), (ptr), 8) ? -EFAULT : 0) : \
+({									     \
+	long __gu_err, __gu_val;					     \
+	__get_user_size(__gu_val,(ptr),(size),__gu_err,-EFAULT);	     \
+	(x) = (__typeof__(*(ptr)))__gu_val;				     \
+	__gu_err;							     \
+}))
 
 extern long __get_user_bad(void);
 

Ignore the previous patch. It didn't include the access_ok() check in 64-bit get_user(). Use this instead.

Enjoy,
-- Jamie

Subject: [PATCH] Add 64-bit get_user and __get_user for i386
Patch: uaccess64-2.6.5-jl2

Add 64-bit get_user and __get_user for i386. This time, include the access_ok() check. Don't ask me how, but this shrinks the kernel too.

@@ -168,17 +168,19 @@
  * Returns zero on success, or -EFAULT on error.
  * On error, the variable @x is set to zero.
  */
-#define get_user(x,ptr)							\
-({	int __ret_gu,__val_gu;						\
-	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;		\
-	case 4:  __get_user_x(4,__ret_gu,__val_gu,ptr); break;		\
-	default: __get_user_x(X,__ret_gu,__val_gu,ptr); break;		\
-	}								\
-	(x) = (__typeof__(*(ptr)))__val_gu;				\
-	__ret_gu;							\
-})
+#define get_user(x,ptr)							      \
+(sizeof (*(ptr)) == 8 ? (copy_from_user((void *)&(x),(ptr),8) ? -EFAULT : 0): \
+({									      \
+  	int __ret_gu,__val_gu;						      \
+	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;		      \
+	case 4:  __get_user_x(4,__ret_gu,__val_gu,ptr); break;		      \
+	default: __get_user_bad(); break;				      \
+	}								      \
+	(x) = (__typeof__(*(ptr)))__val_gu;				      \
+	__ret_gu;							      \
+}))
 
 extern void __put_user_bad(void);
 

@@ -333,13 +335,14 @@
 		: ltype (x), "m"(__m(addr)), "i"(errret), "0"(err))
 
 
-#define __get_user_nocheck(x,ptr,size)				\
-({								\
-	long __gu_err, __gu_val;				\
-	__get_user_size(__gu_val,(ptr),(size),__gu_err,-EFAULT);\
-	(x) = (__typeof__(*(ptr)))__gu_val;			\
-	__gu_err;						\
-})
+#define __get_user_nocheck(x,ptr,size)					     \
+((size) == 8 ? (__copy_from_user_ll((void *)&(x), (ptr), 8) ? -EFAULT : 0) : \
+({									     \
+	long __gu_err, __gu_val;					     \
+	__get_user_size(__gu_val,(ptr),(size),__gu_err,-EFAULT);	     \
+	(x) = (__typeof__(*(ptr)))__gu_val;				     \
+	__gu_err;							     \
+}))
 
 extern long __get_user_bad(void);
 

Patch: sendfile64_get_user-2.6.5
Requires: uaccess64-2.6.5-jl2

Use get_user instead of copy_from_user to read a loff_t. Compiled output is identical on i386.

@@ -644,7 +644,7 @@
 	ssize_t ret;
 
 	if (offset) {
-		if (unlikely(copy_from_user(&pos, offset, sizeof(loff_t))))
+		if (unlikely(get_user(pos, offset)))
 			return -EFAULT;
 		ret = do_sendfile(out_fd, in_fd, &pos, count, 0);
 		if (unlikely(put_user(pos, offset)))

Fwiw, all architectures have 64-bit get_user except i386 (prior to earlier patch), SH and PPC32.

PPC is weird: it has a get_user64 macro which allows 64 bits. ARM is also weird: it has 64-bit get_user, but not __get_user.

-- Jamie

There must be some bug somewhere if the kernel shrinks from this. Although possibly the bug is in gcc ;)

Anyway, please don't do it like this (ie making one case be just a memcpy). If we do this, let's do it right - ie 'd much rather see something like

	#define get_user(x, ptr)					\
	({ __typeof__(*(ptr)) __val_gu;					\
	   int __ret_gu;						\
	   switch (sizeof(__val_gu)) {					\
	   case 1: __get_user_x(1,__ret_gu,__val_gu,ptr); break;	\
	   case 2: __get_user_x(2,__ret_gu,__val_gu,ptr); break;	\
	   case 4: __get_user_x(4,__ret_gu,__val_gu,ptr); break;	\
	   case 8: __get_user_8(__ret_gu,__val_gu,ptr); break;		\
	   default: __get_user_bad(); break;				\
	   }								\
	   (x) = __val_gu;						\
	   __ret_gu;							\
	})

and then you just make "__get_user_8()" look something like

	#define __get_user_8(ret,x,ptr)				\
		__asm__ __volatile__("call __get_user_8"	\
			:"=A" (ret), "=c" (x)			\
			:"1" (ptr))

Which is _different_ from the other "get_user" cases: it passes the address in %ecx, and it returns the error in %ecx too - the return value comes in %edx:%eax. Make the __get_user_8 in getuser.S match those different rules.

What do you think?

Linus

Wrong. It uses two registers for the 1/2/4-byte cases (%ecx and %eax). It uses three for 8-byte, which is necessary: 8-byte result + error code.

That's achieved by using different asm output constraints: "=a" for 1/2/4 bytes and "=A" for 8 bytes.

Just look, it's obvious. This is the old code

__get_user_4:

	addl $3,%eax
	jc bad_get_user
	GET_THREAD_INFO(%edx)
	cmpl TI_ADDR_LIMIT(%edx),%eax
	jae bad_get_user

Change "jae" to "ja" and it's fine. (x86_64 has copied this bug).

Well spotted. It's an artifact of experiments with __gu_val being long, long long etc. for various sizes of get_user() and seeing unusual warnings when *ptr and/or x are pointers.

Patch re-rolled. The only change is the removal of that cast, and a clarifying comment in getuser.S.

I've checked it compiles to identical code.

Enjoy,
-- Jamie

Subject: [PATCH] Add 64-bit get_user and __get_user for i386
Patch: uaccess64-2.6.5-jl4

Added 64-bit get_user and __get_user for i386.

Changed the registers for all __get_user_X so they could all be
consistent: %ecx now contains the address, and %eax or %edx:%eax contain the result.

There was a boundary bug in getuser.S: get_user(x,(char *)0xbfffffff) would fail. Fixed.

Added might_sleep() to get_user().

Added the 8-byte constant-length cases for __copy_from_user() and __copy_to_user().

Changed constant-length short copy_from_user() to be out of line (it was inline), as this is just another way to write get_user(). It calls the same routines as get_user() and one test showed equivalent uses now generate identical code (sendfile64). Constant-length __copy_from_user() remains inline, just like __get_user().

Very slight cosmetic moving of a few definitions into a more consistent arrangement.

This patch shaved about 1.2k off my kernel, due to the un-inlining of copy_from_user() calls which are equivalent to get_user().

diff -urN --exclude-from=dontdiff dual-2.6.5/arch/i386/kernel/i386_ksyms.c uaccess64-2.6.5/arch/i386/kernel/i386_ksyms.c
@@ -103,6 +103,7 @@
 EXPORT_SYMBOL_NOVERS(__get_user_1);
 EXPORT_SYMBOL_NOVERS(__get_user_2);
 EXPORT_SYMBOL_NOVERS(__get_user_4);
+EXPORT_SYMBOL_NOVERS(__get_user_8);
 
 EXPORT_SYMBOL(strpbrk);
 EXPORT_SYMBOL(strstr);

Well, it's not applying against current kernels (which have already un-inlined some of these things), and that comment still isn't clear to me. There's a comment about casting things, but the thing is, the macro contains no casts anywhere...

Other than that, I'm convinced.

Linus

expand_asm_operands in stmt.c checks for promoted variables, and if it sees one, it will generate a temporary with the unpromoted size to replace it in the asm, and then copy the temporary to the promoted variable after the asm, ensuring that the extension happens after the asm.

I think this is a minor gcc bug. We have been promoting char/short return values since the gcc-1.x days. This promotion occurs regardless of whether the target asks for return values to be promoted. This promotion happens in start_function in c-decl.c. Perhaps this is a left over from the K&R C days, as I don't see anything in the C89 standard that requires this. Changing this now might break ports that require the promotion, but were not explicitly asking for it, so changing this might be tricky. I'd suggest opening a gcc bugzilla bug report if we really care about this.

Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com