From: Heiko Carstens <heiko.carstens@de.ibm.com> To: Yury Norov <ynorov@caviumnetworks.com> Cc: linux-s390@vger.kernel.org, arnd@arndb.de, catalin.marinas@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, schwidefsky@de.ibm.com, pinskia@gmail.com, Prasun.Kapoor@caviumnetworks.com, schwab@suse.de, Nathan_Lynch@mentor.com, agraf@suse.de, klimov.linux@gmail.com, broonie@kernel.org, jan.dakinevich@gmail.com, joseph@codesourcery.com, christoph.muellner@theobroma-systems.com, "Zhangjian (Bamvor)" <bamvor.zhangjian@huawei.com>, Bamvor Jian Zhang <bamvor.zhangjian@linaro.org> Subject: Re: [PATCH v6 20/21] all: s390: make compat wrappers the generic solution Date: Wed, 20 Jan 2016 09:16:44 +0100 [thread overview] Message-ID: <20160120081643.GB3395@osiris> (raw) In-Reply-To: <20160119175223.GA6603@yury-N73SV> On Tue, Jan 19, 2016 at 08:52:23PM +0300, Yury Norov wrote: > > > +asmlinkage long compat_sys_creat(const char __user *pathname, > > > umode_t mode); > > > +asmlinkage long compat_sys_link(const char __user *oldname, > > > + const char __user *newname); > > > +asmlinkage long compat_sys_chdir(const char __user *filename); > > > +asmlinkage long compat_sys_mknod(const char __user *filename, > > > umode_t mode, > > > + unsigned dev); > > > > Are these really needed? > > 91 of ~160 wrapped syscalls produce compile time error without it on > arm64: > arch/arm64/kernel/sys_ilp32.c:59:35: error: ‘compat_sys_io_destroy’ undeclared here (not in a function) > #define __SC_WRAP(nr, sym) [nr] = compat_##sym, > ^ > include/uapi/asm-generic/unistd.h:39:1: note: in expansion of macro ‘__SC_WRAP’ > __SC_WRAP(__NR_io_destroy, sys_io_destroy) > ^ > > I think, it's better to leave it as is... I see. The syscall tables on arm seem to be generated in C code. So you need the declarations. > diff --git a/fs/readdir.c b/fs/readdir.c > index ced6791..d34cc49 100644 > --- a/fs/readdir.c > +++ b/fs/readdir.c > @@ -17,6 +17,7 @@ > #include <linux/dirent.h> > #include <linux/security.h> > #include <linux/syscalls.h> > +#include <linux/compat.h> > #include <linux/unistd.h> > > #include <asm/uaccess.h> > @@ -274,8 +275,13 @@ efault: > return -EFAULT; > } > > +#ifndef __ARCH_WANT_COMPAT_SYS_GETDENTS64 > +SYSCALL_DEFINE_WRAP3(getdents64, unsigned int, fd, > + struct linux_dirent64 __user *, dirent, unsigned int, count) > +#else > SYSCALL_DEFINE3(getdents64, unsigned int, fd, > struct linux_dirent64 __user *, dirent, unsigned int, count) > +#endif > { > struct fd f; > struct linux_dirent64 __user * lastdirent; This is a non-obvious change at first glance. I think it would make sense to split this patch into at least three or four separate patches: - one which introduces the infrastructure - one which converts the non-obvious syscalls like this one - one which converts the rest - one which enables the architectures > diff --git a/include/linux/compat.h b/include/linux/compat.h > index a76c917..293864c 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -55,6 +55,52 @@ > } \ > static inline long C_SYSC##name(__MAP(x,__SC_DECL,__VA_ARGS__)) > > +#define SYSCALL_DEFINE_WRAP1(name, ...) SYSCALL_DEFINE_WRAPx(1, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP2(name, ...) SYSCALL_DEFINE_WRAPx(2, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP3(name, ...) SYSCALL_DEFINE_WRAPx(3, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP4(name, ...) SYSCALL_DEFINE_WRAPx(4, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP5(name, ...) SYSCALL_DEFINE_WRAPx(5, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP6(name, ...) SYSCALL_DEFINE_WRAPx(6, _##name, __VA_ARGS__) > + > +#ifndef __SC_COMPAT_TYPE > +#define __SC_COMPAT_TYPE(t, a) \ > + __typeof(__builtin_choose_expr(sizeof(t) > 4, 0L, (t)0)) a > +#endif > + > +#ifndef __SC_COMPAT_CAST > +#define __SC_COMPAT_CAST(t, a) ((t) ((t)(-1) < 0 ? (s64)(s32)(a) : (u64)(u32)(a))) > +#endif You might consider adding a BUILD_BUG_ON() here, like within the s390 variant. Personally I don't like the SYSCALL_DEFINE_WRAPx names too much. But that can be changed easily if somebody comes up with a better name for it. > +#define SYSCALL_DEFINE_WRAPx(x, name, ...) \ > +asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \ > +asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \ > + __attribute__((alias(__stringify(compat_SyS##name)))); \ > +asmlinkage long notrace compat_SyS##name(__MAP(x,__SC_COMPAT_TYPE,__VA_ARGS__)); \ > +asmlinkage long notrace compat_SyS##name(__MAP(x,__SC_COMPAT_TYPE,__VA_ARGS__)) \ > +{ \ > + return sys##name(__MAP(x,__SC_COMPAT_CAST,__VA_ARGS__)); \ > +} \ > +SYSCALL_DEFINEx(x, name, __VA_ARGS__) Given that the system call functions might be inlined, I think it would make sense to remove the "notrace" attribute. Otherwise we would end up with lots of untraceable (and unpatchable) functions. I didn't care back then for the small compat wrappers... > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c > index 0623787..ab14c3f 100644 > --- a/kernel/sys_ni.c > +++ b/kernel/sys_ni.c > @@ -17,27 +17,41 @@ asmlinkage long sys_ni_syscall(void) > } > > cond_syscall(sys_quotactl); > +cond_syscall(compat_sys_quotactl); It might make sense to add a define which adds both variants. Btw. for future versions please add linux-arch to cc.
WARNING: multiple messages have this Message-ID (diff)
From: heiko.carstens@de.ibm.com (Heiko Carstens) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v6 20/21] all: s390: make compat wrappers the generic solution Date: Wed, 20 Jan 2016 09:16:44 +0100 [thread overview] Message-ID: <20160120081643.GB3395@osiris> (raw) In-Reply-To: <20160119175223.GA6603@yury-N73SV> On Tue, Jan 19, 2016 at 08:52:23PM +0300, Yury Norov wrote: > > > +asmlinkage long compat_sys_creat(const char __user *pathname, > > > umode_t mode); > > > +asmlinkage long compat_sys_link(const char __user *oldname, > > > + const char __user *newname); > > > +asmlinkage long compat_sys_chdir(const char __user *filename); > > > +asmlinkage long compat_sys_mknod(const char __user *filename, > > > umode_t mode, > > > + unsigned dev); > > > > Are these really needed? > > 91 of ~160 wrapped syscalls produce compile time error without it on > arm64: > arch/arm64/kernel/sys_ilp32.c:59:35: error: ?compat_sys_io_destroy? undeclared here (not in a function) > #define __SC_WRAP(nr, sym) [nr] = compat_##sym, > ^ > include/uapi/asm-generic/unistd.h:39:1: note: in expansion of macro ?__SC_WRAP? > __SC_WRAP(__NR_io_destroy, sys_io_destroy) > ^ > > I think, it's better to leave it as is... I see. The syscall tables on arm seem to be generated in C code. So you need the declarations. > diff --git a/fs/readdir.c b/fs/readdir.c > index ced6791..d34cc49 100644 > --- a/fs/readdir.c > +++ b/fs/readdir.c > @@ -17,6 +17,7 @@ > #include <linux/dirent.h> > #include <linux/security.h> > #include <linux/syscalls.h> > +#include <linux/compat.h> > #include <linux/unistd.h> > > #include <asm/uaccess.h> > @@ -274,8 +275,13 @@ efault: > return -EFAULT; > } > > +#ifndef __ARCH_WANT_COMPAT_SYS_GETDENTS64 > +SYSCALL_DEFINE_WRAP3(getdents64, unsigned int, fd, > + struct linux_dirent64 __user *, dirent, unsigned int, count) > +#else > SYSCALL_DEFINE3(getdents64, unsigned int, fd, > struct linux_dirent64 __user *, dirent, unsigned int, count) > +#endif > { > struct fd f; > struct linux_dirent64 __user * lastdirent; This is a non-obvious change at first glance. I think it would make sense to split this patch into at least three or four separate patches: - one which introduces the infrastructure - one which converts the non-obvious syscalls like this one - one which converts the rest - one which enables the architectures > diff --git a/include/linux/compat.h b/include/linux/compat.h > index a76c917..293864c 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -55,6 +55,52 @@ > } \ > static inline long C_SYSC##name(__MAP(x,__SC_DECL,__VA_ARGS__)) > > +#define SYSCALL_DEFINE_WRAP1(name, ...) SYSCALL_DEFINE_WRAPx(1, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP2(name, ...) SYSCALL_DEFINE_WRAPx(2, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP3(name, ...) SYSCALL_DEFINE_WRAPx(3, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP4(name, ...) SYSCALL_DEFINE_WRAPx(4, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP5(name, ...) SYSCALL_DEFINE_WRAPx(5, _##name, __VA_ARGS__) > +#define SYSCALL_DEFINE_WRAP6(name, ...) SYSCALL_DEFINE_WRAPx(6, _##name, __VA_ARGS__) > + > +#ifndef __SC_COMPAT_TYPE > +#define __SC_COMPAT_TYPE(t, a) \ > + __typeof(__builtin_choose_expr(sizeof(t) > 4, 0L, (t)0)) a > +#endif > + > +#ifndef __SC_COMPAT_CAST > +#define __SC_COMPAT_CAST(t, a) ((t) ((t)(-1) < 0 ? (s64)(s32)(a) : (u64)(u32)(a))) > +#endif You might consider adding a BUILD_BUG_ON() here, like within the s390 variant. Personally I don't like the SYSCALL_DEFINE_WRAPx names too much. But that can be changed easily if somebody comes up with a better name for it. > +#define SYSCALL_DEFINE_WRAPx(x, name, ...) \ > +asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \ > +asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \ > + __attribute__((alias(__stringify(compat_SyS##name)))); \ > +asmlinkage long notrace compat_SyS##name(__MAP(x,__SC_COMPAT_TYPE,__VA_ARGS__)); \ > +asmlinkage long notrace compat_SyS##name(__MAP(x,__SC_COMPAT_TYPE,__VA_ARGS__)) \ > +{ \ > + return sys##name(__MAP(x,__SC_COMPAT_CAST,__VA_ARGS__)); \ > +} \ > +SYSCALL_DEFINEx(x, name, __VA_ARGS__) Given that the system call functions might be inlined, I think it would make sense to remove the "notrace" attribute. Otherwise we would end up with lots of untraceable (and unpatchable) functions. I didn't care back then for the small compat wrappers... > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c > index 0623787..ab14c3f 100644 > --- a/kernel/sys_ni.c > +++ b/kernel/sys_ni.c > @@ -17,27 +17,41 @@ asmlinkage long sys_ni_syscall(void) > } > > cond_syscall(sys_quotactl); > +cond_syscall(compat_sys_quotactl); It might make sense to add a define which adds both variants. Btw. for future versions please add linux-arch to cc.
next prev parent reply other threads:[~2016-01-20 8:17 UTC|newest] Thread overview: 143+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-01-14 17:22 [RFC5 PATCH v6 00/21] ILP32 for ARM64 Yury Norov 2016-01-14 17:22 ` Yury Norov 2016-01-14 17:22 ` [PATCH v6 01/21] arm64: ilp32: add documentation on the ILP32 ABI " Yury Norov 2016-01-14 17:22 ` Yury Norov 2016-01-14 17:22 ` [PATCH v6 02/21] arm64: ensure the kernel is compiled for LP64 Yury Norov 2016-01-14 17:22 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 03/21] arm64: rename COMPAT to AARCH32_EL0 in Kconfig Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 04/21] arm64: change some CONFIG_COMPAT over to use CONFIG_AARCH32_EL0 instead Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 05/21] arm64: compat: change config dependences to aarch32 Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 06/21] arm64:uapi: set __BITS_PER_LONG correctly for ILP32 and LP64 Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 07/21] thread: move thread bits accessors to separated file Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 08/21] arm64: introduce is_a32_task and is_a32_thread (for AArch32 compat) Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 09/21] arm64: ilp32: add is_ilp32_compat_{task,thread} and TIF_32BIT_AARCH64 Yury Norov 2016-01-14 17:23 ` [PATCH v6 09/21] arm64: ilp32: add is_ilp32_compat_{task, thread} " Yury Norov 2016-01-14 17:23 ` [PATCH v6 10/21] arm64: introduce binfmt_elf32.c Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 11/21] arm64: ilp32: introduce binfmt_ilp32.c Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 12/21] arm64: ptrace: handle ptrace_request differently for aarch32 and ilp32 Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 13/21] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 14/21] arm64: signal: wrap struct ucontext, fp and lr with struct sigframe Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 15/21] arm64: signal: share lp64 signal routines to ilp32 Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 16/21] arm64: signal32: move ilp32 and aarch32 common code to separated file Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 17/21] arm64: ilp32: introduce ilp32-specific handlers for sigframe Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-02-29 8:27 ` Andreas Schwab 2016-02-29 8:27 ` Andreas Schwab 2016-01-14 17:23 ` [PATCH v6 18/21] arm64:ilp32: add vdso-ilp32 and use for signal return Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 19/21] arm64:ilp32: add ARM64_ILP32 to Kconfig Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 20/21] all: s390: make compat wrappers the generic solution Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-14 18:11 ` Yury Norov 2016-01-14 18:11 ` Yury Norov 2016-01-15 12:46 ` Heiko Carstens 2016-01-15 12:46 ` Heiko Carstens 2016-01-19 17:52 ` Yury Norov 2016-01-20 8:16 ` Heiko Carstens [this message] 2016-01-20 8:16 ` Heiko Carstens 2016-01-20 12:17 ` Yury Norov 2016-01-20 12:17 ` Yury Norov 2016-01-14 17:23 ` [PATCH v6 21/21] arm64: ilp32: wrap syscalls to remove top 32-bit vulnerability Yury Norov 2016-01-14 17:23 ` Yury Norov 2016-01-18 13:18 ` [RFC5 PATCH v6 00/21] ILP32 for ARM64 Zhangjian (Bamvor) 2016-01-18 13:18 ` Zhangjian (Bamvor) 2016-01-18 13:26 ` Andreas Schwab 2016-01-18 13:26 ` Andreas Schwab 2016-01-18 13:41 ` Bamvor Zhang Jian 2016-01-18 13:41 ` Bamvor Zhang Jian 2016-01-29 9:59 ` Zhangjian (Bamvor) 2016-01-29 9:59 ` Zhangjian (Bamvor) 2016-01-29 17:09 ` Yury Norov 2016-01-29 17:09 ` Yury Norov 2016-01-30 4:15 ` Zhangjian (Bamvor) 2016-01-30 4:15 ` Zhangjian (Bamvor) 2016-02-18 22:35 ` Yury Norov 2016-02-18 22:35 ` Yury Norov 2016-02-19 8:23 ` Arnd Bergmann 2016-02-19 8:23 ` Arnd Bergmann 2016-02-19 12:59 ` Yury Norov 2016-02-19 12:59 ` Yury Norov 2016-02-19 14:06 ` Arnd Bergmann 2016-02-19 14:06 ` Arnd Bergmann 2016-02-29 15:39 ` Yury Norov 2016-02-29 15:39 ` Yury Norov 2016-02-29 16:00 ` Andreas Schwab 2016-02-29 16:00 ` Andreas Schwab 2016-02-29 16:30 ` Arnd Bergmann 2016-02-29 16:30 ` Arnd Bergmann 2016-02-25 10:50 ` Andreas Schwab 2016-02-25 10:50 ` Andreas Schwab 2016-02-25 20:28 ` Yury Norov 2016-02-25 20:28 ` Yury Norov 2016-03-18 10:28 ` Zhangjian (Bamvor) 2016-03-18 10:28 ` Zhangjian (Bamvor) 2016-03-18 15:49 ` Yury Norov 2016-03-18 15:49 ` Yury Norov 2016-03-18 15:55 ` Alexander Graf 2016-03-18 15:55 ` Alexander Graf 2016-03-18 16:46 ` Yury Norov 2016-03-18 16:46 ` Yury Norov 2016-03-20 8:12 ` Zhangjian (Bamvor) 2016-03-20 8:12 ` Zhangjian (Bamvor) 2016-03-21 11:23 ` Zhangjian (Bamvor) 2016-03-21 11:23 ` Zhangjian (Bamvor) 2016-03-21 18:43 ` Yury Norov 2016-03-21 18:43 ` Yury Norov 2016-03-22 1:49 ` Yury Norov 2016-03-22 1:49 ` Yury Norov 2016-03-21 9:07 ` Andreas Schwab 2016-03-21 9:07 ` Andreas Schwab 2016-03-21 9:43 ` Arnd Bergmann 2016-03-21 9:43 ` Arnd Bergmann 2016-03-21 10:52 ` Andreas Schwab 2016-03-21 10:52 ` Andreas Schwab 2016-03-21 17:02 ` Arnd Bergmann 2016-03-21 17:02 ` Arnd Bergmann 2016-03-26 12:36 ` Zhangjian (Bamvor) 2016-03-26 12:36 ` Zhangjian (Bamvor) 2016-03-29 10:58 ` Arnd Bergmann 2016-03-29 10:58 ` Arnd Bergmann 2016-03-29 12:01 ` Yury Norov 2016-03-29 12:01 ` Yury Norov 2016-03-29 12:42 ` Arnd Bergmann 2016-03-29 12:42 ` Arnd Bergmann 2016-03-29 13:21 ` Zhangjian (Bamvor) 2016-03-29 13:21 ` Zhangjian (Bamvor) 2016-03-29 13:27 ` Arnd Bergmann 2016-03-29 13:27 ` Arnd Bergmann 2016-03-29 15:54 ` Joseph Myers 2016-03-29 15:54 ` Joseph Myers 2016-03-29 19:30 ` Arnd Bergmann 2016-03-29 19:30 ` Arnd Bergmann 2016-03-29 20:15 ` Joseph Myers 2016-03-29 20:15 ` Joseph Myers 2016-03-29 20:24 ` Arnd Bergmann 2016-03-29 20:24 ` Arnd Bergmann 2016-03-29 21:00 ` Joseph Myers 2016-03-29 21:00 ` Joseph Myers 2016-03-29 21:39 ` Arnd Bergmann 2016-03-29 21:39 ` Arnd Bergmann 2016-03-31 7:35 ` Zhangjian (Bamvor) 2016-03-31 7:35 ` Zhangjian (Bamvor) 2016-03-21 18:40 ` Yury Norov 2016-03-21 18:40 ` Yury Norov 2016-03-26 13:08 ` Zhangjian (Bamvor) 2016-03-26 13:08 ` Zhangjian (Bamvor) 2016-03-26 13:45 ` Zhangjian (Bamvor) 2016-03-26 13:45 ` Zhangjian (Bamvor) 2016-03-26 22:46 ` Yury Norov 2016-03-26 22:46 ` Yury Norov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160120081643.GB3395@osiris \ --to=heiko.carstens@de.ibm.com \ --cc=Nathan_Lynch@mentor.com \ --cc=Prasun.Kapoor@caviumnetworks.com \ --cc=agraf@suse.de \ --cc=arnd@arndb.de \ --cc=bamvor.zhangjian@huawei.com \ --cc=bamvor.zhangjian@linaro.org \ --cc=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=christoph.muellner@theobroma-systems.com \ --cc=jan.dakinevich@gmail.com \ --cc=joseph@codesourcery.com \ --cc=klimov.linux@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=pinskia@gmail.com \ --cc=schwab@suse.de \ --cc=schwidefsky@de.ibm.com \ --cc=ynorov@caviumnetworks.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.