| Message ID | 3581410892be6851d804bbbb84fccf06073f1262.1605235762.git.pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: expose FAR_EL1 tag bits in siginfo | expand |
Peter Collingbourne <pcc@google.com> writes: > Define a sa_flags bit, SA_UNSUPPORTED, which 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 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> > Reviewed-by: Dave Martin <Dave.Martin@arm.com> > Link: https://linux-review.googlesource.com/id/Ic2501ad150a3a79c1cf27fb8c99be342e9dffbcb > --- > v11: > - clarify the commit message > > 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 493953fe319b..0126ebda4d31 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 Why this value and why not in numerical order with the other flags? At the very least not being in order with the other bits makes it a little easier to overlook it and define something at that position. Eric > #define SA_NOMASK SA_NODEFER > #define SA_ONESHOT SA_RESETHAND > diff --git a/kernel/signal.c b/kernel/signal.c > index 8f5bd12ee41b..8f34819e80de 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3985,6 +3985,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
On Sat, Nov 14, 2020 at 5:53 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Peter Collingbourne <pcc@google.com> writes: > > > Define a sa_flags bit, SA_UNSUPPORTED, which 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 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> > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> > > Link: https://linux-review.googlesource.com/id/Ic2501ad150a3a79c1cf27fb8c99be342e9dffbcb > > --- > > v11: > > - clarify the commit message > > > > 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 493953fe319b..0126ebda4d31 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 > > Why this value and why not in numerical order with the other flags? > > At the very least not being in order with the other bits makes it > a little easier to overlook it and define something at that position. The value is because this is the first bit that isn't already taken by an architecture-specific flag bit. It seems okay to move it into numerical order. The taken flag bits are listed in the comment that I added in patch 3. Do you think there would be a more prominent way to document them? Maybe we can replace that comment with inline, in-order comments along the lines of: #ifndef SA_NOCLDSTOP #define SA_NOCLDSTOP 0x00000001 #endif #ifndef SA_NOCLDWAIT #define SA_NOCLDWAIT 0x00000002 #endif #ifndef SA_SIGINFO #define SA_SIGINFO 0x00000004 #endif /* 0x00000008 has arch-specific definition */ /* 0x00000010 has arch-specific definition */ etc. And then this patch would add the new bit in the right place. Peter > > Eric > > > > #define SA_NOMASK SA_NODEFER > > #define SA_ONESHOT SA_RESETHAND > > diff --git a/kernel/signal.c b/kernel/signal.c > > index 8f5bd12ee41b..8f34819e80de 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -3985,6 +3985,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
Peter Collingbourne <pcc@google.com> writes: > On Sat, Nov 14, 2020 at 5:53 AM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Peter Collingbourne <pcc@google.com> writes: >> >> > Define a sa_flags bit, SA_UNSUPPORTED, which 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 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> >> > Reviewed-by: Dave Martin <Dave.Martin@arm.com> >> > Link: https://linux-review.googlesource.com/id/Ic2501ad150a3a79c1cf27fb8c99be342e9dffbcb >> > --- >> > v11: >> > - clarify the commit message >> > >> > 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 493953fe319b..0126ebda4d31 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 >> >> Why this value and why not in numerical order with the other flags? >> >> At the very least not being in order with the other bits makes it >> a little easier to overlook it and define something at that position. > > The value is because this is the first bit that isn't already taken by > an architecture-specific flag bit. It seems okay to move it into > numerical order. > > The taken flag bits are listed in the comment that I added in patch 3. > Do you think there would be a more prominent way to document them? > Maybe we can replace that comment with inline, in-order comments along > the lines of: > > #ifndef SA_NOCLDSTOP > #define SA_NOCLDSTOP 0x00000001 > #endif > #ifndef SA_NOCLDWAIT > #define SA_NOCLDWAIT 0x00000002 > #endif > #ifndef SA_SIGINFO > #define SA_SIGINFO 0x00000004 > #endif > /* 0x00000008 has arch-specific definition */ > /* 0x00000010 has arch-specific definition */ > > etc. > > And then this patch would add the new bit in the right place. Yes. That seems like it would be easier to think about. If you could also add the architectures that use those bits that would be helpful. Then if someone wants to look and see what is going on they need to look, and as a bunus in the if in event those architectures are deleted we would know that the bit no longer need to be reserved. Eric p.s. Apologies for some reason I did not see this reply earlier.
diff --git a/include/uapi/asm-generic/signal-defs.h b/include/uapi/asm-generic/signal-defs.h index 493953fe319b..0126ebda4d31 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 8f5bd12ee41b..8f34819e80de 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3985,6 +3985,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