LKML 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: [PATCH 10/12] signal: Redefine signinfo so 64bit fields are possible
Date: Tue, 4 May 2021 00:47:35 +0200
Message-ID: <CANpmjNNOK6Mkxkjx5nD-t-yPQ-oYtaW5Xui=hi3kpY_-Y0=2JA@mail.gmail.com> (raw)
In-Reply-To: <m1o8drfs1m.fsf@fess.ebiederm.org>

On Mon, 3 May 2021 at 23:04, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Eric W. Beiderman" <ebiederm@xmission.com> writes:
> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> >
> > The si_perf code really wants to add a u64 field.  This change enables
> > that by reorganizing the definition of siginfo_t, so that a 64bit
> > field can be added without increasing the alignment of other fields.

If you can, it'd be good to have an explanation for this, because it's
not at all obvious -- some future archeologist will wonder how we ever
came up with this definition of siginfo...

(I see the trick here is that before the union would have changed
alignment, introducing padding after the 3 ints -- but now because the
3 ints are inside the union the union's padding no longer adds padding
for these ints.  Perhaps you can explain it better than I can. Also
see below.)

> I decided to include this change because it is not that complicated,
> and it allows si_perf_data to have the definition that was originally
> desired.

Neat, and long-term I think this seems to be worth having. Sooner or
later something else might want __u64, too.

But right now, due to the slight increase in complexity, we need to
take extra care ensuring nothing broke -- at a high-level I see why
this seems to be safe.

> If this looks difficult to make in the userspace definitions,
> or is otherwise a problem I don't mind dropping this change.  I just
> figured since it was not too difficult and we are changing things
> anyway I should try for everything.

Languages that support inheritance might end up with the simpler
definition here (and depending on which fields they want to access,
they'd have to cast the base siginfo into the one they want). What
will become annoying is trying to describe siginfo_t.

I will run some tests in the morning.

Thanks,
-- Marco

[...]
> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> > index e663bf117b46..1fcede623a73 100644
> > --- a/include/uapi/asm-generic/siginfo.h
> > +++ b/include/uapi/asm-generic/siginfo.h
> > @@ -29,15 +29,33 @@ typedef union sigval {
> >  #define __ARCH_SI_ATTRIBUTES
> >  #endif
> >
> > +#ifndef __ARCH_HAS_SWAPPED_SIGINFO
> > +#define ___SIGINFO_COMMON    \
> > +     int     si_signo;       \
> > +     int     si_errno;       \
> > +     int     si_code
> > +#else
> > +#define ___SIGINFO_COMMON    \
> > +     int     si_signo;       \
> > +     int     si_code;        \
> > +     int     si_errno
> > +#endif /* __ARCH_HAS_SWAPPED_SIGINFO */
> > +
> > +#define __SIGINFO_COMMON     \
> > +     ___SIGINFO_COMMON;      \
> > +     int     _common_pad[__alignof__(void *) != __alignof(int)]

Just to verify my understanding of _common_pad: this is again a legacy
problem, right? I.e. if siginfo was designed from the start like this,
we wouldn't need the _common_pad.


While this looks quite daunting, this is effectively turning siginfo
from a struct with a giant union, into lots of smaller structs, each
of which share a common header a'la inheritance -- until now the
distinction didn't matter, but it starts to matter as soon as
alignment of one child-struct would affect the offsets of another
child-struct (i.e. the old version). Right?

I was wondering if it would make things look cleaner if the above was
encapsulated in a struct, say __sicommon? Then the outermost union
would use 'struct __sicommon _sicommon' and we need #defines for
si_signo, si_code, and si_errno.

Or would it break alignment somewhere?

I leave it to your judgement -- just an idea.

> >  union __sifields {
> >       /* kill() */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               __kernel_pid_t _pid;    /* sender's pid */
> >               __kernel_uid32_t _uid;  /* sender's uid */
> >       } _kill;
> >
> >       /* POSIX.1b timers */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               __kernel_timer_t _tid;  /* timer id */
> >               int _overrun;           /* overrun count */
> >               sigval_t _sigval;       /* same as below */
> > @@ -46,6 +64,7 @@ union __sifields {
> >
> >       /* POSIX.1b signals */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               __kernel_pid_t _pid;    /* sender's pid */
> >               __kernel_uid32_t _uid;  /* sender's uid */
> >               sigval_t _sigval;
> > @@ -53,6 +72,7 @@ union __sifields {
> >
> >       /* SIGCHLD */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               __kernel_pid_t _pid;    /* which child */
> >               __kernel_uid32_t _uid;  /* sender's uid */
> >               int _status;            /* exit code */
> > @@ -62,6 +82,7 @@ union __sifields {
> >
> >       /* SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               void __user *_addr; /* faulting insn/memory ref. */
> >  #ifdef __ia64__
> >               int _imm;               /* immediate value for "break" */
> > @@ -97,35 +118,28 @@ union __sifields {
> >
> >       /* SIGPOLL */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               __ARCH_SI_BAND_T _band; /* POLL_IN, POLL_OUT, POLL_MSG */
> >               int _fd;
> >       } _sigpoll;
> >
> >       /* SIGSYS */
> >       struct {
> > +             __SIGINFO_COMMON;
> >               void __user *_call_addr; /* calling user insn */
> >               int _syscall;   /* triggering system call number */
> >               unsigned int _arch;     /* AUDIT_ARCH_* of syscall */
> >       } _sigsys;
> >  };
> >
> > -#ifndef __ARCH_HAS_SWAPPED_SIGINFO
> > -#define __SIGINFO                    \
> > -struct {                             \
> > -     int si_signo;                   \
> > -     int si_errno;                   \
> > -     int si_code;                    \
> > -     union __sifields _sifields;     \
> > -}
> > -#else
> > +
> >  #define __SIGINFO                    \
> > -struct {                             \
> > -     int si_signo;                   \
> > -     int si_code;                    \
> > -     int si_errno;                   \
> > +union {                                      \
> > +     struct {                        \
> > +             __SIGINFO_COMMON;       \
> > +     };                              \
> >       union __sifields _sifields;     \
> >  }
> > -#endif /* __ARCH_HAS_SWAPPED_SIGINFO */
> >
> >  typedef struct siginfo {
> >       union {

  reply index

Thread overview: 103+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-29  7:48 siginfo_t ABI break on sparc64 from si_addr_lsb move 3y ago 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
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 [this message]
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-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='CANpmjNNOK6Mkxkjx5nD-t-yPQ-oYtaW5Xui=hi3kpY_-Y0=2JA@mail.gmail.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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

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

Example config snippet for mirrors

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


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