On Mon, Nov 16, 2020 at 4:57 AM Dave Martin <Dave.Martin@xxxxxxx> wrote: > > On Fri, Nov 13, 2020 at 05:41:32PM -0800, Peter Collingbourne wrote: > > --- > > These features are implemented in this patch series: > > https://lore.kernel.org/linux-arm-kernel/cover.1605235762.git.pcc@xxxxxxxxxx/ > > which is still under review, so the patch should not be applied > > yet. > > Good to see this -- I was vaguely intending to write something when the > patch landed, so this saves me a job :) > > > man2/sigaction.2 | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/man2/sigaction.2 b/man2/sigaction.2 > > index 6a8142324..82fb69e26 100644 > > --- a/man2/sigaction.2 > > +++ b/man2/sigaction.2 > > @@ -250,6 +250,19 @@ This flag is meaningful only when establishing a signal handler. > > .\" .I sa_sigaction > > .\" field was added in Linux 2.1.86.) > > .\" > > +.TP > > +.BR SA_UNSUPPORTED " (since Linux 5.x)" > > +This flag bit will never be supported by the kernel. It is used as > > +part of the flag support detection protocol described below. > > It is supported, just with a special behaviour that only affects the > sa_flags field of oact. > > It's also a bit wried to say that SA_UNSUPPORTED is supported "since some > specific version of Linux." If we define the special behaviour for this > flag as "being supported", then saying "since Linux 5.x" just works > naturally. If we define SA_UNSUPPORTED using something like the language that you suggest below, maybe we can retroactively consider SA_UNSUPPORTED to have been supported since the beginning of time, so we don't need a "since" clause here. > Can we make a concise statement here about what the flag does and how to > use it? > > It's preferable not to bury specification details in wordy rationale > sections unless there's really no way to describe the behaviour > concisely. > > Maybe: --8<-- > > Check for extended sa_flags support. > > If the attempt to register a handler with this flag set in > act->sa_flags succeeds, and an immediately subsequent sigaction() call > specifying the same signal number n and with non-NULL oact yields > SA_UNSUPPORTED > .I clear > in oact->sa_flags, then extended sa_flags are supported. > > If the handler registration attempt fails, or if the subsequent > sigaction() call yields SA_UNSUPPORTED > .I set > in oact->sa_flags, then the caller must not assume that extended > sa_flags are supported. > > -->8-- I'm not sure about saying "extended sa_flags are supported" here since it could be read to mean that all of the extended sa_flags are supported, which won't be accurate if we add a new sa_flag in the future for example (but I'm not sure that we should talk about 'extended sa_flags" anyway, see below). I would just say that what this flag allows you to do is dynamically probe for flag bit support. I've taken your wording, tried to make it more concise and inserted it here. > Also, would it be a good idea to add something like the following? > > --8<-- > > This flag may be specified alongside extended sa_flags whose meaning > depends on the result of the check. Since the behaviour of the signal > handler cannot be guaranteed unless the check passes, it is wise to > block the affected signal while registering the handler and performing > the check in this case. > > -->8-- That would be good to mention in the "Dynamically probing for flag bit support" section, so I've done it. The language needed to be adjusted because you can't always block a synchronous signal such as SIGSEGV, so I recommend issuing the second sigaction in the signal handler itself in that case. > > > Also, can we reference the "Detecting flag support in sa_flags" by name? > It may be hard for people to find otherwise... Done. > > > > +.TP > > +.BR SA_EXPOSE_TAGBITS " (since Linux 5.x)" > > Shouldn't we mention the probe requirement here? > > --8<-- > > Requires extended sa_flags support; otherwise, the effect of this flag > is unpecified. See <section> [or SA_UNSUPPORTED if we think that's the > more concise description to refer to] for details. I wouldn't say "unspecified" here. In non-supporting kernels, the behavior is as if the flag is not set, and I've documented it in the "Dynamically probing for flag bit support" section. I'd probably leave this part as short as possible (just a reference to SA_UNSUPPORTED) because it will be copy-pasted into the description of any new flags. > > -->8-- > > > +Normally, when delivering a signal, an architecture-specific > > +set of tag bits are cleared from the > > Maybe "address tag bits", to make it clearer which bits we're talking > about. (But you're about to mention si_addr anyway, so perhaps this > clarification is redundant.) I think it's redundant, so I prefer to leave it as is. > > +.I si_addr > > +field of > > +.IR siginfo_t . > > +If this flag is set, the tag bits will be preserved in > > Should we say "any available tag bits" or "an architecture-specific > subset of the tag bits" or similar? > > This would cover us for the case where some arch can't always provide > them all (which I think is the case for MTE on arm64? ... but my memory > is hazy). Right, SIGSEGV/SEGV_MTESERR is the case where we don't have all the bits. I updated this to say that it's architecture specific. > > +.IR si_addr . > > .SS The siginfo_t argument to a SA_SIGINFO handler > > When the > > .B SA_SIGINFO > > @@ -833,6 +846,55 @@ Triggered by a > > .BR seccomp (2) > > filter rule. > > .RE > > +.SS Detecting flag support in sa_flags > > (Could be renamed to "Extended sa_flags support" if we want to be more > concise.) I'm not sure about calling these "extended sa_flags". Maybe in 10 years from now, some programs will be able to rely on SA_EXPOSE_TAGBITS being supported, so from that perspective it would be the same as any other flag and not really an "extended" flag. > > +The Linux kernel supports a mechanism for programs to detect kernel > > +support for > > +.B SA_* > > +flags in > > +.IR sa_flags . > > +This mechanism is quite subtle for backwards compatibility reasons > > +related to the historical behavior of the kernel. > > Can we avoid the word "subtle"? To me, this suggests a euphemism for > "doesn't work reliably", or "nobody knows whether it works". Okay, I removed this part. > Being more explicit about the actual reason for this design might help, > since the reason is actually fairly simple. Maybe: > > --8<-- > > Historically, the sigaction(2) call on Linux accepted unknown bits set > in act->sa_flags without error, and a second sigaction() call would > typically leave those bits set in oact->sa_flags. > > This means that support for new flags cannot be detected simply by > setting a flag in sa_flags. > > -->8-- Yes, something like this would work. I reworded it to describe the old and new behavior in the same paragraph. > > > + > > +Starting with Linux 5.x, the kernel will clear any unrecognized bits > > +from the > > +.I sa_flags > > +value returned via > > +.I oldact > > +if those bits were set when the signal handler was originally installed. > > +Therefore, a program that only needs to be compatible with Linux 5.x > > +and above may test for flag bit support by issuing a second call to > > +.BR sigaction () > > +and testing whether the bit remains set in > > +.IR oldact.sa_flags . > > + > > +Prior to Linux 5.x, unrecognized flag bits were preserved in > > +.I oldact.sa_flags > > +so this protocol on its own would not be sufficient to allow a > > +userspace program to test for flag bit support if it needs to be > > +compatible with kernel versions older than 5.x. Therefore, the > > +.B SA_UNSUPPORTED > > +flag bit was defined, which the kernel will always consider to be > > +unknown. A userspace program that sets this flag bit in > > +.I act.sa_flags > > +and finds that it has been cleared in > > +.I oldact.sa_flags > > +in a subsequent call to > > +.BR sigaction () > > +may trust that any other unknown flag bits have been cleared. > > Is this a bit too much rationale? I don't think we have to justify the > choice of design here -- describing the design and hinting at the reason > for it seems enough. > > If you go with my suggestions for the SA_UNSUPPORTED section, then we > can probably thin out some duplicate details here. Okay, I removed most of this. > > + > > +A reasonably modern program may trust that the flags > > What does "reasonably modern" mean? By this I meant a program that may assume that it is running on Linux 2.6 or above. I will drop the "reasonably modern" part and say "in general, programs may assume support". > > +.BR SA_NOCLDSTOP , > > +.BR SA_NOCLDWAIT , > > +.BR SA_SIGINFO , > > +.BR SA_ONSTACK , > > +.BR SA_RESTART , > > +.BR SA_NODEFER , > > +.B SA_RESETHAND > > +and, if defined by the architecture, > > +.B SA_RESTORER > > +are supported by the kernel, without relying on the flag bit support > > +detection protocol, since these flags have all been supported > > +since Linux 2.6. > > I think it's best just to leave these unspecified, as today. Software > seems to get on just fine without SA_UNSUPPORTED for these. It's probably not useful to document this from the perspective of "you don't need to probe for these", but I think we should say something about these flags, since the new documentation may otherwise mislead developers into thinking that they might want to probe for the old flags (either because they don't think the flags are that old or just out of a desire to follow best practices) which would give them the wrong answer on old kernels, which they may never notice if the kernel on their machine is new enough. I've reworded to document from the perspective of "you can't reliably probe for these", and only then say that you don't normally need to probe for them. > Also, can we have a code example here? The description may leave people > scratching their heads a bit, so I think a basic implementation would > help. That seems like a good idea. I've added one. Peter
