Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: Marco Elver <elver@google.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Florian Weimer <fweimer@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Collingbourne <pcc@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Alexander Potapenko <glider@google.com>,
	sparclinux <sparclinux@vger.kernel.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	kasan-dev <kasan-dev@googlegroups.com>
Subject: Re: siginfo_t ABI break on sparc64 from si_addr_lsb move 3y ago
Date: Fri, 30 Apr 2021 21:07:06 +0200
Message-ID: <YIxVWkT03TqcJLY3@elver.google.com> (raw)
In-Reply-To: <m15z031z0a.fsf@fess.ebiederm.org>

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

  reply index

Thread overview: 118+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-29  7:48 Marco Elver
2021-04-29 17:23 ` Eric W. Biederman
2021-04-29 18:46   ` Marco Elver
2021-04-29 20:48   ` Arnd Bergmann
2021-04-30 17:08     ` Eric W. Biederman
2021-04-30 19:07       ` Marco Elver [this message]
2021-04-30 20:15         ` Eric W. Biederman
2021-04-30 23:50           ` Marco Elver
2021-04-30 22:49         ` [RFC][PATCH 0/3] signal: Move si_trapno into the _si_fault union Eric W. Biederman
2021-04-30 22:50           ` [PATCH 1/3] siginfo: Move si_trapno inside the union inside _si_fault Eric W. Biederman
2021-05-01 10:31             ` Marco Elver
2021-05-02 18:27               ` Eric W. Biederman
2021-04-30 22:54           ` [PATCH 2/3] signal: Implement SIL_FAULT_TRAPNO Eric W. Biederman
2021-04-30 23:19             ` Eric W. Biederman
2021-05-01 10:33             ` Marco Elver
2021-05-02 18:24               ` Eric W. Biederman
2021-04-30 22:55           ` [PATCH 3/3] signal: Use dedicated helpers to send signals with si_trapno set Eric W. Biederman
2021-05-01 10:33             ` Marco Elver
2021-04-30 22:56           ` [PATCH 4/3] signal: Remove __ARCH_SI_TRAPNO Eric W. Biederman
2021-04-30 23:23           ` Is perf_sigtrap synchronous? Eric W. Biederman
2021-05-01  0:28             ` Marco Elver
2021-04-30 23:42           ` [PATCH 5/3] signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency Eric W. Biederman
2021-05-01 10:35             ` Marco Elver
2021-04-30 23:43           ` [PATCH 6/3] signal: Factor force_sig_perf out of perf_sigtrap Eric W. Biederman
2021-05-01 10:45             ` Marco Elver
2021-04-30 23:43           ` [PATCH 7/3] signal: Deliver all of the perf_data in si_perf Eric W. Biederman
2021-05-01 10:47             ` Marco Elver
2021-05-02 18:39               ` Eric W. Biederman
2021-05-02 19:13                 ` Marco Elver
2021-05-03 12:44                 ` Peter Zijlstra
2021-05-03 19:38                   ` Eric W. Biederman
2021-05-03 19:53                     ` Marco Elver
2021-04-30 23:47           ` [RFC][PATCH 0/3] signal: Move si_trapno into the _si_fault union Eric W. Biederman
2021-05-01  0:37             ` Marco Elver
2021-05-01 15:16               ` Eric W. Biederman
2021-05-01 16:24                 ` Marco Elver
2021-05-03 20:25                   ` [PATCH 00/12] signal: sort out si_trapno and si_perf Eric W. Biederman
2021-05-03 20:38                     ` [PATCH 01/12] sparc64: Add compile-time asserts for siginfo_t offsets Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 02/12] arm: " Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 03/12] arm64: " Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 04/12] siginfo: Move si_trapno inside the union inside _si_fault Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 05/12] signal: Implement SIL_FAULT_TRAPNO Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 06/12] signal: Use dedicated helpers to send signals with si_trapno set Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 07/12] signal: Remove __ARCH_SI_TRAPNO Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 08/12] signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 09/12] signal: Factor force_sig_perf out of perf_sigtrap Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 10/12] signal: Redefine signinfo so 64bit fields are possible Eric W. Beiderman
2021-05-03 21:04                         ` Eric W. Biederman
2021-05-03 22:47                           ` Marco Elver
2021-05-04  3:42                             ` Eric W. Biederman
2021-05-04  4:03                               ` Peter Collingbourne
2021-05-04  9:52                                 ` Marco Elver
2021-05-04 16:16                                   ` Eric W. Biederman
2021-05-03 20:38                       ` [PATCH 11/12] signal: Deliver all of the siginfo perf data in _perf Eric W. Beiderman
2021-05-03 20:38                       ` [PATCH 12/12] signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo Eric W. Beiderman
2021-05-04 21:13                     ` [PATCH v3 00/12] signal: sort out si_trapno and si_perf Eric W. Biederman
2021-05-04 22:05                       ` Marco Elver
2021-05-05 14:12                         ` Eric W. Biederman
2021-05-05 14:10                       ` [PATCH v3 01/12] sparc64: Add compile-time asserts for siginfo_t offsets Eric W. Beiderman
2021-05-05 14:10                         ` [PATCH v3 02/12] arm: " Eric W. Beiderman
2021-05-05 14:10                         ` [PATCH v3 03/12] arm64: " Eric W. Beiderman
2021-05-05 14:10                         ` [PATCH v3 04/12] signal: Verify the alignment and size of siginfo_t Eric W. Beiderman
2021-05-05 17:24                           ` Marco Elver
2021-05-05 14:10                         ` [PATCH v3 05/12] siginfo: Move si_trapno inside the union inside _si_fault Eric W. Beiderman
2021-05-05 14:10                         ` [PATCH v3 06/12] signal: Implement SIL_FAULT_TRAPNO Eric W. Beiderman
2021-05-05 17:25                           ` Marco Elver
2021-05-05 14:10                         ` [PATCH v3 07/12] signal: Use dedicated helpers to send signals with si_trapno set Eric W. Beiderman
2021-05-05 17:25                           ` Marco Elver
2021-05-05 14:10                         ` [PATCH v3 08/12] signal: Remove __ARCH_SI_TRAPNO Eric W. Beiderman
2021-05-05 17:25                           ` Marco Elver
2021-05-05 14:10                         ` [PATCH v3 09/12] signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency Eric W. Beiderman
2021-05-05 17:26                           ` Marco Elver
2021-05-05 14:10                         ` [PATCH v3 10/12] signal: Factor force_sig_perf out of perf_sigtrap Eric W. Beiderman
2021-05-05 17:26                           ` Marco Elver
2021-05-06 10:54                           ` Peter Zijlstra
2021-05-05 14:11                         ` [PATCH v3 11/12] signal: Deliver all of the siginfo perf data in _perf Eric W. Beiderman
2021-05-05 17:27                           ` Marco Elver
2021-05-05 14:11                         ` [PATCH v3 12/12] signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo Eric W. Beiderman
2021-05-05 17:27                           ` Marco Elver
2021-05-05 17:28                       ` [PATCH v3 00/12] signal: sort out si_trapno and si_perf Marco Elver
2021-05-06  7:00                       ` Geert Uytterhoeven
2021-05-06 10:43                         ` Marco Elver
2021-05-06 15:28                           ` Eric W. Biederman
2021-05-06 15:14                         ` Eric W. Biederman
2021-05-14  4:54                       ` [GIT PULL] siginfo: ABI fixes for v5.13-rc2 Eric W. Biederman
2021-05-14 19:14                         ` Linus Torvalds
2021-05-14 21:15                           ` Eric W. Biederman
2021-05-14 22:38                             ` Eric W. Biederman
2021-05-16  7:40                         ` Ingo Molnar
2021-05-17 15:29                           ` Eric W. Biederman
2021-05-21 14:59                         ` [GIT PULL] siginfo: ABI fixes for v5.13-rc3 Eric W. Biederman
2021-05-21 16:34                           ` pr-tracker-bot
2021-05-17 19:56                       ` [PATCH v4 0/5] siginfo: ABI fixes for TRAP_PERF Eric W. Biederman
2021-05-17 19:57                         ` [PATCH v4 1/5] siginfo: Move si_trapno inside the union inside _si_fault Eric W. Beiderman
2021-05-17 19:57                           ` [PATCH v4 2/5] signal: Implement SIL_FAULT_TRAPNO Eric W. Beiderman
2021-05-17 19:57                           ` [PATCH v4 3/5] signal: Factor force_sig_perf out of perf_sigtrap Eric W. Beiderman
2021-05-17 19:57                           ` [PATCH v4 4/5] signal: Deliver all of the siginfo perf data in _perf Eric W. Beiderman
2021-05-17 19:57                           ` [PATCH v4 5/5] signalfd: Remove SIL_PERF_EVENT fields from signalfd_siginfo Eric W. Beiderman
2021-05-17 20:53                         ` [PATCH v4 0/5] siginfo: ABI fixes for TRAP_PERF Marco Elver
2021-05-18  3:46                           ` Eric W. Biederman
2021-05-18  6:44                             ` Marco Elver
2021-05-01 16:26               ` [RFC][PATCH 0/3] signal: Move si_trapno into the _si_fault union Marco Elver
2021-07-15 18:09           ` [PATCH 0/6] Final si_trapno bits Eric W. Biederman
2021-07-15 18:11             ` [PATCH 1/6] sparc64: Add compile-time asserts for siginfo_t offsets Eric W. Biederman
2021-07-15 18:11             ` [PATCH 2/6] arm: " Eric W. Biederman
2021-07-15 18:11             ` [PATCH 3/6] arm64: " Eric W. Biederman
2021-07-15 18:12             ` [PATCH 4/6] signal/sparc: si_trapno is only used with SIGILL ILL_ILLTRP Eric W. Biederman
2021-07-16 11:49               ` Marco Elver
2021-07-15 18:12             ` [PATCH 5/6] signal/alpha: si_trapno is only used with SIGFPE and SIGTRAP TRAP_UNK Eric W. Biederman
2021-07-16 11:48               ` Marco Elver
2021-07-15 18:13             ` [PATCH 6/6] signal: Remove the generic __ARCH_SI_TRAPNO support Eric W. Biederman
2021-07-16 11:48               ` Marco Elver
2021-07-16 11:50             ` [PATCH 0/6] Final si_trapno bits Marco Elver
2021-07-16 16:08               ` Eric W. Biederman
2021-07-16 17:15                 ` Marco Elver
2021-07-16 16:06             ` [PATCH 7/7] signal: Verify the alignment and size of siginfo_t Eric W. Biederman
2021-07-16 16:07             ` [PATCH 8/6] signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency Eric W. Biederman
2021-04-30 20:43       ` siginfo_t ABI break on sparc64 from si_addr_lsb move 3y ago Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YIxVWkT03TqcJLY3@elver.google.com \
    --to=elver@google.com \
    --cc=arnd@arndb.de \
    --cc=davem@davemloft.net \
    --cc=dvyukov@google.com \
    --cc=ebiederm@xmission.com \
    --cc=fweimer@redhat.com \
    --cc=glider@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=pcc@google.com \
    --cc=peterz@infradead.org \
    --cc=sparclinux@vger.kernel.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git