| Message ID | 20200923060547.16903-6-hch@lst.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/9] compat.h: fix a spelling error in <linux/compat.h> | expand |
On Wed, Sep 23, 2020 at 03:25:49PM +0100, Al Viro wrote: > On Wed, Sep 23, 2020 at 08:05:43AM +0200, Christoph Hellwig wrote: > > COMPAT_SYSCALL_DEFINE3(readv, compat_ulong_t, fd, > > - const struct compat_iovec __user *,vec, > > + const struct iovec __user *, vec, > > Um... Will it even compile? > > > #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64 > > COMPAT_SYSCALL_DEFINE4(preadv64, unsigned long, fd, > > - const struct compat_iovec __user *,vec, > > + const struct iovec __user *, vec, > > Ditto. Look into include/linux/compat.h and you'll see > > asmlinkage long compat_sys_preadv64(unsigned long fd, > const struct compat_iovec __user *vec, > unsigned long vlen, loff_t pos); > > How does that manage to avoid the compiler screaming bloody > murder? That's a very good question. But it does not just compile but actually works. Probably because all the syscall wrappers mean that we don't actually generate the normal names. I just tried this: --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset, asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count); asmlinkage long sys_write(unsigned int fd, const char __user *buf, size_t count); -asmlinkage long sys_readv(unsigned long fd, +asmlinkage long sys_readv(void *fd, for fun, and the compiler doesn't care either..
On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote: > > That's a very good question. But it does not just compile but actually > > works. Probably because all the syscall wrappers mean that we don't > > actually generate the normal names. I just tried this: > > > > --- a/include/linux/syscalls.h > > +++ b/include/linux/syscalls.h > > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset, > > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count); > > asmlinkage long sys_write(unsigned int fd, const char __user *buf, > > size_t count); > > -asmlinkage long sys_readv(unsigned long fd, > > +asmlinkage long sys_readv(void *fd, > > > > for fun, and the compiler doesn't care either.. > > Try to build it for sparc or ppc... FWIW, declarations in syscalls.h used to serve 4 purposes: 1) syscall table initializers needed symbols declared 2) direct calls needed the same 3) catching mismatches between the declarations and definitions 4) centralized list of all syscalls (2) has been (thankfully) reduced for some time; in any case, ksys_... is used for the remaining ones. (1) and (3) are served by syscalls.h in architectures other than x86, arm64 and s390. On those 3 (1) is done otherwise (near the syscall table initializer) and (3) is not done at all. I wonder if we should do something like SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec, unsigned long, vlen); in syscalls.h instead, and not under that ifdef. Let it expand to declaration of sys_...() in generic case and, on x86, into __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching what SYSCALL_DEFINE ends up using. Similar macro would cover compat_sys_...() declarations. That would restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't be terribly high - cpp would have more to chew through in syscalls.h, but it shouldn't be all that costly. Famous last words, of course... Does anybody see fundamental problems with that?
On Wed, Sep 23, 2020 at 12:39 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote: > > > > That's a very good question. But it does not just compile but actually > > > works. Probably because all the syscall wrappers mean that we don't > > > actually generate the normal names. I just tried this: > > > > > > --- a/include/linux/syscalls.h > > > +++ b/include/linux/syscalls.h > > > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset, > > > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count); > > > asmlinkage long sys_write(unsigned int fd, const char __user *buf, > > > size_t count); > > > -asmlinkage long sys_readv(unsigned long fd, > > > +asmlinkage long sys_readv(void *fd, > > > > > > for fun, and the compiler doesn't care either.. > > > > Try to build it for sparc or ppc... > > FWIW, declarations in syscalls.h used to serve 4 purposes: > 1) syscall table initializers needed symbols declared > 2) direct calls needed the same > 3) catching mismatches between the declarations and definitions > 4) centralized list of all syscalls > > (2) has been (thankfully) reduced for some time; in any case, ksys_... is > used for the remaining ones. > > (1) and (3) are served by syscalls.h in architectures other than x86, arm64 > and s390. On those 3 (1) is done otherwise (near the syscall table initializer) > and (3) is not done at all. > > I wonder if we should do something like > > SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec, > unsigned long, vlen); > in syscalls.h instead, and not under that ifdef. > > Let it expand to declaration of sys_...() in generic case and, on x86, into > __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching > what SYSCALL_DEFINE ends up using. > > Similar macro would cover compat_sys_...() declarations. That would > restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't > be terribly high - cpp would have more to chew through in syscalls.h, > but it shouldn't be all that costly. Famous last words, of course... > > Does anybody see fundamental problems with that? I think this would be a good idea. I have been working on a patchset to clean up the conditional syscall handling (sys_ni.c), and conflicts with the prototypes in syscalls.h have been getting in the way. Having the prototypes use SYSCALL_DECLAREx(...) would solve that issue. -- Brian Gerst
On Wed, Sep 23, 2020 at 06:05:27PM +0100, Al Viro wrote: > On Wed, Sep 23, 2020 at 05:38:31PM +0100, Al Viro wrote: > > On Wed, Sep 23, 2020 at 03:59:01PM +0100, Al Viro wrote: > > > > > > That's a very good question. But it does not just compile but actually > > > > works. Probably because all the syscall wrappers mean that we don't > > > > actually generate the normal names. I just tried this: > > > > > > > > --- a/include/linux/syscalls.h > > > > +++ b/include/linux/syscalls.h > > > > @@ -468,7 +468,7 @@ asmlinkage long sys_lseek(unsigned int fd, off_t offset, > > > > asmlinkage long sys_read(unsigned int fd, char __user *buf, size_t count); > > > > asmlinkage long sys_write(unsigned int fd, const char __user *buf, > > > > size_t count); > > > > -asmlinkage long sys_readv(unsigned long fd, > > > > +asmlinkage long sys_readv(void *fd, > > > > > > > > for fun, and the compiler doesn't care either.. > > > > > > Try to build it for sparc or ppc... > > > > FWIW, declarations in syscalls.h used to serve 4 purposes: > > 1) syscall table initializers needed symbols declared > > 2) direct calls needed the same > > 3) catching mismatches between the declarations and definitions > > 4) centralized list of all syscalls > > > > (2) has been (thankfully) reduced for some time; in any case, ksys_... is > > used for the remaining ones. > > > > (1) and (3) are served by syscalls.h in architectures other than x86, arm64 > > and s390. On those 3 (1) is done otherwise (near the syscall table initializer) > > and (3) is not done at all. > > > > I wonder if we should do something like > > > > SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec, > > unsigned long, vlen); > > in syscalls.h instead, and not under that ifdef. > > > > Let it expand to declaration of sys_...() in generic case and, on x86, into > > __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching > > what SYSCALL_DEFINE ends up using. > > > > Similar macro would cover compat_sys_...() declarations. That would > > restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't > > be terribly high - cpp would have more to chew through in syscalls.h, > > but it shouldn't be all that costly. Famous last words, of course... > > > > Does anybody see fundamental problems with that? > > Just to make it clear - I do not propose to fold that into this series; > there we just need to keep those declarations in sync with fs/read_write.c Agreed. The above idea generally sounds sane to me.
On Wed, Sep 23, 2020 at 6:38 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > I wonder if we should do something like > > SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec, > unsigned long, vlen); > in syscalls.h instead, and not under that ifdef. > > Let it expand to declaration of sys_...() in generic case and, on x86, into > __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching > what SYSCALL_DEFINE ends up using. > > Similar macro would cover compat_sys_...() declarations. That would > restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't > be terribly high - cpp would have more to chew through in syscalls.h, > but it shouldn't be all that costly. Famous last words, of course... > > Does anybody see fundamental problems with that? I've had some ideas along those lines in the past and I think it should work. As a variation of this, the SYSCALL_DEFINEx() macros could go away entirely, leaving only the macro instantiations from the header to require that syntax. It would require first changing the remaining architectures to build the syscall table from C code instead of assembler though. Regardless of that, another advantage of having the SYSCALL_DECLAREx() would be the ability to include that header file from elsewhere with a different macro definition to create a machine-readable version of the interface when combined with the syscall.tbl files. This could be used to create a user space stub for calling into the low-level syscall regardless of the libc interfaces, or for synchronizing the interfaces with strace, qemu-user, or anything that needs to deal with the low-level interface. Arnd
On Wed, Sep 23, 2020 at 08:45:51PM +0200, Arnd Bergmann wrote: > On Wed, Sep 23, 2020 at 6:38 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > I wonder if we should do something like > > > > SYSCALL_DECLARE3(readv, unsigned long, fd, const struct iovec __user *, vec, > > unsigned long, vlen); > > in syscalls.h instead, and not under that ifdef. > > > > Let it expand to declaration of sys_...() in generic case and, on x86, into > > __do_sys_...() and __ia32_sys_...()/__x64_sys_...(), with types matching > > what SYSCALL_DEFINE ends up using. > > > > Similar macro would cover compat_sys_...() declarations. That would > > restore mismatch checking for x86 and friends. AFAICS, the cost wouldn't > > be terribly high - cpp would have more to chew through in syscalls.h, > > but it shouldn't be all that costly. Famous last words, of course... > > > > Does anybody see fundamental problems with that? > > I've had some ideas along those lines in the past and I think it should work. > > As a variation of this, the SYSCALL_DEFINEx() macros could go away > entirely, leaving only the macro instantiations from the header to > require that syntax. It would require first changing the remaining > architectures to build the syscall table from C code instead of > assembler though. > > Regardless of that, another advantage of having the SYSCALL_DECLAREx() > would be the ability to include that header file from elsewhere with a different > macro definition to create a machine-readable version of the interface when > combined with the syscall.tbl files. This could be used to create a user > space stub for calling into the low-level syscall regardless of the > libc interfaces, > or for synchronizing the interfaces with strace, qemu-user, or anything that > needs to deal with the low-level interface. FWIW, after playing with that for a while... Do we really want the compat_sys_...() declarations to live in linux/compat.h? Most of the users of that file don't want those; why not move them to linux/syscalls.h? Reason: there's a lot more users of linux/compat.h than those of linux/syscalls.h - it's pulled by everything in the networking stack, for starters...
From: Arnd Bergmann > Sent: 23 September 2020 19:46 ... > Regardless of that, another advantage of having the SYSCALL_DECLAREx() > would be the ability to include that header file from elsewhere with a different > macro definition to create a machine-readable version of the interface when > combined with the syscall.tbl files. This could be used to create a user > space stub for calling into the low-level syscall regardless of the > libc interfaces, > or for synchronizing the interfaces with strace, qemu-user, or anything that > needs to deal with the low-level interface. A similar 'trick' (that probably won't work here) is to pass the name of a #define function as a parameter to another define. Useful for defining constants and error strings together. eg: #define TRAFFIC_LIGHTS(x) \ x(RED, 0, "Red") \ x(YELLOW, 1, "Yellow) \ x(GREEN, 2, "GREEN) You can then do thing like: #define x(token, value, string) token = value, enum {TRAFFIC_LIGHTS(x) NUM_LIGHTS}; #undef x #define x(token, value, string) [value] = string, const char *colours[] = {TRAFFIC_LIGHTS(x)}; #undef x to initialise constants and a name table that are always in sync. It is also a good way to generate source lines that are over 1MB. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/fs/read_write.c b/fs/read_write.c index 0a68037580b455..eab427b7cc0a3f 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1068,226 +1068,107 @@ SYSCALL_DEFINE6(pwritev2, unsigned long, fd, const struct iovec __user *, vec, return do_pwritev(fd, vec, vlen, pos, flags); } +/* + * Various compat syscalls. Note that they all pretend to take a native + * iovec - import_iovec will properly treat those as compat_iovecs based on + * in_compat_syscall(). + */ #ifdef CONFIG_COMPAT -static size_t compat_readv(struct file *file, - const struct compat_iovec __user *vec, - unsigned long vlen, loff_t *pos, rwf_t flags) -{ - struct iovec iovstack[UIO_FASTIOV]; - struct iovec *iov = iovstack; - struct iov_iter iter; - ssize_t ret; - - ret = import_iovec(READ, (const struct iovec __user *)vec, vlen, - UIO_FASTIOV, &iov, &iter); - if (ret >= 0) { - ret = do_iter_read(file, &iter, pos, flags); - kfree(iov); - } - if (ret > 0) - add_rchar(current, ret); - inc_syscr(current); - return ret; -} - -static size_t do_compat_readv(compat_ulong_t fd, - const struct compat_iovec __user *vec, - compat_ulong_t vlen, rwf_t flags) -{ - struct fd f = fdget_pos(fd); - ssize_t ret; - loff_t pos; - - if (!f.file) - return -EBADF; - pos = f.file->f_pos; - ret = compat_readv(f.file, vec, vlen, &pos, flags); - if (ret >= 0) - f.file->f_pos = pos; - fdput_pos(f); - return ret; - -} - COMPAT_SYSCALL_DEFINE3(readv, compat_ulong_t, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, compat_ulong_t, vlen) { - return do_compat_readv(fd, vec, vlen, 0); -} - -static long do_compat_preadv64(unsigned long fd, - const struct compat_iovec __user *vec, - unsigned long vlen, loff_t pos, rwf_t flags) -{ - struct fd f; - ssize_t ret; - - if (pos < 0) - return -EINVAL; - f = fdget(fd); - if (!f.file) - return -EBADF; - ret = -ESPIPE; - if (f.file->f_mode & FMODE_PREAD) - ret = compat_readv(f.file, vec, vlen, &pos, flags); - fdput(f); - return ret; + return do_readv(fd, vec, vlen, 0); } #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64 COMPAT_SYSCALL_DEFINE4(preadv64, unsigned long, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, unsigned long, vlen, loff_t, pos) { - return do_compat_preadv64(fd, vec, vlen, pos, 0); + return do_preadv(fd, vec, vlen, pos, 0); } #endif COMPAT_SYSCALL_DEFINE5(preadv, compat_ulong_t, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, compat_ulong_t, vlen, u32, pos_low, u32, pos_high) { loff_t pos = ((loff_t)pos_high << 32) | pos_low; - return do_compat_preadv64(fd, vec, vlen, pos, 0); + return do_preadv(fd, vec, vlen, pos, 0); } #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64V2 COMPAT_SYSCALL_DEFINE5(preadv64v2, unsigned long, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, unsigned long, vlen, loff_t, pos, rwf_t, flags) { if (pos == -1) - return do_compat_readv(fd, vec, vlen, flags); - - return do_compat_preadv64(fd, vec, vlen, pos, flags); + return do_readv(fd, vec, vlen, flags); + return do_preadv(fd, vec, vlen, pos, flags); } #endif COMPAT_SYSCALL_DEFINE6(preadv2, compat_ulong_t, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, compat_ulong_t, vlen, u32, pos_low, u32, pos_high, rwf_t, flags) { loff_t pos = ((loff_t)pos_high << 32) | pos_low; if (pos == -1) - return do_compat_readv(fd, vec, vlen, flags); - - return do_compat_preadv64(fd, vec, vlen, pos, flags); -} - -static size_t compat_writev(struct file *file, - const struct compat_iovec __user *vec, - unsigned long vlen, loff_t *pos, rwf_t flags) -{ - struct iovec iovstack[UIO_FASTIOV]; - struct iovec *iov = iovstack; - struct iov_iter iter; - ssize_t ret; - - ret = import_iovec(WRITE, (const struct iovec __user *)vec, vlen, - UIO_FASTIOV, &iov, &iter); - if (ret >= 0) { - file_start_write(file); - ret = do_iter_write(file, &iter, pos, flags); - file_end_write(file); - kfree(iov); - } - if (ret > 0) - add_wchar(current, ret); - inc_syscw(current); - return ret; -} - -static size_t do_compat_writev(compat_ulong_t fd, - const struct compat_iovec __user* vec, - compat_ulong_t vlen, rwf_t flags) -{ - struct fd f = fdget_pos(fd); - ssize_t ret; - loff_t pos; - - if (!f.file) - return -EBADF; - pos = f.file->f_pos; - ret = compat_writev(f.file, vec, vlen, &pos, flags); - if (ret >= 0) - f.file->f_pos = pos; - fdput_pos(f); - return ret; + return do_readv(fd, vec, vlen, flags); + return do_preadv(fd, vec, vlen, pos, flags); } COMPAT_SYSCALL_DEFINE3(writev, compat_ulong_t, fd, - const struct compat_iovec __user *, vec, + const struct iovec __user *, vec, compat_ulong_t, vlen) { - return do_compat_writev(fd, vec, vlen, 0); -} - -static long do_compat_pwritev64(unsigned long fd, - const struct compat_iovec __user *vec, - unsigned long vlen, loff_t pos, rwf_t flags) -{ - struct fd f; - ssize_t ret; - - if (pos < 0) - return -EINVAL; - f = fdget(fd); - if (!f.file) - return -EBADF; - ret = -ESPIPE; - if (f.file->f_mode & FMODE_PWRITE) - ret = compat_writev(f.file, vec, vlen, &pos, flags); - fdput(f); - return ret; + return do_writev(fd, vec, vlen, 0); } #ifdef __ARCH_WANT_COMPAT_SYS_PWRITEV64 COMPAT_SYSCALL_DEFINE4(pwritev64, unsigned long, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, unsigned long, vlen, loff_t, pos) { - return do_compat_pwritev64(fd, vec, vlen, pos, 0); + return do_pwritev(fd, vec, vlen, pos, 0); } #endif COMPAT_SYSCALL_DEFINE5(pwritev, compat_ulong_t, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *,vec, compat_ulong_t, vlen, u32, pos_low, u32, pos_high) { loff_t pos = ((loff_t)pos_high << 32) | pos_low; - return do_compat_pwritev64(fd, vec, vlen, pos, 0); + return do_pwritev(fd, vec, vlen, pos, 0); } #ifdef __ARCH_WANT_COMPAT_SYS_PWRITEV64V2 COMPAT_SYSCALL_DEFINE5(pwritev64v2, unsigned long, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *, vec, unsigned long, vlen, loff_t, pos, rwf_t, flags) { if (pos == -1) - return do_compat_writev(fd, vec, vlen, flags); - - return do_compat_pwritev64(fd, vec, vlen, pos, flags); + return do_writev(fd, vec, vlen, flags); + return do_pwritev(fd, vec, vlen, pos, flags); } #endif COMPAT_SYSCALL_DEFINE6(pwritev2, compat_ulong_t, fd, - const struct compat_iovec __user *,vec, + const struct iovec __user *,vec, compat_ulong_t, vlen, u32, pos_low, u32, pos_high, rwf_t, flags) { loff_t pos = ((loff_t)pos_high << 32) | pos_low; if (pos == -1) - return do_compat_writev(fd, vec, vlen, flags); - - return do_compat_pwritev64(fd, vec, vlen, pos, flags); + return do_writev(fd, vec, vlen, flags); + return do_pwritev(fd, vec, vlen, pos, flags); } - -#endif +#endif /* CONFIG_COMPAT */ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, size_t count, loff_t max)
Now that import_iovec handles compat iovecs as well, all the duplicated code in the compat readv/writev helpers is not needed. Remove them and switch the compat syscall handlers to use the native helpers. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/read_write.c | 179 ++++++++---------------------------------------- 1 file changed, 30 insertions(+), 149 deletions(-)