On Wed, Aug 19, 2020 at 3:39 AM Dave Martin <Dave.Martin@xxxxxxx> wrote: > > On Mon, Aug 17, 2020 at 08:33:48PM -0700, Peter Collingbourne wrote: > > Nit: please say what the patch does. Subject line should summarise > what is done, but should not add new information that is not present in > the description proper. > > (Same for all the other patches.) Will do. > > This allows userspace to detect missing support for flag bits and > > allows the kernel to use non-uapi bits internally, as we are already > > doing in arch/x86 for two flag bits. Now that this change is in > > place, we no longer need the code in arch/x86 that was hiding these > > bits from userspace, so remove it. > > > > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx> > > --- > > View this change in Gerrit: https://linux-review.googlesource.com/q/I35aab6f5be932505d90f3b3450c083b4db1eca86 > > > > arch/arm/include/asm/signal.h | 4 ++++ > > arch/parisc/include/asm/signal.h | 4 ++++ > > arch/x86/kernel/signal_compat.c | 7 ------- > > include/linux/signal_types.h | 12 ++++++++++++ > > kernel/signal.c | 10 ++++++++++ > > 5 files changed, 30 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h > > index 65530a042009..d1070a783993 100644 > > --- a/arch/arm/include/asm/signal.h > > +++ b/arch/arm/include/asm/signal.h > > @@ -17,6 +17,10 @@ typedef struct { > > unsigned long sig[_NSIG_WORDS]; > > } sigset_t; > > > > +#define SA_UAPI_FLAGS \ > > + (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_THIRTYTWO | \ > > + SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND) > > + > > I wonder whether all these per-arch definitions will tend to bitrot when > people add new common flags. > > Can we have a common definition for the common bits, and just add the > extra arch-specific ones here? I think so. We could have something like: #define ARCH_UAPI_SA_FLAGS SA_THIRTYTWO here. Then in signal_types.h we can do: #ifndef ARCH_UAPI_SA_FLAGS #define ARCH_UAPI_SA_FLAGS 0 #endif #define UAPI_SA_FLAGS (... | ARCH_UAPI_SA_FLAGS) I'll do that in v10. > Also, I wonder whether we should avoid the "SA_" prefix here. Maybe > UAPI_SA_FLAGS? Sounds good to me. > > > #define __ARCH_HAS_SA_RESTORER > > > > #include <asm/sigcontext.h> > > diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h > > index 715c96ba2ec8..ad06e14f6e8a 100644 > > --- a/arch/parisc/include/asm/signal.h > > +++ b/arch/parisc/include/asm/signal.h > > @@ -21,6 +21,10 @@ typedef struct { > > unsigned long sig[_NSIG_WORDS]; > > } sigset_t; > > > > +#define SA_UAPI_FLAGS \ > > + (SA_ONSTACK | SA_RESETHAND | SA_NOCLDSTOP | SA_SIGINFO | SA_NODEFER | \ > > + SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT) > > + > > #include <asm/sigcontext.h> > > > > #endif /* !__ASSEMBLY */ > > diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c > > index 9ccbf0576cd0..c599013ae8cb 100644 > > --- a/arch/x86/kernel/signal_compat.c > > +++ b/arch/x86/kernel/signal_compat.c > > @@ -165,16 +165,9 @@ void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oact) > > { > > signal_compat_build_tests(); > > > > - /* Don't leak in-kernel non-uapi flags to user-space */ > > - if (oact) > > - oact->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI); > > - > > if (!act) > > return; > > > > - /* Don't let flags to be set from userspace */ > > - act->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI); > > - > > if (in_ia32_syscall()) > > act->sa.sa_flags |= SA_IA32_ABI; > > if (in_x32_syscall()) > > diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h > > index f8a90ae9c6ec..e792f29b5846 100644 > > --- a/include/linux/signal_types.h > > +++ b/include/linux/signal_types.h > > @@ -68,4 +68,16 @@ struct ksignal { > > int sig; > > }; > > > > +#ifndef SA_UAPI_FLAGS > > +#ifdef SA_RESTORER > > +#define SA_UAPI_FLAGS \ > > + (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > > + SA_NODEFER | SA_RESETHAND | SA_RESTORER) > > +#else > > +#define SA_UAPI_FLAGS \ > > + (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > > + SA_NODEFER | SA_RESETHAND) > > +#endif > > +#endif > > + > > #endif /* _LINUX_SIGNAL_TYPES_H */ > > diff --git a/kernel/signal.c b/kernel/signal.c > > index 42b67d2cea37..348b7981f1ff 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -3984,6 +3984,16 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) > > if (oact) > > *oact = *k; > > > > + /* > > + * Clear unknown flag bits in order to allow userspace to detect missing > > + * support for flag bits and to allow the kernel to use non-uapi bits > > + * internally. > > + */ > > + if (act) > > + act->sa.sa_flags &= SA_UAPI_FLAGS; > > + if (oact) > > + oact->sa.sa_flags &= SA_UAPI_FLAGS; > > + > > Seems reasonable. Thanks. I also decided to check how other operating systems handle unknown flag bits in sigaction.sa_flags. It looks like OpenBSD and illumos also accept unknown bits but (implicitly, as a result of using a different internal representation) end up clearing them in oldact: https://github.com/openbsd/src/blob/f634a6a4b5bf832e9c1de77f7894ae2625e74484/sys/kern/kern_sig.c#L278 https://github.com/illumos/illumos-gate/blob/76f19f5fdc974fe5be5c82a556e43a4df93f1de1/usr/src/uts/common/syscall/sigaction.c#L86 and FreeBSD and NetBSD fail the syscall if unknown bits are set: https://github.com/freebsd/freebsd/blob/eded70c37057857c6e23fae51f86b8f8f43cd2d0/sys/kern/kern_sig.c#L699 https://github.com/NetBSD/src/blob/3365779becdcedfca206091a645a0e8e22b2946e/sys/kern/sys_sig.c#L473 So there is some precedent for doing what we're planning to do here, which makes it yet more likely that we'll be okay doing this. Peter
