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

[Eric W. Biederman]

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?

>   *
>   * 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.

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.

Eric