| Message ID | f37a8b86c53be4cc440a73be4123e0419deefe5f.1598072840.git.pcc@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | arm64: expose FAR_EL1 tag bits in siginfo | expand |
On Fri, Aug 21, 2020 at 10:10:14PM -0700, Peter Collingbourne wrote: Nit: no statement of the chage being made (other than in the subject line). > This bit will never be supported in the uapi. The purpose of this flag > bit is to allow userspace to distinguish an old kernel that does not > clear unknown sa_flags bits from a kernel that supports every flag bit. > > In other words, if userspace finds that this bit remains set in > oldact.sa_flags, it means that the kernel cannot be trusted to have > cleared unknown flag bits from sa_flags, so no assumptions about flag > bit support can be made. This isn't quite right? After a single sigaction() call, oact will contain the sa_flags for the previously registered handler. So a second sigaction() call would be needed to find out the newly effective sa_flags. > > Signed-off-by: Peter Collingbourne <pcc@google.com> > --- > View this change in Gerrit: https://linux-review.googlesource.com/q/Ic2501ad150a3a79c1cf27fb8c99be342e9dffbcb > > include/uapi/asm-generic/signal-defs.h | 7 +++++++ > kernel/signal.c | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/uapi/asm-generic/signal-defs.h b/include/uapi/asm-generic/signal-defs.h > index 319628058a53..e853cbe8722d 100644 > --- a/include/uapi/asm-generic/signal-defs.h > +++ b/include/uapi/asm-generic/signal-defs.h > @@ -14,6 +14,12 @@ > * SA_RESTART flag to get restarting signals (which were the default long ago) > * SA_NODEFER prevents the current signal from being masked in the handler. > * SA_RESETHAND clears the handler when the signal is delivered. > + * SA_UNSUPPORTED is a flag bit that will never be supported. Kernels from > + * before the introduction of SA_UNSUPPORTED did not clear unknown bits from > + * sa_flags when read using the oldact argument to sigaction and rt_sigaction, > + * so this bit allows flag bit support to be detected from userspace while > + * allowing an old kernel to be distinguished from a kernel that supports every > + * flag bit. > * > * SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single > * Unix names RESETHAND and NODEFER respectively. > @@ -42,6 +48,7 @@ > #ifndef SA_RESETHAND > #define SA_RESETHAND 0x80000000 > #endif > +#define SA_UNSUPPORTED 0x00000400 I guess people may debate which bit is chosen, but your consolidation of these definitions should help to reduce the possibility of future collisions. This bit appears unused for now, so I guess I don't have a strong opinion. > #define SA_NOMASK SA_NODEFER > #define SA_ONESHOT SA_RESETHAND > diff --git a/kernel/signal.c b/kernel/signal.c > index f802c82c7bcc..c80e70bde11d 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3984,6 +3984,12 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) > if (oact) > *oact = *k; > > + /* > + * Make sure that we never accidentally claim to support SA_UNSUPPORTED, > + * e.g. by having an architecture use the bit in their uapi. > + */ > + BUILD_BUG_ON(UAPI_SA_FLAGS & SA_UNSUPPORTED); > + Seems reasonable. With the above rewording in the commit message to clarify that a second sigaction() is needed: Reviewed-by: Dave Martin <Dave.Martin@arm.com> [...] Cheers ---Dave
On Tue, Sep 8, 2020 at 8:13 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Fri, Aug 21, 2020 at 10:10:14PM -0700, Peter Collingbourne wrote: > > Nit: no statement of the chage being made (other than in the subject > line). Will fix. > > This bit will never be supported in the uapi. The purpose of this flag > > bit is to allow userspace to distinguish an old kernel that does not > > clear unknown sa_flags bits from a kernel that supports every flag bit. > > > > In other words, if userspace finds that this bit remains set in > > oldact.sa_flags, it means that the kernel cannot be trusted to have > > cleared unknown flag bits from sa_flags, so no assumptions about flag > > bit support can be made. > > This isn't quite right? After a single sigaction() call, oact will > contain the sa_flags for the previously registered handler. So a > second sigaction() call would be needed to find out the newly effective > sa_flags. You're right, this is unclear to say the least. In v11 I will reword like so: In other words, if userspace does something like: act.sa_flags |= SA_UNSUPPORTED; sigaction(SIGSEGV, &act, 0); sigaction(SIGSEGV, 0, &oldact); and finds that SA_UNSUPPORTED remains set in oldact.sa_flags, it means that the kernel cannot be trusted to have cleared unknown flag bits from sa_flags, so no assumptions about flag bit support can be made. > > > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > --- > > View this change in Gerrit: https://linux-review.googlesource.com/q/Ic2501ad150a3a79c1cf27fb8c99be342e9dffbcb > > > > include/uapi/asm-generic/signal-defs.h | 7 +++++++ > > kernel/signal.c | 6 ++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/include/uapi/asm-generic/signal-defs.h b/include/uapi/asm-generic/signal-defs.h > > index 319628058a53..e853cbe8722d 100644 > > --- a/include/uapi/asm-generic/signal-defs.h > > +++ b/include/uapi/asm-generic/signal-defs.h > > @@ -14,6 +14,12 @@ > > * SA_RESTART flag to get restarting signals (which were the default long ago) > > * SA_NODEFER prevents the current signal from being masked in the handler. > > * SA_RESETHAND clears the handler when the signal is delivered. > > + * SA_UNSUPPORTED is a flag bit that will never be supported. Kernels from > > + * before the introduction of SA_UNSUPPORTED did not clear unknown bits from > > + * sa_flags when read using the oldact argument to sigaction and rt_sigaction, > > + * so this bit allows flag bit support to be detected from userspace while > > + * allowing an old kernel to be distinguished from a kernel that supports every > > + * flag bit. > > * > > * SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single > > * Unix names RESETHAND and NODEFER respectively. > > @@ -42,6 +48,7 @@ > > #ifndef SA_RESETHAND > > #define SA_RESETHAND 0x80000000 > > #endif > > +#define SA_UNSUPPORTED 0x00000400 > > I guess people may debate which bit is chosen, but your consolidation > of these definitions should help to reduce the possibility of future > collisions. This bit appears unused for now, so I guess I don't have a > strong opinion. > > > #define SA_NOMASK SA_NODEFER > > #define SA_ONESHOT SA_RESETHAND > > diff --git a/kernel/signal.c b/kernel/signal.c > > index f802c82c7bcc..c80e70bde11d 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -3984,6 +3984,12 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) > > if (oact) > > *oact = *k; > > > > + /* > > + * Make sure that we never accidentally claim to support SA_UNSUPPORTED, > > + * e.g. by having an architecture use the bit in their uapi. > > + */ > > + BUILD_BUG_ON(UAPI_SA_FLAGS & SA_UNSUPPORTED); > > + > > Seems reasonable. > > With the above rewording in the commit message to clarify that a second > sigaction() is needed: > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> Thanks for the review. Peter
On Wed, Oct 07, 2020 at 07:21:25PM -0700, Peter Collingbourne wrote: > On Tue, Sep 8, 2020 at 8:13 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Fri, Aug 21, 2020 at 10:10:14PM -0700, Peter Collingbourne wrote: > > > > Nit: no statement of the chage being made (other than in the subject > > line). > > Will fix. > > > > This bit will never be supported in the uapi. The purpose of this flag > > > bit is to allow userspace to distinguish an old kernel that does not > > > clear unknown sa_flags bits from a kernel that supports every flag bit. > > > > > > In other words, if userspace finds that this bit remains set in > > > oldact.sa_flags, it means that the kernel cannot be trusted to have > > > cleared unknown flag bits from sa_flags, so no assumptions about flag > > > bit support can be made. > > > > This isn't quite right? After a single sigaction() call, oact will > > contain the sa_flags for the previously registered handler. So a > > second sigaction() call would be needed to find out the newly effective > > sa_flags. > > You're right, this is unclear to say the least. In v11 I will reword like so: > > In other words, if userspace does something like: > > act.sa_flags |= SA_UNSUPPORTED; > sigaction(SIGSEGV, &act, 0); > sigaction(SIGSEGV, 0, &oldact); > > and finds that SA_UNSUPPORTED remains set in oldact.sa_flags, it means > that the kernel cannot be trusted to have cleared unknown flag bits > from sa_flags, so no assumptions about flag bit support can be made. Seems reasonable. We'd need to make sure we're clear about which flags this applies for, though. The pre-existing flags can be assumed to be supported irrespective of whether SA_UNSUPPORTED remains set. Cheers ---Dave
diff --git a/include/uapi/asm-generic/signal-defs.h b/include/uapi/asm-generic/signal-defs.h index 319628058a53..e853cbe8722d 100644 --- a/include/uapi/asm-generic/signal-defs.h +++ b/include/uapi/asm-generic/signal-defs.h @@ -14,6 +14,12 @@ * SA_RESTART flag to get restarting signals (which were the default long ago) * SA_NODEFER prevents the current signal from being masked in the handler. * SA_RESETHAND clears the handler when the signal is delivered. + * SA_UNSUPPORTED is a flag bit that will never be supported. Kernels from + * before the introduction of SA_UNSUPPORTED did not clear unknown bits from + * sa_flags when read using the oldact argument to sigaction and rt_sigaction, + * so this bit allows flag bit support to be detected from userspace while + * allowing an old kernel to be distinguished from a kernel that supports every + * flag bit. * * SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single * Unix names RESETHAND and NODEFER respectively. @@ -42,6 +48,7 @@ #ifndef SA_RESETHAND #define SA_RESETHAND 0x80000000 #endif +#define SA_UNSUPPORTED 0x00000400 #define SA_NOMASK SA_NODEFER #define SA_ONESHOT SA_RESETHAND diff --git a/kernel/signal.c b/kernel/signal.c index f802c82c7bcc..c80e70bde11d 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3984,6 +3984,12 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) if (oact) *oact = *k; + /* + * Make sure that we never accidentally claim to support SA_UNSUPPORTED, + * e.g. by having an architecture use the bit in their uapi. + */ + BUILD_BUG_ON(UAPI_SA_FLAGS & SA_UNSUPPORTED); + /* * 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
This bit will never be supported in the uapi. The purpose of this flag bit is to allow userspace to distinguish an old kernel that does not clear unknown sa_flags bits from a kernel that supports every flag bit. In other words, if userspace finds that this bit remains set in oldact.sa_flags, it means that the kernel cannot be trusted to have cleared unknown flag bits from sa_flags, so no assumptions about flag bit support can be made. Signed-off-by: Peter Collingbourne <pcc@google.com> --- View this change in Gerrit: https://linux-review.googlesource.com/q/Ic2501ad150a3a79c1cf27fb8c99be342e9dffbcb include/uapi/asm-generic/signal-defs.h | 7 +++++++ kernel/signal.c | 6 ++++++ 2 files changed, 13 insertions(+)