| Message ID | 1519926248-12591-3-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 01, 2018 at 05:44:07PM +0000, Dave Martin wrote: > Currently a SIGFPE delivered in response to a floating-point > exception trap may have si_code set to 0 on arm64. As reported by > Eric, this is a bad idea since this is the value of SI_USER -- yet > this signal is definitely not the result of kill(2), tgkill(2) etc. > and si_uid and si_pid make limited sense whereas we do want to > yield a value for si_addr (which doesn't exist for SI_USER). [...] > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index e7226c4..9040038 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -39,6 +39,7 @@ > #include <linux/slab.h> > #include <linux/sysctl.h> > > +#include <asm/esr.h> > #include <asm/fpsimd.h> > #include <asm/cputype.h> > #include <asm/simd.h> > @@ -867,18 +868,20 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) > asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) > { > siginfo_t info; > - unsigned int si_code = FPE_FIXME; > - > - if (esr & FPEXC_IOF) > - si_code = FPE_FLTINV; > - else if (esr & FPEXC_DZF) > - si_code = FPE_FLTDIV; > - else if (esr & FPEXC_OFF) > - si_code = FPE_FLTOVF; > - else if (esr & FPEXC_UFF) > - si_code = FPE_FLTUND; > - else if (esr & FPEXC_IXF) > - si_code = FPE_FLTRES; > + unsigned int si_code = FPE_FLTUNK; Happy to take this patch once the dependency on FPE_FLTUNK in core code is resolved. Will
Dave Martin <Dave.Martin@arm.com> writes: > Currently a SIGFPE delivered in response to a floating-point > exception trap may have si_code set to 0 on arm64. As reported by > Eric, this is a bad idea since this is the value of SI_USER -- yet > this signal is definitely not the result of kill(2), tgkill(2) etc. > and si_uid and si_pid make limited sense whereas we do want to > yield a value for si_addr (which doesn't exist for SI_USER). > > It's not entirely clear whether the architecure permits a > "spurious" fp exception trap where none of the exception flag bits > in ESR_ELx is set. (IMHO the architectural intent is to forbid > this.) However, it does permit those bits to contain garbage if > the TFV bit in ESR_ELx is 0. That case isn't currently handled at > all and may result in si_code == 0 or si_code containing a FPE_FLT* > constant corresponding to an exception that did not in fact happen. > > There is nothing sensible we can return for si_code in such cases, > but SI_USER is certainly not appropriate and will lead to violation > of legitimate userspace assumptions. > > This patch allocates a new si_code value FPE_UNKNOWN that at least > does not conflict with any existing SI_* or FPE_* code, and yields > this in si_code for undiagnosable cases. This is probably the best > simplicity/incorrectness tradeoff achieveable without relying on > implementation-dependent features or adding a lot of code. In any > case, there appears to be no perfect solution possible that would > justify a lot of effort here. > > Yielding FPE_UNKNOWN when some well-defined fp exception caused the > trap is a violation of POSIX, but this is forced by the > architecture. We have no realistic prospect of yielding the > correct code in such cases. At present I am not aware of any ARMv8 > implementation that supports trapped floating-point exceptions in > any case. > > The new code may be applicable to other architectures for similar > reasons. > > No attempt is made to provide ESR_ELx to userspace in the signal > frame, since architectural limitations mean that it is unlikely to > provide much diagnostic value, doesn't benefit existing software > and would create ABI with no proven purpose. The existing > mechanism for passing it also has problems of its own which may > result in the wrong value being passed to userspace due to > interaction with mm faults. The implied rework does not appear > justified. > > Reported-by: Eric W. Biederman <ebiederm@xmission.com> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > arch/arm64/include/asm/esr.h | 9 +++++++++ > arch/arm64/include/uapi/asm/siginfo.h | 7 ------- > arch/arm64/kernel/fpsimd.c | 27 +++++++++++++++------------ > 3 files changed, 24 insertions(+), 19 deletions(-) > > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h > index 803443d..ce70c3f 100644 > --- a/arch/arm64/include/asm/esr.h > +++ b/arch/arm64/include/asm/esr.h > @@ -240,6 +240,15 @@ > (((e) & ESR_ELx_SYS64_ISS_OP2_MASK) >> \ > ESR_ELx_SYS64_ISS_OP2_SHIFT)) > > +/* > + * ISS field definitions for floating-point exception traps > + * (FP_EXC_32/FP_EXC_64). > + * > + * (The FPEXC_* constants are used instead for common bits.) > + */ > + > +#define ESR_ELx_FP_EXC_TFV (UL(1) << 23) > + > #ifndef __ASSEMBLY__ > #include <asm/types.h> > > diff --git a/arch/arm64/include/uapi/asm/siginfo.h b/arch/arm64/include/uapi/asm/siginfo.h > index 9b4d912..157e6a8 100644 > --- a/arch/arm64/include/uapi/asm/siginfo.h > +++ b/arch/arm64/include/uapi/asm/siginfo.h > @@ -22,13 +22,6 @@ > #include <asm-generic/siginfo.h> > > /* > - * SIGFPE si_codes > - */ > -#ifdef __KERNEL__ > -#define FPE_FIXME 0 /* Broken dup of SI_USER */ > -#endif /* __KERNEL__ */ > - > -/* > * SIGBUS si_codes > */ > #ifdef __KERNEL__ > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index e7226c4..9040038 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -39,6 +39,7 @@ > #include <linux/slab.h> > #include <linux/sysctl.h> > > +#include <asm/esr.h> > #include <asm/fpsimd.h> > #include <asm/cputype.h> > #include <asm/simd.h> > @@ -867,18 +868,20 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) > asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) > { > siginfo_t info; > - unsigned int si_code = FPE_FIXME; > - > - if (esr & FPEXC_IOF) > - si_code = FPE_FLTINV; > - else if (esr & FPEXC_DZF) > - si_code = FPE_FLTDIV; > - else if (esr & FPEXC_OFF) > - si_code = FPE_FLTOVF; > - else if (esr & FPEXC_UFF) > - si_code = FPE_FLTUND; > - else if (esr & FPEXC_IXF) > - si_code = FPE_FLTRES; > + unsigned int si_code = FPE_FLTUNK; > + > + if (esr & ESR_ELx_FP_EXC_TFV) { > + if (esr & FPEXC_IOF) > + si_code = FPE_FLTINV; > + else if (esr & FPEXC_DZF) > + si_code = FPE_FLTDIV; > + else if (esr & FPEXC_OFF) > + si_code = FPE_FLTOVF; > + else if (esr & FPEXC_UFF) > + si_code = FPE_FLTUND; > + else if (esr & FPEXC_IXF) > + si_code = FPE_FLTRES; > + } > > memset(&info, 0, sizeof(info)); > info.si_signo = SIGFPE;
Will Deacon <will.deacon@arm.com> writes: > On Thu, Mar 01, 2018 at 05:44:07PM +0000, Dave Martin wrote: >> Currently a SIGFPE delivered in response to a floating-point >> exception trap may have si_code set to 0 on arm64. As reported by >> Eric, this is a bad idea since this is the value of SI_USER -- yet >> this signal is definitely not the result of kill(2), tgkill(2) etc. >> and si_uid and si_pid make limited sense whereas we do want to >> yield a value for si_addr (which doesn't exist for SI_USER). > > [...] > >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> index e7226c4..9040038 100644 >> --- a/arch/arm64/kernel/fpsimd.c >> +++ b/arch/arm64/kernel/fpsimd.c >> @@ -39,6 +39,7 @@ >> #include <linux/slab.h> >> #include <linux/sysctl.h> >> >> +#include <asm/esr.h> >> #include <asm/fpsimd.h> >> #include <asm/cputype.h> >> #include <asm/simd.h> >> @@ -867,18 +868,20 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) >> asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) >> { >> siginfo_t info; >> - unsigned int si_code = FPE_FIXME; >> - >> - if (esr & FPEXC_IOF) >> - si_code = FPE_FLTINV; >> - else if (esr & FPEXC_DZF) >> - si_code = FPE_FLTDIV; >> - else if (esr & FPEXC_OFF) >> - si_code = FPE_FLTOVF; >> - else if (esr & FPEXC_UFF) >> - si_code = FPE_FLTUND; >> - else if (esr & FPEXC_IXF) >> - si_code = FPE_FLTRES; >> + unsigned int si_code = FPE_FLTUNK; > > Happy to take this patch once the dependency on FPE_FLTUNK in core code is > resolved. Would it help for me to take the FPE_FLTUNK patch into my siginfo-next branch? So that there is a common branch with the code so we don't need to worry about conflicts. If so I will look at that on Monday. There is another FPE code that appears to be needed for one of the other architectures as well. If we can figure out how to head conflicts off at the pass while everyone is fixing the FPE_FIXME type problems it might help. But it is all simple enough I expect a word to the wise when I send Linus my pull request will be enough to sort things out. Eric
Hi Eric, On Thu, Mar 08, 2018 at 04:40:12PM -0600, Eric W. Biederman wrote: > Will Deacon <will.deacon@arm.com> writes: > > > On Thu, Mar 01, 2018 at 05:44:07PM +0000, Dave Martin wrote: > >> Currently a SIGFPE delivered in response to a floating-point > >> exception trap may have si_code set to 0 on arm64. As reported by > >> Eric, this is a bad idea since this is the value of SI_USER -- yet > >> this signal is definitely not the result of kill(2), tgkill(2) etc. > >> and si_uid and si_pid make limited sense whereas we do want to > >> yield a value for si_addr (which doesn't exist for SI_USER). > > > > [...] > > > >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > >> index e7226c4..9040038 100644 > >> --- a/arch/arm64/kernel/fpsimd.c > >> +++ b/arch/arm64/kernel/fpsimd.c > >> @@ -39,6 +39,7 @@ > >> #include <linux/slab.h> > >> #include <linux/sysctl.h> > >> > >> +#include <asm/esr.h> > >> #include <asm/fpsimd.h> > >> #include <asm/cputype.h> > >> #include <asm/simd.h> > >> @@ -867,18 +868,20 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) > >> asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) > >> { > >> siginfo_t info; > >> - unsigned int si_code = FPE_FIXME; > >> - > >> - if (esr & FPEXC_IOF) > >> - si_code = FPE_FLTINV; > >> - else if (esr & FPEXC_DZF) > >> - si_code = FPE_FLTDIV; > >> - else if (esr & FPEXC_OFF) > >> - si_code = FPE_FLTOVF; > >> - else if (esr & FPEXC_UFF) > >> - si_code = FPE_FLTUND; > >> - else if (esr & FPEXC_IXF) > >> - si_code = FPE_FLTRES; > >> + unsigned int si_code = FPE_FLTUNK; > > > > Happy to take this patch once the dependency on FPE_FLTUNK in core code is > > resolved. > > Would it help for me to take the FPE_FLTUNK patch into my siginfo-next > branch? So that there is a common branch with the code so we don't > need to worry about conflicts. If so I will look at that on Monday. Yes please, that would be helpful actually. I can then pull that into arm64 if you give me a stable branch or tag. Alternatively, I can define FPE_FLTUNK locally and remove it at -rc1. Cheers, Will
On Fri, Mar 09, 2018 at 01:10:18PM +0000, Will Deacon wrote: > Hi Eric, > > On Thu, Mar 08, 2018 at 04:40:12PM -0600, Eric W. Biederman wrote: > > Will Deacon <will.deacon@arm.com> writes: > > > > > On Thu, Mar 01, 2018 at 05:44:07PM +0000, Dave Martin wrote: > > >> Currently a SIGFPE delivered in response to a floating-point > > >> exception trap may have si_code set to 0 on arm64. As reported by > > >> Eric, this is a bad idea since this is the value of SI_USER -- yet > > >> this signal is definitely not the result of kill(2), tgkill(2) etc. > > >> and si_uid and si_pid make limited sense whereas we do want to > > >> yield a value for si_addr (which doesn't exist for SI_USER). > > > > > > [...] > > > > > >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > >> index e7226c4..9040038 100644 > > >> --- a/arch/arm64/kernel/fpsimd.c > > >> +++ b/arch/arm64/kernel/fpsimd.c > > >> @@ -39,6 +39,7 @@ > > >> #include <linux/slab.h> > > >> #include <linux/sysctl.h> > > >> > > >> +#include <asm/esr.h> > > >> #include <asm/fpsimd.h> > > >> #include <asm/cputype.h> > > >> #include <asm/simd.h> > > >> @@ -867,18 +868,20 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) > > >> asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) > > >> { > > >> siginfo_t info; > > >> - unsigned int si_code = FPE_FIXME; > > >> - > > >> - if (esr & FPEXC_IOF) > > >> - si_code = FPE_FLTINV; > > >> - else if (esr & FPEXC_DZF) > > >> - si_code = FPE_FLTDIV; > > >> - else if (esr & FPEXC_OFF) > > >> - si_code = FPE_FLTOVF; > > >> - else if (esr & FPEXC_UFF) > > >> - si_code = FPE_FLTUND; > > >> - else if (esr & FPEXC_IXF) > > >> - si_code = FPE_FLTRES; > > >> + unsigned int si_code = FPE_FLTUNK; > > > > > > Happy to take this patch once the dependency on FPE_FLTUNK in core code is > > > resolved. > > > > Would it help for me to take the FPE_FLTUNK patch into my siginfo-next > > branch? So that there is a common branch with the code so we don't > > need to worry about conflicts. If so I will look at that on Monday. > > Yes please, that would be helpful actually. I can then pull that into > arm64 if you give me a stable branch or tag. Alternatively, I can define > FPE_FLTUNK locally and remove it at -rc1. OK Eric, let me know if you need me to rebase anything for the first 2 patches. Cheers ---Dave
Dave Martin <Dave.Martin@arm.com> writes: > On Fri, Mar 09, 2018 at 01:10:18PM +0000, Will Deacon wrote: >> Hi Eric, >> >> On Thu, Mar 08, 2018 at 04:40:12PM -0600, Eric W. Biederman wrote: >> > Will Deacon <will.deacon@arm.com> writes: >> > > Happy to take this patch once the dependency on FPE_FLTUNK in core code is >> > > resolved. >> > >> > Would it help for me to take the FPE_FLTUNK patch into my siginfo-next >> > branch? So that there is a common branch with the code so we don't >> > need to worry about conflicts. If so I will look at that on Monday. >> >> Yes please, that would be helpful actually. I can then pull that into >> arm64 if you give me a stable branch or tag. Alternatively, I can define >> FPE_FLTUNK locally and remove it at -rc1. > > OK Eric, let me know if you need me to rebase anything for the first 2 > patches. The first patch is now in my siginfo-next tree and avialable at: git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git siginfo-next HEAD: 266da65e9156d93e1126e185259a4aae68188d0e signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions I will not rebase this branch, so please fee free to merge it into other trees. Eric
On Thu, Mar 15, 2018 at 04:13:02PM -0500, Eric W. Biederman wrote: > Dave Martin <Dave.Martin@arm.com> writes: > > > On Fri, Mar 09, 2018 at 01:10:18PM +0000, Will Deacon wrote: > >> Hi Eric, > >> > >> On Thu, Mar 08, 2018 at 04:40:12PM -0600, Eric W. Biederman wrote: > >> > Will Deacon <will.deacon@arm.com> writes: > >> > > Happy to take this patch once the dependency on FPE_FLTUNK in core code is > >> > > resolved. > >> > > >> > Would it help for me to take the FPE_FLTUNK patch into my siginfo-next > >> > branch? So that there is a common branch with the code so we don't > >> > need to worry about conflicts. If so I will look at that on Monday. > >> > >> Yes please, that would be helpful actually. I can then pull that into > >> arm64 if you give me a stable branch or tag. Alternatively, I can define > >> FPE_FLTUNK locally and remove it at -rc1. > > > > OK Eric, let me know if you need me to rebase anything for the first 2 > > patches. > > The first patch is now in my siginfo-next tree and avialable at: > > git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git siginfo-next > HEAD: 266da65e9156d93e1126e185259a4aae68188d0e signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions > > I will not rebase this branch, so please fee free to merge it into other > trees. Thanks, Eric. I've pulled this into the arm64 for-next/core branch and I'll push it out later on. Will
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h index 803443d..ce70c3f 100644 --- a/arch/arm64/include/asm/esr.h +++ b/arch/arm64/include/asm/esr.h @@ -240,6 +240,15 @@ (((e) & ESR_ELx_SYS64_ISS_OP2_MASK) >> \ ESR_ELx_SYS64_ISS_OP2_SHIFT)) +/* + * ISS field definitions for floating-point exception traps + * (FP_EXC_32/FP_EXC_64). + * + * (The FPEXC_* constants are used instead for common bits.) + */ + +#define ESR_ELx_FP_EXC_TFV (UL(1) << 23) + #ifndef __ASSEMBLY__ #include <asm/types.h> diff --git a/arch/arm64/include/uapi/asm/siginfo.h b/arch/arm64/include/uapi/asm/siginfo.h index 9b4d912..157e6a8 100644 --- a/arch/arm64/include/uapi/asm/siginfo.h +++ b/arch/arm64/include/uapi/asm/siginfo.h @@ -22,13 +22,6 @@ #include <asm-generic/siginfo.h> /* - * SIGFPE si_codes - */ -#ifdef __KERNEL__ -#define FPE_FIXME 0 /* Broken dup of SI_USER */ -#endif /* __KERNEL__ */ - -/* * SIGBUS si_codes */ #ifdef __KERNEL__ diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index e7226c4..9040038 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -39,6 +39,7 @@ #include <linux/slab.h> #include <linux/sysctl.h> +#include <asm/esr.h> #include <asm/fpsimd.h> #include <asm/cputype.h> #include <asm/simd.h> @@ -867,18 +868,20 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) { siginfo_t info; - unsigned int si_code = FPE_FIXME; - - if (esr & FPEXC_IOF) - si_code = FPE_FLTINV; - else if (esr & FPEXC_DZF) - si_code = FPE_FLTDIV; - else if (esr & FPEXC_OFF) - si_code = FPE_FLTOVF; - else if (esr & FPEXC_UFF) - si_code = FPE_FLTUND; - else if (esr & FPEXC_IXF) - si_code = FPE_FLTRES; + unsigned int si_code = FPE_FLTUNK; + + if (esr & ESR_ELx_FP_EXC_TFV) { + if (esr & FPEXC_IOF) + si_code = FPE_FLTINV; + else if (esr & FPEXC_DZF) + si_code = FPE_FLTDIV; + else if (esr & FPEXC_OFF) + si_code = FPE_FLTOVF; + else if (esr & FPEXC_UFF) + si_code = FPE_FLTUND; + else if (esr & FPEXC_IXF) + si_code = FPE_FLTRES; + } memset(&info, 0, sizeof(info)); info.si_signo = SIGFPE;
Currently a SIGFPE delivered in response to a floating-point exception trap may have si_code set to 0 on arm64. As reported by Eric, this is a bad idea since this is the value of SI_USER -- yet this signal is definitely not the result of kill(2), tgkill(2) etc. and si_uid and si_pid make limited sense whereas we do want to yield a value for si_addr (which doesn't exist for SI_USER). It's not entirely clear whether the architecure permits a "spurious" fp exception trap where none of the exception flag bits in ESR_ELx is set. (IMHO the architectural intent is to forbid this.) However, it does permit those bits to contain garbage if the TFV bit in ESR_ELx is 0. That case isn't currently handled at all and may result in si_code == 0 or si_code containing a FPE_FLT* constant corresponding to an exception that did not in fact happen. There is nothing sensible we can return for si_code in such cases, but SI_USER is certainly not appropriate and will lead to violation of legitimate userspace assumptions. This patch allocates a new si_code value FPE_UNKNOWN that at least does not conflict with any existing SI_* or FPE_* code, and yields this in si_code for undiagnosable cases. This is probably the best simplicity/incorrectness tradeoff achieveable without relying on implementation-dependent features or adding a lot of code. In any case, there appears to be no perfect solution possible that would justify a lot of effort here. Yielding FPE_UNKNOWN when some well-defined fp exception caused the trap is a violation of POSIX, but this is forced by the architecture. We have no realistic prospect of yielding the correct code in such cases. At present I am not aware of any ARMv8 implementation that supports trapped floating-point exceptions in any case. The new code may be applicable to other architectures for similar reasons. No attempt is made to provide ESR_ELx to userspace in the signal frame, since architectural limitations mean that it is unlikely to provide much diagnostic value, doesn't benefit existing software and would create ABI with no proven purpose. The existing mechanism for passing it also has problems of its own which may result in the wrong value being passed to userspace due to interaction with mm faults. The implied rework does not appear justified. Reported-by: Eric W. Biederman <ebiederm@xmission.com> Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/include/asm/esr.h | 9 +++++++++ arch/arm64/include/uapi/asm/siginfo.h | 7 ------- arch/arm64/kernel/fpsimd.c | 27 +++++++++++++++------------ 3 files changed, 24 insertions(+), 19 deletions(-)