[PATCH v9 3/6] signal: clear non-uapi flag bits when passing/returning sa_flags

[Peter Collingbourne]

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