| Message ID | 68bd2d6544fb17bbe2fb90862e28ec38e079549a.1597720138.git.pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: expose FAR_EL1 tag bits in siginfo | expand |
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.) > 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@google.com> > --- > 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? Also, I wonder whether we should avoid the "SA_" prefix here. Maybe UAPI_SA_FLAGS? > #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. Cheers ---Dave > sigaction_compat_abi(act, oact); > > if (act) { > -- > 2.28.0.220.ged08abb693-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Aug 19, 2020 at 3:39 AM Dave Martin <Dave.Martin@arm.com> 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@google.com> > > --- > > 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
On Wed, Aug 19, 2020 at 04:39:53PM -0700, Peter Collingbourne wrote: > On Wed, Aug 19, 2020 at 3:39 AM Dave Martin <Dave.Martin@arm.com> 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. Thanks [...] > > > 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. Yes, something like that would be fine, I should think. > > Also, I wonder whether we should avoid the "SA_" prefix here. Maybe > > UAPI_SA_FLAGS? > > Sounds good to me. OK, great. [...] > > > 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. Ack, it's good to have that extra evidence to support this approach. This also means that other OSes could adopt the new Linux flag(s) with comatible semantics, if they wanted to. Or have I misunderstood something there? Cheers ---Dave
On Mon, Aug 24, 2020 at 6:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Wed, Aug 19, 2020 at 04:39:53PM -0700, Peter Collingbourne wrote: > > On Wed, Aug 19, 2020 at 3:39 AM Dave Martin <Dave.Martin@arm.com> 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. > > Thanks > > [...] > > > > > 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. > > Yes, something like that would be fine, I should think. > > > > Also, I wonder whether we should avoid the "SA_" prefix here. Maybe > > > UAPI_SA_FLAGS? > > > > Sounds good to me. > > OK, great. > > [...] > > > > > 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 XNU does the same: https://github.com/apple/darwin-xnu/blob/a449c6a3b8014d9406c2ddbdc81795da24aa7443/bsd/kern/kern_sig.c#L480 > > > > 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. > > Ack, it's good to have that extra evidence to support this approach. > > This also means that other OSes could adopt the new Linux flag(s) with > comatible semantics, if they wanted to. Or have I misunderstood > something there? The other OSs could adopt SA_XFLAGS, but they would probably have no need for SA_UNSUPPORTED because they already have a protocol for detecting missing flag support in the kernel (Linux is really the odd one out here in not supporting such a protocol from the start). Userspace programs running on OpenBSD, illumos and XNU could use the Linux protocol without the SA_UNSUPPORTED part, while programs running on FreeBSD and NetBSD could do something like: static bool has_xflags = true; [...] struct sigaction act; act.sa_flags = SA_SIGINFO | SA_XFLAGS; if (sigaction(SIGSEGV, &act, 0) != 0) { has_xflags = false; act.sa_flags = SA_SIGINFO; if (sigaction(SIGSEGV, &act, 0) != 0) perror("sigaction"); } It would probably be possible to write a unified function that would support all three protocols. Peter
On Mon, Aug 24, 2020 at 05:51:34PM -0700, Peter Collingbourne wrote: > On Mon, Aug 24, 2020 at 6:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Wed, Aug 19, 2020 at 04:39:53PM -0700, Peter Collingbourne wrote: > > > On Wed, Aug 19, 2020 at 3:39 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > > On Mon, Aug 17, 2020 at 08:33:48PM -0700, Peter Collingbourne wrote: [...] > > > > > 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 > > XNU does the same: > > https://github.com/apple/darwin-xnu/blob/a449c6a3b8014d9406c2ddbdc81795da24aa7443/bsd/kern/kern_sig.c#L480 > > > > > > > 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. > > > > Ack, it's good to have that extra evidence to support this approach. > > > > This also means that other OSes could adopt the new Linux flag(s) with > > comatible semantics, if they wanted to. Or have I misunderstood > > something there? > > The other OSs could adopt SA_XFLAGS, but they would probably have no > need for SA_UNSUPPORTED because they already have a protocol for > detecting missing flag support in the kernel (Linux is really the odd > one out here in not supporting such a protocol from the start). > Userspace programs running on OpenBSD, illumos and XNU could use the > Linux protocol without the SA_UNSUPPORTED part, while programs running > on FreeBSD and NetBSD could do something like: > > static bool has_xflags = true; > [...] > struct sigaction act; > act.sa_flags = SA_SIGINFO | SA_XFLAGS; > if (sigaction(SIGSEGV, &act, 0) != 0) { > has_xflags = false; > act.sa_flags = SA_SIGINFO; > if (sigaction(SIGSEGV, &act, 0) != 0) > perror("sigaction"); > } > > It would probably be possible to write a unified function that would > support all three protocols. Ack, I think that's about as well as we can reasonably do in the circumstances. Cheers ---Dave
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) + #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; + sigaction_compat_abi(act, oact); if (act) {
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@google.com> --- 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(-)