On Mon, Aug 24, 2020 at 6:40 AM Dave Martin <Dave.Martin@xxxxxxx> 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@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.
>
> 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
