All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Martin <Dave.Martin@arm.com>
To: linux-arm-kernel@lists.infradead.org
Cc: linux-arch@vger.kernel.org, linux-api@vger.kernel.org,
	Will Deacon <will.deacon@arm.com>,
	James Morse <james.morse@arm.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: [PATCH v2 2/3] arm64: fpsimd: Fix bad si_code for undiagnosed SIGFPE
Date: Thu,  1 Mar 2018 17:44:07 +0000	[thread overview]
Message-ID: <1519926248-12591-3-git-send-email-Dave.Martin@arm.com> (raw)
In-Reply-To: <1519926248-12591-1-git-send-email-Dave.Martin@arm.com>

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(-)

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;
-- 
2.1.4

WARNING: multiple messages have this Message-ID (diff)
From: Dave Martin <Dave.Martin@arm.com>
To: linux-arm-kernel@lists.infradead.org
Cc: linux-arch@vger.kernel.org, linux-api@vger.kernel.org,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Will Deacon <will.deacon@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	James Morse <james.morse@arm.com>
Subject: [PATCH v2 2/3] arm64: fpsimd: Fix bad si_code for undiagnosed SIGFPE
Date: Thu,  1 Mar 2018 17:44:07 +0000	[thread overview]
Message-ID: <1519926248-12591-3-git-send-email-Dave.Martin@arm.com> (raw)
Message-ID: <20180301174407.ZvSTgigwqZ0UXi9WprdfHNiIGw-KRW7bSG78wBOPQ44@z> (raw)
In-Reply-To: <1519926248-12591-1-git-send-email-Dave.Martin@arm.com>

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(-)

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;
-- 
2.1.4

WARNING: multiple messages have this Message-ID (diff)
From: Dave.Martin@arm.com (Dave Martin)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 2/3] arm64: fpsimd: Fix bad si_code for undiagnosed SIGFPE
Date: Thu,  1 Mar 2018 17:44:07 +0000	[thread overview]
Message-ID: <1519926248-12591-3-git-send-email-Dave.Martin@arm.com> (raw)
In-Reply-To: <1519926248-12591-1-git-send-email-Dave.Martin@arm.com>

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(-)

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;
-- 
2.1.4

  parent reply	other threads:[~2018-03-01 17:44 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-01 17:44 [PATCH v2 0/3] arm64: Fix invalid si_codes for fault signals Dave Martin
2018-03-01 17:44 ` Dave Martin
2018-03-01 17:44 ` Dave Martin
2018-03-01 17:44 ` [PATCH v2 1/3] signal: Add FPE_FLTUNK si_code for undiagnosable fp exceptions Dave Martin
2018-03-01 17:44   ` Dave Martin
2018-03-01 17:44   ` Dave Martin
2018-03-08 17:11   ` Will Deacon
2018-03-08 17:11     ` Will Deacon
2018-03-08 22:35   ` Eric W. Biederman
2018-03-08 22:35     ` Eric W. Biederman
2018-03-08 22:35     ` Eric W. Biederman
2018-03-01 17:44 ` Dave Martin [this message]
2018-03-01 17:44   ` [PATCH v2 2/3] arm64: fpsimd: Fix bad si_code for undiagnosed SIGFPE Dave Martin
2018-03-01 17:44   ` Dave Martin
2018-03-08 17:11   ` Will Deacon
2018-03-08 17:11     ` Will Deacon
2018-03-08 17:11     ` Will Deacon
2018-03-08 22:40     ` Eric W. Biederman
2018-03-08 22:40       ` Eric W. Biederman
2018-03-08 22:40       ` Eric W. Biederman
2018-03-09 13:10       ` Will Deacon
2018-03-09 13:10         ` Will Deacon
2018-03-09 13:10         ` Will Deacon
2018-03-09 14:25         ` Dave Martin
2018-03-09 14:25           ` Dave Martin
2018-03-09 14:25           ` Dave Martin
2018-03-15 21:13           ` Eric W. Biederman
2018-03-15 21:13             ` Eric W. Biederman
2018-03-15 21:13             ` Eric W. Biederman
2018-03-20 10:04             ` Will Deacon
2018-03-20 10:04               ` Will Deacon
2018-03-20 10:04               ` Will Deacon
2018-03-08 22:37   ` Eric W. Biederman
2018-03-08 22:37     ` Eric W. Biederman
2018-03-08 22:37     ` Eric W. Biederman
2018-03-01 17:44 ` [PATCH v2 3/3] arm64: signal: Ensure si_code is valid for all fault signals Dave Martin
2018-03-01 17:44   ` Dave Martin
2018-03-01 17:44   ` Dave Martin
2018-03-08 16:37   ` Will Deacon
2018-03-08 16:37     ` Will Deacon
2018-03-08 16:37     ` Will Deacon

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=1519926248-12591-3-git-send-email-Dave.Martin@arm.com \
    --to=dave.martin@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=ebiederm@xmission.com \
    --cc=james.morse@arm.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.