siginfo_t ABI break on sparc64 from si_addr_lsb move 3y ago
diff mbox series

Message ID YIpkvGrBFGlB5vNj@elver.google.com
State New, archived
Headers show
Series
  • siginfo_t ABI break on sparc64 from si_addr_lsb move 3y ago
Related show

Commit Message

Marco Elver April 29, 2021, 7:48 a.m. UTC
Hello,  Eric,

By inspecting the logs I've seen that about 3 years ago there had been a
number of siginfo_t cleanups. This included moving si_addr_lsb:

	b68a68d3dcc1 ("signal: Move addr_lsb into the _sigfault union for clarity")
	859d880cf544 ("signal: Correct the offset of si_pkey in struct siginfo")
 	8420f71943ae ("signal: Correct the offset of si_pkey and si_lower in struct siginfo on m68k")

In an ideal world, we could just have si_addr + the union in _sigfault,
but it seems there are more corner cases. :-/

The reason I've stumbled upon this is that I wanted to add the just
merged si_perf [1] field to glibc. But what I noticed is that glibc's
definition and ours are vastly different around si_addr_lsb, si_lower,
si_upper, and si_pkey.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=42dec9a936e7696bea1f27d3c5a0068cd9aa95fd

In our current definition of siginfo_t, si_addr_lsb is placed into the
same union as si_lower, si_upper, and si_pkey (and now si_perf). From
the logs I see that si_lower, si_upper, and si_pkey are padded because
si_addr_lsb used to be outside the union, which goes back to
"signal: Move addr_lsb into the _sigfault union for clarity".

Since then, si_addr_lsb must also be pointer-aligned, because the union
containing it must be pointer-aligned (because si_upper, si_lower). On
all architectures where si_addr_lsb is right after si_addr, this is
perfectly fine, because si_addr itself is a pointer...

... except for the anomaly that are 64-bit architectures that define
__ARCH_SI_TRAPNO and want that 'int si_trapno'. Like, for example
sparc64, which means siginfo_t's ABI has been subtly broken on sparc64
since v4.16.

The following static asserts illustrate this:

Comments

Eric W. Biederman April 29, 2021, 5:23 p.m. UTC | #1
Marco Elver <elver@google.com> writes:

> Hello,  Eric,
>
> By inspecting the logs I've seen that about 3 years ago there had been a
> number of siginfo_t cleanups. This included moving si_addr_lsb:
>
> 	b68a68d3dcc1 ("signal: Move addr_lsb into the _sigfault union for clarity")
> 	859d880cf544 ("signal: Correct the offset of si_pkey in struct siginfo")
>  	8420f71943ae ("signal: Correct the offset of si_pkey and si_lower in struct siginfo on m68k")
>
> In an ideal world, we could just have si_addr + the union in _sigfault,
> but it seems there are more corner cases. :-/
>
> The reason I've stumbled upon this is that I wanted to add the just
> merged si_perf [1] field to glibc. But what I noticed is that glibc's
> definition and ours are vastly different around si_addr_lsb, si_lower,
> si_upper, and si_pkey.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=42dec9a936e7696bea1f27d3c5a0068cd9aa95fd
>
> In our current definition of siginfo_t, si_addr_lsb is placed into the
> same union as si_lower, si_upper, and si_pkey (and now si_perf). From
> the logs I see that si_lower, si_upper, and si_pkey are padded because
> si_addr_lsb used to be outside the union, which goes back to
> "signal: Move addr_lsb into the _sigfault union for clarity".
>
> Since then, si_addr_lsb must also be pointer-aligned, because the union
> containing it must be pointer-aligned (because si_upper, si_lower). On
> all architectures where si_addr_lsb is right after si_addr, this is
> perfectly fine, because si_addr itself is a pointer...
>
> ... except for the anomaly that are 64-bit architectures that define
> __ARCH_SI_TRAPNO and want that 'int si_trapno'. Like, for example
> sparc64, which means siginfo_t's ABI has been subtly broken on sparc64
> since v4.16.
>
> The following static asserts illustrate this:
>
> --- a/arch/sparc/kernel/signal_64.c
> +++ b/arch/sparc/kernel/signal_64.c
> @@ -556,3 +556,37 @@ void do_notify_resume(struct pt_regs *regs, unsigned long orig_i0, unsigned long
>  	user_enter();
>  }
>  
> +static_assert(offsetof(siginfo_t, si_signo)	== 0);
> +static_assert(offsetof(siginfo_t, si_errno)	== 4);
> +static_assert(offsetof(siginfo_t, si_code)	== 8);
> +static_assert(offsetof(siginfo_t, si_pid)	== 16);
> +static_assert(offsetof(siginfo_t, si_uid)	== 20);
> +static_assert(offsetof(siginfo_t, si_tid)	== 16);
> +static_assert(offsetof(siginfo_t, si_overrun)	== 20);
> +static_assert(offsetof(siginfo_t, si_status)	== 24);
> +static_assert(offsetof(siginfo_t, si_utime)	== 32);
> +static_assert(offsetof(siginfo_t, si_stime)	== 40);
> +static_assert(offsetof(siginfo_t, si_value)	== 24);
> +static_assert(offsetof(siginfo_t, si_int)	== 24);
> +static_assert(offsetof(siginfo_t, si_ptr)	== 24);
> +static_assert(offsetof(siginfo_t, si_addr)	== 16);
> +static_assert(offsetof(siginfo_t, si_trapno)	== 24);
> +#if 1 /* Correct offsets, obtained from v4.14 */
> +static_assert(offsetof(siginfo_t, si_addr_lsb)	== 28);
> +static_assert(offsetof(siginfo_t, si_lower)	== 32);
> +static_assert(offsetof(siginfo_t, si_upper)	== 40);
> +static_assert(offsetof(siginfo_t, si_pkey)	== 32);
> +#else /* Current offsets, as of v4.16 */
> +static_assert(offsetof(siginfo_t, si_addr_lsb)	== 32);
> +static_assert(offsetof(siginfo_t, si_lower)	== 40);
> +static_assert(offsetof(siginfo_t, si_upper)	== 48);
> +static_assert(offsetof(siginfo_t, si_pkey)	== 40);
> +#endif
> +static_assert(offsetof(siginfo_t, si_band)	== 16);
> +static_assert(offsetof(siginfo_t, si_fd)	== 20);
>
> ---
>
> Granted, nobody seems to have noticed because I don't even know if these
> fields have use on sparc64. But I don't yet see this as justification to
> leave things as-is...
>
> The collateral damage of this, and the acute problem that I'm having is
> defining si_perf in a sort-of readable and portable way in siginfo_t
> definitions that live outside the kernel, where sparc64 does not yet
> have broken si_addr_lsb. And the same difficulty applies to the kernel
> if we want to unbreak sparc64, while not wanting to move si_perf for
> other architectures.
>
> There are 2 options I see to solve this:
>
> 1. Make things simple again. We could just revert the change moving
>    si_addr_lsb into the union, and sadly accept we'll have to live with
>    that legacy "design" mistake. (si_perf stays in the union, but will
>    unfortunately change its offset for all architectures... this one-off
>    move might be ok because it's new.)
>
> 2. Add special cases to retain si_addr_lsb in the union on architectures
>    that do not have __ARCH_SI_TRAPNO (the majority). I have added a
>    draft patch that would do this below (with some refactoring so that
>    it remains sort-of readable), as an experiment to see how complicated
>    this gets.
>
> Which option do you prefer? Are there better options?

Personally the most important thing to have is a single definition
shared by all architectures so that we consolidate testing.

A little piece of me cries a little whenever I see how badly we
implemented the POSIX design.  As specified by POSIX the fields can be
place in siginfo such that 32bit and 64bit share a common definition.
Unfortunately we did not addpadding after si_addr on 32bit to
accommodate a 64bit si_addr.

I find it unfortunate that we are adding yet another definition that
requires translation between 32bit and 64bit, but I am glad
that at least the translation is not architecture specific.  That common
definition is what has allowed this potential issue to be caught
and that makes me very happy to see.

Let's go with Option 3.

Confirm BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR are not
in use on any architecture that defines __ARCH_SI_TRAPNO, and then fixup
the userspace definitions of these fields.

To the kernel I would add some BUILD_BUG_ON's to whatever the best
maintained architecture (sparc64?) that implements __ARCH_SI_TRAPNO just
to confirm we don't create future regressions by accident.

I did a quick search and the architectures that define __ARCH_SI_TRAPNO
are sparc, mips, and alpha.  All have 64bit implementations.  A further
quick search shows that none of those architectures have faults that
use BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR, nor do
they appear to use mm/memory-failure.c

So it doesn't look like we have an ABI regression to fix.

Eric
Marco Elver April 29, 2021, 6:46 p.m. UTC | #2
On Thu, 29 Apr 2021 at 19:24, Eric W. Biederman <ebiederm@xmission.com> wrote:
[...]
> > Granted, nobody seems to have noticed because I don't even know if these
> > fields have use on sparc64. But I don't yet see this as justification to
> > leave things as-is...
> >
> > The collateral damage of this, and the acute problem that I'm having is
> > defining si_perf in a sort-of readable and portable way in siginfo_t
> > definitions that live outside the kernel, where sparc64 does not yet
> > have broken si_addr_lsb. And the same difficulty applies to the kernel
> > if we want to unbreak sparc64, while not wanting to move si_perf for
> > other architectures.
> >
> > There are 2 options I see to solve this:
> >
> > 1. Make things simple again. We could just revert the change moving
> >    si_addr_lsb into the union, and sadly accept we'll have to live with
> >    that legacy "design" mistake. (si_perf stays in the union, but will
> >    unfortunately change its offset for all architectures... this one-off
> >    move might be ok because it's new.)
> >
> > 2. Add special cases to retain si_addr_lsb in the union on architectures
> >    that do not have __ARCH_SI_TRAPNO (the majority). I have added a
> >    draft patch that would do this below (with some refactoring so that
> >    it remains sort-of readable), as an experiment to see how complicated
> >    this gets.
> >
> > Which option do you prefer? Are there better options?
>
> Personally the most important thing to have is a single definition
> shared by all architectures so that we consolidate testing.
>
> A little piece of me cries a little whenever I see how badly we
> implemented the POSIX design.  As specified by POSIX the fields can be
> place in siginfo such that 32bit and 64bit share a common definition.
> Unfortunately we did not addpadding after si_addr on 32bit to
> accommodate a 64bit si_addr.

I think it's even worse than that, see the fun I had with siginfo last
week: https://lkml.kernel.org/r/20210422191823.79012-1-elver@google.com
... because of the 3 initial ints and no padding after them, we can't
portably add __u64 fields to siginfo, and are forever forced to have
subtly different behaviour between 32-bit and 64-bit architectures.
:-/

> I find it unfortunate that we are adding yet another definition that
> requires translation between 32bit and 64bit, but I am glad
> that at least the translation is not architecture specific.  That common
> definition is what has allowed this potential issue to be caught
> and that makes me very happy to see.
>
> Let's go with Option 3.
>
> Confirm BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR are not
> in use on any architecture that defines __ARCH_SI_TRAPNO, and then fixup
> the userspace definitions of these fields.
>
> To the kernel I would add some BUILD_BUG_ON's to whatever the best
> maintained architecture (sparc64?) that implements __ARCH_SI_TRAPNO just
> to confirm we don't create future regressions by accident.
>
> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
> are sparc, mips, and alpha.  All have 64bit implementations.  A further
> quick search shows that none of those architectures have faults that
> use BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR, nor do
> they appear to use mm/memory-failure.c
>
> So it doesn't look like we have an ABI regression to fix.

That sounds fine to me -- my guess was that they're not used on these
architectures, but I just couldn't make that call.

I have patches adding compile-time asserts for sparc64, arm, arm64
ready to go. I'll send them after some more testing.

Thanks,
-- Marco
Arnd Bergmann April 29, 2021, 8:48 p.m. UTC | #3
On Thu, Apr 29, 2021 at 7:23 PM Eric W. Biederman <ebiederm@xmission.com> wrote:

> > Which option do you prefer? Are there better options?
>
> Personally the most important thing to have is a single definition
> shared by all architectures so that we consolidate testing.
>
> A little piece of me cries a little whenever I see how badly we
> implemented the POSIX design.  As specified by POSIX the fields can be
> place in siginfo such that 32bit and 64bit share a common definition.
> Unfortunately we did not addpadding after si_addr on 32bit to
> accommodate a 64bit si_addr.
>
> I find it unfortunate that we are adding yet another definition that
> requires translation between 32bit and 64bit, but I am glad
> that at least the translation is not architecture specific.  That common
> definition is what has allowed this potential issue to be caught
> and that makes me very happy to see.
>
> Let's go with Option 3.
>
> Confirm BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR are not
> in use on any architecture that defines __ARCH_SI_TRAPNO, and then fixup
> the userspace definitions of these fields.
>
> To the kernel I would add some BUILD_BUG_ON's to whatever the best
> maintained architecture (sparc64?) that implements __ARCH_SI_TRAPNO just
> to confirm we don't create future regressions by accident.
>
> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
> are sparc, mips, and alpha.  All have 64bit implementations.

I think you (slightly) misread: mips has "#undef __ARCH_SI_TRAPNO", not
"#define __ARCH_SI_TRAPNO". This means it's only sparc and
alpha.

I can see that the alpha instance was added to the kernel during linux-2.5,
but never made it into the glibc or uclibc copy of the struct definition, and
musl doesn't support alpha or sparc. Debian codesearch only turns up
sparc (and BSD) references to si_trapno.

> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
> are sparc, mips, and alpha.  All have 64bit implementations.  A further
> quick search shows that none of those architectures have faults that
> use BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR, nor do
> they appear to use mm/memory-failure.c
>
> So it doesn't look like we have an ABI regression to fix.

Even better!

So if sparc is the only user of _trapno and it uses none of the later
fields in _sigfault, I wonder if we could take even more liberty at
trying to have a slightly saner definition. Can you think of anything that
might break if we put _trapno inside of the union along with _perf
and _addr_lsb?

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.

       Arnd
Eric W. Biederman April 30, 2021, 5:08 p.m. UTC | #4
Arnd Bergmann <arnd@arndb.de> writes:

> On Thu, Apr 29, 2021 at 7:23 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> > Which option do you prefer? Are there better options?
>>
>> Personally the most important thing to have is a single definition
>> shared by all architectures so that we consolidate testing.
>>
>> A little piece of me cries a little whenever I see how badly we
>> implemented the POSIX design.  As specified by POSIX the fields can be
>> place in siginfo such that 32bit and 64bit share a common definition.
>> Unfortunately we did not addpadding after si_addr on 32bit to
>> accommodate a 64bit si_addr.
>>
>> I find it unfortunate that we are adding yet another definition that
>> requires translation between 32bit and 64bit, but I am glad
>> that at least the translation is not architecture specific.  That common
>> definition is what has allowed this potential issue to be caught
>> and that makes me very happy to see.
>>
>> Let's go with Option 3.
>>
>> Confirm BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR are not
>> in use on any architecture that defines __ARCH_SI_TRAPNO, and then fixup
>> the userspace definitions of these fields.
>>
>> To the kernel I would add some BUILD_BUG_ON's to whatever the best
>> maintained architecture (sparc64?) that implements __ARCH_SI_TRAPNO just
>> to confirm we don't create future regressions by accident.
>>
>> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
>> are sparc, mips, and alpha.  All have 64bit implementations.
>
> I think you (slightly) misread: mips has "#undef __ARCH_SI_TRAPNO", not
> "#define __ARCH_SI_TRAPNO". This means it's only sparc and
> alpha.
>
> I can see that the alpha instance was added to the kernel during linux-2.5,
> but never made it into the glibc or uclibc copy of the struct definition, and
> musl doesn't support alpha or sparc. Debian codesearch only turns up
> sparc (and BSD) references to si_trapno.



>> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
>> are sparc, mips, and alpha.  All have 64bit implementations.  A further
>> quick search shows that none of those architectures have faults that
>> use BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR, nor do
>> they appear to use mm/memory-failure.c
>>
>> So it doesn't look like we have an ABI regression to fix.
>
> Even better!
>
> So if sparc is the only user of _trapno and it uses none of the later
> fields in _sigfault, I wonder if we could take even more liberty at
> trying to have a slightly saner definition. Can you think of anything that
> might break if we put _trapno inside of the union along with _perf
> and _addr_lsb?

On sparc si_trapno is only set when SIGILL ILL_TRP is set.  So we can
limit si_trapno to that combination, and it should not be a problem for
a new signal/si_code pair to use that storage.  Precisely because it is
new.

Similarly on alpha si_trapno is only set for:

SIGFPE {FPE_INTOVF, FPE_INTDIV, FPE_FLTOVF, FPE_FLTDIV, FPE_FLTUND,
FPE_FLTINV, FPE_FLTRES, FPE_FLTUNK} and SIGTRAP {TRAP_UNK}.

Placing si_trapno into the union would also make the problem that the
union is pointer aligned a non-problem as then the union immediate
follows a pointer.

I hadn't had a chance to look before but we must deal with this.  The
definition of perf_sigtrap in 42dec9a936e7696bea1f27d3c5a0068cd9aa95fd
is broken on sparc, alpha, and ia64 as it bypasses the code in
kernel/signal.c that ensures the si_trapno or the ia64 special fields
are set.

Not to mention that perf_sigtrap appears to abuse si_errno.

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.

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

Eric
Marco Elver April 30, 2021, 7:07 p.m. UTC | #5
On Fri, Apr 30, 2021 at 12:08PM -0500, Eric W. Biederman wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
[...] 
> >> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
> >> are sparc, mips, and alpha.  All have 64bit implementations.  A further
> >> quick search shows that none of those architectures have faults that
> >> use BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR, nor do
> >> they appear to use mm/memory-failure.c
> >>
> >> So it doesn't look like we have an ABI regression to fix.
> >
> > Even better!
> >
> > So if sparc is the only user of _trapno and it uses none of the later
> > fields in _sigfault, I wonder if we could take even more liberty at
> > trying to have a slightly saner definition. Can you think of anything that
> > might break if we put _trapno inside of the union along with _perf
> > and _addr_lsb?
> 
> On sparc si_trapno is only set when SIGILL ILL_TRP is set.  So we can
> limit si_trapno to that combination, and it should not be a problem for
> a new signal/si_code pair to use that storage.  Precisely because it is
> new.
> 
> Similarly on alpha si_trapno is only set for:
> 
> SIGFPE {FPE_INTOVF, FPE_INTDIV, FPE_FLTOVF, FPE_FLTDIV, FPE_FLTUND,
> FPE_FLTINV, FPE_FLTRES, FPE_FLTUNK} and SIGTRAP {TRAP_UNK}.
> 
> Placing si_trapno into the union would also make the problem that the
> union is pointer aligned a non-problem as then the union immediate
> follows a pointer.
> 
> I hadn't had a chance to look before but we must deal with this.  The
> definition of perf_sigtrap in 42dec9a936e7696bea1f27d3c5a0068cd9aa95fd
> is broken on sparc, alpha, and ia64 as it bypasses the code in
> kernel/signal.c that ensures the si_trapno or the ia64 special fields
> are set.
> 
> Not to mention that perf_sigtrap appears to abuse si_errno.

There are a few other places in the kernel that repurpose si_errno
similarly, e.g. arch/arm64/kernel/ptrace.c, kernel/seccomp.c -- it was
either that or introduce another field or not have it. It is likely we
could do without, but if there are different event types the user would
have to sacrifice a few bits of si_perf to encode the event type, and
I'd rather keep those bits for something else. Thus the decision fell to
use si_errno.

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.

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

> > 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?

Thanks,
-- Marco
Eric W. Biederman April 30, 2021, 8:15 p.m. UTC | #6
Marco Elver <elver@google.com> writes:

> On Fri, Apr 30, 2021 at 12:08PM -0500, Eric W. Biederman wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
> [...] 
>> >> I did a quick search and the architectures that define __ARCH_SI_TRAPNO
>> >> are sparc, mips, and alpha.  All have 64bit implementations.  A further
>> >> quick search shows that none of those architectures have faults that
>> >> use BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, SEGV_PKUERR, nor do
>> >> they appear to use mm/memory-failure.c
>> >>
>> >> So it doesn't look like we have an ABI regression to fix.
>> >
>> > Even better!
>> >
>> > So if sparc is the only user of _trapno and it uses none of the later
>> > fields in _sigfault, I wonder if we could take even more liberty at
>> > trying to have a slightly saner definition. Can you think of anything that
>> > might break if we put _trapno inside of the union along with _perf
>> > and _addr_lsb?
>> 
>> On sparc si_trapno is only set when SIGILL ILL_TRP is set.  So we can
>> limit si_trapno to that combination, and it should not be a problem for
>> a new signal/si_code pair to use that storage.  Precisely because it is
>> new.
>> 
>> Similarly on alpha si_trapno is only set for:
>> 
>> SIGFPE {FPE_INTOVF, FPE_INTDIV, FPE_FLTOVF, FPE_FLTDIV, FPE_FLTUND,
>> FPE_FLTINV, FPE_FLTRES, FPE_FLTUNK} and SIGTRAP {TRAP_UNK}.
>> 
>> Placing si_trapno into the union would also make the problem that the
>> union is pointer aligned a non-problem as then the union immediate
>> follows a pointer.
>> 
>> I hadn't had a chance to look before but we must deal with this.  The
>> definition of perf_sigtrap in 42dec9a936e7696bea1f27d3c5a0068cd9aa95fd
>> is broken on sparc, alpha, and ia64 as it bypasses the code in
>> kernel/signal.c that ensures the si_trapno or the ia64 special fields
>> are set.
>> 
>> Not to mention that perf_sigtrap appears to abuse si_errno.
>
> There are a few other places in the kernel that repurpose si_errno
> similarly, e.g. arch/arm64/kernel/ptrace.c, kernel/seccomp.c -- it was
> either that or introduce another field or not have it. It is likely we
> could do without, but if there are different event types the user would
> have to sacrifice a few bits of si_perf to encode the event type, and
> I'd rather keep those bits for something else. Thus the decision fell to
> use si_errno.

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

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

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.

>> 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.
>
>> > 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
Arnd Bergmann April 30, 2021, 8:43 p.m. UTC | #7
On Fri, Apr 30, 2021 at 7:08 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> 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.
>
> > 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.

Ok.

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

ia64 never had perf support. It had oprofile until very recently, and it
had a custom thing before that. My feeling is that it's increasingly
unlikely to ever gain perf support in the future, given that oprofile
(in user space) has required kernel perf support (in kernel) for a
long time and nobody cared about that being broken either.

sparc64 has perf support for Sun UltraSPARC 3/3+/3i/4+/T1/T2/T3
and Oracle SPARC T4/T5/M7 but lacks support for most CPUs from
Oracle, Fujitsu and the rest, in particular anything from the last
ten years.
Alpha has perf support for EV67, EV68, EV7, EV79, and EV69, i.e.
anything from 1996 to the end in 2004.

      Arnd
Marco Elver April 30, 2021, 11:50 p.m. UTC | #8
On Fri, 30 Apr 2021 at 22:15, Eric W. Biederman <ebiederm@xmission.com> wrote:
[...]
> arm64 only abuses si_errno in compat code for bug compatibility with
> arm32.
>
> > 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.
>
> >> 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.
> >
> >> > 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
>

Patch
diff mbox series

--- a/arch/sparc/kernel/signal_64.c
+++ b/arch/sparc/kernel/signal_64.c
@@ -556,3 +556,37 @@  void do_notify_resume(struct pt_regs *regs, unsigned long orig_i0, unsigned long
 	user_enter();
 }
 
+static_assert(offsetof(siginfo_t, si_signo)	== 0);
+static_assert(offsetof(siginfo_t, si_errno)	== 4);
+static_assert(offsetof(siginfo_t, si_code)	== 8);
+static_assert(offsetof(siginfo_t, si_pid)	== 16);
+static_assert(offsetof(siginfo_t, si_uid)	== 20);
+static_assert(offsetof(siginfo_t, si_tid)	== 16);
+static_assert(offsetof(siginfo_t, si_overrun)	== 20);
+static_assert(offsetof(siginfo_t, si_status)	== 24);
+static_assert(offsetof(siginfo_t, si_utime)	== 32);
+static_assert(offsetof(siginfo_t, si_stime)	== 40);
+static_assert(offsetof(siginfo_t, si_value)	== 24);
+static_assert(offsetof(siginfo_t, si_int)	== 24);
+static_assert(offsetof(siginfo_t, si_ptr)	== 24);
+static_assert(offsetof(siginfo_t, si_addr)	== 16);
+static_assert(offsetof(siginfo_t, si_trapno)	== 24);
+#if 1 /* Correct offsets, obtained from v4.14 */
+static_assert(offsetof(siginfo_t, si_addr_lsb)	== 28);
+static_assert(offsetof(siginfo_t, si_lower)	== 32);
+static_assert(offsetof(siginfo_t, si_upper)	== 40);
+static_assert(offsetof(siginfo_t, si_pkey)	== 32);
+#else /* Current offsets, as of v4.16 */
+static_assert(offsetof(siginfo_t, si_addr_lsb)	== 32);
+static_assert(offsetof(siginfo_t, si_lower)	== 40);
+static_assert(offsetof(siginfo_t, si_upper)	== 48);
+static_assert(offsetof(siginfo_t, si_pkey)	== 40);
+#endif
+static_assert(offsetof(siginfo_t, si_band)	== 16);
+static_assert(offsetof(siginfo_t, si_fd)	== 20);

---

Granted, nobody seems to have noticed because I don't even know if these
fields have use on sparc64. But I don't yet see this as justification to
leave things as-is...

The collateral damage of this, and the acute problem that I'm having is
defining si_perf in a sort-of readable and portable way in siginfo_t
definitions that live outside the kernel, where sparc64 does not yet
have broken si_addr_lsb. And the same difficulty applies to the kernel
if we want to unbreak sparc64, while not wanting to move si_perf for
other architectures.

There are 2 options I see to solve this:

1. Make things simple again. We could just revert the change moving
   si_addr_lsb into the union, and sadly accept we'll have to live with
   that legacy "design" mistake. (si_perf stays in the union, but will
   unfortunately change its offset for all architectures... this one-off
   move might be ok because it's new.)

2. Add special cases to retain si_addr_lsb in the union on architectures
   that do not have __ARCH_SI_TRAPNO (the majority). I have added a
   draft patch that would do this below (with some refactoring so that
   it remains sort-of readable), as an experiment to see how complicated
   this gets.

Option (1) means we'll forever be wasting the space where si_addr_lsb
lives (including the padding). It'd also mean we'd move si_perf for
_all_ architectures -- this might be acceptable, given there is no
stable release with it yet -- the fix just needs to be merged before the
release of v5.13! It is the simpler option though -- and I don't know if
we need all this complexity.

Option (2) perhaps results in better space utilization. Maybe that's
better long-term if we worry about space in some rather distant future
-- where we need those 8 bytes on 64-bit architectures to not exceed 128
bytes. This option, however, doesn't just make us carry this complexity
forever, but also forces it onto everyone else, like glibc and other
libcs (including those in other languages with C FFIs) which have their
own definition of siginfo_t.

Which option do you prefer? Are there better options?

Many thanks,
-- Marco

------ >8 ------

Option #2 draft:

diff --git a/arch/sparc/kernel/signal_64.c b/arch/sparc/kernel/signal_64.c
index a0eec62c825d..150ee27b1423 100644
--- a/arch/sparc/kernel/signal_64.c
+++ b/arch/sparc/kernel/signal_64.c
@@ -556,3 +556,37 @@  void do_notify_resume(struct pt_regs *regs, unsigned long orig_i0, unsigned long
 	user_enter();
 }
 
+/*
+ * Compile-time assertions for siginfo_t offsets. Unlike other architectures,
+ * sparc64 is special, because it requires si_trapno (int), and the following
+ * si_addr_lsb (short) need not be word aligned. Accidental changes around the
+ * offset of si_addr_lsb and the following fields would only be caught here.
+ */
+static_assert(offsetof(siginfo_t, si_signo)	== 0);
+static_assert(offsetof(siginfo_t, si_errno)	== 4);
+static_assert(offsetof(siginfo_t, si_code)	== 8);
+static_assert(offsetof(siginfo_t, si_pid)	== 16);
+static_assert(offsetof(siginfo_t, si_uid)	== 20);
+static_assert(offsetof(siginfo_t, si_tid)	== 16);
+static_assert(offsetof(siginfo_t, si_overrun)	== 20);
+static_assert(offsetof(siginfo_t, si_status)	== 24);
+static_assert(offsetof(siginfo_t, si_utime)	== 32);
+static_assert(offsetof(siginfo_t, si_stime)	== 40);
+static_assert(offsetof(siginfo_t, si_value)	== 24);
+static_assert(offsetof(siginfo_t, si_int)	== 24);
+static_assert(offsetof(siginfo_t, si_ptr)	== 24);
+static_assert(offsetof(siginfo_t, si_addr)	== 16);
+static_assert(offsetof(siginfo_t, si_trapno)	== 24);
+#if 1 /* Correct offsets, obtained from v4.14 */
+static_assert(offsetof(siginfo_t, si_addr_lsb)	== 28);
+static_assert(offsetof(siginfo_t, si_lower)	== 32);
+static_assert(offsetof(siginfo_t, si_upper)	== 40);
+static_assert(offsetof(siginfo_t, si_pkey)	== 32);
+#else /* Current offsets, as of v4.16 */
+static_assert(offsetof(siginfo_t, si_addr_lsb)	== 32);
+static_assert(offsetof(siginfo_t, si_lower)	== 40);
+static_assert(offsetof(siginfo_t, si_upper)	== 48);
+static_assert(offsetof(siginfo_t, si_pkey)	== 40);
+#endif
+static_assert(offsetof(siginfo_t, si_band)	== 16);
+static_assert(offsetof(siginfo_t, si_fd)	== 20);
diff --git a/include/linux/compat.h b/include/linux/compat.h
index f0d2dd35d408..5ea9f9c748dd 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -158,6 +158,31 @@  typedef union compat_sigval {
 	compat_uptr_t	sival_ptr;
 } compat_sigval_t;
 
+struct __compat_sigfault_addin {
+#ifdef __ARCH_SI_TRAPNO
+	int _trapno;	/* TRAP # which caused the signal */
+#endif
+	/*
+	 * used when si_code=BUS_MCEERR_AR or
+	 * used when si_code=BUS_MCEERR_AO
+	 */
+	short int _addr_lsb;	/* Valid LSB of the reported address. */
+
+/* See include/asm-generic/uapi/siginfo.h */
+#ifdef __ARCH_SI_TRAPNO
+#	define __COMPAT_SIGFAULT_ADDIN_FIXED struct __compat_sigfault_addin _addin;
+#	define __COMPAT_SIGFAULT_ADDIN_UNION
+#	define __COMPAT_SIGFAULT_LEGACY_UNION_PAD
+#else
+#	define __COMPAT_SIGFAULT_ADDIN_FIXED
+#	define __COMPAT_SIGFAULT_ADDIN_UNION struct __compat_sigfault_addin _addin;
+#	define __COMPAT_SIGFAULT_LEGACY_UNION_PAD				\
+		char _unused[__alignof__(compat_uptr_t) < sizeof(short) ?       \
+				   sizeof(short) :				\
+				   __alignof__(compat_uptr_t)];
+#endif
+};
+
 typedef struct compat_siginfo {
 	int si_signo;
 #ifndef __ARCH_HAS_SWAPPED_SIGINFO
@@ -214,26 +239,18 @@  typedef struct compat_siginfo {
 		/* SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT */
 		struct {
 			compat_uptr_t _addr;	/* faulting insn/memory ref. */
-#ifdef __ARCH_SI_TRAPNO
-			int _trapno;	/* TRAP # which caused the signal */
-#endif
-#define __COMPAT_ADDR_BND_PKEY_PAD  (__alignof__(compat_uptr_t) < sizeof(short) ? \
-				     sizeof(short) : __alignof__(compat_uptr_t))
+			__COMPAT_SIGFAULT_ADDIN_FIXED
 			union {
-				/*
-				 * used when si_code=BUS_MCEERR_AR or
-				 * used when si_code=BUS_MCEERR_AO
-				 */
-				short int _addr_lsb;	/* Valid LSB of the reported address. */
+				__COMPAT_SIGFAULT_ADDIN_UNION
 				/* used when si_code=SEGV_BNDERR */
 				struct {
-					char _dummy_bnd[__COMPAT_ADDR_BND_PKEY_PAD];
+					__COMPAT_SIGFAULT_LEGACY_UNION_PAD
 					compat_uptr_t _lower;
 					compat_uptr_t _upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
 				struct {
-					char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD];
+					__COMPAT_SIGFAULT_LEGACY_UNION_PAD
 					u32 _pkey;
 				} _addr_pkey;
 				/* used when si_code=TRAP_PERF */
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index 03d6f6d2c1fe..f1c1a0300ac8 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -29,6 +29,45 @@  typedef union sigval {
 #define __ARCH_SI_ATTRIBUTES
 #endif
 
+/*
+ * The _sigfault portion of __sifields after si_addr varies depending on
+ * architecture; capture these rules here.
+ */
+struct __sifields_sigfault_addin {
+#ifdef __ARCH_SI_TRAPNO
+	int _trapno;	/* TRAP # which caused the signal */
+#endif
+	/*
+	 * used when si_code=BUS_MCEERR_AR or
+	 * used when si_code=BUS_MCEERR_AO
+	 */
+	short _addr_lsb; /* LSB of the reported address */
+
+#if defined(__ARCH_SI_TRAPNO)
+/*
+ * If we have si_trapno between si_addr and si_addr_lsb, we cannot safely move
+ * it inside the union due to alignment of si_trapno+si_addr_lsb vs. the union.
+ */
+#	define __SI_SIGFAULT_ADDIN_FIXED struct __sifields_sigfault_addin _addin;
+#	define __SI_SIGFAULT_ADDIN_UNION
+#	define __SI_SIGFAULT_LEGACY_UNION_PAD
+#else
+/*
+ * Safe to move si_addr_lsb inside the union. We will benefit from better space
+ * usage for new fields added to the union.
+ *
+ * Fields that were added after si_addr_lsb, before it become part of the union,
+ * require padding to retain the ABI. New fields do not require padding.
+ */
+#	define __SI_SIGFAULT_ADDIN_FIXED
+#	define __SI_SIGFAULT_ADDIN_UNION struct __sifields_sigfault_addin _addin;
+#	define __SI_SIGFAULT_LEGACY_UNION_PAD				\
+		char _unused[__alignof__(void *) < sizeof(short) ?	\
+					   sizeof(short) :		\
+					   __alignof__(void *)];
+#endif
+};
+
 union __sifields {
 	/* kill() */
 	struct {
@@ -63,32 +102,23 @@  union __sifields {
 	/* SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT */
 	struct {
 		void __user *_addr; /* faulting insn/memory ref. */
-#ifdef __ARCH_SI_TRAPNO
-		int _trapno;	/* TRAP # which caused the signal */
-#endif
 #ifdef __ia64__
 		int _imm;		/* immediate value for "break" */
 		unsigned int _flags;	/* see ia64 si_flags */
 		unsigned long _isr;	/* isr */
 #endif
-
-#define __ADDR_BND_PKEY_PAD  (__alignof__(void *) < sizeof(short) ? \
-			      sizeof(short) : __alignof__(void *))
+		__SI_SIGFAULT_ADDIN_FIXED
 		union {
-			/*
-			 * used when si_code=BUS_MCEERR_AR or
-			 * used when si_code=BUS_MCEERR_AO
-			 */
-			short _addr_lsb; /* LSB of the reported address */
+			__SI_SIGFAULT_ADDIN_UNION
 			/* used when si_code=SEGV_BNDERR */
 			struct {
-				char _dummy_bnd[__ADDR_BND_PKEY_PAD];
+				__SI_SIGFAULT_LEGACY_UNION_PAD
 				void __user *_lower;
 				void __user *_upper;
 			} _addr_bnd;
 			/* used when si_code=SEGV_PKUERR */
 			struct {
-				char _dummy_pkey[__ADDR_BND_PKEY_PAD];
+				__SI_SIGFAULT_LEGACY_UNION_PAD
 				__u32 _pkey;
 			} _addr_pkey;
 			/* used when si_code=TRAP_PERF */
@@ -151,9 +181,9 @@  typedef struct siginfo {
 #define si_ptr		_sifields._rt._sigval.sival_ptr
 #define si_addr		_sifields._sigfault._addr
 #ifdef __ARCH_SI_TRAPNO
-#define si_trapno	_sifields._sigfault._trapno
+#define si_trapno	_sifields._sigfault._addin._trapno
 #endif
-#define si_addr_lsb	_sifields._sigfault._addr_lsb
+#define si_addr_lsb	_sifields._sigfault._addin._addr_lsb
 #define si_lower	_sifields._sigfault._addr_bnd._lower
 #define si_upper	_sifields._sigfault._addr_bnd._upper
 #define si_pkey		_sifields._sigfault._addr_pkey._pkey