sendfile -EOVERFLOW on AMD64

sendfile -EOVERFLOW on AMD64

[Jan Kasprzak]

	Hello,

today (after the Fedora Core 2 release) I have discovered that my FTP
server (running AMD64) cannot correctly send the >2GB files using sendfile().
The server is ProFTPd, and the strace looks like this:

[...]
alarm(288)                              = 0
sendfile(14, 11, [139377240], 4231263656) = 2008106407
alarm(0)                                = 117
alarm(117)                              = 0
alarm(0)                                = 117
alarm(117)                              = 0
sendfile(14, 11, [2147483647], 2223157249) = -1 EOVERFLOW (Value too large for defined data type)

The image (FC2-i386-DVD.iso) has 4370640896 bytes. The FTP server is native
x86_64 binary, not a 32-bit one.

Thanks for any hints.

-Yenya

-- 
| Jan "Yenya" Kasprzak  <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839      Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/   Czech Linux Homepage: http://www.linux.cz/ |
 Any compiler or language that likes to hide things like memory allocations
 behind your back just isn't a good choice for a kernel.   --Linus Torvalds

Re: sendfile -EOVERFLOW on AMD64

[Andi Kleen]

Jan Kasprzak <kas@informatics.muni.cz> writes:
>
> The image (FC2-i386-DVD.iso) has 4370640896 bytes. The FTP server is native
> x86_64 binary, not a 32-bit one.

sys_sendfile limits itself dumbly to 2GB even on 64bit architectures.
This patch should fix it on x86-64, although other 64bit ports may 
need a similar patch. Just removing the limit in read_write 
is not easy, because it would need fixes in all the 32bit emulation
layers.

-Andi

diff -u linux-2.6.6-amd64/include/asm-x86_64/unistd.h-o linux-2.6.6-amd64/include/asm-x86_64/unistd.h
--- linux-2.6.6-amd64/include/asm-x86_64/unistd.h-o	2004-05-09 14:30:09.000000000 +0200
+++ linux-2.6.6-amd64/include/asm-x86_64/unistd.h	2004-05-19 11:27:00.000000000 +0200
@@ -98,7 +98,7 @@
 __SYSCALL(__NR_getpid, sys_getpid)
 
 #define __NR_sendfile                           40
-__SYSCALL(__NR_sendfile, sys_sendfile)
+__SYSCALL(__NR_sendfile, sys_sendfile64)
 #define __NR_socket                             41
 __SYSCALL(__NR_socket, sys_socket)
 #define __NR_connect                            42

Re: sendfile -EOVERFLOW on AMD64

[Jan Kasprzak]

Andi Kleen wrote:
: Jan Kasprzak <kas@informatics.muni.cz> writes:
: >
: > The image (FC2-i386-DVD.iso) has 4370640896 bytes. The FTP server is native
: > x86_64 binary, not a 32-bit one.
: 
: sys_sendfile limits itself dumbly to 2GB even on 64bit architectures.
: This patch should fix it on x86-64, although other 64bit ports may 
: need a similar patch. Just removing the limit in read_write 
: is not easy, because it would need fixes in all the 32bit emulation
: layers.
: 
	It partly helped, thanks. But there is still one more problem
- it looks like sendfile() returns 32-bit value instead of 64-bit.
My debug info looks like this:

sendfile(offset=0, count=4370640896)
    = -767073160, offset=3527894136

where I do

	long val = sendfile(...); printf(...%ld..., val);

-Yenya


-- 
| Jan "Yenya" Kasprzak  <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839      Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/   Czech Linux Homepage: http://www.linux.cz/ |
 Any compiler or language that likes to hide things like memory allocations
 behind your back just isn't a good choice for a kernel.   --Linus Torvalds

Re: sendfile -EOVERFLOW on AMD64

[Jakub Jelinek]

On Wed, May 19, 2004 at 12:38:56PM +0200, Jan Kasprzak wrote:
> Andi Kleen wrote:
> : Jan Kasprzak <kas@informatics.muni.cz> writes:
> : >
> : > The image (FC2-i386-DVD.iso) has 4370640896 bytes. The FTP server is native
> : > x86_64 binary, not a 32-bit one.
> : 
> : sys_sendfile limits itself dumbly to 2GB even on 64bit architectures.
> : This patch should fix it on x86-64, although other 64bit ports may 
> : need a similar patch. Just removing the limit in read_write 
> : is not easy, because it would need fixes in all the 32bit emulation
> : layers.
> : 
> 	It partly helped, thanks. But there is still one more problem
> - it looks like sendfile() returns 32-bit value instead of 64-bit.
> My debug info looks like this:
> 
> sendfile(offset=0, count=4370640896)
>     = -767073160, offset=3527894136
> 
> where I do
> 
> 	long val = sendfile(...); printf(...%ld..., val);

What filesystem you're using?
For XFS I'd expect this:
STATIC ssize_t
linvfs_sendfile(
        struct file             *filp,
        loff_t                  *ppos,
        size_t                  count,
        read_actor_t            actor,
        void                    *target)
{
        vnode_t                 *vp = LINVFS_GET_VP(filp->f_dentry->d_inode);
        int                     error;

        VOP_SENDFILE(vp, filp, ppos, 0, count, actor, target, NULL, error);
        return error;
}

(note error is int, not ssize_t), but I don't see anything obvious
for other filesystems.

	Jakub

Re: sendfile -EOVERFLOW on AMD64

[Jan Kasprzak]

Jakub Jelinek wrote:
: For XFS I'd expect this:
: STATIC ssize_t
: linvfs_sendfile(
:         struct file             *filp,
:         loff_t                  *ppos,
:         size_t                  count,
:         read_actor_t            actor,
:         void                    *target)
: {
:         vnode_t                 *vp = LINVFS_GET_VP(filp->f_dentry->d_inode);
:         int                     error;
: 
:         VOP_SENDFILE(vp, filp, ppos, 0, count, actor, target, NULL, error);
:         return error;
: }
: 
: (note error is int, not ssize_t), but I don't see anything obvious
: for other filesystems.
: 
	Yes, XFS. I will look at it  in the evening.

Thanks,

-Yenya

-- 
| Jan "Yenya" Kasprzak  <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839      Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/   Czech Linux Homepage: http://www.linux.cz/ |
 Any compiler or language that likes to hide things like memory allocations
 behind your back just isn't a good choice for a kernel.   --Linus Torvalds

Re: sendfile -EOVERFLOW on AMD64

[Nathan Scott]

On Wed, May 19, 2004 at 06:58:06AM -0400, Jakub Jelinek wrote:
> ...
> (note error is int, not ssize_t), but I don't see anything obvious
> for other filesystems.

Thanks, I'll fix that up.

cheers.

-- 
Nathan

Re: sendfile -EOVERFLOW on AMD64

[Nathan Scott]

On Wed, May 19, 2004 at 01:01:05PM +0200, Jan Kasprzak wrote:
> ...
> 	Yes, XFS. I will look at it  in the evening.
> 
> Thanks,

The only spot needing fixing is that one pointed out above; from
a quick look ssize_t seems to be used correctly in the rest of
the XFS sendfile code.

cheers.

-- 
Nathan

Re: sendfile -EOVERFLOW on AMD64

[Andi Kleen]

> (note error is int, not ssize_t), but I don't see anything obvious
> for other filesystems.

sendfile64 on 32bit hosts seems to be quite fishy too.

It works with ssize_t, which is 32bit only, but there are no checks
that the transfered file is not >4GB.  It would just wrap in this case.
It would be better to add such checks for 32bit hosts to sendfile64.

Or am I missing something?

-Andi

Re: sendfile -EOVERFLOW on AMD64

[Jakub Jelinek]

On Wed, May 19, 2004 at 02:44:27PM +0200, Andi Kleen wrote:
> > (note error is int, not ssize_t), but I don't see anything obvious
> > for other filesystems.
> 
> sendfile64 on 32bit hosts seems to be quite fishy too.
> 
> It works with ssize_t, which is 32bit only, but there are no checks
> that the transfered file is not >4GB.  It would just wrap in this case.
> It would be better to add such checks for 32bit hosts to sendfile64.
> 
> Or am I missing something?

The userland prototypes are:
extern ssize_t sendfile (int __out_fd, int __in_fd, off_t *__offset,
                         size_t __count) __THROW;
extern ssize_t sendfile64 (int __out_fd, int __in_fd, __off64_t *__offset,
                           size_t __count) __THROW;
Thus you really cannot transfer more than 4G-1 bytes in one call on 32-bit
arches.
Actually, already any counts >= 2GB-1 might be problematic, but we are
there on the same boat as with e.g. read(2).  For read, POSIX says:
"If the value of nbyte is greater than {SSIZE_MAX}, the result is
implementation-defined."
so portable programs really shouldn't try to transfer more than that
in one go but the kernel certainly should try to handle sizes up to
SIZE_MAX-4096 or something like that.

	Jakub

Re: sendfile -EOVERFLOW on AMD64

[Andi Kleen]

> The userland prototypes are:
> extern ssize_t sendfile (int __out_fd, int __in_fd, off_t *__offset,
>                          size_t __count) __THROW;
> extern ssize_t sendfile64 (int __out_fd, int __in_fd, __off64_t *__offset,
>                            size_t __count) __THROW;
> Thus you really cannot transfer more than 4G-1 bytes in one call on 32-bit
> arches.

True.

> Actually, already any counts >= 2GB-1 might be problematic, but we are
> there on the same boat as with e.g. read(2).  For read, POSIX says:
> "If the value of nbyte is greater than {SSIZE_MAX}, the result is
> implementation-defined."
> so portable programs really shouldn't try to transfer more than that
> in one go but the kernel certainly should try to handle sizes up to
> SIZE_MAX-4096 or something like that.

Hmm, ssize_t is signed. If there are any if (ret < 0) checks in the
kernel it will fail too. So the real limit would be 0x7fffffff

Perhaps that should be enforced like this?


diff -u linux/fs/read_write.c-o linux/fs/read_write.c
--- linux/fs/read_write.c-o	2004-05-18 10:55:54.000000000 +0200
+++ linux/fs/read_write.c	2004-05-19 14:57:06.000000000 +0200
@@ -700,6 +700,9 @@
 	off_t off;
 	ssize_t ret;
 
+	if (sizeof(size_t) == 4 && (int)count < 0)
+		return -EOVERFLOW;
+
 	if (offset) {
 		if (unlikely(get_user(off, offset)))
 			return -EFAULT;
@@ -718,6 +721,9 @@
 	loff_t pos;
 	ssize_t ret;
 
+	if (sizeof(size_t) == 4 && (int)count < 0)
+		return -EOVERFLOW;
+		
 	if (offset) {
 		if (unlikely(copy_from_user(&pos, offset, sizeof(loff_t))))
 			return -EFAULT;

-Andi

Re: sendfile -EOVERFLOW on AMD64

[Jan Kasprzak]

Jakub Jelinek wrote:
:         int                     error;
: 
:         VOP_SENDFILE(vp, filp, ppos, 0, count, actor, target, NULL, error);
:         return error;
: }
: 
: (note error is int, not ssize_t), but I don't see anything obvious
: for other filesystems.
: 
	Yes, this fixed the problem - now I can sendfile() the whole
DVD image without problem. Thanks, and send both these fixes to Linus.

-Yenya

-- 
| Jan "Yenya" Kasprzak  <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839      Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/   Czech Linux Homepage: http://www.linux.cz/ |
 Any compiler or language that likes to hide things like memory allocations
 behind your back just isn't a good choice for a kernel.   --Linus Torvalds