Re: siginfo_t ABI break on sparc64 from si_addr_lsb move 3y ago

On Fri, 30 Apr 2021 at 22:15, Eric W. Biederman [off-list ref] wrote:
[...]

arm64 only abuses si_errno in compat code for bug compatibility with
arm32.

quoted

Given it'd be wasted space otherwise, and we define the semantics of
whatever is stored in siginfo on the new signal, it'd be good to keep.

Except you don't completely.  You are not defining a new signal.  You
are extending the definition of SIGTRAP.  Anything generic that
responds to all SIGTRAPs can reasonably be looking at si_errno.

I see where you're coming from, and agree with this if si_errno
already had some semantics for some subset of SIGTRAPs. I've tried to
analyze the situation a bit further, since siginfo seems to be a giant
minefield and semantics is underspecified at best. :-)

Do any of the existing SIGTRAPs define si_errno to be set? As far as I
can tell, they don't.

If this is true, I think there are benefits and downsides to
repurposing si_errno (similar to what SIGSYS did). The obvious
downside is as you suggest, it's not always a real errno. The benefit
is that we avoid introducing more and more fields -- i.e. if we permit
si_errno to be repurposed for SIGTRAP and its value depends on the
precise si_code, too, we simplify siginfo's overall definition (also
given every new field needs more code in kernel/signal.c, too).

Given SIGTRAPs are in response to some user-selected event in the
user's code (breakpoints, ptrace, etc. ... now perf events), the user
must already check the si_code to select the right action because
SIGTRAPs are not alike (unlike, e.g. SIGSEGV). Because of this, I
think that repurposing si_errno in an si_code-dependent way for
SIGTRAPs is safe.

If you think it is simply untenable to repurpose si_errno for
SIGTRAPs, please confirm -- I'll just send a minimal patch to fix (I'd
probably just remove setting it... everything else is too intrusive as
a "fix".. sigh).

The cleanups as you outline below seem orthogonal and not urgent for
5.13 (all changes and cleanups for __ARCH_SI_TRAPNO seem too intrusive
without -next exposure).

Thanks,
-- Marco

Further you are already adding a field with si_perf you can just as
easily add a second field with well defined semantics for that data.

quoted

quoted

The code is only safe if the analysis that says we can move si_trapno
and perhaps the ia64 fields into the union is correct.  It looks like
ia64 much more actively uses it's signal extension fields including for
SIGTRAP, so I am not at all certain the generic definition of
perf_sigtrap is safe on ia64.

Trying to understand the requirements of si_trapno myself: safe here
would mean that si_trapno is not required if we fire our SIGTRAP /
TRAP_PERF.

As far as I can tell that is the case -- see below.

quoted

quoted

I suppose in theory sparc64 or alpha might start using the other
fields in the future, and an application might be compiled against
mismatched headers, but that is unlikely and is already broken
with the current headers.

If we localize the use of si_trapno to just a few special cases on alpha
and sparc I think we don't even need to worry about breaking userspace
on any architecture.  It will complicate siginfo_layout, but it is a
complication that reflects reality.

I don't have a clue how any of this affects ia64.  Does perf work on
ia64?  Does perf work on sparc, and alpha?

If perf works on ia64 we need to take a hard look at what is going on
there as well.

No perf on ia64, but it seems alpha and sparc have perf:

      $ git grep 'select.*HAVE_PERF_EVENTS$' -- arch/
      arch/alpha/Kconfig:     select HAVE_PERF_EVENTS    <--
      arch/arc/Kconfig:       select HAVE_PERF_EVENTS
      arch/arm/Kconfig:       select HAVE_PERF_EVENTS
      arch/arm64/Kconfig:     select HAVE_PERF_EVENTS
      arch/csky/Kconfig:      select HAVE_PERF_EVENTS
      arch/hexagon/Kconfig:   select HAVE_PERF_EVENTS
      arch/mips/Kconfig:      select HAVE_PERF_EVENTS
      arch/nds32/Kconfig:     select HAVE_PERF_EVENTS
      arch/parisc/Kconfig:    select HAVE_PERF_EVENTS
      arch/powerpc/Kconfig:   select HAVE_PERF_EVENTS
      arch/riscv/Kconfig:     select HAVE_PERF_EVENTS
      arch/s390/Kconfig:      select HAVE_PERF_EVENTS
      arch/sh/Kconfig:        select HAVE_PERF_EVENTS
      arch/sparc/Kconfig:     select HAVE_PERF_EVENTS    <--
      arch/x86/Kconfig:       select HAVE_PERF_EVENTS
      arch/xtensa/Kconfig:    select HAVE_PERF_EVENTS

Now, given ia64 is not an issue, I wanted to understand the semantics of
si_trapno. Per https://man7.org/linux/man-pages/man2/sigaction.2.html, I
see:

      int si_trapno;    /* Trap number that caused
                           hardware-generated signal
                           (unused on most architectures) */

... its intended semantics seem to suggest it would only be used by some
architecture-specific signal like you identified above. So if the
semantics is some code of a hardware trap/fault, then we're fine and do
not need to set it.

Also bearing in mind we define the semantics any new signal, and given
most architectures do not have si_trapno, definitions of new generic
signals should probably not include odd architecture specific details
related to old architectures.

From all this, my understanding now is that we can move si_trapno into
the union, correct? What else did you have in mind?

Yes.  Let's move si_trapno into the union.

That implies a few things like siginfo_layout needs to change.

The helpers in kernel/signal.c can change to not imply that
if you define __ARCH_SI_TRAPNO you must always define and
pass in si_trapno.  A force_sig_trapno could be defined instead
to handle the cases that alpha and sparc use si_trapno.

It would be nice if a force_sig_perf_trap could be factored
out of perf_trap and placed in kernel/signal.c.

My experience (especially this round) is that it becomes much easier to
audit the users of siginfo if there is a dedicated function in
kernel/signal.c that is simply passed the parameters that need
to be placed in siginfo.

So I would very much like to see if I can make force_sig_info static.

Eric