sys_read: add a compat_sys_read for 64bit system
Subsystems (5)
RFC PATCH 1/1 sys_read: add a compat_sys_read for 64bit system
Test 32 progress and 64 progress on the 64bit system with this progress:
int main(int argc, char **argv)
{
int fd = 0;
int i, ret = 0;
char buf[512];
unsigned long count = -1;
fd = open("/tmp", O_RDONLY);
if (fd < -1) {
printf("Pls check the directory is exist?\n");
return -1;
}
errno = 0;
ret = read(fd, NULL, count);
printf("Ret is %d errno %d\n", ret, errno);
close(fd);
return 0;
}we get the different errno. The 64 progress we get errno is -14 while the 32 progress is -21.
The reason is that, the user progress would use a 32bit count, while the sys_read size_t in kernel is 64bit. When the uesrspace count is -1(0xffffffff), it goes to the sys_read, it would be change to a positive number.
So I think we should add a compat_sys_read for the read syscall. I test it on x86 or arm64 platform. The patch works well.
As well this patch may do work for the 'tile' 64 system. I think it may enter the same result on mips/parisc/powerpc/sparc. The s390 do the compat_sys_s390_read for the compat sys_read.
--- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/read_write.c | 8 ++++++++ include/linux/compat.h | 2 ++ include/uapi/asm-generic/unistd.h | 2 +- 4 files changed, 12 insertions(+), 2 deletions(-)
arch/x86/entry/syscalls/syscall_32.tbl +1 -1
@@ -9,7 +9,7 @@ 0 i386 restart_syscall sys_restart_syscall 1 i386 exit sys_exit 2 i386 fork sys_fork sys_fork -3 i386 read sys_read +3 i386 read sys_read compat_sys_read 4 i386 write sys_write 5 i386 open sys_open compat_sys_open 6 i386 close sys_close
fs/read_write.c +8 -0
@@ -613,6 +613,14 @@ SYSCALL_DEFINE3(write, unsigned int, fd, const char __user *, buf,return ret; } +#ifdef CONFIG_COMPAT +COMPAT_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, + compat_size_t, count) +{ +return sys_read (fd, buf, (compat_ssize_t )count); +} +#endif + SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf, size_t, count, loff_t, pos) {
include/linux/compat.h +2 -0
@@ -332,6 +332,8 @@ asmlinkage long compat_sys_keyctl(u32 option, u32 arg2, u32 arg3, u32 arg4, u32 arg5);asmlinkage long compat_sys_ustat (unsigned dev,struct compat_ustat __user *u32); +asmlinkage ssize_t compat_sys_read(unsigned int fd, +char __user * buf,compat_size_t count); asmlinkage ssize_t compat_sys_readv(compat_ulong_t fd,const struct compat_iovec __user *vec,compat_ulong_t vlen); asmlinkage ssize_t compat_sys_writev(compat_ulong_t fd,
include/uapi/asm-generic/unistd.h +1 -1
@@ -201,7 +201,7 @@ __SC_COMP(__NR_getdents64, sys_getdents64, compat_sys_getdents64)#define __NR3264_lseek 62 __SC_3264(__NR3264_lseek, sys_llseek, sys_lseek)#define __NR_read 63 -__SYSCALL(__NR_read, sys_read) +__SC_COMP(__NR_read, sys_read, compat_sys_read)#define __NR_write 64 __SYSCALL(__NR_write, sys_write)#define __NR_readv 65
Signature · 1 line
2.7.0
Hi,
Quoted 26 lines · Weidong Wang
Test 32 progress and 64 progress on the 64bit system with this progress:
int
main(int argc, char **argv){ int fd = 0; int i, ret = 0; char buf[512]; unsigned long count = -1; fd = open("/tmp", O_RDONLY); if (fd < -1) { printf("Pls check the directory is exist?\n"); return -1; } errno = 0; ret = read(fd, NULL, count); printf("Ret is %d errno %d\n", ret, errno); close(fd); return 0; }we get the different errno. The 64 progress we get errno is -14 while the 32 progress is -21.
Quoted 7 lines
The reason is that, the user progress would use a 32bit count, while the
sys_readsize_tin kernel is 64bit. When the uesrspace count is -1(0xffffffff), it goes to thesys_read, it would be change to a positive number.So I think we should add a
compat_sys_readfor the read syscall. I test it on x86 or arm64 platform. The patch works well.
As weidong said, we tested on x86, x86_64, aarch64 ilp32, aarch64 lp64. We do not familiar with other architecture, cc linux-api, hope could get more input.
Regards
Bamvor
Quoted 72 lines
As well this patch may do work for the 'tile' 64 system. I think it may enter the same result on mips/parisc/powerpc/sparc. The s390 do the
compat_sys_s390_readfor the compatsys_read.Signed-off-by Weidong Wang <[email protected]>--- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/read_write.c | 8 ++++++++ include/linux/compat.h | 2 ++ include/uapi/asm-generic/unistd.h | 2 +- 4 files changed, 12 insertions(+), 2 deletions(-)arch/x86/entry/syscalls/syscall_32.tbl +1
-1@@ -9,7 +9,7 @@ 0 i386 restart_syscall sys_restart_syscall 1 i386 exit sys_exit 2 i386 fork sys_fork sys_fork -3 i386 read sys_read +3 i386 read sys_read compat_sys_read 4 i386 write sys_write 5 i386 open sys_open compat_sys_open 6 i386 close sys_closefs/read_write.c +8
-0@@ -613,6 +613,14 @@ SYSCALL_DEFINE3(write, unsigned int, fd, const char __user *, buf,return ret; } +#ifdef CONFIG_COMPAT +COMPAT_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, + compat_size_t, count) +{ +return sys_read (fd, buf, (compat_ssize_t )count); +} +#endif + SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf, size_t, count, loff_t, pos) {include/linux/compat.h +2
-0@@ -332,6 +332,8 @@ asmlinkage long compat_sys_keyctl(u32 option, u32 arg2, u32 arg3, u32 arg4, u32 arg5);asmlinkage long compat_sys_ustat (unsigned dev,struct compat_ustat __user *u32); +asmlinkage ssize_t compat_sys_read(unsigned int fd, +char __user * buf,compat_size_t count); asmlinkage ssize_t compat_sys_readv(compat_ulong_t fd,const struct compat_iovec __user *vec,compat_ulong_t vlen); asmlinkage ssize_t compat_sys_writev(compat_ulong_t fd,include/uapi/asm-generic/unistd.h +1
-1@@ -201,7 +201,7 @@ __SC_COMP(__NR_getdents64, sys_getdents64, compat_sys_getdents64)#define __NR3264_lseek 62 __SC_3264(__NR3264_lseek, sys_llseek, sys_lseek)#define __NR_read 63 -__SYSCALL(__NR_read, sys_read) +__SC_COMP(__NR_read, sys_read, compat_sys_read)#define __NR_write 64 __SYSCALL(__NR_write, sys_write)#define __NR_readv 65
H. Peter Anvin
reviewerQuoted 114 lines · Zhangjian (Bamvor)
Hi,
Quoted 23 lines · Weidong Wang
Test 32 progress and 64 progress on the 64bit system with this progress:
int
main(int argc, char **argv){ int fd = 0; int i, ret = 0; char buf[512]; unsigned long count = -1; fd = open("/tmp", O_RDONLY); if (fd < -1) { printf("Pls check the directory is exist?\n"); return -1; } errno = 0; ret = read(fd, NULL, count); printf("Ret is %d errno %d\n", ret, errno); close(fd); return 0; }Quoted 6 lines
we get the different errno. The 64 progress we get errno is -14 while the 32 progress is -21.
The reason is that, the user progress would use a 32bit count, while the
sys_readsize_tin kernel is 64bit. When the uesrspace count is -1(0xffffffff), it goes to thesys_read, it would be change to apositive
Quoted 3 lines
number.
So I think we should add a
compat_sys_readfor the read syscall. Itest it
Quoted 1 line
on x86 or arm64 platform. The patch works well.
As weidong said, we tested on x86,
x86_64, aarch64 ilp32, aarch64 lp64. We do not familiar with other architecture, cc linux-api, hope could get more
input.Regards
Bamvor
Quoted 6 lines
As well this patch may do work for the 'tile' 64 system. I think it may enter the same result on mips/parisc/powerpc/sparc. The s390 do the
compat_sys_s390_readfor the compatsys_read.Signed-off-by Weidong Wang <[email protected]>Review
Quoted 8 lines
--- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/read_write.c | 8 ++++++++ include/linux/compat.h | 2 ++ include/uapi/asm-generic/unistd.h | 2 +- 4 files changed, 12 insertions(+), 2 deletions(-)(quoted diff) +0
-0b/arch/x86/entry/syscalls/syscall_32.tbl
fs/read_write.c SYSCALL_DEFINE3(write, unsigned int, fd, const
Quoted 17 lines
index 4cddd17..ebc24e3 100644
arch/x86/entry/syscalls/syscall_32.tbl +1
-1@@ -9,7 +9,7 @@ 0 i386 restart_syscall sys_restart_syscall 1 i386 exit sys_exit 2 i386 fork sys_fork sys_fork -3 i386 read sys_read +3 i386 read sys_read compat_sys_read 4 i386 write sys_write 5 i386 open sys_open compat_sys_open 6 i386 close sys_closefs/read_write.c +0
-0@@ -613,6 +613,14 @@ SYSCALL_DEFINE3(write, unsigned int, fd, constchar
__user*, buf,Quoted 19 lines
fs/read_write.c +8
-0return ret; } +#ifdef CONFIG_COMPAT +COMPAT_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, + compat_size_t, count) +{ +return sys_read (fd, buf, (compat_ssize_t )count); +} +#endif + SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf, size_t, count, loff_t, pos) {include/linux/compat.h +0
-0include/linux/compat.h asmlinkage long compat_sys_keyctl(u32 option, · line 333
Quoted 2 lines
include/linux/compat.h +0
-0@@ -332,6 +332,8 @@ asmlinkage long compat_sys_keyctl(u32 option, u32 arg2, u32 arg3, u32 arg4, u32 arg5); asmlinkage long compat_sys_ustat(unsigned dev, struct compat_ustat
__user*u32);Review
Quoted 7 lines
include/linux/compat.h +2
-0+asmlinkage ssize_t compat_sys_read(unsigned int fd, +char __user * buf,compat_size_t count); asmlinkage ssize_t compat_sys_readv(compat_ulong_t fd,const struct compat_iovec __user *vec,compat_ulong_t vlen); asmlinkage ssize_t compat_sys_writev(compat_ulong_t fd,(quoted diff) +0
-0
b/include/uapi/asm-generic/unistd.hinclude/uapi/asm-generic/unistd.h __SC_COMP(__NR_getdents64, sys_getdents64,
Quoted 4 lines
index a26415b..745818a 100644
include/uapi/asm-generic/unistd.h +0
-0@@ -201,7 +201,7 @@ __SC_COMP(__NR_getdents64, sys_getdents64,
compat_sys_getdents64)
Quoted 9 lines
include/uapi/asm-generic/unistd.h __SC_COMP(__NR_getdents64, sys_getdents64, compat_sys_getdents64) · line 207
Quoted 9 lines
include/uapi/asm-generic/unistd.h +1
-1#define __NR3264_lseek 62 __SC_3264(__NR3264_lseek, sys_llseek, sys_lseek)#define __NR_read 63 -__SYSCALL(__NR_read, sys_read) +__SC_COMP(__NR_read, sys_read, compat_sys_read)#define __NR_write 64 __SYSCALL(__NR_write, sys_write)#define __NR_readv 65
Does this cause any actual problems? Also, it seems extremely unlikely read() would be the only system call so affected.
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
Hi, Peter
Quoted 115 lines · H. Peter Anvin
Quoted 114 lines · Zhangjian (Bamvor)
Hi,
Quoted 23 lines · Weidong Wang
Test 32 progress and 64 progress on the 64bit system with this progress:
int
main(int argc, char **argv){ int fd = 0; int i, ret = 0; char buf[512]; unsigned long count = -1; fd = open("/tmp", O_RDONLY); if (fd < -1) { printf("Pls check the directory is exist?\n"); return -1; } errno = 0; ret = read(fd, NULL, count); printf("Ret is %d errno %d\n", ret, errno); close(fd); return 0; }Quoted 6 lines
we get the different errno. The 64 progress we get errno is -14 while the 32 progress is -21.
The reason is that, the user progress would use a 32bit count, while the
sys_readsize_tin kernel is 64bit. When the uesrspace count is -1(0xffffffff), it goes to thesys_read, it would be change to apositive
Quoted 3 lines
number.
So I think we should add a
compat_sys_readfor the read syscall. Itest it
Quoted 1 line
on x86 or arm64 platform. The patch works well.
As weidong said, we tested on x86,
x86_64, aarch64 ilp32, aarch64 lp64. We do not familiar with other architecture, cc linux-api, hope could get more
input.Regards
Bamvor
Quoted 6 lines
As well this patch may do work for the 'tile' 64 system. I think it may enter the same result on mips/parisc/powerpc/sparc. The s390 do the
compat_sys_s390_readfor the compatsys_read.Signed-off-by Weidong Wang <[email protected]>Review
Quoted 8 lines
--- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/read_write.c | 8 ++++++++ include/linux/compat.h | 2 ++ include/uapi/asm-generic/unistd.h | 2 +- 4 files changed, 12 insertions(+), 2 deletions(-)(quoted diff) +0
-0b/arch/x86/entry/syscalls/syscall_32.tbl
fs/read_write.c SYSCALL_DEFINE3(write, unsigned int, fd, const
Quoted 17 lines
index 4cddd17..ebc24e3 100644
arch/x86/entry/syscalls/syscall_32.tbl +1
-1@@ -9,7 +9,7 @@ 0 i386 restart_syscall sys_restart_syscall 1 i386 exit sys_exit 2 i386 fork sys_fork sys_fork -3 i386 read sys_read +3 i386 read sys_read compat_sys_read 4 i386 write sys_write 5 i386 open sys_open compat_sys_open 6 i386 close sys_closefs/read_write.c +0
-0@@ -613,6 +613,14 @@ SYSCALL_DEFINE3(write, unsigned int, fd, constchar
__user*, buf,Quoted 19 lines
fs/read_write.c +8
-0return ret; } +#ifdef CONFIG_COMPAT +COMPAT_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, + compat_size_t, count) +{ +return sys_read (fd, buf, (compat_ssize_t )count); +} +#endif + SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf, size_t, count, loff_t, pos) {include/linux/compat.h +0
-0include/linux/compat.h asmlinkage long compat_sys_keyctl(u32 option, · line 333
Quoted 2 lines
include/linux/compat.h +0
-0@@ -332,6 +332,8 @@ asmlinkage long compat_sys_keyctl(u32 option, u32 arg2, u32 arg3, u32 arg4, u32 arg5); asmlinkage long compat_sys_ustat(unsigned dev, struct compat_ustat
__user*u32);Review
Quoted 7 lines
include/linux/compat.h +2
-0+asmlinkage ssize_t compat_sys_read(unsigned int fd, +char __user * buf,compat_size_t count); asmlinkage ssize_t compat_sys_readv(compat_ulong_t fd,const struct compat_iovec __user *vec,compat_ulong_t vlen); asmlinkage ssize_t compat_sys_writev(compat_ulong_t fd,(quoted diff) +0
-0
b/include/uapi/asm-generic/unistd.hinclude/uapi/asm-generic/unistd.h __SC_COMP(__NR_getdents64, sys_getdents64,
Quoted 4 lines
index a26415b..745818a 100644
include/uapi/asm-generic/unistd.h +0
-0@@ -201,7 +201,7 @@ __SC_COMP(__NR_getdents64, sys_getdents64,
compat_sys_getdents64)
Quoted 11 lines
Quoted 9 lines
include/uapi/asm-generic/unistd.h __SC_COMP(__NR_getdents64, sys_getdents64, compat_sys_getdents64) · line 207
Quoted 9 lines
include/uapi/asm-generic/unistd.h +1
-1#define __NR3264_lseek 62 __SC_3264(__NR3264_lseek, sys_llseek, sys_lseek)#define __NR_read 63 -__SYSCALL(__NR_read, sys_read) +__SC_COMP(__NR_read, sys_read, compat_sys_read)#define __NR_write 64 __SYSCALL(__NR_write, sys_write)#define __NR_readv 65Does this cause any actual problems? Also, it seems extremely unlikely
read()would be the only system call so affected.
It do not cause any actual problems. And write may be affected too. Without this patch, the errno is different when 32 application migrate from 32bit kernel to 64bit kernel.
Regards
Bamvor
Quoted 1 line
Quoted 29 lines · Zhangjian (Bamvor)
Hi,
Quoted 26 lines · Weidong Wang
Test 32 progress and 64 progress on the 64bit system with this progress:
int
main(int argc, char **argv){ int fd = 0; int i, ret = 0; char buf[512]; unsigned long count = -1; fd = open("/tmp", O_RDONLY); if (fd < -1) { printf("Pls check the directory is exist?\n"); return -1; } errno = 0; ret = read(fd, NULL, count); printf("Ret is %d errno %d\n", ret, errno); close(fd); return 0; }we get the different errno. The 64 progress we get errno is -14 while the 32 progress is -21.
On 64-bit, you get -14 == -EFAULT. Seems reasonable: you passed a bad pointer.
On 32-bit, you get -21 == -EISDIR. Also seems reasonable: fd is a directory.
Quoted 5 lines
Quoted 5 lines
The reason is that, the user progress would use a 32bit count, while the
sys_readsize_tin kernel is 64bit. When the uesrspace count is -1(0xffffffff), it goes to thesys_read, it would be change to a positive number.
That parameter is size_t, which is unsigned. It's a positive number in both cases.
I don't think there's a bug here.
Quoted 42 lines · Andy Lutomirski
Quoted 29 lines · Zhangjian (Bamvor)
Hi,
Quoted 26 lines · Weidong Wang
Test 32 progress and 64 progress on the 64bit system with this progress:
int
main(int argc, char **argv){ int fd = 0; int i, ret = 0; char buf[512]; unsigned long count = -1; fd = open("/tmp", O_RDONLY); if (fd < -1) { printf("Pls check the directory is exist?\n"); return -1; } errno = 0; ret = read(fd, NULL, count); printf("Ret is %d errno %d\n", ret, errno); close(fd); return 0; }we get the different errno. The 64 progress we get errno is -14 while the 32 progress is -21.
On 64-bit, you get -14 ==
-EFAULT. Seems reasonable: you passed a bad pointer.On 32-bit, you get -21 ==
-EISDIR. Also seems reasonable: fd is a directory.Quoted 5 lines
Quoted 5 lines
The reason is that, the user progress would use a 32bit count, while the
sys_readsize_tin kernel is 64bit. When the uesrspace count is -1(0xffffffff), it goes to thesys_read, it would be change to a positive number.
Quoted 5 lines
That parameter is
size_t, which is unsigned. It's a positive number in both cases.I don't think there's a bug here.
Yep.
In the progress open the '/tmp' is a directory. If we do open a file '/tmp/files' (exist file), the result would be different on x86-64bit machine.
On 64-bit, we get -14 == -EFAULT.
On 32-bit, we get the length of the file, the errno is 0.
Regards,
Weidong
Quoted 2 lines
.