| Message ID | 20200428195645.1365019-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixup! signal: factor copy_siginfo_to_external32 from copy_siginfo_to_user32 | expand |
Le 28/04/2020 à 21:56, Arnd Bergmann a écrit : > I think I found a way to improve the x32 handling: > > This is a simplification over Christoph's "[PATCH 2/7] signal: factor > copy_siginfo_to_external32 from copy_siginfo_to_user32", reducing the > x32 specifics in the common code to a single #ifdef/#endif check, in > order to keep it more readable for everyone else. > > Christoph, if you like it, please fold into your patch. > > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Jeremy Kerr <jk@ozlabs.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: "Eric W . Biederman" <ebiederm@xmission.com> > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-fsdevel@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/x86/include/asm/compat.h | 10 ++++++++++ > arch/x86/kernel/signal.c | 23 +++++++++++++++++++++++ > kernel/signal.c | 15 ++------------- > 3 files changed, 35 insertions(+), 13 deletions(-) > [...] > diff --git a/kernel/signal.c b/kernel/signal.c > index 1a81602050b4..935facca4860 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3318,29 +3318,18 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > } > } > > +#ifndef CONFIG_X86_X32_ABI Can it be declared __weak instead of enclosing it in an #ifndef ? > int copy_siginfo_to_user32(struct compat_siginfo __user *to, > const struct kernel_siginfo *from) > -#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION) > -{ > - return __copy_siginfo_to_user32(to, from, in_x32_syscall()); > -} > -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, > - const struct kernel_siginfo *from, bool x32_ABI) > -#endif > { > struct compat_siginfo new; > > copy_siginfo_to_external32(&new, from); > -#ifdef CONFIG_X86_X32_ABI > - if (x32_ABI && from->si_signo == SIGCHLD) { > - new._sifields._sigchld_x32._utime = from->si_utime; > - new._sifields._sigchld_x32._stime = from->si_stime; > - } > -#endif > if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) > return -EFAULT; > return 0; > } > +#endif > > static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > const struct compat_siginfo *from) > Christophe
On Wed, Apr 29, 2020 at 08:17:22AM +0200, Christophe Leroy wrote: >> +#ifndef CONFIG_X86_X32_ABI > > Can it be declared __weak instead of enclosing it in an #ifndef ? I really hate the __weak ifdefs. But my plan was to move to a CONFIG_ARCH_COPY_SIGINFO_TO_USER32 and have x86 select it.
On Tue, Apr 28, 2020 at 09:56:26PM +0200, Arnd Bergmann wrote: > I think I found a way to improve the x32 handling: > > This is a simplification over Christoph's "[PATCH 2/7] signal: factor > copy_siginfo_to_external32 from copy_siginfo_to_user32", reducing the > x32 specifics in the common code to a single #ifdef/#endif check, in > order to keep it more readable for everyone else. > > Christoph, if you like it, please fold into your patch. What do you think of this version? This one always overrides copy_siginfo_to_user32 for the x86 compat case to keep the churn down, and improves the copy_siginfo_to_external32 documentation a bit. --- From 5ca642c4c744ce6460ebb91fe30ec7a064d28e96 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Wed, 29 Apr 2020 08:38:07 +0200 Subject: signal: factor copy_siginfo_to_external32 from copy_siginfo_to_user32 To remove the use of set_fs in the coredump code there needs to be a way to convert a kernel siginfo to a userspace compat siginfo. Factour out a copy_siginfo_to_external32 helper from copy_siginfo_to_user32 that fills out the compat_siginfo, but does so on a kernel space data structure. Handling of the x32 SIGCHLD magic is kept out of copy_siginfo_to_external32, as coredumps are never caused by SIGCHLD. To simplify the mess that the current copy_siginfo_to_user32 is, we allow architectures to override it, and keep thus keep the x32 SIGCHLD magic confined to x86-64. Contains improvements from Eric W. Biederman <ebiederm@xmission.com> and Arnd Bergmann <arnd@arndb.de>. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/Kconfig | 3 ++ arch/x86/Kconfig | 1 + arch/x86/kernel/signal.c | 22 ++++++++ include/linux/compat.h | 2 + kernel/signal.c | 108 ++++++++++++++++++++------------------- 5 files changed, 83 insertions(+), 53 deletions(-) diff --git a/arch/Kconfig b/arch/Kconfig index 786a85d4ad40d..d5bc9f1ee1747 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -815,6 +815,9 @@ config OLD_SIGACTION config COMPAT_OLD_SIGACTION bool +config ARCH_COPY_SIGINFO_TO_USER32 + bool + config COMPAT_32BIT_TIME bool "Provide system calls for 32-bit time_t" default !64BIT || COMPAT diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 1197b5596d5ad..ad13facbe9ffe 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2906,6 +2906,7 @@ config X86_X32 config COMPAT_32 def_bool y depends on IA32_EMULATION || X86_32 + select ARCH_COPY_SIGINFO_TO_USER32 select HAVE_UID16 select OLD_SIGSUSPEND3 diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 83b74fb38c8fc..d2b09866105a2 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -511,6 +511,28 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig, } #endif /* CONFIG_X86_32 */ +#ifdef CONFIG_ARCH_COPY_SIGINFO_TO_USER32 +int __copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from, bool x32_ABI) +{ + struct compat_siginfo new; + + copy_siginfo_to_external32(&new, from); + if (x32_ABI && from->si_signo == SIGCHLD) { + new._sifields._sigchld_x32._utime = from->si_utime; + new._sifields._sigchld_x32._stime = from->si_stime; + } + if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) + return -EFAULT; + return 0; +} +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + return __copy_siginfo_to_user32(to, from, in_x32_syscall()); +} +#endif /* CONFIG_ARCH_COPY_SIGINFO_TO_USER32 */ + static int x32_setup_rt_frame(struct ksignal *ksig, compat_sigset_t *set, struct pt_regs *regs) diff --git a/include/linux/compat.h b/include/linux/compat.h index 0480ba4db5929..adbfe8f688d92 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -402,6 +402,8 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask, unsigned long bitmap_size); long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask, unsigned long bitmap_size); +void copy_siginfo_to_external32(struct compat_siginfo *to, + const struct kernel_siginfo *from); int copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo __user *from); int copy_siginfo_to_user32(struct compat_siginfo __user *to, const kernel_siginfo_t *from); int get_compat_sigevent(struct sigevent *event, diff --git a/kernel/signal.c b/kernel/signal.c index 284fc1600063b..710655df1269d 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3235,96 +3235,98 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from) } #ifdef CONFIG_COMPAT -int copy_siginfo_to_user32(struct compat_siginfo __user *to, - const struct kernel_siginfo *from) -#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION) -{ - return __copy_siginfo_to_user32(to, from, in_x32_syscall()); -} -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, - const struct kernel_siginfo *from, bool x32_ABI) -#endif +/** + * copy_siginfo_to_external32: copy a kernel signinfo into a 32-bit user one + * @to: compat siginfo destination + * @from: kernel siginfo source + * + * This function does not work properly for SIGCHLD on x32, but it does not need + * to as SIGCHLD never causes a coredump as this function is only intended to + * be used either by the coredump code or to implement copy_siginfo_to_user32, + * which can have its own arch version to deal with things like x32. + */ +void copy_siginfo_to_external32(struct compat_siginfo *to, + const struct kernel_siginfo *from) { - struct compat_siginfo new; - memset(&new, 0, sizeof(new)); + memset(to, 0, sizeof(*to)); - new.si_signo = from->si_signo; - new.si_errno = from->si_errno; - new.si_code = from->si_code; + to->si_signo = from->si_signo; + to->si_errno = from->si_errno; + to->si_code = from->si_code; switch(siginfo_layout(from->si_signo, from->si_code)) { case SIL_KILL: - new.si_pid = from->si_pid; - new.si_uid = from->si_uid; + to->si_pid = from->si_pid; + to->si_uid = from->si_uid; break; case SIL_TIMER: - new.si_tid = from->si_tid; - new.si_overrun = from->si_overrun; - new.si_int = from->si_int; + to->si_tid = from->si_tid; + to->si_overrun = from->si_overrun; + to->si_int = from->si_int; break; case SIL_POLL: - new.si_band = from->si_band; - new.si_fd = from->si_fd; + to->si_band = from->si_band; + to->si_fd = from->si_fd; break; case SIL_FAULT: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif break; case SIL_FAULT_MCEERR: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif - new.si_addr_lsb = from->si_addr_lsb; + to->si_addr_lsb = from->si_addr_lsb; break; case SIL_FAULT_BNDERR: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif - new.si_lower = ptr_to_compat(from->si_lower); - new.si_upper = ptr_to_compat(from->si_upper); + to->si_lower = ptr_to_compat(from->si_lower); + to->si_upper = ptr_to_compat(from->si_upper); break; case SIL_FAULT_PKUERR: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif - new.si_pkey = from->si_pkey; + to->si_pkey = from->si_pkey; break; case SIL_CHLD: - new.si_pid = from->si_pid; - new.si_uid = from->si_uid; - new.si_status = from->si_status; -#ifdef CONFIG_X86_X32_ABI - if (x32_ABI) { - new._sifields._sigchld_x32._utime = from->si_utime; - new._sifields._sigchld_x32._stime = from->si_stime; - } else -#endif - { - new.si_utime = from->si_utime; - new.si_stime = from->si_stime; - } + to->si_pid = from->si_pid; + to->si_uid = from->si_uid; + to->si_status = from->si_status; + to->si_utime = from->si_utime; + to->si_stime = from->si_stime; break; case SIL_RT: - new.si_pid = from->si_pid; - new.si_uid = from->si_uid; - new.si_int = from->si_int; + to->si_pid = from->si_pid; + to->si_uid = from->si_uid; + to->si_int = from->si_int; break; case SIL_SYS: - new.si_call_addr = ptr_to_compat(from->si_call_addr); - new.si_syscall = from->si_syscall; - new.si_arch = from->si_arch; + to->si_call_addr = ptr_to_compat(from->si_call_addr); + to->si_syscall = from->si_syscall; + to->si_arch = from->si_arch; break; } +} +#ifndef CONFIG_ARCH_COPY_SIGINFO_TO_USER32 +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + struct compat_siginfo new; + + copy_siginfo_to_external32(&new, from); if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) return -EFAULT; - return 0; } +#endif /* ARCH_COPY_SIGINFO_TO_USER32 */ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo *from)
On Wed, Apr 29, 2020 at 8:45 AM Christoph Hellwig <hch@lst.de> wrote: > > On Tue, Apr 28, 2020 at 09:56:26PM +0200, Arnd Bergmann wrote: > > I think I found a way to improve the x32 handling: > > > > This is a simplification over Christoph's "[PATCH 2/7] signal: factor > > copy_siginfo_to_external32 from copy_siginfo_to_user32", reducing the > > x32 specifics in the common code to a single #ifdef/#endif check, in > > order to keep it more readable for everyone else. > > > > Christoph, if you like it, please fold into your patch. > > What do you think of this version? This one always overrides > copy_siginfo_to_user32 for the x86 compat case to keep the churn down, > and improves the copy_siginfo_to_external32 documentation a bit. Looks good to me. I preferred checking for X32 explicitly (so we can find and kill off the #ifdef if we ever remove X32 for good), but there is little difference in the end. Arnd
On Wed, Apr 29, 2020 at 10:07:11AM +0200, Arnd Bergmann wrote: > > What do you think of this version? This one always overrides > > copy_siginfo_to_user32 for the x86 compat case to keep the churn down, > > and improves the copy_siginfo_to_external32 documentation a bit. > > Looks good to me. I preferred checking for X32 explicitly (so we can > find and kill off the #ifdef if we ever remove X32 for good), but there is > little difference in the end. Is there any realistic chance we'll get rid of x32?
On Wed, Apr 29, 2020 at 11:42 AM Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Apr 29, 2020 at 10:07:11AM +0200, Arnd Bergmann wrote: > > > What do you think of this version? This one always overrides > > > copy_siginfo_to_user32 for the x86 compat case to keep the churn down, > > > and improves the copy_siginfo_to_external32 documentation a bit. > > > > Looks good to me. I preferred checking for X32 explicitly (so we can > > find and kill off the #ifdef if we ever remove X32 for good), but there is > > little difference in the end. > > Is there any realistic chance we'll get rid of x32? When we discussed it last year, there were a couple of users that replied saying they actively use it for a full system, and some others said they run specific programs built as x32 as it results in much faster (10% to 20%) execution of the same binaries compared to either i686 or x86_64. I expect both of these to get less common over time as stuff bitrots and more of the workloads that benefit most from the higher performance (cross-compilers, hpc) run out of virtual address space. Debian popcon numbers are too small to be reliable but they do show a trend at https://popcon.debian.org/stat/sub-x32.png I would just ask again every few years, and eventually we'll decide it's not worth keeping any more. I do expect most 32-bit machines to stop getting kernel updates before 2030 and we can probably remove a bunch of architectures including x32 before then, though at least armv7 users will have to get kernel updates for substantially longer. Arnd
I did another pass at this, reducing the overhead of the x32 magic in common code down to renaming copy_siginfo_to_user32 to copy_siginfo_to_user32 and having a conditional #define to give it the old name back: --- From 45e5263d7c24d854bb446b7e69dc53729ed842bc Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Wed, 29 Apr 2020 11:57:10 +0200 Subject: signal: refactor copy_siginfo_to_user32 Factor out a copy_siginfo_to_external32 helper from copy_siginfo_to_user32 that fills out the compat_siginfo, but does so on a kernel space data structure. With that we can let architectures override copy_siginfo_to_user32 with their own implementations using copy_siginfo_to_external32. That allows moving the x32 SIGCHLD purely to x86 architecture code. As a nice side effect copy_siginfo_to_external32 also comes in handy for avoiding a set_fs() call in the coredump code later on. Contains improvements from Eric W. Biederman <ebiederm@xmission.com> and Arnd Bergmann <arnd@arndb.de>. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/x86/ia32/ia32_signal.c | 2 +- arch/x86/include/asm/compat.h | 8 ++- arch/x86/kernel/signal.c | 28 ++++++++- include/linux/compat.h | 11 +++- kernel/signal.c | 106 +++++++++++++++++----------------- 5 files changed, 96 insertions(+), 59 deletions(-) diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index f9d8804144d09..81cf22398cd16 100644 --- a/arch/x86/ia32/ia32_signal.c +++ b/arch/x86/ia32/ia32_signal.c @@ -350,7 +350,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig, unsafe_put_user(*(__u64 *)set, (__u64 *)&frame->uc.uc_sigmask, Efault); user_access_end(); - if (__copy_siginfo_to_user32(&frame->info, &ksig->info, false)) + if (__copy_siginfo_to_user32(&frame->info, &ksig->info)) return -EFAULT; /* Set up registers for signal handler */ diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h index 52e9f3480f690..d4edf281fff49 100644 --- a/arch/x86/include/asm/compat.h +++ b/arch/x86/include/asm/compat.h @@ -214,7 +214,11 @@ static inline bool in_compat_syscall(void) #endif struct compat_siginfo; -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, - const kernel_siginfo_t *from, bool x32_ABI); + +#ifdef CONFIG_X86_X32_ABI +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const kernel_siginfo_t *from); +#define copy_siginfo_to_user32 copy_siginfo_to_user32 +#endif /* CONFIG_X86_X32_ABI */ #endif /* _ASM_X86_COMPAT_H */ diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 83b74fb38c8fc..f3df262e370b3 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -37,6 +37,7 @@ #include <asm/vm86.h> #ifdef CONFIG_X86_64 +#include <linux/compat.h> #include <asm/proto.h> #include <asm/ia32_unistd.h> #endif /* CONFIG_X86_64 */ @@ -511,6 +512,31 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig, } #endif /* CONFIG_X86_32 */ +#ifdef CONFIG_X86_X32_ABI +static int x32_copy_siginfo_to_user(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + struct compat_siginfo new; + + copy_siginfo_to_external32(&new, from); + if (from->si_signo == SIGCHLD) { + new._sifields._sigchld_x32._utime = from->si_utime; + new._sifields._sigchld_x32._stime = from->si_stime; + } + if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) + return -EFAULT; + return 0; +} + +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + if (in_x32_syscall()) + return x32_copy_siginfo_to_user(to, from); + return __copy_siginfo_to_user32(to, from); +} +#endif /* CONFIG_X86_X32_ABI */ + static int x32_setup_rt_frame(struct ksignal *ksig, compat_sigset_t *set, struct pt_regs *regs) @@ -543,7 +569,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig, user_access_end(); if (ksig->ka.sa.sa_flags & SA_SIGINFO) { - if (__copy_siginfo_to_user32(&frame->info, &ksig->info, true)) + if (x32_copy_siginfo_to_user(&frame->info, &ksig->info)) return -EFAULT; } diff --git a/include/linux/compat.h b/include/linux/compat.h index 0480ba4db5929..e432df9be2e4b 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -402,8 +402,15 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask, unsigned long bitmap_size); long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask, unsigned long bitmap_size); -int copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo __user *from); -int copy_siginfo_to_user32(struct compat_siginfo __user *to, const kernel_siginfo_t *from); +void __copy_siginfo_to_external32(struct compat_siginfo *to, + const struct kernel_siginfo *from); +int copy_siginfo_from_user32(kernel_siginfo_t *to, + const struct compat_siginfo __user *from); +int __copy_siginfo_to_user32(struct compat_siginfo __user *to, + const kernel_siginfo_t *from); +#ifndef copy_siginfo_to_user32 +#define copy_siginfo_to_user32 __copy_siginfo_to_user32 +#endif int get_compat_sigevent(struct sigevent *event, const struct compat_sigevent __user *u_event); diff --git a/kernel/signal.c b/kernel/signal.c index 284fc1600063b..3a74e67c12425 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3235,94 +3235,94 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from) } #ifdef CONFIG_COMPAT -int copy_siginfo_to_user32(struct compat_siginfo __user *to, - const struct kernel_siginfo *from) -#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION) -{ - return __copy_siginfo_to_user32(to, from, in_x32_syscall()); -} -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, - const struct kernel_siginfo *from, bool x32_ABI) -#endif +/** + * copy_siginfo_to_external32: copy a kernel signinfo into a 32-bit user one + * @to: compat siginfo destination + * @from: kernel siginfo source + * + * This function does not work properly for SIGCHLD on x32, but it does not need + * to as SIGCHLD never causes a coredump as this function is only intended to + * be used either by the coredump code or to implement copy_siginfo_to_user32, + * which can have its own arch version to deal with things like x32. + */ +void copy_siginfo_to_external32(struct compat_siginfo *to, + const struct kernel_siginfo *from) { - struct compat_siginfo new; - memset(&new, 0, sizeof(new)); + memset(to, 0, sizeof(*to)); - new.si_signo = from->si_signo; - new.si_errno = from->si_errno; - new.si_code = from->si_code; + to->si_signo = from->si_signo; + to->si_errno = from->si_errno; + to->si_code = from->si_code; switch(siginfo_layout(from->si_signo, from->si_code)) { case SIL_KILL: - new.si_pid = from->si_pid; - new.si_uid = from->si_uid; + to->si_pid = from->si_pid; + to->si_uid = from->si_uid; break; case SIL_TIMER: - new.si_tid = from->si_tid; - new.si_overrun = from->si_overrun; - new.si_int = from->si_int; + to->si_tid = from->si_tid; + to->si_overrun = from->si_overrun; + to->si_int = from->si_int; break; case SIL_POLL: - new.si_band = from->si_band; - new.si_fd = from->si_fd; + to->si_band = from->si_band; + to->si_fd = from->si_fd; break; case SIL_FAULT: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif break; case SIL_FAULT_MCEERR: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif - new.si_addr_lsb = from->si_addr_lsb; + to->si_addr_lsb = from->si_addr_lsb; break; case SIL_FAULT_BNDERR: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif - new.si_lower = ptr_to_compat(from->si_lower); - new.si_upper = ptr_to_compat(from->si_upper); + to->si_lower = ptr_to_compat(from->si_lower); + to->si_upper = ptr_to_compat(from->si_upper); break; case SIL_FAULT_PKUERR: - new.si_addr = ptr_to_compat(from->si_addr); + to->si_addr = ptr_to_compat(from->si_addr); #ifdef __ARCH_SI_TRAPNO - new.si_trapno = from->si_trapno; + to->si_trapno = from->si_trapno; #endif - new.si_pkey = from->si_pkey; + to->si_pkey = from->si_pkey; break; case SIL_CHLD: - new.si_pid = from->si_pid; - new.si_uid = from->si_uid; - new.si_status = from->si_status; -#ifdef CONFIG_X86_X32_ABI - if (x32_ABI) { - new._sifields._sigchld_x32._utime = from->si_utime; - new._sifields._sigchld_x32._stime = from->si_stime; - } else -#endif - { - new.si_utime = from->si_utime; - new.si_stime = from->si_stime; - } + to->si_pid = from->si_pid; + to->si_uid = from->si_uid; + to->si_status = from->si_status; + to->si_utime = from->si_utime; + to->si_stime = from->si_stime; break; case SIL_RT: - new.si_pid = from->si_pid; - new.si_uid = from->si_uid; - new.si_int = from->si_int; + to->si_pid = from->si_pid; + to->si_uid = from->si_uid; + to->si_int = from->si_int; break; case SIL_SYS: - new.si_call_addr = ptr_to_compat(from->si_call_addr); - new.si_syscall = from->si_syscall; - new.si_arch = from->si_arch; + to->si_call_addr = ptr_to_compat(from->si_call_addr); + to->si_syscall = from->si_syscall; + to->si_arch = from->si_arch; break; } +} +int __copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + struct compat_siginfo new; + + copy_siginfo_to_external32(&new, from); if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) return -EFAULT; - return 0; }
On Wed, Apr 29, 2020 at 1:53 PM Christoph Hellwig <hch@lst.de> wrote: > > I did another pass at this, reducing the overhead of the x32 magic > in common code down to renaming copy_siginfo_to_user32 to > copy_siginfo_to_user32 and having a conditional #define to give it > the old name back: Nice! I guess this is about as good as it gets, so we can stop spending more time on it now ;-) Arnd
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h index 52e9f3480f69..9341ea3da757 100644 --- a/arch/x86/include/asm/compat.h +++ b/arch/x86/include/asm/compat.h @@ -214,7 +214,17 @@ static inline bool in_compat_syscall(void) #endif struct compat_siginfo; +#ifdef CONFIG_X86_X32_ABI int __copy_siginfo_to_user32(struct compat_siginfo __user *to, const kernel_siginfo_t *from, bool x32_ABI); +#else +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const kernel_siginfo_t *from); +static inline int __copy_siginfo_to_user32(struct compat_siginfo __user *to, + const kernel_siginfo_t *from, bool x32_ABI) +{ + return copy_siginfo_to_user32(to, from); +} +#endif #endif /* _ASM_X86_COMPAT_H */ diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 83b74fb38c8f..0e166571d28b 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -511,6 +511,29 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig, } #endif /* CONFIG_X86_32 */ +#ifdef CONFIG_X86_X32_ABI +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + return __copy_siginfo_to_user32(to, from, in_x32_syscall()); +} + +int __copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from, bool x32_ABI) +{ + struct compat_siginfo new; + + copy_siginfo_to_external32(&new, from); + if (x32_ABI && from->si_signo == SIGCHLD) { + new._sifields._sigchld_x32._utime = from->si_utime; + new._sifields._sigchld_x32._stime = from->si_stime; + } + if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) + return -EFAULT; + return 0; +} +#endif + static int x32_setup_rt_frame(struct ksignal *ksig, compat_sigset_t *set, struct pt_regs *regs) diff --git a/kernel/signal.c b/kernel/signal.c index 1a81602050b4..935facca4860 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3318,29 +3318,18 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, } } +#ifndef CONFIG_X86_X32_ABI int copy_siginfo_to_user32(struct compat_siginfo __user *to, const struct kernel_siginfo *from) -#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION) -{ - return __copy_siginfo_to_user32(to, from, in_x32_syscall()); -} -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, - const struct kernel_siginfo *from, bool x32_ABI) -#endif { struct compat_siginfo new; copy_siginfo_to_external32(&new, from); -#ifdef CONFIG_X86_X32_ABI - if (x32_ABI && from->si_signo == SIGCHLD) { - new._sifields._sigchld_x32._utime = from->si_utime; - new._sifields._sigchld_x32._stime = from->si_stime; - } -#endif if (copy_to_user(to, &new, sizeof(struct compat_siginfo))) return -EFAULT; return 0; } +#endif static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo *from)
I think I found a way to improve the x32 handling: This is a simplification over Christoph's "[PATCH 2/7] signal: factor copy_siginfo_to_external32 from copy_siginfo_to_user32", reducing the x32 specifics in the common code to a single #ifdef/#endif check, in order to keep it more readable for everyone else. Christoph, if you like it, please fold into your patch. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Jeremy Kerr <jk@ozlabs.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: "Eric W . Biederman" <ebiederm@xmission.com> Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/x86/include/asm/compat.h | 10 ++++++++++ arch/x86/kernel/signal.c | 23 +++++++++++++++++++++++ kernel/signal.c | 15 ++------------- 3 files changed, 35 insertions(+), 13 deletions(-)