| Message ID | m1tuni8ano.fsf_-_@fess.ebiederm.org |
|---|---|
| Headers | show |
| Series | signal: sort out si_trapno and si_perf | expand |
On Tue, 4 May 2021 at 23:13, Eric W. Biederman <ebiederm@xmission.com> wrote: > > This set of changes sorts out the ABI issues with SIGTRAP TRAP_PERF, and > hopefully will can get merged before any userspace code starts using the > new ABI. > > The big ideas are: > - Placing the asserts first to prevent unexpected ABI changes > - si_trapno becomming ordinary fault subfield. > - struct signalfd_siginfo is almost full > > This set of changes starts out with Marco's static_assert changes and > additional one of my own that enforces the fact that the alignment of > siginfo_t is also part of the ABI. Together these build time > checks verify there are no unexpected ABI changes in the changes > that follow. > > The field si_trapno is changed to become an ordinary extension of the > _sigfault member of siginfo. > > The code is refactored a bit and then si_perf_type is added along side > si_perf_data in the _perf subfield of _sigfault of siginfo_t. > > Finally the signalfd_siginfo fields are removed as they appear to be > filling up the structure without userspace actually being able to use > them. > > v2: https://lkml.kernel.org/r/m14kfjh8et.fsf_-_@fess.ebiederm.org > v1: https://lkml.kernel.org/r/m1zgxfs7zq.fsf_-_@fess.ebiederm.org > > Eric W. Biederman (9): > signal: Verify the alignment and size of siginfo_t > siginfo: Move si_trapno inside the union inside _si_fault > signal: Implement SIL_FAULT_TRAPNO > signal: Use dedicated helpers to send signals with si_trapno set > signal: Remove __ARCH_SI_TRAPNO > signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency > signal: Factor force_sig_perf out of perf_sigtrap > signal: Deliver all of the siginfo perf data in _perf > signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo > > Marco Elver (3): > sparc64: Add compile-time asserts for siginfo_t offsets > arm: Add compile-time asserts for siginfo_t offsets > arm64: Add compile-time asserts for siginfo_t offsets I can't seem to see the rest of them in my inbox. LKML also is missing them: https://lore.kernel.org/linux-api/m1tuni8ano.fsf_-_@fess.ebiederm.org/ Something must have swallowed them. Could you resend? I'll then test in the morning. Thanks, -- Marco
Marco Elver <elver@google.com> writes: > On Tue, 4 May 2021 at 23:13, Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> This set of changes sorts out the ABI issues with SIGTRAP TRAP_PERF, and >> hopefully will can get merged before any userspace code starts using the >> new ABI. >> >> The big ideas are: >> - Placing the asserts first to prevent unexpected ABI changes >> - si_trapno becomming ordinary fault subfield. >> - struct signalfd_siginfo is almost full >> >> This set of changes starts out with Marco's static_assert changes and >> additional one of my own that enforces the fact that the alignment of >> siginfo_t is also part of the ABI. Together these build time >> checks verify there are no unexpected ABI changes in the changes >> that follow. >> >> The field si_trapno is changed to become an ordinary extension of the >> _sigfault member of siginfo. >> >> The code is refactored a bit and then si_perf_type is added along side >> si_perf_data in the _perf subfield of _sigfault of siginfo_t. >> >> Finally the signalfd_siginfo fields are removed as they appear to be >> filling up the structure without userspace actually being able to use >> them. >> >> v2: https://lkml.kernel.org/r/m14kfjh8et.fsf_-_@fess.ebiederm.org >> v1: https://lkml.kernel.org/r/m1zgxfs7zq.fsf_-_@fess.ebiederm.org >> >> Eric W. Biederman (9): >> signal: Verify the alignment and size of siginfo_t >> siginfo: Move si_trapno inside the union inside _si_fault >> signal: Implement SIL_FAULT_TRAPNO >> signal: Use dedicated helpers to send signals with si_trapno set >> signal: Remove __ARCH_SI_TRAPNO >> signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency >> signal: Factor force_sig_perf out of perf_sigtrap >> signal: Deliver all of the siginfo perf data in _perf >> signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo >> >> Marco Elver (3): >> sparc64: Add compile-time asserts for siginfo_t offsets >> arm: Add compile-time asserts for siginfo_t offsets >> arm64: Add compile-time asserts for siginfo_t offsets > > I can't seem to see the rest of them in my inbox. LKML also is missing > them: https://lore.kernel.org/linux-api/m1tuni8ano.fsf_-_@fess.ebiederm.org/ > > Something must have swallowed them. Could you resend? > I'll then test in the morning. They got stuck going out you should see them any time now. Sorry about that. Eric
On Tue, 4 May 2021 at 23:13, Eric W. Biederman <ebiederm@xmission.com> wrote: > This set of changes sorts out the ABI issues with SIGTRAP TRAP_PERF, and > hopefully will can get merged before any userspace code starts using the > new ABI. > > The big ideas are: > - Placing the asserts first to prevent unexpected ABI changes > - si_trapno becomming ordinary fault subfield. > - struct signalfd_siginfo is almost full > > This set of changes starts out with Marco's static_assert changes and > additional one of my own that enforces the fact that the alignment of > siginfo_t is also part of the ABI. Together these build time > checks verify there are no unexpected ABI changes in the changes > that follow. > > The field si_trapno is changed to become an ordinary extension of the > _sigfault member of siginfo. > > The code is refactored a bit and then si_perf_type is added along side > si_perf_data in the _perf subfield of _sigfault of siginfo_t. > > Finally the signalfd_siginfo fields are removed as they appear to be > filling up the structure without userspace actually being able to use > them. > > v2: https://lkml.kernel.org/r/m14kfjh8et.fsf_-_@fess.ebiederm.org > v1: https://lkml.kernel.org/r/m1zgxfs7zq.fsf_-_@fess.ebiederm.org > > Eric W. Biederman (9): > signal: Verify the alignment and size of siginfo_t > siginfo: Move si_trapno inside the union inside _si_fault > signal: Implement SIL_FAULT_TRAPNO > signal: Use dedicated helpers to send signals with si_trapno set > signal: Remove __ARCH_SI_TRAPNO > signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency > signal: Factor force_sig_perf out of perf_sigtrap > signal: Deliver all of the siginfo perf data in _perf > signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo > > Marco Elver (3): > sparc64: Add compile-time asserts for siginfo_t offsets > arm: Add compile-time asserts for siginfo_t offsets > arm64: Add compile-time asserts for siginfo_t offsets > > arch/alpha/include/uapi/asm/siginfo.h | 2 - > arch/alpha/kernel/osf_sys.c | 2 +- > arch/alpha/kernel/signal.c | 4 +- > arch/alpha/kernel/traps.c | 24 ++--- > arch/alpha/mm/fault.c | 4 +- > arch/arm/kernel/signal.c | 39 +++++++ > arch/arm64/kernel/signal.c | 39 +++++++ > arch/arm64/kernel/signal32.c | 39 +++++++ > arch/mips/include/uapi/asm/siginfo.h | 2 - > arch/sparc/include/uapi/asm/siginfo.h | 3 - > arch/sparc/kernel/process_64.c | 2 +- > arch/sparc/kernel/signal32.c | 37 +++++++ > arch/sparc/kernel/signal_64.c | 36 +++++++ > arch/sparc/kernel/sys_sparc_32.c | 2 +- > arch/sparc/kernel/sys_sparc_64.c | 2 +- > arch/sparc/kernel/traps_32.c | 22 ++-- > arch/sparc/kernel/traps_64.c | 44 ++++---- > arch/sparc/kernel/unaligned_32.c | 2 +- > arch/sparc/mm/fault_32.c | 2 +- > arch/sparc/mm/fault_64.c | 2 +- > arch/x86/kernel/signal_compat.c | 15 ++- > fs/signalfd.c | 23 ++--- > include/linux/compat.h | 10 +- > include/linux/sched/signal.h | 13 +-- > include/linux/signal.h | 3 +- > include/uapi/asm-generic/siginfo.h | 20 ++-- > include/uapi/linux/signalfd.h | 4 +- > kernel/events/core.c | 11 +- > kernel/signal.c | 113 +++++++++++++-------- > .../selftests/perf_events/sigtrap_threads.c | 12 +-- > 30 files changed, 373 insertions(+), 160 deletions(-) Looks good, thanks a lot! I ran selftests/perf_events on x86-64, and build-tested x86-32, arm, arm64, sparc, alpha. I added my Reviewed/Acked-by to the various patches. Thanks, -- Marco
Hi Eric, On Tue, May 4, 2021 at 11:14 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > This set of changes sorts out the ABI issues with SIGTRAP TRAP_PERF, and > hopefully will can get merged before any userspace code starts using the > new ABI. > > The big ideas are: > - Placing the asserts first to prevent unexpected ABI changes > - si_trapno becomming ordinary fault subfield. > - struct signalfd_siginfo is almost full > > This set of changes starts out with Marco's static_assert changes and > additional one of my own that enforces the fact that the alignment of > siginfo_t is also part of the ABI. Together these build time > checks verify there are no unexpected ABI changes in the changes > that follow. > > The field si_trapno is changed to become an ordinary extension of the > _sigfault member of siginfo. > > The code is refactored a bit and then si_perf_type is added along side > si_perf_data in the _perf subfield of _sigfault of siginfo_t. > > Finally the signalfd_siginfo fields are removed as they appear to be > filling up the structure without userspace actually being able to use > them. Thanks for your series, which is now in next-20210506. > arch/alpha/include/uapi/asm/siginfo.h | 2 - > arch/alpha/kernel/osf_sys.c | 2 +- > arch/alpha/kernel/signal.c | 4 +- > arch/alpha/kernel/traps.c | 24 ++--- > arch/alpha/mm/fault.c | 4 +- > arch/arm/kernel/signal.c | 39 +++++++ > arch/arm64/kernel/signal.c | 39 +++++++ > arch/arm64/kernel/signal32.c | 39 +++++++ > arch/mips/include/uapi/asm/siginfo.h | 2 - > arch/sparc/include/uapi/asm/siginfo.h | 3 - > arch/sparc/kernel/process_64.c | 2 +- > arch/sparc/kernel/signal32.c | 37 +++++++ > arch/sparc/kernel/signal_64.c | 36 +++++++ > arch/sparc/kernel/sys_sparc_32.c | 2 +- > arch/sparc/kernel/sys_sparc_64.c | 2 +- > arch/sparc/kernel/traps_32.c | 22 ++-- > arch/sparc/kernel/traps_64.c | 44 ++++---- > arch/sparc/kernel/unaligned_32.c | 2 +- > arch/sparc/mm/fault_32.c | 2 +- > arch/sparc/mm/fault_64.c | 2 +- > arch/x86/kernel/signal_compat.c | 15 ++- No changes needed for other architectures? All m68k configs are broken with arch/m68k/kernel/signal.c:626:35: error: 'siginfo_t' {aka 'struct siginfo'} has no member named 'si_perf'; did you mean 'si_errno'? See e.g. http://kisskb.ellerman.id.au/kisskb/buildresult/14537820/ There are still a few more references left to si_perf: $ git grep -n -w si_perf Next/merge.log:2902:Merging userns/for-next (4cf4e48fff05 signal: sort out si_trapno and si_perf) arch/m68k/kernel/signal.c:626: BUILD_BUG_ON(offsetof(siginfo_t, si_perf) != 0x10); include/uapi/linux/perf_event.h:467: * siginfo_t::si_perf, e.g. to permit user to identify the event. tools/testing/selftests/perf_events/sigtrap_threads.c:46:/* Unique value to check si_perf is correctly set from perf_event_attr::sig_data. */ Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, May 06, 2021 at 09:00AM +0200, Geert Uytterhoeven wrote: [...] > No changes needed for other architectures? > All m68k configs are broken with > > arch/m68k/kernel/signal.c:626:35: error: 'siginfo_t' {aka 'struct > siginfo'} has no member named 'si_perf'; did you mean 'si_errno'? > > See e.g. http://kisskb.ellerman.id.au/kisskb/buildresult/14537820/ > > There are still a few more references left to si_perf: > > $ git grep -n -w si_perf > Next/merge.log:2902:Merging userns/for-next (4cf4e48fff05 signal: sort > out si_trapno and si_perf) > arch/m68k/kernel/signal.c:626: BUILD_BUG_ON(offsetof(siginfo_t, > si_perf) != 0x10); > include/uapi/linux/perf_event.h:467: * siginfo_t::si_perf, e.g. to > permit user to identify the event. > tools/testing/selftests/perf_events/sigtrap_threads.c:46:/* Unique > value to check si_perf is correctly set from > perf_event_attr::sig_data. */ I think we're missing the below in "signal: Deliver all of the siginfo perf data in _perf". Thanks, -- Marco ------ >8 ------ diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c index a4b7ee1df211..8f215e79e70e 100644 --- a/arch/m68k/kernel/signal.c +++ b/arch/m68k/kernel/signal.c @@ -623,7 +623,8 @@ static inline void siginfo_build_tests(void) BUILD_BUG_ON(offsetof(siginfo_t, si_pkey) != 0x12); /* _sigfault._perf */ - BUILD_BUG_ON(offsetof(siginfo_t, si_perf) != 0x10); + BUILD_BUG_ON(offsetof(siginfo_t, si_perf_data) != 0x10); + BUILD_BUG_ON(offsetof(siginfo_t, si_perf_type) != 0x14); /* _sigpoll */ BUILD_BUG_ON(offsetof(siginfo_t, si_band) != 0x0c); diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index bf8143505c49..f92880a15645 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -464,7 +464,7 @@ struct perf_event_attr { /* * User provided data if sigtrap=1, passed back to user via - * siginfo_t::si_perf, e.g. to permit user to identify the event. + * siginfo_t::si_perf_data, e.g. to permit user to identify the event. */ __u64 sig_data; }; diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c b/tools/testing/selftests/perf_events/sigtrap_threads.c index fde123066a8c..8e83cf91513a 100644 --- a/tools/testing/selftests/perf_events/sigtrap_threads.c +++ b/tools/testing/selftests/perf_events/sigtrap_threads.c @@ -43,7 +43,7 @@ static struct { siginfo_t first_siginfo; /* First observed siginfo_t. */ } ctx; -/* Unique value to check si_perf is correctly set from perf_event_attr::sig_data. */ +/* Unique value to check si_perf_data is correctly set from perf_event_attr::sig_data. */ #define TEST_SIG_DATA(addr) (~(unsigned long)(addr)) static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
Geert Uytterhoeven <geert@linux-m68k.org> writes: > Hi Eric, > > On Tue, May 4, 2021 at 11:14 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> This set of changes sorts out the ABI issues with SIGTRAP TRAP_PERF, and >> hopefully will can get merged before any userspace code starts using the >> new ABI. >> >> The big ideas are: >> - Placing the asserts first to prevent unexpected ABI changes >> - si_trapno becomming ordinary fault subfield. >> - struct signalfd_siginfo is almost full >> >> This set of changes starts out with Marco's static_assert changes and >> additional one of my own that enforces the fact that the alignment of >> siginfo_t is also part of the ABI. Together these build time >> checks verify there are no unexpected ABI changes in the changes >> that follow. >> >> The field si_trapno is changed to become an ordinary extension of the >> _sigfault member of siginfo. >> >> The code is refactored a bit and then si_perf_type is added along side >> si_perf_data in the _perf subfield of _sigfault of siginfo_t. >> >> Finally the signalfd_siginfo fields are removed as they appear to be >> filling up the structure without userspace actually being able to use >> them. > > Thanks for your series, which is now in next-20210506. > >> arch/alpha/include/uapi/asm/siginfo.h | 2 - >> arch/alpha/kernel/osf_sys.c | 2 +- >> arch/alpha/kernel/signal.c | 4 +- >> arch/alpha/kernel/traps.c | 24 ++--- >> arch/alpha/mm/fault.c | 4 +- >> arch/arm/kernel/signal.c | 39 +++++++ >> arch/arm64/kernel/signal.c | 39 +++++++ >> arch/arm64/kernel/signal32.c | 39 +++++++ >> arch/mips/include/uapi/asm/siginfo.h | 2 - >> arch/sparc/include/uapi/asm/siginfo.h | 3 - >> arch/sparc/kernel/process_64.c | 2 +- >> arch/sparc/kernel/signal32.c | 37 +++++++ >> arch/sparc/kernel/signal_64.c | 36 +++++++ >> arch/sparc/kernel/sys_sparc_32.c | 2 +- >> arch/sparc/kernel/sys_sparc_64.c | 2 +- >> arch/sparc/kernel/traps_32.c | 22 ++-- >> arch/sparc/kernel/traps_64.c | 44 ++++---- >> arch/sparc/kernel/unaligned_32.c | 2 +- >> arch/sparc/mm/fault_32.c | 2 +- >> arch/sparc/mm/fault_64.c | 2 +- >> arch/x86/kernel/signal_compat.c | 15 ++- > > No changes needed for other architectures? > All m68k configs are broken with Thanks. I hadn't realized that si_perf asserts existed on m68k. Thankfully linux-next caught this these. Looking a little more deeply, it is strange that this is tested on m68k. The architecture does not implement HAVE_PERF_EVENTS so it is impossible for this signal to be generated. On the off chance this these new signals will appear on m68k someday I will update the assertion. > arch/m68k/kernel/signal.c:626:35: error: 'siginfo_t' {aka 'struct > siginfo'} has no member named 'si_perf'; did you mean 'si_errno'? > > See e.g. http://kisskb.ellerman.id.au/kisskb/buildresult/14537820/ > > There are still a few more references left to si_perf: > > $ git grep -n -w si_perf > Next/merge.log:2902:Merging userns/for-next (4cf4e48fff05 signal: sort > out si_trapno and si_perf) > arch/m68k/kernel/signal.c:626: BUILD_BUG_ON(offsetof(siginfo_t, > si_perf) != 0x10); > include/uapi/linux/perf_event.h:467: * siginfo_t::si_perf, e.g. to > permit user to identify the event. > tools/testing/selftests/perf_events/sigtrap_threads.c:46:/* Unique > value to check si_perf is correctly set from > perf_event_attr::sig_data. */ I will sweep them up as well. Eric
For the moment I am adding this to my for-next branch. I plan to respin and fold this in but I am not certain what my schedule looks like today. So I figure making certain I have a fix out (so I stop breaking m68k) is more important than having a perfect patch. Eric From: "Eric W. Biederman" <ebiederm@xmission.com> Date: Thu, 6 May 2021 10:17:10 -0500 Subject: [PATCH] signal: Remove the last few si_perf references I accidentially overlooked a few references to si_perf when sorting out the ABI update those references now. Fixes: f6a2c711f1e3 ("signal: Deliver all of the siginfo perf data in _perf") Suggested-by: Marco Elver <elver@google.com> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- arch/m68k/kernel/signal.c | 3 ++- include/uapi/linux/perf_event.h | 2 +- tools/testing/selftests/perf_events/sigtrap_threads.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c index a4b7ee1df211..8f215e79e70e 100644 --- a/arch/m68k/kernel/signal.c +++ b/arch/m68k/kernel/signal.c @@ -623,7 +623,8 @@ static inline void siginfo_build_tests(void) BUILD_BUG_ON(offsetof(siginfo_t, si_pkey) != 0x12); /* _sigfault._perf */ - BUILD_BUG_ON(offsetof(siginfo_t, si_perf) != 0x10); + BUILD_BUG_ON(offsetof(siginfo_t, si_perf_data) != 0x10); + BUILD_BUG_ON(offsetof(siginfo_t, si_perf_type) != 0x14); /* _sigpoll */ BUILD_BUG_ON(offsetof(siginfo_t, si_band) != 0x0c); diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index e54e639248c8..7b14753b3d38 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -464,7 +464,7 @@ struct perf_event_attr { /* * User provided data if sigtrap=1, passed back to user via - * siginfo_t::si_perf, e.g. to permit user to identify the event. + * siginfo_t::si_perf_data, e.g. to permit user to identify the event. */ __u64 sig_data; }; diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c b/tools/testing/selftests/perf_events/sigtrap_threads.c index fde123066a8c..8e83cf91513a 100644 --- a/tools/testing/selftests/perf_events/sigtrap_threads.c +++ b/tools/testing/selftests/perf_events/sigtrap_threads.c @@ -43,7 +43,7 @@ static struct { siginfo_t first_siginfo; /* First observed siginfo_t. */ } ctx; -/* Unique value to check si_perf is correctly set from perf_event_attr::sig_data. */ +/* Unique value to check si_perf_data is correctly set from perf_event_attr::sig_data. */ #define TEST_SIG_DATA(addr) (~(unsigned long)(addr)) static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
Linus,
Please pull the for-v5.13-rc2 branch from the git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-v5.13-rc2
HEAD: addd6821190ebf1e9fece0b7848db667fd280e2e signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo
During the merge window an issue with si_perf and the siginfo ABI came
up. The alpha and sparc siginfo structure layout had changed with the
addition of SIGTRAP TRAP_PERF and the new field si_perf.
The reason only alpha and sparc were affected is that they are the
only architectures that use si_trapno.
Looking deeper it was discovered that si_trapno is used for only
a few select signals on alpha and sparc, and that none of the
other _sigfault fields past si_addr are used at all. Which means
technically no regression on alpha and sparc.
While the alignment concerns might be dismissed the abuse of
si_errno by SIGTRAP TRAP_PERF does have the potential to cause
regressions in existing userspace.
While we still have time before userspace starts using and depending on
the new definition siginfo for SIGTRAP TRAP_PERF this set of changes
cleans up siginfo_t.
- The si_trapno field is demoted from magic alpha and sparc status and
made an ordinary union member of the _sigfault member of siginfo_t.
Without moving it of course.
- si_perf is replaced with si_perf_data and si_perf_type ending the
abuse of si_errno.
- BUILD_BUG_ONs are added and various helpers are modified to
accommodate this change.
- Unnecessary additions to signalfd_siginfo are removed.
v3: https://lkml.kernel.org/r/m1tuni8ano.fsf_-_@fess.ebiederm.org
v2: https://lkml.kernel.org/r/m14kfjh8et.fsf_-_@fess.ebiederm.org
v1: https://lkml.kernel.org/r/m1zgxfs7zq.fsf_-_@fess.ebiederm.org
You might notice a recent rebase. This code has been sitting in
linux-next as ef566ba2d7d9 ("signal: Remove the last few si_perf
references"). Which results in the exact same code as the branch
I am sending you but the commits differ to keep git bisect working.
The difference is that I squashed a fix for a mips BUILD_BUG_ON about
si_perf into the commit that replaces si_perf with si_perf_data and
si_perf_type. This keeps the kernel building on all architectures for
all commits keeping git-bisect working for everyone.
Eric W. Biederman (9):
signal: Verify the alignment and size of siginfo_t
siginfo: Move si_trapno inside the union inside _si_fault
signal: Implement SIL_FAULT_TRAPNO
signal: Use dedicated helpers to send signals with si_trapno set
signal: Remove __ARCH_SI_TRAPNO
signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency
signal: Factor force_sig_perf out of perf_sigtrap
signal: Deliver all of the siginfo perf data in _perf
signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo
Marco Elver (3):
sparc64: Add compile-time asserts for siginfo_t offsets
arm: Add compile-time asserts for siginfo_t offsets
arm64: Add compile-time asserts for siginfo_t offsets
arch/alpha/include/uapi/asm/siginfo.h | 2 -
arch/alpha/kernel/osf_sys.c | 2 +-
arch/alpha/kernel/signal.c | 4 +-
arch/alpha/kernel/traps.c | 24 ++---
arch/alpha/mm/fault.c | 4 +-
arch/arm/kernel/signal.c | 39 +++++++
arch/arm64/kernel/signal.c | 39 +++++++
arch/arm64/kernel/signal32.c | 39 +++++++
arch/m68k/kernel/signal.c | 3 +-
arch/mips/include/uapi/asm/siginfo.h | 2 -
arch/sparc/include/uapi/asm/siginfo.h | 3 -
arch/sparc/kernel/process_64.c | 2 +-
arch/sparc/kernel/signal32.c | 37 +++++++
arch/sparc/kernel/signal_64.c | 36 +++++++
arch/sparc/kernel/sys_sparc_32.c | 2 +-
arch/sparc/kernel/sys_sparc_64.c | 2 +-
arch/sparc/kernel/traps_32.c | 22 ++--
arch/sparc/kernel/traps_64.c | 44 ++++----
arch/sparc/kernel/unaligned_32.c | 2 +-
arch/sparc/mm/fault_32.c | 2 +-
arch/sparc/mm/fault_64.c | 2 +-
arch/x86/kernel/signal_compat.c | 15 ++-
fs/signalfd.c | 23 ++---
include/linux/compat.h | 10 +-
include/linux/sched/signal.h | 13 +--
include/linux/signal.h | 3 +-
include/uapi/asm-generic/siginfo.h | 20 ++--
include/uapi/linux/perf_event.h | 2 +-
include/uapi/linux/signalfd.h | 4 +-
kernel/events/core.c | 11 +-
kernel/signal.c | 113 +++++++++++++--------
.../selftests/perf_events/sigtrap_threads.c | 14 +--
32 files changed, 377 insertions(+), 163 deletions(-)
Eric
On Thu, May 13, 2021 at 9:55 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Please pull the for-v5.13-rc2 branch from the git tree: I really don't like this tree. The immediate cause for "no" is the silly #if IS_ENABLED(CONFIG_SPARC) and #if IS_ENABLED(CONFIG_ALPHA) code in kernel/signal.c. It has absolutely zero business being there, when those architectures have a perfectly fine arch/*/kernel/signal.c file where that code would make much more sense *WITHOUT* any odd preprocessor games. But there are other oddities too, like the new send_sig_fault_trapno(SIGFPE, si_code, (void __user *) regs->pc, 0, current); in the alpha code, which fundamentally seems bogus: using send_sig_fault_trapno() with a '0' for trapno seems entirely incorrect, since the *ONLY* point of that function is to set si_trapno to something non-zero. So it would seem that a plain send_sig_fault() without that 0 would be the right thing to do. This also mixes in a lot of other stuff than just the fixes. Which would have been ok during the merge window, but I'm definitely not happy about it now. Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, May 13, 2021 at 9:55 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Please pull the for-v5.13-rc2 branch from the git tree: > > I really don't like this tree. > > The immediate cause for "no" is the silly > > #if IS_ENABLED(CONFIG_SPARC) > > and > > #if IS_ENABLED(CONFIG_ALPHA) > > code in kernel/signal.c. It has absolutely zero business being there, > when those architectures have a perfectly fine arch/*/kernel/signal.c > file where that code would make much more sense *WITHOUT* any odd > preprocessor games. The code is generic it just happens those functions are only used on sparc and alpha. Further I really want to make filling out siginfo_t happen in dedicated functions as much as possible in kernel/signal.c. The probably of getting it wrong without a helper functions is very strong. As the code I am fixing demonstrates. The IS_ENABLED(arch) is mostly there so we can delete the code if/when the architectures are retired in another decade or so. > But there are other oddities too, like the new > > send_sig_fault_trapno(SIGFPE, si_code, (void __user *) regs->pc, > 0, current); > > in the alpha code, which fundamentally seems bogus: using > send_sig_fault_trapno() with a '0' for trapno seems entirely > incorrect, since the *ONLY* point of that function is to set si_trapno > to something non-zero. > > So it would seem that a plain send_sig_fault() without that 0 would be > the right thing to do. As it happens the floating point emulation code on alpha is inconsistent with the non floating point emulation code. When using real floating point hardware SIGFPE on alpha always set si_trapno. The floating point emulation code does not look like it has ever set si_trapno. I continued to used send_sig_fault_trapno to point out that inconsistency. If alpha floating point emulation was in active use I expect we would care enough to put something other than 0 in there. > This also mixes in a lot of other stuff than just the fixes. Which > would have been ok during the merge window, but I'm definitely not > happy about it now. If the breakage that came with SIGTRAP TRAP_PERF had not been discovered during the merge window I would not be sending this now. It took a little time to dig to the bottom, then the code needed just a little extra time to sit, so there were not surprises. As for mixing things, I am not quite certain what you are referring to. All of the changes relate to keeping people from shooting themselves in the foot with when using siginfo. The most noise comes from send_sig_fault vs send_sig_fault_trapno, and force_sig_fault vs force_sig_fault_trapno. That is fundamental to the siginfo fix as it is there to ensure that is safe to treat si_trapno as an ordinary _sigfault union member. Which in turns makes alpha and sparc no longer special with respect to _sigfault, just a little eccentric. I will concede that renaming SIL_PERF_EVENT to SIL_FAULT_PERF_EVENT is unnecessary, but it certainly makes it clear that we are dealing with _sigfault and not some other part of siginfo_t. Eric
ebiederm@xmission.com (Eric W. Biederman) writes: > Linus Torvalds <torvalds@linux-foundation.org> writes: > >> On Thu, May 13, 2021 at 9:55 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >>> >>> Please pull the for-v5.13-rc2 branch from the git tree: >> >> I really don't like this tree. >> >> The immediate cause for "no" is the silly >> >> #if IS_ENABLED(CONFIG_SPARC) >> >> and >> >> #if IS_ENABLED(CONFIG_ALPHA) >> >> code in kernel/signal.c. It has absolutely zero business being there, >> when those architectures have a perfectly fine arch/*/kernel/signal.c >> file where that code would make much more sense *WITHOUT* any odd >> preprocessor games. > > The code is generic it just happens those functions are only used on > sparc and alpha. Further I really want to make filling out siginfo_t > happen in dedicated functions as much as possible in kernel/signal.c. > The probably of getting it wrong without a helper functions is very > strong. As the code I am fixing demonstrates. > > The IS_ENABLED(arch) is mostly there so we can delete the code if/when > the architectures are retired in another decade or so. There is also the question of why alpha allows userspace to block SIGFPE. If it turns out that alpha is just silly by allowing synchronous exceptions to be blocked, then the code really becomes generic and shared shared between sparc and alpha. Which is really why the code does not make sense in some architecture specific version of signal.c. That and the fact the two functions are almost identical. If you want I can remove the #ifdefs and we can take up slightly more space until someone implements -ffunction-sections. Do you know if alpha will be stuck triggering the same floating point error if the SIGFPE is blocked or can alpha somehow continue past it? If alpha using send_sig instead of force_sig is historical and does not reflect the reality of the hardware alpha can be converted and several of the send_sig variants can be removed. Otherwise alpha remains the odd man out, and the code can remain until all of the alpha hardware dies. (I don't think anyone is manufacturing alpha hardware anymore). I would look it up but I have lost access to whatever alpha documentation I had. Eric
* Eric W. Biederman <ebiederm@xmission.com> wrote: > Looking deeper it was discovered that si_trapno is used for only > a few select signals on alpha and sparc, and that none of the > other _sigfault fields past si_addr are used at all. Which means > technically no regression on alpha and sparc. If there's no functional regression on any platform, could much of this wait until v5.14, or do we want some of these cleanups right now? The fixes seem to be for long-existing bugs, not fresh regressions, AFAICS. The asserts & cleanups are useful, but not regression fixes. I.e. this is a bit scary: > 32 files changed, 377 insertions(+), 163 deletions(-) at -rc2 time. Thanks, Ingo
Ingo Molnar <mingo@kernel.org> writes: > * Eric W. Biederman <ebiederm@xmission.com> wrote: > >> Looking deeper it was discovered that si_trapno is used for only >> a few select signals on alpha and sparc, and that none of the >> other _sigfault fields past si_addr are used at all. Which means >> technically no regression on alpha and sparc. > > If there's no functional regression on any platform, could much of this > wait until v5.14, or do we want some of these cleanups right now? > > The fixes seem to be for long-existing bugs, not fresh regressions, AFAICS. > The asserts & cleanups are useful, but not regression fixes. > > I.e. this is a bit scary: The new ABI for SIGTRAP TRAP perf that came in the merge window is broken and wrong. We need to revert/disable the new SIGTRAP TRAP_PERF or have a fix before v5.13. The issue is old crap getting in the way of a new addition. I think I might see a smaller code change on how to get to something compatible with this. >> 32 files changed, 377 insertions(+), 163 deletions(-) > > at -rc2 time. The additions are all tests to make certain everything is fine. The actual code change without the assertions (tests) is essentially a wash. Eric
Linus,
Please pull the for-v5.13-rc3 branch from the git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-v5.13-rc3
HEAD: 922e3013046b79b444c87eda5baf43afae1326a8 signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo
During the merge window an issue with si_perf and the siginfo ABI came
up. The alpha and sparc siginfo structure layout had changed with the
addition of SIGTRAP TRAP_PERF and the new field si_perf.
The reason only alpha and sparc were affected is that they are the
only architectures that use si_trapno.
Looking deeper it was discovered that si_trapno is used for only
a few select signals on alpha and sparc, and that none of the
other _sigfault fields past si_addr are used at all. Which means
technically no regression on alpha and sparc.
While the alignment concerns might be dismissed the abuse of
si_errno by SIGTRAP TRAP_PERF does have the potential to cause
regressions in existing userspace.
While we still have time before userspace starts using and depending on
the new definition siginfo for SIGTRAP TRAP_PERF this set of changes
cleans up siginfo_t.
- The si_trapno field is demoted from magic alpha and sparc status and
made an ordinary union member of the _sigfault member of siginfo_t.
Without moving it of course.
- si_perf is replaced with si_perf_data and si_perf_type ending the
abuse of si_errno.
- Unnecessary additions to signalfd_siginfo are removed.
v4: https://lkml.kernel.org/r/m1a6ot5e2h.fsf_-_@fess.ebiederm.org
v3: https://lkml.kernel.org/r/m1tuni8ano.fsf_-_@fess.ebiederm.org
v2: https://lkml.kernel.org/r/m14kfjh8et.fsf_-_@fess.ebiederm.org
v1: https://lkml.kernel.org/r/m1zgxfs7zq.fsf_-_@fess.ebiederm.org
This version drops the tests and fine grained handling of si_trapno
on alpha and sparc (replaced assuming si_trapno is valid for
all but the faults that defined different data).
Hopefully this is enough to not be scary as a fix for the ABI issues.
Tested-by: Marco Elver <elver@google.com>
Eric W. Biederman (5):
siginfo: Move si_trapno inside the union inside _si_fault
signal: Implement SIL_FAULT_TRAPNO
signal: Factor force_sig_perf out of perf_sigtrap
signal: Deliver all of the siginfo perf data in _perf
signalfd: Remove SIL_PERF_EVENT fields from signalfd_siginfo
arch/m68k/kernel/signal.c | 3 +-
arch/x86/kernel/signal_compat.c | 9 +++-
fs/signalfd.c | 23 ++++-----
include/linux/compat.h | 10 ++--
include/linux/sched/signal.h | 1 +
include/linux/signal.h | 1 +
include/uapi/asm-generic/siginfo.h | 15 +++---
include/uapi/linux/perf_event.h | 2 +-
include/uapi/linux/signalfd.h | 4 +-
kernel/events/core.c | 11 +---
kernel/signal.c | 59 +++++++++++++---------
.../selftests/perf_events/sigtrap_threads.c | 14 ++---
12 files changed, 79 insertions(+), 73 deletions(-)
Eric
The pull request you sent on Fri, 21 May 2021 09:59:53 -0500:
> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-v5.13-rc3
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a0e31f3a38e77612ed8967aaad28db6d3ee674b5
Thank you!
This set of changes sorts out the ABI issues with SIGTRAP TRAP_PERF, and hopefully will can get merged before any userspace code starts using the new ABI. The big ideas are: - Placing the asserts first to prevent unexpected ABI changes - si_trapno becomming ordinary fault subfield. - struct signalfd_siginfo is almost full This set of changes starts out with Marco's static_assert changes and additional one of my own that enforces the fact that the alignment of siginfo_t is also part of the ABI. Together these build time checks verify there are no unexpected ABI changes in the changes that follow. The field si_trapno is changed to become an ordinary extension of the _sigfault member of siginfo. The code is refactored a bit and then si_perf_type is added along side si_perf_data in the _perf subfield of _sigfault of siginfo_t. Finally the signalfd_siginfo fields are removed as they appear to be filling up the structure without userspace actually being able to use them. v2: https://lkml.kernel.org/r/m14kfjh8et.fsf_-_@fess.ebiederm.org v1: https://lkml.kernel.org/r/m1zgxfs7zq.fsf_-_@fess.ebiederm.org Eric W. Biederman (9): signal: Verify the alignment and size of siginfo_t siginfo: Move si_trapno inside the union inside _si_fault signal: Implement SIL_FAULT_TRAPNO signal: Use dedicated helpers to send signals with si_trapno set signal: Remove __ARCH_SI_TRAPNO signal: Rename SIL_PERF_EVENT SIL_FAULT_PERF_EVENT for consistency signal: Factor force_sig_perf out of perf_sigtrap signal: Deliver all of the siginfo perf data in _perf signalfd: Remove SIL_FAULT_PERF_EVENT fields from signalfd_siginfo Marco Elver (3): sparc64: Add compile-time asserts for siginfo_t offsets arm: Add compile-time asserts for siginfo_t offsets arm64: Add compile-time asserts for siginfo_t offsets arch/alpha/include/uapi/asm/siginfo.h | 2 - arch/alpha/kernel/osf_sys.c | 2 +- arch/alpha/kernel/signal.c | 4 +- arch/alpha/kernel/traps.c | 24 ++--- arch/alpha/mm/fault.c | 4 +- arch/arm/kernel/signal.c | 39 +++++++ arch/arm64/kernel/signal.c | 39 +++++++ arch/arm64/kernel/signal32.c | 39 +++++++ arch/mips/include/uapi/asm/siginfo.h | 2 - arch/sparc/include/uapi/asm/siginfo.h | 3 - arch/sparc/kernel/process_64.c | 2 +- arch/sparc/kernel/signal32.c | 37 +++++++ arch/sparc/kernel/signal_64.c | 36 +++++++ arch/sparc/kernel/sys_sparc_32.c | 2 +- arch/sparc/kernel/sys_sparc_64.c | 2 +- arch/sparc/kernel/traps_32.c | 22 ++-- arch/sparc/kernel/traps_64.c | 44 ++++---- arch/sparc/kernel/unaligned_32.c | 2 +- arch/sparc/mm/fault_32.c | 2 +- arch/sparc/mm/fault_64.c | 2 +- arch/x86/kernel/signal_compat.c | 15 ++- fs/signalfd.c | 23 ++--- include/linux/compat.h | 10 +- include/linux/sched/signal.h | 13 +-- include/linux/signal.h | 3 +- include/uapi/asm-generic/siginfo.h | 20 ++-- include/uapi/linux/signalfd.h | 4 +- kernel/events/core.c | 11 +- kernel/signal.c | 113 +++++++++++++-------- .../selftests/perf_events/sigtrap_threads.c | 12 +-- 30 files changed, 373 insertions(+), 160 deletions(-)