ppc compat v4.16 regression: sending SIGTRAP or SIGFPE via kill() returns wrong values in si_pid and si_uid
Subsystems (29)
Hi,
There seems to be a regression in v4.16 on ppc compat very similar to sparc compat regression reported earlier at
https://marc.info/?l=linux-sparc&m=151501500704383 .
The symptoms are exactly the same: the same signal_receive test from the strace test suite fails with the same diagnostics:
https://build.opensuse.org/public/build/home:ldv_alt/openSUSE_Factory_PowerPC/ppc/strace/_log
Unfortunately, I do not have any means to investigate further, so just passing this information on to those who care.
Signature · 1 line
ldv
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Hi,
Quoted 7 lines · Dmitry V. Levin
There seems to be a regression in v4.16 on ppc compat very similar to sparc compat regression reported earlier at
https://marc.info/?l=linux-sparc&m=151501500704383 .The symptoms are exactly the same: the same
signal_receivetest from the strace test suite fails with the same diagnostics:
https://build.opensuse.org/public/build/home:ldv_alt/openSUSE_Factory_PowerPC/ppc/strace/_log
The log is big, just look for "KERNEL BUG".
Quoted 2 lines
Unfortunately, I do not have any means to investigate further, so just passing this information on to those who care.
OK, the faulty commit is v4.16-rc1~159^2~39
("signal/powerpc: Document conflicts with SI_USER and SIGFPE and SIGTRAP").
One might think that a commit called "Document conflicts" shouldn't introduce an ABI regression, but this one definitely does by defining FPE_FIXME and TRAP_FIXME in arch/powerpc/include/uapi/asm/siginfo.h that affect siginfo_layout().
A similar commit v4.16-rc1~159^2~37
("signal/arm: Document conflicts with SI_USER and SIGFPE") must have introduced a similar ABI regression to compat arm.
An earlier commit v4.14-rc1~60^2^2~5
("signal/sparc: Document a conflict with SI_USER with SIGFPE") introduced a similar ABI regression to compat sparc.
There is a clear pattern of sneaking in ABI changes using innocently looking commit messages.
Signature · 8 lines
ldv -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/1b9648bb/attachment.sig>
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 3 lines · Dmitry V. Levin
There is a clear pattern of sneaking in ABI changes using innocently looking commit messages.
Yes, this siginfo stuff has been a mess.
Eric - this needs to stop. Or we need to revert all that garbage entirely.
Send a fix. And stop changing the siginfo layout or field values.
Linus
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
In reply to Dmitry V. LevinQuoted 3 lines · Dmitry V. Levin
A similar commit v4.16-rc1~159^2~37
("signal/arm: Document conflicts withSI_USERand SIGFPE") must have introduced a similar ABI regression to compat arm.
So, could you explain how can this change cause a regression?
+#define FPE_FIXME 0
- vfp_raise_sigfpe(0, regs);
+ vfp_raise_sigfpe(FPE_FIXME, regs);
I think you're talking garbage here - look at the damned change. It subsitutes a definition for a constant, and vfp_raise_sigfpe() ends up receiving exactly the same value bother before and after the change.
The change is rather incomplete though because it should have also changed:
int si_code = 0;
as well.
So, the commit log is accurate in this case: it _is_ about documenting the conflicting cases between SI_USER and SIGFPE and that bit of the change has no ABI effect.
What does slightly annoy me is the creation of uapi/asm/siginfo.h to contain a definition that _isn't_ to be exposed as part of the UAPI. If it's not part of the UAPI, it doesn't belong in a UAPI header, period. In any case, I don't think that is exposed to userspace.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Review
Quoted 10 lines · Russell King - ARM Linux
Quoted 3 lines · Dmitry V. Levin
A similar commit v4.16-rc1~159^2~37
("signal/arm: Document conflicts withSI_USERand SIGFPE") must have introduced a similar ABI regression to compat arm.So, could you explain how can this change cause a regression?
(quoted diff) +2
-1+#define FPE_FIXME 0 - vfp_raise_sigfpe(0, regs); + vfp_raise_sigfpe(FPE_FIXME, regs);
No, this hunk hasn't caused the regression, but another one did:
arch/arm/include/uapi/asm/siginfo.h new file +13 -0
@@ -0,0 +1,13 @@ +#ifndef __ASM_SIGINFO_H +#define __ASM_SIGINFO_H + +#include <asm-generic/siginfo.h> + +/* +* SIGFPE si_codes +*/ +#ifdef __KERNEL__ +#define FPE_FIXME 0/* Broken dup of SI_USER */ +#endif /* __KERNEL__ */ + +#endif
This is due to FPE_FIXME handling in kernel/signal.c
Signature · 8 lines
ldv -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/0dccfbf9/attachment-0001.sig>
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 34 lines · Dmitry V. Levin
Review
Quoted 10 lines · Russell King - ARM Linux
Quoted 3 lines · Dmitry V. Levin
A similar commit v4.16-rc1~159^2~37
("signal/arm: Document conflicts withSI_USERand SIGFPE") must have introduced a similar ABI regression to compat arm.So, could you explain how can this change cause a regression?
(quoted diff) +2
-1+#define FPE_FIXME 0 - vfp_raise_sigfpe(0, regs); + vfp_raise_sigfpe(FPE_FIXME, regs);No, this hunk hasn't caused the regression, but another one did:
arch/arm/include/uapi/asm/siginfo.h new file +13
-0@@ -0,0 +1,13 @@ +#ifndef __ASM_SIGINFO_H +#define __ASM_SIGINFO_H + +#include <asm-generic/siginfo.h> + +/* +* SIGFPE si_codes +*/ +#ifdef __KERNEL__ +#define FPE_FIXME 0/* Broken dup of SI_USER */ +#endif /* __KERNEL__ */ + +#endif
Quoted 1 line
This is due to
FPE_FIXMEhandling inkernel/signal.c
Building strace 4.22 on ARM and running the test suite reveals no problems with the signal_receive test, tested on both 4.14 and 4.16 kernels - there's no "KERNEL BUG" reports in any of the test results. However, stock strace 4.22 source doesn't appear to contain the "KERNEL BUG" string anywhere, so this may be a Suse specific addition to the test:
~/src/strace-4.22$ grep -ri 'KERNEL BUG' . ./strace.1:Arguably, every instance of such behavior is a kernel bug.) ./strace.1.in:Arguably, every instance of such behavior is a kernel bug.) ./NEWS: * Worked around a kernel bug in tracing privileged executables.
./ChangeLog: aarch64: workaround gcc+kernel bug. ./ChangeLog: tests: workaround kernel bugs in seccomp-strict.test and prctl-seccomp-strict.test ./ChangeLog: instead. We don't want the testsuite failing due to kernel bugs. ./ChangeLog: First guess is that it's a workaround for old kernel bugs: ./ChangeLog: This kernel bug is fixed long ago. This change removes the workaround.
Any ideas where the "KERNEL BUG" in Suse builds is coming from? Any ideas how to test it on other architectures (iow, where can we get source that contains this test?)
Based on previous experience, unfortunately folk don't tend to report user ABI regressions to kernel developers, so we'd probably never know that there's a problem - I do think the safer thing would've been to leave it well alone, and just accept that we'll end up copying more words to userspace than is actually intended.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 35 lines · Russell King - ARM Linux
Quoted 34 lines · Dmitry V. Levin
Review
Quoted 10 lines · Russell King - ARM Linux
Quoted 3 lines · Dmitry V. Levin
A similar commit v4.16-rc1~159^2~37
("signal/arm: Document conflicts withSI_USERand SIGFPE") must have introduced a similar ABI regression to compat arm.So, could you explain how can this change cause a regression?
(quoted diff) +2
-1+#define FPE_FIXME 0 - vfp_raise_sigfpe(0, regs); + vfp_raise_sigfpe(FPE_FIXME, regs);No, this hunk hasn't caused the regression, but another one did:
arch/arm/include/uapi/asm/siginfo.h new file +13
-0@@ -0,0 +1,13 @@ +#ifndef __ASM_SIGINFO_H +#define __ASM_SIGINFO_H + +#include <asm-generic/siginfo.h> + +/* +* SIGFPE si_codes +*/ +#ifdef __KERNEL__ +#define FPE_FIXME 0/* Broken dup of SI_USER */ +#endif /* __KERNEL__ */ + +#endif
Quoted 5 lines
Quoted 1 line
This is due to
FPE_FIXMEhandling inkernel/signal.cBuilding strace 4.22 on ARM and running the test suite reveals no problems with the
signal_receivetest, tested on both 4.14 and 4.16 kernels - there's no "KERNEL BUG" reports in any of the test results.
https://build.opensuse.org/public/build/home:ldv_alt/openSUSE_Factory_ARM/armv7l/strace/_log
- the test just fails there with
[ 50s] + uname -a
[ 50s] Linux armbuild01 4.16.0-1-lpae #1 SMP PREEMPT Wed Apr 4 13:35:56 UTC 2018 (e16f96d) armv7l armv7l armv7l GNU/Linux
...
[ 570s] FAIL: signal_receive.gen
[ 570s] ---- SIGFPE {si_signo=SIGFPE, si_code=SI_USER, si_pid=25332, si_uid=399} ---
[ 570s] +--- SIGFPE {si_signo=SIGFPE, si_code=SI_USER, si_pid=25332, si_uid=0} ---
[ 570s] signal_receive.gen.test: failed test: ../../strace -a16 -e trace=kill ../signal_receive output mismatchQuoted 3 lines
However, stock strace 4.22 source doesn't appear to contain the "KERNEL BUG" string anywhere, so this may be a Suse specific addition to the test:
The "KERNEL BUG" diagnostics I was talking about was added to strace yesterday as a part of workaround commit, see
https://github.com/strace/strace/commit/34c7794cc16e2511eda7b1d5767c655a83b17309
Before that change the test just failed.
[...]
Quoted 1 line
Any ideas where the "KERNEL BUG" in Suse builds is coming from?
strace developers use OBS to test strace.git for regressions.
The build environment is provided by OBS, all the rest comes from strace.git.
Quoted 2 lines
Any ideas how to test it on other architectures (iow, where can we get source that contains this test?)
Just use master branch of https://github.com/strace/strace
or https://gitlab.com/strace/strace (they are the same).
Quoted 5 lines
Based on previous experience, unfortunately folk don't tend to report user ABI regressions to kernel developers, so we'd probably never know that there's a problem - I do think the safer thing would've been to leave it well alone, and just accept that we'll end up copying more words to userspace than is actually intended.
Well, these changes caused visible regressions in strace test suite on arm, ppc, and sparc - this is the reason why I have reported them to kernel developers.
Signature · 8 lines
ldv -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/2641684d/attachment.sig>
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 4 lines · Dmitry V. Levin
The "KERNEL BUG" diagnostics I was talking about was added to strace yesterday as a part of workaround commit, see
https://github.com/strace/strace/commit/34c7794cc16e2511eda7b1d5767c655a83b17309
Before that change the test just failed.
Ah, seeing the test case really helps to see exactly what and why it's broken. Yes, Eric's commit was definitely wrong and needs to be reverted, because it incorrectly changes what happens when kill(1) is used to deliver a SIGFPE signal to a process.
Eric, please sort this out - you have a much better handle on whether there are any dependencies here that would need to be resolved from a simple revert of the offending commits, but that revert must happen because you've caused a user visible regression.
The original code _was_ safe even if it wasn't correct to the specs, as we'd end up copying the si_addr field (as the si_pid copy) and a zeroed field as the si_uid copy. It was just that si_code was technically wrong, and that's something that would be even more dangerous to change now.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Does this attached patch perhaps fix the ARM case?
It just uses FPE_FLTUNK as the default si_code for SIGFPE, which seems sane enough. And then gets rid of FPE_FIXME, which should resolve the nasty case.
Hmm? Entirely untested, and I didn't really look at the test-case in question since I can't really run it anyway.
Well, I could run it all on x86-64, but it doesn't have that FPE_FIXME case at all.
Linus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 1163 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/e30ddd2a/attachment.bin>
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 8 lines · Linus Torvalds
Does this attached patch perhaps fix the ARM case?
It just uses
FPE_FLTUNKas the defaultsi_codefor SIGFPE, which seems sane enough. And then gets rid ofFPE_FIXME, which should resolve the nasty case.Hmm? Entirely untested, and I didn't really look at the test-case in question since I can't really run it anyway.
I'll test tomorrow and let you know.
Quoted 10 lines
arch/arm/include/uapi/asm/siginfo.h | 7 ------- arch/arm/vfp/vfpmodule.c | 4 ++-- 2 files changed, 2 insertions(+), 9 deletions(-)arch/arm/include/uapi/asm/siginfo.h +0
-0@@ -3,11 +3,4 @@
arch/arm/include/uapi/asm/siginfo.h line 6
Quoted 10 lines
arch/arm/include/uapi/asm/siginfo.h +0
-7#include <asm-generic/siginfo.h> -/* -* SIGFPE si_codes -*/ -#ifdef __KERNEL__ -#define FPE_FIXME 0/* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ -#endif
This file was created to contain FPE_FIXME, by the "signal/arm: Document conflicts with SI_USER and SIGFPE" commit so if we're removing it, it would be better to remove the whole file.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 4 lines · Russell King - ARM Linux
This file was created to contain
FPE_FIXME, by the "signal/arm: Document conflicts withSI_USERand SIGFPE" commit so if we're removing it, it would be better to remove the whole file.
Fair enough. I'm not going to commit that anyway since I can't test it, but yes, if it tests ok that sounds like the right thing to do.
Linus
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 9 lines · Linus Torvalds
Quoted 4 lines · Russell King - ARM Linux
This file was created to contain
FPE_FIXME, by the "signal/arm: Document conflicts withSI_USERand SIGFPE" commit so if we're removing it, it would be better to remove the whole file.Fair enough. I'm not going to commit that anyway since I can't test it, but yes, if it tests ok that sounds like the right thing to do.
Yes, it does solve the problem at hand with strace - the exact patch I tested against 4.16 is below.
Testing this exact code path (exceptions set to VFP_EXCEPTION_ERROR) is something that can only happen if the hardware does something stupid, and I don't have a way of making it do that, so the code path can't be tested.
However, FPE_FLTUNK is not defined in older kernels, so while we can fix it this way for the current merge window, that doesn't help 4.16. How we solve that depends what happens with Eric's patch (266da65e9156 ("signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions")) that introduces FPE_FLTUNK - and there's also the problem of NSIGFPE, which kernel/signal.c uses in the path that selects the siginfo layout.
Given that the path we're talking about is unlikely to happen (as mentioned in my second paragraph) I still think reverting Eric's patch is the right way forward for older kernels.
(Note, my previous comment about the si_code initialiser was incorrect.)
arch/arm/include/uapi/asm/siginfo.h deleted +0 -13
@@ -1,13 +0,0 @@ -#ifndef __ASM_SIGINFO_H -#define __ASM_SIGINFO_H - -#include <asm-generic/siginfo.h> - -/* -* SIGFPE si_codes -*/ -#ifdef __KERNEL__ -#define FPE_FIXME 0/* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ - -#endif
arch/arm/vfp/vfpmodule.c +5 -1
@@ -28,6 +28,10 @@#include <asm/thread_notify.h> #include <asm/vfp.h> +#ifndef FPE_FLTUNK +#define FPE_FLTUNK 14 +#endif +#include "vfpinstr.h" #include "vfp.h"
@@ -257,7 +261,7 @@ static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_if (exceptions ==VFP_EXCEPTION_ERROR ) {vfp_panic ("unhandled bounce" , inst); -vfp_raise_sigfpe (FPE_FIXME , regs); +vfp_raise_sigfpe (FPE_FLTUNK , regs);return ; }
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 3 lines · Russell King - ARM Linux
Yes, it does solve the problem at hand with strace - the exact patch I tested against 4.16 is below.
Ok, good.
Quoted 2 lines
However,
FPE_FLTUNKis not defined in older kernels, so while we can fix it this way for the current merge window, that doesn't help 4.16.
I wonder if we should even bother with FPE_FLTUNK.
I suspect we might as well use FPE_FLTINV, I suspect, and not have this complexity at all. That case is not worth worrying about, since it's a "this shouldn't happen anyway" and the *real* reason will be in the kernel logs due to vfs_panic().
So it's not like this is something that the user should ever care about the si_code about.
Quoted 3 lines
Given that the path we're talking about is unlikely to happen (as mentioned in my second paragraph) I still think reverting Eric's patch is the right way forward for older kernels.
I'd much rather get rid of that FPE_FIXME, and leave that whole mess behind.
So the attached patch seems simple and should work with 4.16 too.
Let's not leave this as some kind of nasty maintenance issue, and just go for simple and stupid.
Hmm?
Linus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 1037 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180413/9e2e47ed/attachment.bin>
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 13 lines · Linus Torvalds
Quoted 3 lines · Russell King - ARM Linux
Yes, it does solve the problem at hand with strace - the exact patch I tested against 4.16 is below.
Ok, good.
Quoted 2 lines
However,
FPE_FLTUNKis not defined in older kernels, so while we can fix it this way for the current merge window, that doesn't help 4.16.I wonder if we should even bother with
FPE_FLTUNK.
Quoted 7 lines
I suspect we might as well use
FPE_FLTINV, I suspect, and not have this complexity at all. That case is not worth worrying about, since it's a "this shouldn't happen anyway" and the *real* reason will be in the kernel logs due tovfs_panic().So it's not like this is something that the user should ever care about the
si_codeabout.
Ack, my intended meaning for FPE_FLTUNK is that the fp exception is either spurious or we can't tell easily (or possibly at all) which FPE_XXX should be returned. It's up to userspace to figure it out if it really cares. Previously we were accidentally returning SI_USER in si_code for arm64.
This case on arm looks like a more serious error for which FPE_FLTINV may be more appropriate anyway.
[...]
Cheers
---Dave
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 22 lines · Dave Martin
Quoted 13 lines · Linus Torvalds
Quoted 3 lines · Russell King - ARM Linux
Yes, it does solve the problem at hand with strace - the exact patch I tested against 4.16 is below.
Ok, good.
Quoted 2 lines
However,
FPE_FLTUNKis not defined in older kernels, so while we can fix it this way for the current merge window, that doesn't help 4.16.I wonder if we should even bother with
FPE_FLTUNK.Quoted 7 lines
I suspect we might as well use
FPE_FLTINV, I suspect, and not have this complexity at all. That case is not worth worrying about, since it's a "this shouldn't happen anyway" and the *real* reason will be in the kernel logs due tovfs_panic().So it's not like this is something that the user should ever care about the
si_codeabout.
Quoted 8 lines
Ack, my intended meaning for
FPE_FLTUNKis that the fp exception is either spurious or we can't tell easily (or possibly at all) whichFPE_XXXshould be returned. It's up to userspace to figure it out if it really cares. Previously we were accidentally returningSI_USERinsi_codefor arm64.This case on arm looks like a more serious error for which
FPE_FLTINVmay be more appropriate anyway.
No. The cases where we get to this point are:
1. A trap concerning a coprocessor register transfer instruction (iow, move between a VFP register and ARM register.)
2. A trap concerning a coprocessor register load or save instruction.
(In both of these, "concerning" means that the VFP hardware provides such an instruction as the reason for the fault, *not* that it is the faulting instruction.)
3. A combination of the exception bits (EX and DEX) on certain VFP implementations.
All of these can be summarised as "the hardware went wrong in some way" rather than "the user program did something wrong."
FPE_FLTINV means "floating point invalid operation". Does it really cover the case where hardware has failed, or is it intended to cover the case where userspace did something wrong and asked for an invalid operation from the FP hardware?
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 5 lines · Russell King - ARM Linux
FPE_FLTINVmeans "floating point invalid operation". Does it really cover the case where hardware has failed, or is it intended to cover the case where userspace did something wrong and asked for an invalid operation from the FP hardware?
Note that the number of people who actually look at the si_code is approximately zero.
But the ones that _do_ check the si_code are certainly not going to check it against a new code that they don't know about.
I suspect that if you start searching for FLT_xyz occurrences in code, approximately 100% of them are from the kernel code that generates them, not from any actual users.
So I'd be very surprised if you can find *anybody* who cares about that exact value (with the possible exceptions of test-suites).
Sadly, google code-search is no more. It was useful for things like that.
Linus
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 14 lines · Linus Torvalds
Quoted 5 lines · Russell King - ARM Linux
FPE_FLTINVmeans "floating point invalid operation". Does it really cover the case where hardware has failed, or is it intended to cover the case where userspace did something wrong and asked for an invalid operation from the FP hardware?Note that the number of people who actually look at the
si_codeis approximately zero.But the ones that _do_ check the
si_codeare certainly not going to check it against a new code that they don't know about.
Quoted 8 lines
I suspect that if you start searching for
FLT_xyzoccurrences in code, approximately 100% of them are from the kernel code that generates them, not from any actual users.So I'd be very surprised if you can find *anybody* who cares about that exact value (with the possible exceptions of test-suites).
Sadly, google code-search is no more. It was useful for things like that.
I've found https://codesearch.debian.net/ useful for digging into this kind of question, though it tends to throw up a lot of false positives.
Most uses I've seen do nothing more than use the FPE_xyz value to format diagnostic messages while dying. I struggled to find code that made a meaningful functional decision based on the value, though that's not proof...
Cheers
---Dave
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 5 lines · Dave Martin
Most uses I've seen do nothing more than use the
FPE_xyzvalue to format diagnostic messages while dying. I struggled to find code that made a meaningful functional decision based on the value, though that's not proof...
Yeah. I've seen code that cares about SIGFPE deeply, but it's almost invariably about some emulated environment (eg Java VM, or CPU emulation).
And the siginfo data is basically never good enough for those environments anyway on its own, so they will go and look at the actual instruction that caused the fault and the register state instead, because they need *all* the information.
The cases that use si_code are the ones that just trapped signals in order to give a more helpful abort message.
So I could certainly imagine that si_code is actually used by somebody who then decides to actuall act differently on it, but aside from perhaps printing out a different message, it sounds far-fetched.
Linus
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 16 lines · Linus Torvalds
Quoted 5 lines · Dave Martin
Most uses I've seen do nothing more than use the
FPE_xyzvalue to format diagnostic messages while dying. I struggled to find code that made a meaningful functional decision based on the value, though that's not proof...Yeah. I've seen code that cares about SIGFPE deeply, but it's almost invariably about some emulated environment (eg Java VM, or CPU emulation).
And the siginfo data is basically never good enough for those environments anyway on its own, so they will go and look at the actual instruction that caused the fault and the register state instead, because they need *all* the information.
Quoted 6 lines
The cases that use
si_codeare the ones that just trapped signals in order to give a more helpful abort message.So I could certainly imagine that
si_codeis actually used by somebody who then decides to actuall act differently on it, but aside from perhaps printing out a different message, it sounds far-fetched.
Okay, in that case let's just use FPE_FLTINV. That makes the patch easily back-portable for stable kernels.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 20 lines · Russell King - ARM Linux
Quoted 19 lines · Linus Torvalds
Quoted 5 lines · Dave Martin
Most uses I've seen do nothing more than use the
FPE_xyzvalue to format diagnostic messages while dying. I struggled to find code that made a meaningful functional decision based on the value, though that's not proof...Yeah. I've seen code that cares about SIGFPE deeply, but it's almost invariably about some emulated environment (eg Java VM, or CPU emulation).
And the siginfo data is basically never good enough for those environments anyway on its own, so they will go and look at the actual instruction that caused the fault and the register state instead, because they need *all* the information.
The cases that use
si_codeare the ones that just trapped signals in order to give a more helpful abort message.
Quoted 6 lines
Quoted 3 lines
So I could certainly imagine that
si_codeis actually used by somebody who then decides to actuall act differently on it, but aside from perhaps printing out a different message, it sounds far-fetched.Okay, in that case let's just use
FPE_FLTINV. That makes the patch easily back-portable for stable kernels.
If we want to I don't think backporting 266da65e9156 ("signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions") would be at all difficult.
What it is changing has been stable for quite a while. The surroundings might change and so it might require some trivial manual fixup but I don't expect any problems.
Not that I want to derail the consensus but if we want to backport similar fixes for arm64 or the other architectures that wind up using FPE_FLTUNK for their fix we would need to backport 266da65e9156 anyway.
Eric
cover Dealing with the aliases of SI_USER
In reply to Russell King - ARM LinuxLinus,
Would you consider the patchset below for -rc2?
Dealing with the aliases of SI_USER has been a challenge as we have had a b0rked ABI in some cases since 2.5.
So far no one except myself has suggested that changing the si_code of from 0 to something else for those problematic aliases of SI_USER is going to be a problem. So it looks like just fixing the issue is a real possibility.
Fixing the cases that do kill(SIGFPE, ...) because at least test cases care seems important.
The best fixes to backport appear to be the real architecture fixes that remove the aliases for SI_USER as those are deep fixes that fundamentally fix the problems, and are also very small changes.
I am not yet brave enough to merge architectural fixes like that without
arch maintainers buy-in. Getting at least an ack if nothing else takes
a little bit of time.
Still we have a arm fix upthread and David Miller has given his nod to a sparc fix that uses FPE_FLTUNK. So it appears real architecture fixes are progressing. Further I have looked and that leaves only
powerpc, parisc, ia64, and alpha. The new si_code FPE_FLTUNK appears
to address most of those, and there is an untested patch for parisc.
So real progress appears possible.
The generic code can do better, and that is what this rfc patchset is about. It ensures siginfo is fully initialized and uses copy_to_user to copy siginfo to userspace. This takes siginfo_layout out of the picture and so for non-compat non-signalfd siginfos the status quo returns to what it was before I introduced siginfo_layout (AKA regressions go bye-bye).
I believe given the issues these changes are a candiate for -rc2. Otherwise I will keep these changes for the next merge window.
Eric W. Biederman (3):
signal: Ensure every siginfo we send has all bits initialized
signal: Reduce copy_siginfo_to_user to just copy_to_user
signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout
arch/alpha/kernel/osf_sys.c | 1 + arch/alpha/kernel/signal.c | 2 + arch/alpha/kernel/traps.c | 5 ++ arch/alpha/mm/fault.c | 2 + arch/arc/mm/fault.c | 2 + arch/arm/kernel/ptrace.c | 1 + arch/arm/kernel/swp_emulate.c | 1 + arch/arm/kernel/traps.c | 5 ++ arch/arm/mm/alignment.c | 1 + arch/arm/mm/fault.c | 5 ++ arch/arm/vfp/vfpmodule.c | 3 +- arch/arm64/kernel/fpsimd.c | 2 +- arch/arm64/kernel/sys_compat.c | 1 + arch/arm64/kernel/traps.c | 1 + arch/arm64/mm/fault.c | 18 ++++-- arch/c6x/kernel/traps.c | 1 + arch/hexagon/kernel/traps.c | 1 + arch/hexagon/mm/vm_fault.c | 1 + arch/ia64/kernel/brl_emu.c | 1 + arch/ia64/kernel/signal.c | 2 + arch/ia64/kernel/traps.c | 27 ++++++++- arch/ia64/kernel/unaligned.c | 1 + arch/ia64/mm/fault.c | 4 +- arch/m68k/kernel/traps.c | 2 + arch/microblaze/kernel/exceptions.c | 1 + arch/microblaze/mm/fault.c | 4 +- arch/mips/mm/fault.c | 1 + arch/nds32/kernel/traps.c | 6 +- arch/nds32/mm/fault.c | 1 + arch/nios2/kernel/traps.c | 1 + arch/openrisc/kernel/traps.c | 5 +- arch/openrisc/mm/fault.c | 1 + arch/parisc/kernel/ptrace.c | 1 + arch/parisc/kernel/traps.c | 2 + arch/parisc/kernel/unaligned.c | 1 + arch/parisc/math-emu/driver.c | 1 + arch/parisc/mm/fault.c | 1 + arch/powerpc/kernel/process.c | 1 + arch/powerpc/kernel/traps.c | 3 +- arch/powerpc/mm/fault.c | 1 + arch/powerpc/platforms/cell/spufs/fault.c | 2 +- arch/riscv/kernel/traps.c | 1 + arch/s390/kernel/traps.c | 5 +- arch/s390/mm/fault.c | 2 + arch/sh/kernel/hw_breakpoint.c | 1 + arch/sh/kernel/traps_32.c | 2 + arch/sh/math-emu/math.c | 1 + arch/sh/mm/fault.c | 1 + arch/sparc/kernel/process_64.c | 1 + arch/sparc/kernel/sys_sparc_32.c | 1 + arch/sparc/kernel/traps_32.c | 10 ++++ arch/sparc/kernel/traps_64.c | 14 +++++ arch/sparc/kernel/unaligned_32.c | 1 + arch/sparc/mm/fault_32.c | 1 + arch/sparc/mm/fault_64.c | 1 + arch/um/kernel/trap.c | 2 + arch/unicore32/kernel/fpu-ucf64.c | 2 +- arch/unicore32/mm/fault.c | 3 + arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- arch/x86/kernel/ptrace.c | 2 +- arch/x86/kernel/traps.c | 3 + arch/x86/kernel/umip.c | 1 + arch/x86/kvm/mmu.c | 1 + arch/x86/mm/fault.c | 1 + arch/xtensa/kernel/traps.c | 1 + arch/xtensa/mm/fault.c | 1 + include/linux/ptrace.h | 1 - include/linux/tracehook.h | 1 + kernel/signal.c | 93 +------------------------------ virt/kvm/arm/mmu.c | 1 + 70 files changed, 165 insertions(+), 115 deletions(-)
Eric
RFC PATCH 1/3 signal: Ensure every siginfo we send has all bits initialized
Call clear_siginfo to ensure every stack allocated siginfo is properly initialized before being passed to the signal sending functions.
Note: It is not safe to depend on C initializers to initialize struct siginfo on the stack because C is allowed to skip holes when initializing a structure.
The initialization of struct siginfo in tracehook_report_syscall_exit was moved from the helper user_single_step_siginfo into tracehook_report_syscall_exit itself, to make it clear that the local variable siginfo gets fully initialized.
In a few cases the scope of struct siginfo has been reduced to make it clear that siginfo siginfo is not used on other paths in the function in which it is declared.
Instances of using memset to initialize siginfo have been replaced with calls clear_siginfo for clarity.
--- arch/alpha/kernel/osf_sys.c | 1 + arch/alpha/kernel/signal.c | 2 ++ arch/alpha/kernel/traps.c | 5 +++++ arch/alpha/mm/fault.c | 2 ++ arch/arc/mm/fault.c | 2 ++ arch/arm/kernel/ptrace.c | 1 + arch/arm/kernel/swp_emulate.c | 1 + arch/arm/kernel/traps.c | 5 +++++ arch/arm/mm/alignment.c | 1 + arch/arm/mm/fault.c | 5 +++++ arch/arm/vfp/vfpmodule.c | 3 +-- arch/arm64/kernel/fpsimd.c | 2 +- arch/arm64/kernel/sys_compat.c | 1 + arch/arm64/kernel/traps.c | 1 + arch/arm64/mm/fault.c | 18 ++++++++++++------ arch/c6x/kernel/traps.c | 1 + arch/hexagon/kernel/traps.c | 1 + arch/hexagon/mm/vm_fault.c | 1 + arch/ia64/kernel/brl_emu.c | 1 + arch/ia64/kernel/signal.c | 2 ++ arch/ia64/kernel/traps.c | 27 ++++++++++++++++++++++++--- arch/ia64/kernel/unaligned.c | 1 + arch/ia64/mm/fault.c | 4 +++- arch/m68k/kernel/traps.c | 2 ++ arch/microblaze/kernel/exceptions.c | 1 + arch/microblaze/mm/fault.c | 4 +++- arch/mips/mm/fault.c | 1 + arch/nds32/kernel/traps.c | 6 +++++- arch/nds32/mm/fault.c | 1 + arch/nios2/kernel/traps.c | 1 + arch/openrisc/kernel/traps.c | 5 ++++- arch/openrisc/mm/fault.c | 1 + arch/parisc/kernel/ptrace.c | 1 + arch/parisc/kernel/traps.c | 2 ++ arch/parisc/kernel/unaligned.c | 1 + arch/parisc/math-emu/driver.c | 1 + arch/parisc/mm/fault.c | 1 + arch/powerpc/kernel/process.c | 1 + arch/powerpc/kernel/traps.c | 3 +-- arch/powerpc/mm/fault.c | 1 + arch/powerpc/platforms/cell/spufs/fault.c | 2 +- arch/riscv/kernel/traps.c | 1 + arch/s390/kernel/traps.c | 5 ++++- arch/s390/mm/fault.c | 2 ++ arch/sh/kernel/hw_breakpoint.c | 1 + arch/sh/kernel/traps_32.c | 2 ++ arch/sh/math-emu/math.c | 1 + arch/sh/mm/fault.c | 1 + arch/sparc/kernel/process_64.c | 1 + arch/sparc/kernel/sys_sparc_32.c | 1 + arch/sparc/kernel/traps_32.c | 10 ++++++++++ arch/sparc/kernel/traps_64.c | 14 ++++++++++++++ arch/sparc/kernel/unaligned_32.c | 1 + arch/sparc/mm/fault_32.c | 1 + arch/sparc/mm/fault_64.c | 1 + arch/um/kernel/trap.c | 2 ++ arch/unicore32/kernel/fpu-ucf64.c | 2 +- arch/unicore32/mm/fault.c | 3 +++ arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- arch/x86/kernel/ptrace.c | 2 +- arch/x86/kernel/traps.c | 3 +++ arch/x86/kernel/umip.c | 1 + arch/x86/kvm/mmu.c | 1 + arch/x86/mm/fault.c | 1 + arch/xtensa/kernel/traps.c | 1 + arch/xtensa/mm/fault.c | 1 + include/linux/ptrace.h | 1 - include/linux/tracehook.h | 1 + virt/kvm/arm/mmu.c | 1 + 69 files changed, 163 insertions(+), 24 deletions(-)
arch/alpha/kernel/osf_sys.c +1 -0
@@ -881,6 +881,7 @@ SYSCALL_DEFINE5(osf_setsysinfo, unsigned long, op, void __user *, buffer,if (fex &IEEE_TRAP_ENABLE_DZE ) si_code =FPE_FLTDIV ;if (fex &IEEE_TRAP_ENABLE_INV ) si_code =FPE_FLTINV ; +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_code = si_code;
arch/alpha/kernel/signal.c +2 -0
@@ -221,6 +221,7 @@ do_sigreturn(struct sigcontext __user *sc)if (ptrace_cancel_bpt (current)) {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_BRKPT ;
@@ -255,6 +256,7 @@ do_rt_sigreturn(struct rt_sigframe __user *frame)if (ptrace_cancel_bpt (current)) {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_BRKPT ;
arch/alpha/kernel/traps.c +5 -0
@@ -228,6 +228,7 @@ do_entArith(unsigned long summary, unsigned long write_mask, }die_if_kernel ("Arithmetic fault" , regs,0 ,NULL ); +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_code = si_code;
@@ -241,6 +242,7 @@ do_entIF(unsigned long type, struct pt_regs *regs)siginfo_t info;int signo, code; +clear_siginfo (&info);if ((regs->ps & ~IPL_MAX ) ==0 ) {if (type ==1 ) {const unsigned int *data
@@ -430,6 +432,7 @@ do_entDbg(struct pt_regs *regs)die_if_kernel ("Instruction fault" , regs,0 ,NULL ); +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLOPC ;
@@ -761,6 +764,8 @@ do_entUnaUser(void __user * va, unsigned long opcode,siginfo_t info;long error; +clear_siginfo (&info); +/* Check the UAC bits to decide what the user wants us to do with the unaliged access. */
arch/alpha/mm/fault.c +2 -0
@@ -91,6 +91,8 @@ do_page_fault(unsigned long address, unsigned long mmcsr,siginfo_t info;unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ; +clear_siginfo (&info); +/* As of EV6, a load into $31/$f31 is a prefetch, and never faults (or is suppressed by the PALcode). Support that for older CPUs by ignoring such an instruction. */
arch/arc/mm/fault.c +2 -0
@@ -70,6 +70,8 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)int write = regs->ecr_cause &ECR_C_PROTV_STORE ;/* ST/EX */ unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ; +clear_siginfo (&info); +/* * We fault-in kernel-space virtual memory on-demand. The * 'reference' page table is init_mm.pgd.
arch/arm/kernel/ptrace.c +1 -0
@@ -205,6 +205,7 @@ void ptrace_break(struct task_struct *tsk, struct pt_regs *regs) {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_BRKPT ;
arch/arm/kernel/swp_emulate.c +1 -0
@@ -112,6 +112,7 @@ static void set_segfault(struct pt_regs *regs, unsigned long addr) {siginfo_t info; +clear_siginfo (&info);down_read (¤t->mm->mmap_sem);if (find_vma (current->mm, addr) ==NULL ) info.si_code =SEGV_MAPERR ;
arch/arm/kernel/traps.c +5 -0
@@ -439,6 +439,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)siginfo_t info;void __user *pc; +clear_siginfo (&info); pc = (void __user *)instruction_pointer (regs);if (processor_mode (regs) ==SVC_MODE ) {
@@ -537,6 +538,7 @@ static int bad_syscall(int n, struct pt_regs *regs) {siginfo_t info; +clear_siginfo (&info);if ((current->personality &PER_MASK ) !=PER_LINUX ) {send_sig (SIGSEGV , current,1 );return regs->ARM_r0;
@@ -604,6 +606,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs) {siginfo_t info; +clear_siginfo (&info);if ((no >>16 ) != (__ARM_NR_BASE>>16 ))return bad_syscall (no, regs);
@@ -740,6 +743,8 @@ baddataabort(int code, unsigned long instr, struct pt_regs *regs)unsigned long addr =instruction_pointer (regs);siginfo_t info; +clear_siginfo (&info); +#ifdef CONFIG_DEBUG_USER if (user_debug &UDBG_BADABORT ) { pr_err("[%d] %s: bad data abort: code %d instr 0x%08lx\n",
arch/arm/mm/alignment.c +1 -0
@@ -950,6 +950,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)if (ai_usermode &UM_SIGNAL ) {siginfo_t si; +clear_siginfo (&si); si.si_signo =SIGBUS ; si.si_errno =0 ; si.si_code =BUS_ADRALN ;
arch/arm/mm/fault.c +5 -0
@@ -163,6 +163,8 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr, {struct siginfo si; +clear_siginfo (&si); +#ifdef CONFIG_DEBUG_USER if (((user_debug &UDBG_SEGV ) && (sig ==SIGSEGV )) || ((user_debug &UDBG_BUS ) && (sig ==SIGBUS ))) {
@@ -550,6 +552,7 @@ do_DataAbort(unsigned long addr, unsigned int fsr, struct pt_regs *regs)const struct fsr_info *inf = fsr_info +fsr_fs (fsr);struct siginfo info; +if (!inf->fn (addr, fsr & ~FSR_LNX_PF , regs))return ;
@@ -557,6 +560,7 @@ do_DataAbort(unsigned long addr, unsigned int fsr, struct pt_regs *regs) inf->name, fsr, addr);show_pte (current->mm, addr); +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
@@ -589,6 +593,7 @@ do_PrefetchAbort(unsigned long addr, unsigned int ifsr, struct pt_regs *regs)pr_alert ("Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n" , inf->name, ifsr, addr); +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
arch/arm/vfp/vfpmodule.c +1 -2
@@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) {siginfo_t info; -memset (&info,0 ,sizeof (info)); - +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_code = sicode; info.si_addr = (void __user *)(instruction_pointer (regs) -4 );
arch/arm64/kernel/fpsimd.c +1 -1
@@ -882,7 +882,7 @@ asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) si_code =FPE_FLTRES ; } -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_code = si_code; info.si_addr = (void __user *)instruction_pointer (regs);
arch/arm64/kernel/sys_compat.c +1 -0
@@ -112,6 +112,7 @@ long compat_arm_syscall(struct pt_regs *regs)break ; } +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLTRP ;
arch/arm64/kernel/traps.c +1 -0
@@ -634,6 +634,7 @@ asmlinkage void bad_el0_sync(struct pt_regs *regs, int reason, unsigned int esr)siginfo_t info;void __user *pc = (void __user *)instruction_pointer (regs); +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLOPC ;
arch/arm64/mm/fault.c +12 -6
@@ -305,11 +305,12 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re*/ if (user_mode (regs)) {const struct fault_info *inf =esr_to_fault_info (esr); -struct siginfo si = { - .si_signo = inf->sig, - .si_code = inf->code, - .si_addr = (void __user *)addr, - }; +struct siginfo si; + +clear_siginfo (&si); + si.si_signo = inf->sig; + si.si_code = inf->code; + si.si_addr = (void __user *)addr;__do_user_fault (&si, esr); }else {
@@ -583,6 +584,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)nmi_exit (); } +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
@@ -687,6 +689,7 @@ asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,show_pte (addr); } +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
@@ -729,6 +732,7 @@ asmlinkage void __exception do_sp_pc_abort(unsigned long addr,local_irq_enable (); } +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
@@ -772,7 +776,6 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, struct pt_regs *regs) {const struct fault_info *inf = debug_fault_info +DBG_ESR_EVT (esr); -struct siginfo info;int rv;/*
@@ -788,6 +791,9 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,if (!inf->fn (addr, esr, regs)) { rv =1 ; }else { +struct siginfo info; + +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
arch/c6x/kernel/traps.c +1 -0
@@ -246,6 +246,7 @@ static void do_trap(struct exception_info *except_info, struct pt_regs *regs)unsigned long addr =instruction_pointer (regs);siginfo_t info; +clear_siginfo (&info);if (except_info->code !=TRAP_BRKPT ) pr_err("TRAP: %s PC[0x%lx] signo[%d] code[%d]\n", except_info->kernel_str, regs->pc,
arch/hexagon/kernel/traps.c +1 -0
@@ -414,6 +414,7 @@ void do_trap0(struct pt_regs *regs)if (user_mode (regs)) {struct siginfo info; +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ;/*
arch/hexagon/mm/vm_fault.c +1 -0
@@ -56,6 +56,7 @@ void do_page_fault(unsigned long address, long cause, struct pt_regs *regs)const struct exception_table_entry *fixup;unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ; +clear_siginfo (&info);/* * If we're in an interrupt or have no user context, * then must not take the fault.
arch/ia64/kernel/brl_emu.c +1 -0
@@ -62,6 +62,7 @@ ia64_emulate_brl (struct pt_regs *regs, unsigned long ar_ec)struct illegal_op_return rv;long tmp_taken, unimplemented_address; +clear_siginfo (&siginfo); rv.fkt = (unsigned long )-1 ;/*
arch/ia64/kernel/signal.c +2 -0
@@ -153,6 +153,7 @@ ia64_rt_sigreturn (struct sigscratch *scr)return retval; give_sigsegv: +clear_siginfo (&si); si.si_signo =SIGSEGV ; si.si_errno =0 ; si.si_code =SI_KERNEL ;
@@ -236,6 +237,7 @@ force_sigsegv_info (int sig, void __user *addr)unsigned long flags;struct siginfo si; +clear_siginfo (&si);if (sig ==SIGSEGV ) {/* * Acquiring siglock around the sa_handler-update is almost
arch/ia64/kernel/traps.c +24 -3
@@ -104,6 +104,7 @@ __kprobes ia64_bad_break (unsigned long break_num, struct pt_regs *regs)int sig, code;/* SIGILL, SIGFPE, SIGSEGV, and SIGBUS want these field initialized: */ +clear_siginfo (&siginfo); siginfo.si_addr = (void __user *) (regs->cr_iip +ia64_psr (regs)->ri); siginfo.si_imm = break_num; siginfo.si_flags =0 ;/* clear __ISR_VALID */
@@ -293,7 +294,6 @@ handle_fpu_swa (int fp_fault, struct pt_regs *regs, unsigned long isr) {long exception, bundle[2 ];unsigned long fault_ip; -struct siginfo siginfo; fault_ip = regs->cr_iip;if (!fp_fault && (ia64_psr (regs)->ri ==0 ))
@@ -344,10 +344,13 @@ handle_fpu_swa (int fp_fault, struct pt_regs *regs, unsigned long isr)printk (KERN_ERR "handle_fpu_swa: fp_emulate() returned -1\n" );return -1 ; }else { +struct siginfo siginfo; +/* is next instruction a trap? */ if (exception &2 ) {ia64_increment_ip (regs); } +clear_siginfo (&siginfo); siginfo.si_signo =SIGFPE ; siginfo.si_errno =0 ; siginfo.si_code =FPE_FIXME ;/* default code */
@@ -372,6 +375,9 @@ handle_fpu_swa (int fp_fault, struct pt_regs *regs, unsigned long isr)return -1 ; }else if (exception !=0 ) {/* raise exception */ +struct siginfo siginfo; + +clear_siginfo (&siginfo); siginfo.si_signo =SIGFPE ; siginfo.si_errno =0 ; siginfo.si_code =FPE_FIXME ;/* default code */
@@ -420,7 +426,7 @@ ia64_illegal_op_fault (unsigned long ec, long arg1, long arg2, long arg3,if (die_if_kernel (buf, ®s,0 ))return rv; -memset (&si,0 ,sizeof (si)); +clear_siginfo (&si); si.si_signo =SIGILL ; si.si_code =ILL_ILLOPC ; si.si_addr = (void __user *) (regs.cr_iip +ia64_psr (®s)->ri);
@@ -434,7 +440,6 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, long arg7, struct pt_regs regs) {unsigned long code, error = isr, iip; -struct siginfo siginfo;char buf[128 ];int result, sig;static const char *reason[] = {
@@ -485,6 +490,7 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa,case 26 :/* NaT Consumption */ if (user_mode (®s)) { +struct siginfo siginfo;void __user *addr;if (((isr >>4 ) &0xf ) ==2 ) {
@@ -499,6 +505,7 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, addr = (void __user *) (regs.cr_iip +ia64_psr (®s)->ri); } +clear_siginfo (&siginfo); siginfo.si_signo = sig; siginfo.si_code = code; siginfo.si_errno =0 ;
@@ -515,6 +522,9 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa,case 31 :/* Unsupported Data Reference */ if (user_mode (®s)) { +struct siginfo siginfo; + +clear_siginfo (&siginfo); siginfo.si_signo =SIGILL ; siginfo.si_code =ILL_ILLOPN ; siginfo.si_errno =0 ;
@@ -531,6 +541,10 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa,case 29 :/* Debug */ case 35 :/* Taken Branch Trap */ case 36 :/* Single Step Trap */ + { +struct siginfo siginfo; + +clear_siginfo (&siginfo);if (fsys_mode (current, ®s)) {extern char __kernel_syscall_via_break[];/*
@@ -578,11 +592,15 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, siginfo.si_isr = isr;force_sig_info (SIGTRAP , &siginfo, current);return ; + }case 32 :/* fp fault */ case 33 :/* fp trap */ result =handle_fpu_swa ((vector ==32 ) ?1 :0 , ®s, isr);if ((result <0 ) || (current->thread.flags &IA64_THREAD_FPEMU_SIGFPE )) { +struct siginfo siginfo; + +clear_siginfo (&siginfo); siginfo.si_signo =SIGFPE ; siginfo.si_errno =0 ; siginfo.si_code =FPE_FLTINV ;
@@ -616,6 +634,9 @@ ia64_fault (unsigned long vector, unsigned long isr, unsigned long ifa, }else {/* Unimplemented Instr. Address Trap */ if (user_mode (®s)) { +struct siginfo siginfo; + +clear_siginfo (&siginfo); siginfo.si_signo =SIGILL ; siginfo.si_code =ILL_BADIADDR ; siginfo.si_errno =0 ;
arch/ia64/kernel/unaligned.c +1 -0
@@ -1537,6 +1537,7 @@ ia64_handle_unaligned (unsigned long ifa, struct pt_regs *regs)/* NOT_REACHED */ } force_sigbus: +clear_siginfo (&si); si.si_signo =SIGBUS ; si.si_errno =0 ; si.si_code =BUS_ADRALN ;
arch/ia64/mm/fault.c +3 -1
@@ -85,7 +85,6 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *reint signal =SIGSEGV , code =SEGV_MAPERR ;struct vm_area_struct *vma, *prev_vma;struct mm_struct *mm = current->mm; -struct siginfo si;unsigned long mask;int fault;unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ;
@@ -249,6 +248,9 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *rereturn ; }if (user_mode (regs)) { +struct siginfo si; + +clear_siginfo (&si); si.si_signo = signal; si.si_errno =0 ; si.si_code = code;
arch/m68k/kernel/traps.c +2 -0
@@ -1011,6 +1011,7 @@ asmlinkage void trap_c(struct frame *fp)int vector = (fp->ptregs.vector >>2 ) &0xff ;siginfo_t info; +clear_siginfo (&info);if (fp->ptregs.sr &PS_S ) {if (vector ==VEC_TRACE ) {/* traced a trapping instruction on a 68020/30,
@@ -1163,6 +1164,7 @@ asmlinkage void fpemu_signal(int signal, int code, void *addr) {siginfo_t info; +clear_siginfo (&info); info.si_signo = signal; info.si_errno =0 ; info.si_code = code;
arch/microblaze/kernel/exceptions.c +1 -0
@@ -65,6 +65,7 @@ void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)if (kernel_mode (regs))die ("Exception in kernel mode" , regs, signr); +clear_siginfo (&info); info.si_signo = signr; info.si_errno =0 ; info.si_code = code;
arch/microblaze/mm/fault.c +3 -1
@@ -88,7 +88,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long address, {struct vm_area_struct *vma;struct mm_struct *mm = current->mm; -siginfo_t info;int code =SEGV_MAPERR ;int is_write = error_code &ESR_S ;int fault;
@@ -295,6 +294,9 @@ void do_page_fault(struct pt_regs *regs, unsigned long address, do_sigbus:up_read (&mm->mmap_sem);if (user_mode (regs)) { +siginfo_t info; + +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRERR ;
arch/mips/mm/fault.c +1 -0
@@ -63,6 +63,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,return ;#endif +clear_siginfo (&info); info.si_code =SEGV_MAPERR ;/*
arch/nds32/kernel/traps.c +5 -1
@@ -229,6 +229,7 @@ int bad_syscall(int n, struct pt_regs *regs)return regs->uregs[0 ]; } +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLTRP ;
@@ -292,7 +293,7 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, tsk->thread.trap_no =ENTRY_DEBUG_RELATED ; tsk->thread.error_code = error_code; -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_code = si_code; info.si_addr = (void __user *)instruction_pointer (regs);
@@ -323,6 +324,7 @@ void unhandled_interruption(struct pt_regs *regs)show_regs (regs);if (!user_mode (regs))do_exit (SIGKILL ); +clear_siginfo (&si); si.si_signo =SIGKILL ; si.si_errno =0 ;force_sig_info (SIGKILL , &si, current);
@@ -337,6 +339,7 @@ void unhandled_exceptions(unsigned long entry, unsigned long addr,show_regs (regs);if (!user_mode (regs))do_exit (SIGKILL ); +clear_siginfo (&si); si.si_signo =SIGKILL ; si.si_errno =0 ; si.si_addr = (void *)addr;
@@ -368,6 +371,7 @@ void do_revinsn(struct pt_regs *regs)show_regs (regs);if (!user_mode (regs))do_exit (SIGILL ); +clear_siginfo (&si); si.si_signo =SIGILL ; si.si_errno =0 ;force_sig_info (SIGILL , &si, current);
arch/nds32/mm/fault.c +1 -0
@@ -77,6 +77,7 @@ void do_page_fault(unsigned long entry, unsigned long addr,unsigned int mask =VM_READ |VM_WRITE |VM_EXEC ;unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ; +clear_siginfo (&info); error_code = error_code & (ITYPE_mskINST | ITYPE_mskETYPE); tsk = current; mm = tsk->mm;
arch/nios2/kernel/traps.c +1 -0
@@ -28,6 +28,7 @@ static void _send_sig(int signo, int code, unsigned long addr) {siginfo_t info; +clear_siginfo (&info); info.si_signo = signo; info.si_errno =0 ; info.si_code = code;
arch/openrisc/kernel/traps.c +4 -1
@@ -251,7 +251,7 @@ void __init trap_init(void) asmlinkage void do_trap(struct pt_regs *regs, unsigned long address) {siginfo_t info; -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_code =TRAP_TRACE ; info.si_addr = (void *)address;
@@ -266,6 +266,7 @@ asmlinkage void do_unaligned_access(struct pt_regs *regs, unsigned long address)if (user_mode (regs)) {/* Send a SIGBUS */ +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
@@ -285,6 +286,7 @@ asmlinkage void do_bus_fault(struct pt_regs *regs, unsigned long address)if (user_mode (regs)) {/* Send a SIGBUS */ +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRERR ;
@@ -485,6 +487,7 @@ asmlinkage void do_illegal_instruction(struct pt_regs *regs,if (user_mode (regs)) {/* Send a SIGILL */ +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLOPC ;
arch/openrisc/mm/fault.c +1 -0
@@ -56,6 +56,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address,int fault;unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ; +clear_siginfo (&info); tsk = current;/*
arch/parisc/kernel/ptrace.c +1 -0
@@ -90,6 +90,7 @@ void user_enable_single_step(struct task_struct *task)ptrace_disable (task);/* Don't wake up the task, but let the parent know something happened. */ +clear_siginfo (&si); si.si_code =TRAP_TRACE ; si.si_addr = (void __user *) (task_regs (task)->iaoq[0 ] & ~3 ); si.si_signo =SIGTRAP ;
arch/parisc/kernel/traps.c +2 -0
@@ -299,6 +299,7 @@ static void handle_gdb_break(struct pt_regs *regs, int wot) {struct siginfo si; +clear_siginfo (&si); si.si_signo =SIGTRAP ; si.si_errno =0 ; si.si_code = wot;
@@ -489,6 +490,7 @@ void notrace handle_interruption(int code, struct pt_regs *regs)unsigned long fault_space =0 ;struct siginfo si; +clear_siginfo (&si);if (code ==1 )pdc_console_restart ();/* switch back to pdc if HPMC */ else
arch/parisc/kernel/unaligned.c +1 -0
@@ -455,6 +455,7 @@ void handle_unaligned(struct pt_regs *regs)struct siginfo si; registerint flop=0 ;/* true if this is a flop */ +clear_siginfo (&si);__inc_irq_stat (irq_unaligned_count);/* log a message with pacing */
arch/parisc/math-emu/driver.c +1 -0
@@ -93,6 +93,7 @@ handle_fpe(struct pt_regs *regs)*/ __u64 frcopy[36 ]; +clear_siginfo (&si);memcpy (frcopy, regs->fr,sizeof regs->fr); frcopy[32 ] =0 ;
arch/parisc/mm/fault.c +1 -0
@@ -356,6 +356,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,struct siginfo si;unsigned int lsb =0 ; +clear_siginfo (&si);switch (code) {case 15 :/* Data TLB miss fault/Data page fault */ /* send SIGSEGV when outside of vma */
arch/powerpc/kernel/process.c +1 -0
@@ -632,6 +632,7 @@ void do_break (struct pt_regs *regs, unsigned long address,hw_breakpoint_disable ();/* Deliver the signal to userspace */ +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_HWBKPT ;
arch/powerpc/kernel/traps.c +1 -2
@@ -296,7 +296,6 @@ NOKPROBE_SYMBOL(die);void user_single_step_siginfo (struct task_struct *tsk,struct pt_regs *regs,siginfo_t *info) { -memset (info,0 ,sizeof (*info)); info->si_signo =SIGTRAP ; info->si_code =TRAP_TRACE ; info->si_addr = (void __user *)regs->nip;
@@ -334,7 +333,7 @@ void _exception_pkey(int signr, struct pt_regs *regs, int code,*/ thread_pkey_regs_save (¤t->thread); -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info); info.si_signo = signr; info.si_code = code; info.si_addr = (void __user *) addr;
arch/powerpc/mm/fault.c +1 -0
@@ -168,6 +168,7 @@ static int do_sigbus(struct pt_regs *regs, unsigned long address,return SIGBUS ; current->thread.trap_nr =BUS_ADRERR ; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRERR ;
arch/powerpc/platforms/cell/spufs/fault.c +1 -1
@@ -44,7 +44,7 @@ static void spufs_handle_event(struct spu_context *ctx,return ; } -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info);switch (type) {case SPE_EVENT_INVALID_DMA :
arch/riscv/kernel/traps.c +1 -0
@@ -68,6 +68,7 @@ static inline void do_trap_siginfo(int signo, int code, {siginfo_t info; +clear_siginfo (&info); info.si_signo = signo; info.si_errno =0 ; info.si_code = code;
arch/s390/kernel/traps.c +4 -1
@@ -47,6 +47,7 @@ void do_report_trap(struct pt_regs *regs, int si_signo, int si_code, char *str)siginfo_t info;if (user_mode (regs)) { +clear_siginfo (&info); info.si_signo = si_signo; info.si_errno =0 ; info.si_code = si_code;
@@ -86,6 +87,7 @@ void do_per_trap(struct pt_regs *regs)return ;if (!current->ptrace)return ; +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_HWBKPT ;
@@ -165,7 +167,6 @@ void translation_exception(struct pt_regs *regs)void illegal_op (struct pt_regs *regs) { -siginfo_t info;__u8 opcode[6 ];__u16 __user *location;int is_uprobe_insn =0 ;
@@ -178,6 +179,8 @@ void illegal_op(struct pt_regs *regs)return ;if (*((__u16 *) opcode) ==S390_BREAKPOINT_U16 ) {if (current->ptrace) { +siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_BRKPT ;
arch/s390/mm/fault.c +2 -0
@@ -268,6 +268,7 @@ static noinline void do_sigsegv(struct pt_regs *regs, int si_code)struct siginfo si;report_user_fault (regs,SIGSEGV ,1 ); +clear_siginfo (&si); si.si_signo =SIGSEGV ; si.si_errno =0 ; si.si_code = si_code;
@@ -323,6 +324,7 @@ static noinline void do_sigbus(struct pt_regs *regs)* Send a sigbus, regardless of whether we were in kernel * or user mode. */ +clear_siginfo (&si); si.si_signo =SIGBUS ; si.si_errno =0 ; si.si_code =BUS_ADRERR ;
arch/sh/kernel/hw_breakpoint.c +1 -0
@@ -349,6 +349,7 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args)if (!arch_check_bp_in_kernelspace (bp)) {siginfo_t info; +clear_siginfo (&info); info.si_signo = args->signr; info.si_errno =notifier_to_errno (rc); info.si_code =TRAP_HWBKPT ;
arch/sh/kernel/traps_32.c +2 -0
@@ -537,6 +537,7 @@ asmlinkage void do_address_error(struct pt_regs *regs,"access (PC %lx PR %lx)\n" , current->comm, regs->pc, regs->pr); +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code = si_code;
@@ -600,6 +601,7 @@ asmlinkage void do_divide_error(unsigned long r4) {siginfo_t info; +clear_siginfo (&info);switch (r4) {case TRAP_DIVZERO_ERROR : info.si_code =FPE_INTDIV ;
arch/sh/math-emu/math.c +1 -0
@@ -560,6 +560,7 @@ static int ieee_fpe_handler(struct pt_regs *regs) ~(FPSCR_CAUSE_MASK |FPSCR_FLAG_MASK );task_thread_info (tsk)->status |=TS_USEDFPU ; }else { +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_code =FPE_FLTINV ;
arch/sh/mm/fault.c +1 -0
@@ -44,6 +44,7 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address, {siginfo_t info; +clear_siginfo (&info); info.si_signo = si_signo; info.si_errno =0 ; info.si_code = si_code;
arch/sparc/kernel/process_64.c +1 -0
@@ -520,6 +520,7 @@ static void stack_unaligned(unsigned long sp) {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
arch/sparc/kernel/sys_sparc_32.c +1 -0
@@ -152,6 +152,7 @@ sparc_breakpoint (struct pt_regs *regs)#ifdef DEBUG_SPARC_BREAKPOINT printk ("TRAP: Entering kernel PC=%x, nPC=%x\n" , regs->pc, regs->npc);#endif +clear_siginfo (&info); info.si_signo =SIGTRAP ; info.si_errno =0 ; info.si_code =TRAP_BRKPT ;
arch/sparc/kernel/traps_32.c +10 -0
@@ -104,6 +104,7 @@ void do_hw_interrupt(struct pt_regs *regs, unsigned long type)if (regs->psr &PSR_PS )die_if_kernel ("Kernel bad trap" , regs); +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLTRP ;
@@ -124,6 +125,7 @@ void do_illegal_instruction(struct pt_regs *regs, unsigned long pc, unsigned lon regs->pc, *(unsigned long *)regs->pc);#endif +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLOPC ;
@@ -139,6 +141,7 @@ void do_priv_instruction(struct pt_regs *regs, unsigned long pc, unsigned long nif (psr &PSR_PS )die_if_kernel ("Penguin instruction from Penguin mode??!?!" , regs); +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_PRVOPC ;
@@ -165,6 +168,7 @@ void do_memaccess_unaligned(struct pt_regs *regs, unsigned long pc, unsigned loninstruction_dump ((unsigned long *) regs->pc);printk ("do_MNA!\n" );#endif +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
@@ -303,6 +307,7 @@ void do_fpe_trap(struct pt_regs *regs, unsigned long pc, unsigned long npc, } fsr = fpt->thread.fsr; +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_addr = (void __user *)pc;
@@ -336,6 +341,7 @@ void handle_tag_overflow(struct pt_regs *regs, unsigned long pc, unsigned long nif (psr &PSR_PS )die_if_kernel ("Penguin overflow trap from kernel mode" , regs); +clear_siginfo (&info); info.si_signo =SIGEMT ; info.si_errno =0 ; info.si_code =EMT_TAGOVF ;
@@ -365,6 +371,7 @@ void handle_reg_access(struct pt_regs *regs, unsigned long pc, unsigned long npcprintk ("Register Access Exception at PC %08lx NPC %08lx PSR %08lx\n" , pc, npc, psr);#endif +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_OBJERR ;
@@ -378,6 +385,7 @@ void handle_cp_disabled(struct pt_regs *regs, unsigned long pc, unsigned long np {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_COPROC ;
@@ -395,6 +403,7 @@ void handle_cp_exception(struct pt_regs *regs, unsigned long pc, unsigned long nprintk ("Co-Processor Exception at PC %08lx NPC %08lx PSR %08lx\n" , pc, npc, psr);#endif +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_COPROC ;
@@ -408,6 +417,7 @@ void handle_hw_divzero(struct pt_regs *regs, unsigned long pc, unsigned long npc {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_code =FPE_INTDIV ;
arch/sparc/kernel/traps_64.c +14 -0
@@ -107,6 +107,7 @@ void bad_trap(struct pt_regs *regs, long lvl) regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLTRP ;
@@ -206,6 +207,7 @@ void spitfire_insn_access_exception(struct pt_regs *regs, unsigned long sfsr, un regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGSEGV ; info.si_errno =0 ; info.si_code =SEGV_MAPERR ;
@@ -247,6 +249,7 @@ void sun4v_insn_access_exception(struct pt_regs *regs, unsigned long addr, unsig regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGSEGV ; info.si_errno =0 ; info.si_code =SEGV_MAPERR ;
@@ -338,6 +341,7 @@ void spitfire_data_access_exception(struct pt_regs *regs, unsigned long sfsr, unif (is_no_fault_exception (regs))return ; +clear_siginfo (&info); info.si_signo =SIGSEGV ; info.si_errno =0 ; info.si_code =SEGV_MAPERR ;
@@ -595,6 +599,7 @@ static void spitfire_ue_log(unsigned long afsr, unsigned long afar, unsigned lon regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_OBJERR ;
@@ -2211,6 +2216,7 @@ bool sun4v_nonresum_error_user_handled(struct pt_regs *regs, addr +=PAGE_SIZE ; } } +clear_siginfo (&info); info.si_signo =SIGKILL ; info.si_errno =0 ; info.si_trapno =0 ;
@@ -2221,6 +2227,7 @@ bool sun4v_nonresum_error_user_handled(struct pt_regs *regs,if (attrs &SUN4V_ERR_ATTRS_PIO ) {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_code =BUS_ADRERR ; info.si_addr = (void __user *)sun4v_get_vaddr (regs);
@@ -2368,6 +2375,7 @@ static void do_fpe_common(struct pt_regs *regs) regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_addr = (void __user *)regs->tpc;
@@ -2440,6 +2448,7 @@ void do_tof(struct pt_regs *regs) regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGEMT ; info.si_errno =0 ; info.si_code =EMT_TAGOVF ;
@@ -2465,6 +2474,7 @@ void do_div0(struct pt_regs *regs) regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_code =FPE_INTDIV ;
@@ -2666,6 +2676,7 @@ void do_illegal_instruction(struct pt_regs *regs) } } } +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_ILLOPC ;
@@ -2692,6 +2703,7 @@ void mem_address_unaligned(struct pt_regs *regs, unsigned long sfar, unsigned loif (is_no_fault_exception (regs))return ; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
@@ -2717,6 +2729,7 @@ void sun4v_do_mna(struct pt_regs *regs, unsigned long addr, unsigned long type_cif (is_no_fault_exception (regs))return ; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
@@ -2785,6 +2798,7 @@ void do_privop(struct pt_regs *regs) regs->tpc &=0xffffffff ; regs->tnpc &=0xffffffff ; } +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_PRVOPC ;
arch/sparc/kernel/unaligned_32.c +1 -0
@@ -313,6 +313,7 @@ static void user_mna_trap_fault(struct pt_regs *regs, unsigned int insn) {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
arch/sparc/mm/fault_32.c +1 -0
@@ -129,6 +129,7 @@ static void __do_fault_siginfo(int code, int sig, struct pt_regs *regs, {siginfo_t info; +clear_siginfo (&info); info.si_signo = sig; info.si_code = code; info.si_errno =0 ;
arch/sparc/mm/fault_64.c +1 -0
@@ -172,6 +172,7 @@ static void do_fault_siginfo(int code, int sig, struct pt_regs *regs,unsigned long addr;siginfo_t info; +clear_siginfo (&info); info.si_code = code; info.si_signo = sig; info.si_errno =0 ;
arch/um/kernel/trap.c +2 -0
@@ -164,6 +164,7 @@ static void bad_segv(struct faultinfo fi, unsigned long ip) {struct siginfo si; +clear_siginfo (&si); si.si_signo =SIGSEGV ; si.si_code =SEGV_ACCERR ; si.si_addr = (void __user *)FAULT_ADDRESS (fi);
@@ -220,6 +221,7 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,int is_write =FAULT_WRITE (fi);unsigned long address =FAULT_ADDRESS (fi); +clear_siginfo (&si);if (!is_user && regs) current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
arch/unicore32/kernel/fpu-ucf64.c +1 -1
@@ -56,7 +56,7 @@ void ucf64_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) {siginfo_t info; -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_code = sicode;
arch/unicore32/mm/fault.c +3 -0
@@ -125,6 +125,7 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr, tsk->thread.address = addr; tsk->thread.error_code = fsr; tsk->thread.trap_no =14 ; +clear_siginfo (&si); si.si_signo = sig; si.si_errno =0 ; si.si_code = code;
@@ -472,6 +473,7 @@ asmlinkage void do_DataAbort(unsigned long addr, unsigned int fsr,printk (KERN_ALERT "Unhandled fault: %s (0x%03x) at 0x%08lx\n" , inf->name, fsr, addr); +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
@@ -491,6 +493,7 @@ asmlinkage void do_PrefetchAbort(unsigned long addr,printk (KERN_ALERT "Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n" , inf->name, ifsr, addr); +clear_siginfo (&info); info.si_signo = inf->sig; info.si_errno =0 ; info.si_code = inf->code;
arch/x86/entry/vsyscall/vsyscall_64.c +1 -1
@@ -107,7 +107,7 @@ static bool write_ok_or_segv(unsigned long ptr, size_t size) thread->cr2 = ptr; thread->trap_nr =X86_TRAP_PF ; -memset (&info,0 ,sizeof (info)); +clear_siginfo (&info); info.si_signo =SIGSEGV ; info.si_errno =0 ; info.si_code =SEGV_MAPERR ;
arch/x86/kernel/ptrace.c +1 -1
@@ -1377,7 +1377,6 @@ static void fill_sigtrap_info(struct task_struct *tsk, tsk->thread.trap_nr =X86_TRAP_DB ; tsk->thread.error_code = error_code; -memset (info,0 ,sizeof (*info)); info->si_signo =SIGTRAP ; info->si_code = si_code; info->si_addr =user_mode (regs) ? (void __user *)regs->ip :NULL ;
@@ -1395,6 +1394,7 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, {struct siginfo info; +clear_siginfo (&info);fill_sigtrap_info (tsk, regs, error_code, si_code, &info);/* Send us the fake SIGTRAP */ force_sig_info (SIGTRAP , &info, tsk);
arch/x86/kernel/traps.c +3 -0
@@ -299,6 +299,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,if (notify_die (DIE_TRAP , str, regs, error_code, trapnr, signr) !=NOTIFY_STOP ) {cond_local_irq_enable (regs); +clear_siginfo (&info);do_trap (trapnr, signr, str, regs, error_code,fill_trap_info (regs, signr, trapnr, &info)); }
@@ -854,6 +855,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr) task->thread.trap_nr = trapnr; task->thread.error_code = error_code; +clear_siginfo (&info); info.si_signo =SIGFPE ; info.si_errno =0 ; info.si_addr = (void __user *)uprobe_get_trap_addr (regs);
@@ -929,6 +931,7 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)RCU_LOCKDEP_WARN (!rcu_is_watching (),"entry code didn't wake RCU" );local_irq_enable (); +clear_siginfo (&info); info.si_signo =SIGILL ; info.si_errno =0 ; info.si_code =ILL_BADSTK ;
arch/x86/kernel/umip.c +1 -0
@@ -278,6 +278,7 @@ static void force_sig_info_umip_fault(void __user *addr, struct pt_regs *regs) tsk->thread.error_code =X86_PF_USER |X86_PF_WRITE ; tsk->thread.trap_nr =X86_TRAP_PF ; +clear_siginfo (&info); info.si_signo =SIGSEGV ; info.si_errno =0 ; info.si_code =SEGV_MAPERR ;
arch/x86/kvm/mmu.c +1 -0
@@ -3007,6 +3007,7 @@ static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct * {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_MCEERR_AR ;
arch/x86/mm/fault.c +1 -0
@@ -209,6 +209,7 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address,unsigned lsb =0 ;siginfo_t info; +clear_siginfo (&info); info.si_signo = si_signo; info.si_errno =0 ; info.si_code = si_code;
arch/xtensa/kernel/traps.c +1 -0
@@ -334,6 +334,7 @@ do_unaligned_user (struct pt_regs *regs)"(pid = %d, pc = %#010lx)\n" , regs->excvaddr, current->comm, task_pid_nr(current), regs->pc); +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_ADRALN ;
arch/xtensa/mm/fault.c +1 -0
@@ -45,6 +45,7 @@ void do_page_fault(struct pt_regs *regs)int fault;unsigned int flags =FAULT_FLAG_ALLOW_RETRY |FAULT_FLAG_KILLABLE ; +clear_siginfo (&info); info.si_code =SEGV_MAPERR ;/* We fault-in kernel-space virtual memory on-demand. The
include/linux/ptrace.h +0 -1
@@ -345,7 +345,6 @@ extern void user_single_step_siginfo(struct task_struct *tsk,static inline void user_single_step_siginfo (struct task_struct *tsk,struct pt_regs *regs,siginfo_t *info) { -memset (info,0 ,sizeof (*info)); info->si_signo =SIGTRAP ; }#endif
include/linux/tracehook.h +1 -0
@@ -124,6 +124,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step) {if (step) {siginfo_t info; +clear_siginfo (&info);user_single_step_siginfo (current, regs, &info);force_sig_info (SIGTRAP , &info, current);return ;
virt/kvm/arm/mmu.c +1 -0
@@ -1289,6 +1289,7 @@ static void kvm_send_hwpoison_signal(unsigned long address, {siginfo_t info; +clear_siginfo (&info); info.si_signo =SIGBUS ; info.si_errno =0 ; info.si_code =BUS_MCEERR_AR ;
Signature · 1 line
2.14.1
Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized
Quoted 13 lines · Eric W. Biederman
Call
clear_siginfoto ensure every stack allocated siginfo is properly initialized before being passed to the signal sending functions.Note: It is not safe to depend on C initializers to initialize struct siginfo on the stack because C is allowed to skip holes when initializing a structure.
The initialization of struct siginfo in
tracehook_report_syscall_exitwas moved from the helperuser_single_step_siginfointotracehook_report_syscall_exititself, to make it clear that the local variable siginfo gets fully initialized.
Quoted 8 lines
In a few cases the scope of struct siginfo has been reduced to make it clear that siginfo siginfo is not used on other paths in the function in which it is declared.
Instances of using memset to initialize siginfo have been replaced with calls
clear_siginfofor clarity.Signed-off-by "Eric W. Biederman" <[email protected]>
[...]
Hmmm
memset()/clear_siginfo() may ensure that there are no uninitialised explicit fields except for those in inactive union members, but I'm not sure that this approach is guaranteed to sanitise the padding seen by userspace.
Rationale below, though it's a bit theoretical...
With this in mind, I tend agree with Linus that hiding memset() calls from the maintainer may be a bad idea unless they are also hidden from the compiler. If the compiler sees the memset() it may be able to optimise it in ways that wouldn't be possible for some other random external function call, including optimising all or part of the call out.
As a result, the breakdown into individual put_user()s etc. in copy_siginfo_to_user() may still be valuable even if all paths have the memset().
(Rationale for an arch/arm example:)
arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 222
Quoted 12 lines
arch/arm/vfp/vfpmodule.c +1
-2@@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) {siginfo_t info; -memset (&info,0 ,sizeof (info)); - +clear_siginfo (&info); info.si_signo =SIGFPE ;
/* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take
unspecified values */
arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 224
Quoted 2 lines
arch/arm/vfp/vfpmodule.c +0
-0info.si_code = sicode; info.si_addr = (void __user *)(instruction_pointer (regs) -4 );
/* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields
other than than those corresponding to _sigfault take unspecified values */
So I don't see why the compiler needs to ensure that any of the affected bytes are zero: it could potentially skip a lot of the memset() as a result, in theory.
I've not seen a compiler actually take advantage of that, but I'm now not sure what forbids it.
If this can happen, I only see two watertight workarounds:
1) Ensure that there is no implicit padding in any UAPI structure, e.g.aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding in fpr_set()"). This would include tail-padding of any union member that is smaller than the containing union.
It would be significantly more effort to ensure this for siginfo though.
2) Poke all values directly into allocated or user memory directly via pointers to paddingless types; never assign to objects on the kernel stack if you care what ends up in the padding, e.g., what your copy_siginfo_to_user() does prior to this series.
If I'm not barking up the wrong tree, memset() cannot generally be used to determine the value of padding bytes, but it may still be useful for forcing otherwise uninitialised members to sane initial values.
This likely affects many more things than just siginfo.
[...]
Cheers
---Dave
[1] n1570 6.2.6.1.6: When a value is stored in an object of structure or union type, including in a member object, the bytes of the object representation that correspond to any padding bytes take unspecified values [...]
[2] n1570 6.2.6.1.7: When a value is stored in a member of an object of union type, the bytes of the object representation that do not correspond to that member but do correspond to other members take unspecified values.
Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized
Quoted 16 lines · Dave Martin
Hmmm
memset()/clear_siginfo() may ensure that there are no uninitialised explicit fields except for those in inactive union members, but I'm not sure that this approach is guaranteed to sanitise the padding seen by userspace.
Rationale below, though it's a bit theoretical...
With this in mind, I tend agree with Linus that hiding
memset()calls from the maintainer may be a bad idea unless they are also hidden from the compiler. If the compiler sees thememset()it may be able to optimise it in ways that wouldn't be possible for some other random external function call, including optimising all or part of the call out.
Quoted 3 lines
As a result, the breakdown into individual put_user()s etc. in
copy_siginfo_to_user()may still be valuable even if all paths have thememset().
The breakdown into individual put_user()s is known to be problematically slow, and is actually wrong.
Even exclusing the SI_USER duplication in a small number of cases the fields filled out in siginfo by architecture code are not the fields that copy_siginfo_to_user is copying. Which is much worse. The code looks safe but is not.
My intention is to leave 0 instances of clear_siginfo in the architecture specific code. Ideally struct siginfo will be limited to kernel/signal.c but I am not certain I can quite get that far. The function do_coredump appears to have a legit need for siginfo.
Quoted 25 lines
(Rationale for an
arch/armexample:)arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 222
Quoted 12 lines
arch/arm/vfp/vfpmodule.c +1
-2@@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) {siginfo_t info; -memset (&info,0 ,sizeof (info)); - +clear_siginfo (&info); info.si_signo =SIGFPE ;/* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take
unspecified values */arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 224
Quoted 2 lines
arch/arm/vfp/vfpmodule.c +0
-0info.si_code = sicode; info.si_addr = (void __user *)(instruction_pointer (regs) -4 );/* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields
other than than those corresponding to_sigfaulttake unspecified values */
Quoted 6 lines
So I don't see why the compiler needs to ensure that any of the affected bytes are zero: it could potentially skip a lot of the
memset()as a result, in theory.I've not seen a compiler actually take advantage of that, but I'm now not sure what forbids it.
I took a quick look at gcc-4.9 which I have handy.
The passes -f-no-strict-aliasing which helps, and gcc actually documents that if you access things through the union it will not take advantage of c11.
gcc-4.9 Documents it this way:
Quoted 43 lines
-fstrict-aliasing'
Allow the compiler to assume the strictest aliasing rules applicable to the language being compiled. For C (and C++), this activates optimizations based on the type of expressions. In particular, an object of one type is assumed never to reside at the same address as an object of a different type, unless the types are almost the same. For example, an 'unsigned int' can alias an 'int', but not a 'void*' or a 'double'. A character type may alias any other type.Pay special attention to code like this:
uniona_union{
int i;
double d;
};int
f(){
uniona_uniont;
t.d = 3.0;
return t.i;
}
The practice of reading from a different union member than the one most recently written to (called "type-punning") is common. Even with '-fstrict-aliasing', type-punning is allowed, provided the memory is accessed through the union type. So, the code above works as expected.If this can happen, I only see two watertight workarounds:
1) Ensure that there is no implicit padding in any UAPI structure, e.g.
aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding infpr_set()"). This would include tail-padding of any union member that is smaller than the containing union.It would be significantly more effort to ensure this for siginfo though.
2) Poke all values directly into allocated or user memory directly via pointers to paddingless types; never assign to objects on the kernel stack if you care what ends up in the padding, e.g., what your
copy_siginfo_to_user()does prior to this series.
Quoted 6 lines
If I'm not barking up the wrong tree,
memset()cannot generally be used to determine the value of padding bytes, but it may still be useful for forcing otherwise uninitialised members to sane initial values.This likely affects many more things than just siginfo.
Unless gcc has changed it's stance on type-punning through unions or it's semantics with -fno-strict_aliasing we should be good.
Eric
Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized
Quoted 18 lines · Eric W. Biederman
Quoted 16 lines · Dave Martin
Hmmm
memset()/clear_siginfo() may ensure that there are no uninitialised explicit fields except for those in inactive union members, but I'm not sure that this approach is guaranteed to sanitise the padding seen by userspace.
Rationale below, though it's a bit theoretical...
With this in mind, I tend agree with Linus that hiding
memset()calls from the maintainer may be a bad idea unless they are also hidden from the compiler. If the compiler sees thememset()it may be able to optimise it in ways that wouldn't be possible for some other random external function call, including optimising all or part of the call out.
Quoted 6 lines
Quoted 3 lines
As a result, the breakdown into individual put_user()s etc. in
copy_siginfo_to_user()may still be valuable even if all paths have thememset().The breakdown into individual put_user()s is known to be problematically slow, and is actually wrong.
Slowness certainly looked like a potential problem.
Quoted 9 lines
Even exclusing the
SI_USERduplication in a small number of cases the fields filled out in siginfo by architecture code are not the fields thatcopy_siginfo_to_useris copying. Which is much worse. The code looks safe but is not.My intention is to leave 0 instances of
clear_siginfoin the architecture specific code. Ideally struct siginfo will be limited tokernel/signal.cbut I am not certain I can quite get that far. The functiondo_coredumpappears to have a legit need for siginfo.
So, you mean we can't detect that the caller didn't initialise all the members, or initialised the wrong union member?
What would be the alternative? Have a separate interface for each SIL_ type, with only kernel/signal.c translating that into the siginfo_t that userspace sees?
Either way, I don't see how we force the caller to initilise the whole structure.
Quoted 32 lines
Quoted 25 lines
(Rationale for an
arch/armexample:)arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 222
Quoted 12 lines
arch/arm/vfp/vfpmodule.c +1
-2@@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) {siginfo_t info; -memset (&info,0 ,sizeof (info)); - +clear_siginfo (&info); info.si_signo =SIGFPE ;/* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take
unspecified values */arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 224
Quoted 2 lines
arch/arm/vfp/vfpmodule.c +0
-0info.si_code = sicode; info.si_addr = (void __user *)(instruction_pointer (regs) -4 );/* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields
other than than those corresponding to_sigfaulttake unspecified values */Quoted 6 lines
So I don't see why the compiler needs to ensure that any of the affected bytes are zero: it could potentially skip a lot of the
memset()as a result, in theory.I've not seen a compiler actually take advantage of that, but I'm now not sure what forbids it.
Quoted 34 lines
I took a quick look at gcc-4.9 which I have handy.
The passes -f-no-strict-aliasing which helps, and gcc actually documents that if you access things through the union it will not take advantage of c11.
gcc-4.9 Documents it this way:
Quoted 26 lines
-fstrict-aliasing'
Allow the compiler to assume the strictest aliasing rules applicable to the language being compiled. For C (and C++), this activates optimizations based on the type of expressions. In particular, an object of one type is assumed never to reside at the same address as an object of a different type, unless the types are almost the same. For example, an 'unsigned int' can alias an 'int', but not a 'void*' or a 'double'. A character type may alias any other type.Pay special attention to code like this:
uniona_union{
int i;
double d;
};int
f(){
uniona_uniont;
t.d = 3.0;
return t.i;
}
The practice of reading from a different union member than the one most recently written to (called "type-punning") is common. Even with '-fstrict-aliasing', type-punning is allowed, provided the memory is accessed through the union type. So, the code above works as expected.
This makes the C standard look precise (I love the "works as expected"), and says nothing about the cumulative effect of assigning to multiple members of a union, or about the effects on padding bytes.
I'm not convinced that all of this falls under strict-aliasing, but I'd have to do more digging to confirm it.
Quoted 20 lines
Quoted 20 lines
If this can happen, I only see two watertight workarounds:
1) Ensure that there is no implicit padding in any UAPI structure, e.g.
aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding infpr_set()"). This would include tail-padding of any union member that is smaller than the containing union.It would be significantly more effort to ensure this for siginfo though.
2) Poke all values directly into allocated or user memory directly via pointers to paddingless types; never assign to objects on the kernel stack if you care what ends up in the padding, e.g., what your
copy_siginfo_to_user()does prior to this series.If I'm not barking up the wrong tree,
memset()cannot generally be used to determine the value of padding bytes, but it may still be useful for forcing otherwise uninitialised members to sane initial values.
Quoted 4 lines
Quoted 1 line
This likely affects many more things than just siginfo.
Unless gcc has changed it's stance on type-punning through unions or it's semantics with -fno-strict_aliasing we should be good.
In practice you're probably right.
Today, gcc is pretty conservative in this area, and I haven't been able to convince clang to optimise away memset in this way either.
My concern is that is this assumption turns out to be wrong it may be some time before anybody notices, because the leakage of kernel stack may be the only symptom.
I'll try to nail down a compiler guy to see if we can get a promise on this at least with -fno-strict-aliasing.
I wonder whether it's worth protecting ourselves with something like:
static void clear_siginfo(siginfo_t *si)
{
asm ("" : "=m" (*si));
memset(si, 0, sizeof(*si));
asm ("" : "+m" (*si));
}Probably needs to be thought about more widely though. I guess it's out of scope for this series.
Cheers
---Dave
Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized
Quoted 33 lines · Dave Martin
Quoted 18 lines · Eric W. Biederman
Quoted 16 lines · Dave Martin
Hmmm
memset()/clear_siginfo() may ensure that there are no uninitialised explicit fields except for those in inactive union members, but I'm not sure that this approach is guaranteed to sanitise the padding seen by userspace.
Rationale below, though it's a bit theoretical...
With this in mind, I tend agree with Linus that hiding
memset()calls from the maintainer may be a bad idea unless they are also hidden from the compiler. If the compiler sees thememset()it may be able to optimise it in ways that wouldn't be possible for some other random external function call, including optimising all or part of the call out.Quoted 6 lines
Quoted 3 lines
As a result, the breakdown into individual put_user()s etc. in
copy_siginfo_to_user()may still be valuable even if all paths have thememset().The breakdown into individual put_user()s is known to be problematically slow, and is actually wrong.
Slowness certainly looked like a potential problem.
Quoted 5 lines
Even exclusing the
SI_USERduplication in a small number of cases the fields filled out in siginfo by architecture code are not the fields thatcopy_siginfo_to_useris copying. Which is much worse. The code looks safe but is not.
Quoted 7 lines
Quoted 4 lines
My intention is to leave 0 instances of
clear_siginfoin the architecture specific code. Ideally struct siginfo will be limited tokernel/signal.cbut I am not certain I can quite get that far. The functiondo_coredumpappears to have a legit need for siginfo.So, you mean we can't detect that the caller didn't initialise all the members, or initialised the wrong union member?
Correct. Even when we smuggled the the union member in the upper bits of si_code we got it wrong. So an interface that helps out and does more and is harder to misues looks desirable.
Quoted 3 lines
What would be the alternative? Have a separate interface for each
SIL_type, with onlykernel/signal.ctranslating that into thesiginfo_tthat userspace sees?
Yes. It really isn't bad as architecture specific code only generates faults. In general faults only take a pointer. I have already merged the needed helpers into kernel/signal.c
Quoted 2 lines
Either way, I don't see how we force the caller to initilise the whole structure.
In general the plan is to convert the callers to call force_sig_fault, and then there is no need to have siginfo in the architecture specific code. I have all of the necessary helpers are already merged into kernel/signal.c
Quoted 117 lines
Quoted 33 lines
Quoted 26 lines
(Rationale for an
arch/armexample:)arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 222
Quoted 12 lines
arch/arm/vfp/vfpmodule.c +1
-2@@ -218,8 +218,7 @@ static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) {siginfo_t info; -memset (&info,0 ,sizeof (info)); - +clear_siginfo (&info); info.si_signo =SIGFPE ;/* by c11 (n1570) 6.2.6.1 para 6 [1], all padding bytes in info now take
unspecified values */arch/arm/vfp/vfpmodule.c static void vfp_raise_sigfpe(unsigned int sicode, struct pt_regs *regs) · line 224
Quoted 2 lines
arch/arm/vfp/vfpmodule.c +0
-0info.si_code = sicode; info.si_addr = (void __user *)(instruction_pointer (regs) -4 );/* by c11 (n1570) 6.2.6.1 para 7 [2], all bytes of the union info._sifields
other than than those corresponding to_sigfaulttake unspecified values */Quoted 6 lines
So I don't see why the compiler needs to ensure that any of the affected bytes are zero: it could potentially skip a lot of the
memset()as a result, in theory.I've not seen a compiler actually take advantage of that, but I'm now not sure what forbids it.
Quoted 34 lines
I took a quick look at gcc-4.9 which I have handy.
The passes -f-no-strict-aliasing which helps, and gcc actually documents that if you access things through the union it will not take advantage of c11.
gcc-4.9 Documents it this way:
Quoted 26 lines
-fstrict-aliasing'
Allow the compiler to assume the strictest aliasing rules applicable to the language being compiled. For C (and C++), this activates optimizations based on the type of expressions. In particular, an object of one type is assumed never to reside at the same address as an object of a different type, unless the types are almost the same. For example, an 'unsigned int' can alias an 'int', but not a 'void*' or a 'double'. A character type may alias any other type.Pay special attention to code like this:
uniona_union{
int i;
double d;
};int
f(){
uniona_uniont;
t.d = 3.0;
return t.i;
}
The practice of reading from a different union member than the one most recently written to (called "type-punning") is common. Even with '-fstrict-aliasing', type-punning is allowed, provided the memory is accessed through the union type. So, the code above works as expected.This makes the C standard look precise (I love the "works as expected"), and says nothing about the cumulative effect of assigning to multiple members of a union, or about the effects on padding bytes.
I'm not convinced that all of this falls under strict-aliasing, but I'd have to do more digging to confirm it.
Quoted 20 lines
Quoted 20 lines
If this can happen, I only see two watertight workarounds:
1) Ensure that there is no implicit padding in any UAPI structure, e.g.
aeb1f39d814b: ("arm64/ptrace: Avoid uninitialised struct padding infpr_set()"). This would include tail-padding of any union member that is smaller than the containing union.It would be significantly more effort to ensure this for siginfo though.
2) Poke all values directly into allocated or user memory directly via pointers to paddingless types; never assign to objects on the kernel stack if you care what ends up in the padding, e.g., what your
copy_siginfo_to_user()does prior to this series.If I'm not barking up the wrong tree,
memset()cannot generally be used to determine the value of padding bytes, but it may still be useful for forcing otherwise uninitialised members to sane initial values.Quoted 4 lines
Quoted 1 line
This likely affects many more things than just siginfo.
Unless gcc has changed it's stance on type-punning through unions or it's semantics with -fno-strict_aliasing we should be good.
In practice you're probably right.
Today, gcc is pretty conservative in this area, and I haven't been able to convince clang to optimise away memset in this way either.
My concern is that is this assumption turns out to be wrong it may be some time before anybody notices, because the leakage of kernel stack may be the only symptom.
I'll try to nail down a compiler guy to see if we can get a promise on this at least with -fno-strict-aliasing.
I wonder whether it's worth protecting ourselves with something like:
static void
clear_siginfo(siginfo_t *si)
Quoted 8 lines
{ asm ("" : "=m" (*si)); memset(si, 0, sizeof(*si)); asm ("" : "+m" (*si)); }Probably needs to be thought about more widely though. I guess it's out of scope for this series.
It is definitely a question worth asking.
Eric
Re: [RFC PATCH 1/3] signal: Ensure every siginfo we send has all bits initialized
Quoted 3 lines · Eric W. Biederman
Quoted 1 line · Dave Martin
On Tue, Apr 17, 2018 at 02:37:38PM -0500, Eric W. Biederman wrote:
[...]
Quoted 12 lines
Quoted 7 lines
Quoted 4 lines
My intention is to leave 0 instances of
clear_siginfoin the architecture specific code. Ideally struct siginfo will be limited tokernel/signal.cbut I am not certain I can quite get that far. The functiondo_coredumpappears to have a legit need for siginfo.So, you mean we can't detect that the caller didn't initialise all the members, or initialised the wrong union member?
Correct. Even when we smuggled the the union member in the upper bits of
si_codewe got it wrong. So an interface that helps out and does more and is harder to misues looks desirable.
Quoted 7 lines
Quoted 3 lines
What would be the alternative? Have a separate interface for each
SIL_type, with onlykernel/signal.ctranslating that into thesiginfo_tthat userspace sees?Yes. It really isn't bad as architecture specific code only generates faults. In general faults only take a pointer. I have already merged the needed helpers into
kernel/signal.c
Good point. I hadn't considered that only one class of signal comes from the arch code, but now that you point it out, it sounds right.
Quoted 7 lines
Quoted 2 lines
Either way, I don't see how we force the caller to initilise the whole structure.
In general the plan is to convert the callers to call
force_sig_fault, and then there is no need to have siginfo in the architecture specific code. I have all of the necessary helpers are already merged intokernel/signal.c
Makes sense.
I wonder if all the relevant siginfo data could be passed to force_sig_fault() (or whatever) as arguments. Then the problem of uninitialised fields goes away. Perhaps that's what you had in mind.
[...]
Quoted 26 lines
Quoted 26 lines
Quoted 2 lines
Unless gcc has changed it's stance on type-punning through unions or it's semantics with -fno-strict_aliasing we should be good.
In practice you're probably right.
Today, gcc is pretty conservative in this area, and I haven't been able to convince clang to optimise away memset in this way either.
My concern is that is this assumption turns out to be wrong it may be some time before anybody notices, because the leakage of kernel stack may be the only symptom.
I'll try to nail down a compiler guy to see if we can get a promise on this at least with -fno-strict-aliasing.
I wonder whether it's worth protecting ourselves with something like:
static void
clear_siginfo(siginfo_t *si){ asm ("" : "=m" (*si)); memset(si, 0, sizeof(*si)); asm ("" : "+m" (*si)); }
Quoted 4 lines
Quoted 2 lines
Probably needs to be thought about more widely though. I guess it's out of scope for this series.
It is definitely a question worth asking.
I may follow it up later if I find myself at a loose end...
Cheers
---Dave
RFC PATCH 2/3 signal: Reduce copy_siginfo_to_user to just copy_to_user
In reply to Eric W. BiedermanNow that every instance of struct siginfo is now initialized it is no longer necessary to copy struct siginfo piece by piece to userspace but instead the entire structure can be copied.
As well as making the code simpler and more efficient this means that copy_sinfo_to_user no longer cares which union member of struct siginfo is in use.
In practice this means that all 32bit architectures that define FPE_FIXME will handle properly send SI_USER when kill(SIGFPE) is sent. While still performing their historic architectural brokenness when 0 is used a floating pointer signal. This matches the current behavior of 64bit architectures that define FPE_FIXME who get lucky and an overloaded SI_USER has continuted to work through copy_siginfo_to_user because the 8 byte si_addr occupies the same bytes in struct siginfo as the 4 byte si_pid and the 4 byte si_uid.
Problematic architectures still need to fix their ABI so that signalfd and 32bit compat code will work properly.
--- kernel/signal.c | 84 ++------------------------------------------------------- 1 file changed, 2 insertions(+), 82 deletions(-)
kernel/signal.c +2 -82
@@ -2850,89 +2850,9 @@ enum siginfo_layout siginfo_layout(int sig, int si_code)int copy_siginfo_to_user (siginfo_t __user *to,const siginfo_t *from) { -int err; - -if (!access_ok (VERIFY_WRITE , to,sizeof (siginfo_t))) +if (copy_to_user (to, from ,sizeof (struct siginfo )))return -EFAULT ; -if (from->si_code <0 ) -return __copy_to_user (to, from,sizeof (siginfo_t)) - ? -EFAULT :0 ; -/* -* If you change siginfo_t structure, please be sure -* this code is fixed accordingly. -* Please remember to update the signalfd_copyinfo() function -* inside fs/signalfd.c too, in case siginfo_t changes. -* It should never copy any pad contained in the structure -* to avoid security leaks, but must copy the generic -* 3 ints plus the relevant union member. -*/ - err =__put_user (from->si_signo, &to->si_signo); - err |=__put_user (from->si_errno, &to->si_errno); - err |=__put_user (from->si_code, &to->si_code); -switch (siginfo_layout (from->si_signo, from->si_code)) { -case SIL_KILL : - err |=__put_user (from->si_pid, &to->si_pid); - err |=__put_user (from->si_uid, &to->si_uid); -break ; -case SIL_TIMER : -/* Unreached SI_TIMER is negative */ -break ; -case SIL_POLL : - err |=__put_user (from->si_band, &to->si_band); - err |=__put_user (from->si_fd, &to->si_fd); -break ; -case SIL_FAULT : - err |=__put_user (from->si_addr, &to->si_addr); -#ifdef __ARCH_SI_TRAPNO - err |=__put_user (from->si_trapno, &to->si_trapno); -#endif -#ifdef __ia64__ - err |=__put_user (from->si_imm, &to->si_imm); - err |=__put_user (from->si_flags, &to->si_flags); - err |=__put_user (from->si_isr, &to->si_isr); -#endif -/* -* Other callers might not initialize the si_lsb field, -* so check explicitly for the right codes here. -*/ -#ifdef BUS_MCEERR_AR -if (from->si_signo ==SIGBUS && from->si_code ==BUS_MCEERR_AR ) - err |=__put_user (from->si_addr_lsb, &to->si_addr_lsb); -#endif -#ifdef BUS_MCEERR_AO -if (from->si_signo ==SIGBUS && from->si_code ==BUS_MCEERR_AO ) - err |=__put_user (from->si_addr_lsb, &to->si_addr_lsb); -#endif -#ifdef SEGV_BNDERR -if (from->si_signo ==SIGSEGV && from->si_code ==SEGV_BNDERR ) { - err |=__put_user (from->si_lower, &to->si_lower); - err |=__put_user (from->si_upper, &to->si_upper); - } -#endif -#ifdef SEGV_PKUERR -if (from->si_signo ==SIGSEGV && from->si_code ==SEGV_PKUERR ) - err |=__put_user (from->si_pkey, &to->si_pkey); -#endif -break ; -case SIL_CHLD : - err |=__put_user (from->si_pid, &to->si_pid); - err |=__put_user (from->si_uid, &to->si_uid); - err |=__put_user (from->si_status, &to->si_status); - err |=__put_user (from->si_utime, &to->si_utime); - err |=__put_user (from->si_stime, &to->si_stime); -break ; -case SIL_RT : - err |=__put_user (from->si_pid, &to->si_pid); - err |=__put_user (from->si_uid, &to->si_uid); - err |=__put_user (from->si_ptr, &to->si_ptr); -break ; -case SIL_SYS : - err |=__put_user (from->si_call_addr, &to->si_call_addr); - err |=__put_user (from->si_syscall, &to->si_syscall); - err |=__put_user (from->si_arch, &to->si_arch); -break ; - } -return err; +return 0 ; }#ifdef CONFIG_COMPAT
Signature · 1 line
2.14.1
RFC PATCH 3/3 signal: Stop special casing TRAP_FIXME and FPE_FIXME in siginfo_layout
In reply to Eric W. BiedermanAfter more experience with the cases where no one the si_code of 0 is used both as a signal specific si_code, and as SI_USER it appears that no one cares about the signal specific si_code case and the good solution is to just fix the architectures by using a different si_code.
In none of the conversations has anyone even suggested that anything depends on the signal specific redefinition of SI_USER.
There are at least test cases that care when si_code as 0 does not work as si_user.
So make things simple and keep the generic code from introducing problems by removing the special casing of TRAP_FIXME and FPE_FIXME. This will ensure the generic case of sending a signal with kill will always set SI_USER and work.
The architecture specific, and signal specific overloads that set si_code to 0 will now have problems with signalfd and the 32bit compat versions of siginfo copying. At least until they are fixed.
--- kernel/signal.c | 9 --------- 1 file changed, 9 deletions(-)
kernel/signal.c +0 -9
@@ -2835,15 +2835,6 @@ enum siginfo_layout siginfo_layout(int sig, int si_code) layout =SIL_POLL ; else if (si_code < 0) layout =SIL_RT ; -/* Tests to support buggy kernel ABIs */ -#ifdef TRAP_FIXME -if ((sig ==SIGTRAP ) && (si_code ==TRAP_FIXME )) - layout =SIL_FAULT ; -#endif -#ifdef FPE_FIXME -if ((sig ==SIGFPE ) && (si_code ==FPE_FIXME )) - layout =SIL_FAULT ; -#endif }return layout; }
Signature · 1 line
2.14.1
Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER
In reply to Eric W. Biederman(
Quoted 2 lines · Eric W. Biederman
Would you consider the patchset below for -rc2?
Ugh.
I have an irrational dislike of "clear_siginfo()". It's a nasty broken interface, which just mis-spells "memset()", and makes it non-obvious that you could clear it other ways too (eg "kzalloc()" or whatever).
And this series brings in a lot of new users.
Honestly, both "clear_siginfo()" and "copy_siginfo()" are crazy interfaces. They may have made sense when converting code, but not so much now.
If we want to have a function that initializes a siginfo, I think it should _actually_ initialize it, and at least take the two fields that a siginfo has to have to be valid: si_signo and si_code.
So I'd rather see a patch that does something like this:
- clear_siginfo(info); - info->si_signo = sig; - info->si_errno = 0; - info->si_code = SI_USER; - info->si_pid = 0; - info->si_uid = 0; + init_siginfo(info, sig, SI_USER);
which at least makes that function be *worth* something, not just a bad spelling of memset. It's not just the removal of pointless "set to zero", it's the whole concept of "this function actually makes a valid siginfo", which is lacking in the existing function.
(Yeah, yeah, si_errno is "generic" too and always part of a siginfo, but nobody cares. It's pretty much always set to zero, it would be stupid to add that interface to the "init_siginfo()" function. So just clearing it is fine, the one or two places that want to set it to some silly value can do it themselves).
Then your series would incidentally also:
(a) make for fewer lines overall, rather than add lines
(b) make it clear that we always initialize si_code, which now *must*
be a valid value with all the recent siginfo changes.
Hmm?
The other thing we should do is to get rid of the stupid padding. Right now "struct siginfo" is pointlessly padded to 128 bytes. That is completely insane, when it's always just zero in the kernel.
So put that _pad[] thing inside #ifndef __KERNEL__, and make copy_siginfo_to_user() write the padding zeroes when copying to user space. The reason for the padding is "future expansion", so we do want to tell the user space that it's maybe up to 128 bytes in size, but if we don't fill it all, we shouldn't waste time and memory on clearing the padding internally.
I'm certainly *hoping* nobody depends on the whole 128 bytes in rt_sigqueueinfo(). In theory you can fill it all (as long as si_code is negative), but the man-page only says "si_value", and the compat function doesn't copy any more than that either, so any user that tries to fill in more than si_value is already broken. In fact, it might even be worth enforcing that in rt_sigqueueinfo(), just to see if anybody plays any games..
On x86-64, without the pointless padding, the size of 'struct siginfo' inside the kernel would be 48 bytes. That's quite a big difference for something that is often allocated on the kernel stack.
So I'm certainly willing to make those kinds of changes, but let's make them real *improvements* now, ok? Wasn't that the point of all the cleanups in the end?
Linus
Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER
Quoted 8 lines · Linus Torvalds
(
Quoted 2 lines · Eric W. Biederman
Would you consider the patchset below for -rc2?
Ugh.
The point of this series is to squash the potential for regressions even from the weird broken code that fills in fields for si_code 0 that do not match SI_USER.
The copy_siginfo_to_user32 never handled that so we don't need to worry about that. All of the signals that abuse si_code 0 go through force_sig_info so signalfd can not catch them. Which means that only copy_siginfo_to_user needs to be worried about.
Last time I was benchmarking I could not see a difference between copying the entire siginfo and only a small part so I don't see the point of optimizing now.
For an actual cleanup I intend to go farther than you are proposing and convert everthing to the set of helper functions I have added to kernel/signal.c force_sig_fault, force_sig_mceerr, force_sig_bnderr, force_sig_pkuerr.
When I am done there will be maybe 5 instances of clear_siginfo outside of those helper functions. At which point I won't care if we remove clear_siginfo and just use memset. That is a real improvement
that looks something like: "105 files changed, 933 insertions(+), 1698 deletions(-)" That is my end goal with all of this.
You complained to me about regressions and you are right with the current handling of FPE_FIXME TRAP_FIXME the code is not bug compatible with old versions of linux, and people have noticed.
What this patchset represents is the bare minimum needed to convert copy_siginfo_to_user to a copy_to_user, and remove the possibility of regressions.
To that end I need to ensure every struct siginfo in the entire kernel is memset to 0. A structure initializer is absolutely not enough. So I don't mind people thinking clear_siginfo is necessary.
To that end clear_siginfo where it does not take the size of struct siginfo as a parameter is much less suceptible to typos, and allows me to ensure every instance of struct siginfo is fully initialized.
I fully intend to remove practically every instance of struct siginfo from the architecture specific code, and use the aforementioned helpers. The trouble is that some of the architecture code need refactoring for that to happen. Even your proposed init_siginfo can't be widely used as a common pattern is to fill in siginfo partially in the code with si_code and si_signo being filled in almost last.
So in short. I intend to remove most of these clear_siginfo calls when I remove the siginfos later. This series is focused on making copy_siginfo_to_user simple enough we don't need to use siginfo_layout, and thus can be fully backwards compatibile with ourselves and we won't need to worry about regressions. I have aimed to keep this simple enough we can merge this after -rc1 because we don't want regressions.
Eric
ps. I intend to place this change first in my series wether or not it makes it into -rc2 so that I can be certain we remove any possible regressions in behavior on the buggy architectures. Then I can take my time and ensure the non-trivial changes of refactoring etc are done carefully so I don't introduce other bugs. I need that so I can sleep at night.
pps. I can look at some of your other suggestions but cleverness leads to regressions, and if you are going to complain at me harshly when I have been being careful and taking things seriously I am not particularly willing to take unnecessary chances.
Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER
In reply to Linus TorvaldsQuoted 4 lines · Linus Torvalds
(
On Sun, Apr 15, 2018 at 8:56 AM, Eric W. Biederman <[email protected]> wrote:
[snip bit about wanting what is effectively force_sig_fault instead of clear_siginfo everywhere]
Quoted 11 lines
The other thing we should do is to get rid of the stupid padding. Right now "struct siginfo" is pointlessly padded to 128 bytes. That is completely insane, when it's always just zero in the kernel.
So put that _pad[] thing inside #ifndef
__KERNEL__, and makecopy_siginfo_to_user()write the padding zeroes when copying to user space. The reason for the padding is "future expansion", so we do want to tell the user space that it's maybe up to 128 bytes in size, but if we don't fill it all, we shouldn't waste time and memory on clearing the padding internally.
Quoted 7 lines
I'm certainly *hoping* nobody depends on the whole 128 bytes in
rt_sigqueueinfo(). In theory you can fill it all (as long assi_codeis negative), but the man-page only says "si_value", and the compat function doesn't copy any more than that either, so any user that tries to fill in more thansi_valueis already broken. In fact, it might even be worth enforcing that inrt_sigqueueinfo(), just to see if anybody plays any games..
From my earlier looking I think this is all doable except detecting if
Quoted 3 lines
On x86-64, without the pointless padding, the size of 'struct siginfo' inside the kernel would be 48 bytes. That's quite a big difference for something that is often allocated on the kernel stack.
From my earlier looking I can say that I know of no case where signals are injected into the kernel that we need more bytes than the kernel currently provides.
The two cases I know of are signal reinjection for checkpoint/restart
or something that just uses pid, uid and ptr. Generally that is
enough.
If we just truncate siginfo to everything except the pad bytes in the kernel there should be no problems. What I don't see how to do is to limit the size of struct siginfo the kernel accepts in a forward compatible way. Something that would fail if for some reason you used more siginfo bytes.
Looking at userspace. glibc always memsets siginfo_t to 0. Criu just uses whatever it captured with PTRACE_PEEKSIGINFO, and criu uses unitialized memory for the target of PTRACE_PEEKSIGINFO.
If we truncate siginfo in the kernel then we check for a known si_code in which case we are safe to truncate siginfo. If the si_code is unknown then we should check to see if the extra bytes are 0. That should work with everything. Plus it is a natural point to test to see if userspace is using signals that the kernel does not currently support.
I will put that in my queue.
Quoted 3 lines
So I'm certainly willing to make those kinds of changes, but let's make them real *improvements* now, ok? Wasn't that the point of all the cleanups in the end?
Definitely. With the strace test case causing people to talk about regressions I was just looking to see if it would make sense to do something minimal for -rc2 so reduce concerns about regressions.
Now I am going to focus on getting what I can ready for the next merge window.
Eric
Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER
In reply to Linus TorvaldsOn Sun, Apr 15, 2018 at 11:16:04AM -0700, Linus Torvalds wrote:
[...]
Quoted 3 lines
The other thing we should do is to get rid of the stupid padding. Right now "struct siginfo" is pointlessly padded to 128 bytes. That is completely insane, when it's always just zero in the kernel.
Agreed, inside the kernel the padding achieves nothing.
Quoted 7 lines
So put that _pad[] thing inside #ifndef
__KERNEL__, and makecopy_siginfo_to_user()write the padding zeroes when copying to user space. The reason for the padding is "future expansion", so we do want to tell the user space that it's maybe up to 128 bytes in size, but if we don't fill it all, we shouldn't waste time and memory on clearing the padding internally.
Quoted 7 lines
I'm certainly *hoping* nobody depends on the whole 128 bytes in
rt_sigqueueinfo(). In theory you can fill it all (as long assi_codeis negative), but the man-page only says "si_value", and the compat function doesn't copy any more than that either, so any user that tries to fill in more thansi_valueis already broken. In fact, it might even be worth enforcing that inrt_sigqueueinfo(), just to see if anybody plays any games..
[...]
Digression:
Since we don't traditionally zero the tail-padding in the user sigframe, is there a reliable way for userspace to detect newly-added fields in siginfo other than by having an explicit sigaction sa_flags flag to request them? I imagine something like [1] below from the userspace perspective.
On a separate thread, the issue of how to report syndrome information for SIGSEGV came up [2] (such as whether the faulting instruction was a read or a write). This information is useful (and used) by things like userspace sanitisers and qemu. Currently, reporting this to userspace relies on arch-specific cruft in the sigframe.
We're committed to maintaining what's already in each arch sigframe, but it would be preferable to have a portable way of adding information to siginfo in a generic way. si_code doesn't really work for that, since si_codes are mutually exclusive: I can't see a way of adding supplementary information using si_code.
Anyway, that would be a separate RFC in the future (if ever).
Cheers
---Dave
[1]
static volatile int have_extflags = 0;
static void handler(int n, siginfo_t *si, void *uc)
{
/* ... */
if (have_extflags) {
/* Check si->si_extflags */
} else {
/* fallback */
}
/* ... */
}int main(void)
{
/* ... */
struct sigaction sa;
/* ... */
sa.sa_flags = SA_SIGINFO | SA_SIGINFO_EXTFLAGS;
sa.sa_sigaction = handler;
if (!sigaction(SIGSEGV, &sa, NULL)) {
have_extflags = 1;
} else {
sa.sa_flags &= ~SA_SIGINFO_EXTFLAGS;
if (sigaction(SIGSEGV, &sa, NULL))
goto error;
}
/* ... */
}[2] [RFC PATCH] arm64: fault: Don't leak data in ESR context for user fault on kernel VA
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/571428.html
Re: [RFC PATCH 0/3] Dealing with the aliases of SI_USER
Quoted 41 lines · Dave Martin
On Sun, Apr 15, 2018 at 11:16:04AM -0700, Linus Torvalds wrote:
[...]
Quoted 3 lines
The other thing we should do is to get rid of the stupid padding. Right now "struct siginfo" is pointlessly padded to 128 bytes. That is completely insane, when it's always just zero in the kernel.
Agreed, inside the kernel the padding achieves nothing.
Quoted 7 lines
So put that _pad[] thing inside #ifndef
__KERNEL__, and makecopy_siginfo_to_user()write the padding zeroes when copying to user space. The reason for the padding is "future expansion", so we do want to tell the user space that it's maybe up to 128 bytes in size, but if we don't fill it all, we shouldn't waste time and memory on clearing the padding internally.Quoted 7 lines
I'm certainly *hoping* nobody depends on the whole 128 bytes in
rt_sigqueueinfo(). In theory you can fill it all (as long assi_codeis negative), but the man-page only says "si_value", and the compat function doesn't copy any more than that either, so any user that tries to fill in more thansi_valueis already broken. In fact, it might even be worth enforcing that inrt_sigqueueinfo(), just to see if anybody plays any games..[...]
Digression:
Since we don't traditionally zero the tail-padding in the user sigframe, is there a reliable way for userspace to detect newly-added fields in siginfo other than by having an explicit sigaction
sa_flagsflag to request them? I imagine something like [1] below from the userspace perspective.On a separate thread, the issue of how to report syndrome information for SIGSEGV came up [2] (such as whether the faulting instruction was a read or a write). This information is useful (and used) by things like userspace sanitisers and qemu. Currently, reporting this to userspace relies on arch-specific cruft in the sigframe.
Quoted 7 lines
We're committed to maintaining what's already in each arch sigframe, but it would be preferable to have a portable way of adding information to siginfo in a generic way.
si_codedoesn't really work for that, sincesi_codesare mutually exclusive: I can't see a way of adding supplementary information usingsi_code.Anyway, that would be a separate RFC in the future (if ever).
So far what I have seen done is ``registers'' added to sigcontext. Which it looks like you have done with esr.
Scrubbing information from faults to where the addresses point outside of the userspace mapping makes sense.
I think before I would pursue what you are talking about on a generic level I would want to look at the fact that we handle unblockable faults wrong. While unlikely it is possible for someone to send a thread specific signal at just the right time, and have that signal delivered before the synchronous fault.
Then we could pass through additional arguments through that new ``generic'' path. Especially what are arguments such as tsk->thread.fault_address and tsk->thread.fault_code.
We can do anything we like with a new SA_flag as that allows us to change the format of the sigframe.
If we are very careful we can add generic fields after that crazy union anonymous union in the _sigfault case of struct siginfo.
The trick would be to find something that would be enough so that people don't need to implement their own instruction decoder to see what is going on. Something that is applicable to every sigfault case not just SIGSEGV. Something that we can and want to implement on multiple architectures.
The point being doing something generic can be a lot of work, even if it is worth it in the end.
Quoted 38 lines
[1]
static volatile int
have_extflags= 0;static void
handler(int n, siginfo_t *si, void *uc){ /* ... */ if (have_extflags) { /* Check si->si_extflags */ } else { /* fallback */ } /* ... */ }int main(void)
{ /* ... */ struct sigaction sa; /* ... */ sa.sa_flags = SA_SIGINFO | SA_SIGINFO_EXTFLAGS; sa.sa_sigaction = handler; if (!sigaction(SIGSEGV, &sa, NULL)) { have_extflags = 1; } else { sa.sa_flags &= ~SA_SIGINFO_EXTFLAGS; if (sigaction(SIGSEGV, &sa, NULL)) goto error; } /* ... */ }
Quoted 2 lines
[2] [RFC PATCH] arm64: fault: Don't leak data in ESR context for user fault on kernel VA
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/571428.html
Eric
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
In reply to Russell King - ARM LinuxQuoted 42 lines · Russell King - ARM Linux
Quoted 22 lines · Dave Martin
Quoted 13 lines · Linus Torvalds
Quoted 3 lines · Russell King - ARM Linux
Yes, it does solve the problem at hand with strace - the exact patch I tested against 4.16 is below.
Ok, good.
Quoted 2 lines
However,
FPE_FLTUNKis not defined in older kernels, so while we can fix it this way for the current merge window, that doesn't help 4.16.I wonder if we should even bother with
FPE_FLTUNK.Quoted 7 lines
I suspect we might as well use
FPE_FLTINV, I suspect, and not have this complexity at all. That case is not worth worrying about, since it's a "this shouldn't happen anyway" and the *real* reason will be in the kernel logs due tovfs_panic().So it's not like this is something that the user should ever care about the
si_codeabout.Quoted 8 lines
Ack, my intended meaning for
FPE_FLTUNKis that the fp exception is either spurious or we can't tell easily (or possibly at all) whichFPE_XXXshould be returned. It's up to userspace to figure it out if it really cares. Previously we were accidentally returningSI_USERinsi_codefor arm64.This case on arm looks like a more serious error for which
FPE_FLTINVmay be more appropriate anyway.No. The cases where we get to this point are:
1. A trap concerning a coprocessor register transfer instruction (iow, move between a VFP register and ARM register.)
2. A trap concerning a coprocessor register load or save instruction.(In both of these, "concerning" means that the VFP hardware provides such an instruction as the reason for the fault, *not* that it is the faulting instruction.)
Quoted 5 lines
3. A combination of the exception bits (EX and DEX) on certain VFP implementations.
All of these can be summarised as "the hardware went wrong in some way" rather than "the user program did something wrong."
Although my understanding of VFP bounces is a bit hazy, I think this is broadly in line with my assumptions.
Quoted 4 lines
FPE_FLTINVmeans "floating point invalid operation". Does it really cover the case where hardware has failed, or is it intended to cover the case where userspace did something wrong and asked for an invalid operation from the FP hardware?
So, there's an argument that FPE_FLTINV is not really correct. My rationale was that there is nothing correct that we can return, and FPE_FLTINV may be no worse than the alternatives.
If we can only hit this case as the result of a hardware failure or kernel bug though, should this be delivered as SIGKILL instead?
That's the approach I eventually followed for various exceptions on arm64 that were theoretically delivered to userspace with si_code==0, but really should be impossible unless and kernel and/or hardware is buggy.
If that's the case though, I don't see how a userspace testsuite is hitting this code path. Maybe I've misunderstood the context of this thread.
Cheers
---Dave
Russell King - ARM Linux
maintainerRe: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 3 lines · Dave Martin
If that's the case though, I don't see how a userspace testsuite is hitting this code path. Maybe I've misunderstood the context of this thread.
It isn't hitting this exact case.
The userspace testsuite is hitting an entirely different case:
kill(getpid(), SIGFPE);
As one expects, this generates a SIGFPE to the current process, which then inspects the siginfo structure. Being a userspace generated signal, si_code is set to SI_USER, which has the value 0.
With FPE_FIXME defined to zero, as Eric has done:
enum siginfo_layout siginfo_layout(int sig, int si_code)
{
enum siginfo_layout layout = SIL_KILL;
if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
...
} else {
...
#ifdef FPE_FIXME
if ((sig == SIGFPE) && (si_code == FPE_FIXME))
layout = SIL_FAULT;
#endif
}
return layout;
}This causes siginfo_layout() to return SIL_FAULT for this userspace generated signal, rather than the correct SIL_KILL.
This affects which fields we copy to userspace.
SI_USER is defined to pass si_pid and si_uid to the userspace process, which on ARM are the first two consecutive 32-bit quantities in the union, which is done when siginfo_layout() returns SIL_KILL. However, when SIL_FAULT is returned, we only copy si_addr in the union, which on ARM is just one 32-bit quantity.
Consequently, userspace sees a correct value for si_pid, and si_uid remains set to whatever was there in userspace. In the case of the strace program, that's zero. This means if you run the strace testsuite as root, the problem doesn't appear, but if you run it as a non-root user, it will.
So, the testsuite case has little to do with the behaviour of the VFP handling - it's to do with the behaviour of the kernel's signal handling.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: [email protected] down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
Quoted 43 lines · Russell King - ARM Linux
Quoted 3 lines · Dave Martin
If that's the case though, I don't see how a userspace testsuite is hitting this code path. Maybe I've misunderstood the context of this thread.
It isn't hitting this exact case.
The userspace testsuite is hitting an entirely different case:
kill(getpid(), SIGFPE);As one expects, this generates a SIGFPE to the current process, which then inspects the siginfo structure. Being a userspace generated signal,
si_codeis set toSI_USER, which has the value 0.With
FPE_FIXMEdefined to zero, as Eric has done:enum
siginfo_layoutsiginfo_layout(int sig, int si_code){ enum siginfo_layout layout = SIL_KILL; if ((si_code > SI_USER) && (si_code < SI_KERNEL)) { ... } else { ... #ifdef FPE_FIXME if ((sig == SIGFPE) && (si_code == FPE_FIXME)) layout = SIL_FAULT; #endif } return layout; }This causes
siginfo_layout()to returnSIL_FAULTfor this userspace generated signal, rather than the correctSIL_KILL.This affects which fields we copy to userspace.
SI_USERis defined to passsi_pidandsi_uidto the userspace process, which on ARM are the first two consecutive 32-bit quantities in the union, which is done whensiginfo_layout()returnsSIL_KILL. However, whenSIL_FAULTis returned, we only copysi_addrin the union, which on ARM is just one 32-bit quantity.
Quoted 8 lines
Consequently, userspace sees a correct value for
si_pid, andsi_uidremains set to whatever was there in userspace. In the case of the strace program, that's zero. This means if you run the strace testsuite as root, the problem doesn't appear, but if you run it as a non-root user, it will.So, the testsuite case has little to do with the behaviour of the VFP handling - it's to do with the behaviour of the kernel's signal handling.
Oh, right. So, going back to the unhandled VFP bounce question, is it reasonable for that to be a SIGKILL? That avoids the question of what si_code userspace should see, because userspace doesn't get to see siginfo at all in that case: it's dead.
Or do we hit this in real situations that we want userspace to bail out of more gracefully?
Cheers
---Dave
Re: sparc/ppc/arm compat siginfo ABI regressions: sending SIGFPE via kill() returns wrong values in si_pid and si_uid
In reply to Linus TorvaldsQuoted 24 lines · Linus Torvalds
Does this attached patch perhaps fix the ARM case?
It just uses
FPE_FLTUNKas the defaultsi_codefor SIGFPE, which seems sane enough. And then gets rid ofFPE_FIXME, which should resolve the nasty case.Hmm? Entirely untested, and I didn't really look at the test-case in question since I can't really run it anyway.
Well, I could run it all on x86-64, but it doesn't have that
FPE_FIXMEcase at all.Linus
arch/arm/include/uapi/asm/siginfo.h | 7 ------- arch/arm/vfp/vfpmodule.c | 4 ++-- 2 files changed, 2 insertions(+), 9 deletions(-)arch/arm/include/uapi/asm/siginfo.h +0
-0@@ -3,11 +3,4 @@
arch/arm/include/uapi/asm/siginfo.h line 6
Quoted 10 lines
arch/arm/include/uapi/asm/siginfo.h +0
-7#include <asm-generic/siginfo.h> -/* -* SIGFPE si_codes -*/ -#ifdef __KERNEL__ -#define FPE_FIXME 0/* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ -#endif
Looks like the whole file should go away.
arch/arm/vfp/vfpmodule.c static void vfp_panic(char *reason, u32 inst) · line 254
Quoted 10 lines
arch/arm/vfp/vfpmodule.c +1
-1@@ -251,13 +251,13 @@ static void vfp_panic(char *reason, u32 inst)*/ static void vfp_raise_exceptions (u32 exceptions,u32 inst,u32 fpscr,struct pt_regs *regs) { -int si_code =0 ; +int si_code =FPE_FLTUNK ;
Note that this change would affect the following code at the end of vfp_raise_exceptions:
if (si_code) vfp_raise_sigfpe(si_code, regs);
arch/arm/vfp/vfpmodule.c static void vfp_panic(char *reason, u32 inst) · line 262
Quoted 9 lines
arch/arm/vfp/vfpmodule.c +1
-1pr_debug ("VFP: raising exceptions %08x\n" , exceptions);if (exceptions ==VFP_EXCEPTION_ERROR ) {vfp_panic ("unhandled bounce" , inst); -vfp_raise_sigfpe (FPE_FIXME , regs); +vfp_raise_sigfpe (si_code, regs);return ; }
To be on the safe side, I'd just change it this way:
arch/arm/vfp/vfpmodule.c +1 -1
@@ -257,7 +257,7 @@ static void vfp_raise_exceptions(u32 exceptions, u32 inst, u32 fpscr, struct pt_if (exceptions ==VFP_EXCEPTION_ERROR ) {vfp_panic ("unhandled bounce" , inst); -vfp_raise_sigfpe (FPE_FIXME , regs); +vfp_raise_sigfpe (FPE_FLTUNK , regs);return ; }
Signature · 8 lines
ldv -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180412/8a5a5062/attachment.sig>