| Message ID | 20200414070142.288696-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] powerpc/spufs: simplify spufs core dumping | expand |
Christoph Hellwig <hch@lst.de> writes: > Instead of an architecture specific calling convention in common code > just pass a flags argument with architecture specific values. This bothers me because it makes all architectures pay for the sins of x32. Further it starts burying the details of the what is happening in architecture specific helpers. Hiding the fact that there is only one niche architecture that does anything weird. I am very sensitive to hiding away signal handling details right now because way to much of the signal handling code got hidden in architecture specific files and was quite buggy because as a result. My general sense is putting all of the weird details up front and center in kernel/signal.c is the only way for this code will be looked at and successfully maintained. How about these patches to solve set_fs with binfmt_elf instead: Eric W. Biederman (2): signal: Factor copy_siginfo_to_external32 from copy_siginfo_to_user32 signal: Remove the set_fs in binfmt_elf.c:fill_siginfo_note fs/binfmt_elf.c | 5 +---- fs/compat_binfmt_elf.c | 2 +- include/linux/compat.h | 1 + include/linux/signal.h | 7 +++++++ kernel/signal.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------- Eric
On Fri, Apr 17, 2020 at 04:08:23PM -0500, Eric W. Biederman wrote: > This bothers me because it makes all architectures pay for the sins of > x32. Further it starts burying the details of the what is happening in > architecture specific helpers. Hiding the fact that there is only > one niche architecture that does anything weird. > > I am very sensitive to hiding away signal handling details right now > because way to much of the signal handling code got hidden in > architecture specific files and was quite buggy because as a result. I much prefer the unconditional flags over the crazy ifdef mess in the current coe and your version. Except for that and the rather strange and confusing copy_siginfo_to_external32 name it pretty much looks the same.
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index f9d8804144d0..2bf188942d5c 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, SA_IA32_ABI)) 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 52e9f3480f69..a787c9a82030 100644 --- a/arch/x86/include/asm/compat.h +++ b/arch/x86/include/asm/compat.h @@ -213,8 +213,4 @@ static inline bool in_compat_syscall(void) #define in_compat_syscall in_compat_syscall /* override the generic impl */ #endif -struct compat_siginfo; -int __copy_siginfo_to_user32(struct compat_siginfo __user *to, - const kernel_siginfo_t *from, bool x32_ABI); - #endif /* _ASM_X86_COMPAT_H */ diff --git a/arch/x86/include/asm/signal.h b/arch/x86/include/asm/signal.h index 33d3c88a7225..b3f7a14da428 100644 --- a/arch/x86/include/asm/signal.h +++ b/arch/x86/include/asm/signal.h @@ -28,6 +28,9 @@ typedef struct { #define SA_IA32_ABI 0x02000000u #define SA_X32_ABI 0x01000000u +#define compat_siginfo_flags() \ + (in_x32_syscall() ? SA_X32_ABI : SA_IA32_ABI) + #ifndef CONFIG_COMPAT typedef sigset_t compat_sigset_t; #endif diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 83b74fb38c8f..bbd451631790 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -543,7 +543,8 @@ 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 (__copy_siginfo_to_user32(&frame->info, &ksig->info, + SA_X32_ABI)) return -EFAULT; } diff --git a/include/linux/compat.h b/include/linux/compat.h index 0480ba4db592..14eec6116110 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -404,6 +404,8 @@ 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); +int __copy_siginfo_to_user32(struct compat_siginfo __user *to, + const kernel_siginfo_t *from, unsigned int flags); 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 e58a6c619824..092fee008242 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3235,15 +3235,8 @@ 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 + const struct kernel_siginfo *from, unsigned int flags) { struct compat_siginfo new; memset(&new, 0, sizeof(new)); @@ -3298,7 +3291,7 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to, new.si_uid = from->si_uid; new.si_status = from->si_status; #ifdef CONFIG_X86_X32_ABI - if (x32_ABI) { + if (flags & SA_X32_ABI) { new._sifields._sigchld_x32._utime = from->si_utime; new._sifields._sigchld_x32._stime = from->si_stime; } else @@ -3326,6 +3319,16 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to, return 0; } +#ifndef compat_siginfo_flags +#define compat_siginfo_flags() 0 +#endif + +int copy_siginfo_to_user32(struct compat_siginfo __user *to, + const struct kernel_siginfo *from) +{ + return __copy_siginfo_to_user32(to, from, compat_siginfo_flags()); +} + static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo *from) {
Instead of an architecture specific calling convention in common code just pass a flags argument with architecture specific values. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/x86/ia32/ia32_signal.c | 2 +- arch/x86/include/asm/compat.h | 4 ---- arch/x86/include/asm/signal.h | 3 +++ arch/x86/kernel/signal.c | 3 ++- include/linux/compat.h | 2 ++ kernel/signal.c | 21 ++++++++++++--------- 6 files changed, 20 insertions(+), 15 deletions(-)