[PATCH v18] arm64: expose FAR_EL1 tag bits in siginfo

[Peter Collingbourne]

On Tue, Nov 17, 2020 at 1:37 PM Eric W. Biederman <ebiederm at xmission.com> wrote:
>
> Peter Collingbourne <pcc at google.com> writes:
>
> > diff --git a/include/uapi/asm-generic/signal-defs.h b/include/uapi/asm-generic/signal-defs.h
> > index c790f67304ba..fe929e7b77ca 100644
> > --- a/include/uapi/asm-generic/signal-defs.h
> > +++ b/include/uapi/asm-generic/signal-defs.h
> > @@ -20,6 +20,8 @@
> >   * 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_EXPOSE_TAGBITS exposes an architecture-defined set of tag bits in
> > + * siginfo.si_addr.
>
> Do we perhaps want to say the high bits of si_addr?

I wouldn't want to make any specific claims about which bits of
si_addr are involved here since they would not necessarily be the high
bits. For example, imagine a software implementation of tagged
addresses that works by constructing a fake top-level page table such
that it contains N identical references to what from the kernel's
perspective would be the true top-level page table. Assuming that your
architecture does not give you the full 64 bits of address space to
use with page tables your tag bits would be somewhere in the middle.

> >   *
> >   * SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single
> >   * Unix names RESETHAND and NODEFER respectively.
> > @@ -41,6 +43,7 @@
> >  /* 0x00000100 used on sparc */
> >  /* 0x00000200 used on sparc */
> >  #define SA_UNSUPPORTED       0x00000400
> > +#define SA_EXPOSE_TAGBITS    0x00000800
> >  /* 0x00010000 used on mips */
> >  /* 0x01000000 used on x86 */
> >  /* 0x02000000 used on x86 */
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 8f34819e80de..576de3d9bd86 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2658,6 +2658,26 @@ bool get_signal(struct ksignal *ksig)
> >
> >               ka = &sighand->action[signr-1];
> >
> > +             if (!(ka->sa.sa_flags & SA_EXPOSE_TAGBITS)) {
> > +                     switch (siginfo_layout(signr, ksig->info.si_code)) {
> > +                     case SIL_FAULT:
> > +                     case SIL_FAULT_MCEERR:
> > +                     case SIL_FAULT_BNDERR:
> > +                     case SIL_FAULT_PKUERR:
> > +                             ksig->info.si_addr = arch_untagged_si_addr(
> > +                                     ksig->info.si_addr, signr,
> > +                                     ksig->info.si_code);
> > +                             break;
> > +                     case SIL_KILL:
> > +                     case SIL_TIMER:
> > +                     case SIL_POLL:
> > +                     case SIL_CHLD:
> > +                     case SIL_RT:
> > +                     case SIL_SYS:
> > +                             break;
> > +                     }
> > +             }
> > +
> >               /* Trace actually delivered signals. */
> >               trace_signal_deliver(signr, &ksig->info, ka);
>
> Overall this looks good.
>
> It is a common path so I wonder about the generated code, and how close
> to a noop this becomes on x86 and other architetures without tag bits.

I would in general expect compilers to be able to optimize all of this
away on non-arm64 architectures because the compiler has enough
information to know that the code isn't doing anything. Indeed I
compiled the kernel for x86_64 with and without this change and the
size of kernel/signal.o was the same.

> Can you put this blob of code in it's own function (perhaps called
> hide_si_addr_tag_bits) and call it right after "ksig->sig = signr" line?
>
> Effectively that is the same place in the code but it gets it outside of
> the sighand lock.  The tracing code does not care as it does not look at
> si_addr.
>
> I am concerned with the complexity of reading get_signal and using a
> well named inline function should make it unnecessary to read that
> code unless you care about what is going on.
>
> Further having the code outside of the lock at the end of get_signal
> with nothing else going on seems much easier to reason about.  The code
> is get_signal is something that needs reading and reasoning about.

Okay, done in v19.

Peter