[patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[Chuck Ebbert]

  This patch fixes a problem with incorrect floating-point exception
signal delivery on i386 kernels.  In some cases, an error code of zero
is delivered instead of the correct code, as the output from my test
program shows:


Before patch:

$ ./fpsig
handler: signum = 8, errno = 0, code = 0
handler: fpu cwd = 0xb40, fpu swd = 0xbaa0


After:

$ ./fpsig
handler: signum = 8, errno = 0, code = 6
handler: fpu cwd = 0xb40, fpu swd = 0xbaa0


2.4 also has this problem; the patch applies with offsets on 2.4.31
but I didn't test it beyond that.  Patch also applies to 2.6.13-rc6-mm1
with offsets.

x86-64 also looks to be affected but I have no way to test it


Test program:

/* i387 fp signal test */

#define _GNU_SOURCE
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <signal.h>
#include <errno.h>

__attribute__ ((aligned(4096))) unsigned char altstack[4096];
unsigned short cw = 0x0b40; /* unmask all exceptions, round up */
struct sigaction sa;
stack_t ss = {
	.ss_sp   = &altstack[2047],
	.ss_size = sizeof(altstack)/2,
};

static void handler(int nr, siginfo_t *si, void *uc)
{
	printf("handler: signum = %d, errno = %d, code = %d\n",
		si->si_signo, si->si_errno, si->si_code);
	printf("handler: fpu cwd = 0x%hx, fpu swd = 0x%hx\n",
		*(unsigned short *)&altstack[0xd84],
		*(unsigned short *)&altstack[0xd88]);
	exit(1);
}

int main(int argc, char * const argv[])
{
	sa.sa_sigaction = handler;
	sa.sa_flags     = SA_ONSTACK | SA_SIGINFO;

	if (sigaltstack(&ss, 0))
		perror("sigaltstack");
	if (sigaction(SIGFPE, &sa, NULL))
		perror("sigaction");

	asm volatile ("fnclex ; fldcw %0" : : "m" (cw));
	asm volatile ( /*  st(1) = 3.0, st = 1.0  */
	    "fld1 ; fld1 ; faddp ; fld1 ; faddp ; fld1");
	asm volatile (
	    "fdivp ; fwait");  /*  1.0 / 3.0  */

	return 0;
}


Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>


 arch/i386/kernel/traps.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

--- 2.6.13-rc6a.orig/arch/i386/kernel/traps.c
+++ 2.6.13-rc6a/arch/i386/kernel/traps.c
@@ -778,7 +778,7 @@ void math_error(void __user *eip)
 {
 	struct task_struct * task;
 	siginfo_t info;
-	unsigned short cwd, swd;
+	unsigned short cwd, swd, wd;
 
 	/*
 	 * Save the info for the exception handler and clear the error.
@@ -803,7 +803,11 @@ void math_error(void __user *eip)
 	 */
 	cwd = get_fpu_cwd(task);
 	swd = get_fpu_swd(task);
-	switch (((~cwd) & swd & 0x3f) | (swd & 0x240)) {
+	wd = swd & 0x3f & ~cwd;
+	if (wd & 1)
+		wd |= swd & 0x240;
+
+	switch (wd) {
 		case 0x000:
 		default:
 			break;
__
Chuck

Re: [patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[Andi Kleen]

On Mon, 22 Aug 2005 19:43:57 -0400
Chuck Ebbert <76306.1226@compuserve.com> wrote:

>   This patch fixes a problem with incorrect floating-point exception
> signal delivery on i386 kernels.  In some cases, an error code of zero
> is delivered instead of the correct code, as the output from my test
> program shows:

...

How about you describe what you actually changed and why so that not 
every reviewer has to look up all the bits in the manual?

-Andi

Re: [patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[Chuck Ebbert]

On Tue, 23 Aug 2005 11:55:21 -0700 (PDT), Linus Torvalds wrote:

> Wouldn't this simpler patch result in exactly the same behaviour?

 I thought the extra code would be good documentation, but the comments
work just as well.  This is a little clearer (hand edited patch:)

--- a/arch/i386/kernel/traps.c
+++ b/arch/i386/kernel/traps.c
@@ -803,15 +803,17 @@ void math_error(void __user *eip)
 	 */
 	cwd = get_fpu_cwd(task);
 	swd = get_fpu_swd(task);
-	switch (((~cwd) & swd & 0x3f) | (swd & 0x240)) {
+	switch (swd & ~cwd & 0x3f) {
 		case 0x000:
 		default:
 			break;
 		case 0x001: /* Invalid Op */
-		case 0x041: /* Stack Fault */
-		case 0x241: /* Stack Fault | Direction */
+			/*
+			 * swd & 0x240 == 0x040: Stack Underflow
+			 * swd & 0x240 == 0x240: Stack Overflow
+			 * User must clear the SF bit (0x40) if set
+			 */
 			info.si_code = FPE_FLTINV;
-			/* Should we clear the SF or let user space do it ???? */
 			break;
 		case 0x002: /* Denormalize */
 		case 0x010: /* Underflow */
__
Chuck

Re: [patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[Linus Torvalds]

On Mon, 22 Aug 2005, Chuck Ebbert wrote:
>
> i386 floating-point exception handling has a bug that can cause error
> code 0 to be sent instead of the proper code during signal delivery.

Looking at your patch, I think it's too complicated.

The fact is, none of the "switch()" cases even _care_ about bits "0x240" 
from swd. The bug itself seems to be that we even look at it.

Wouldn't this simpler patch result in exactly the same behaviour?

		Linus
---
diff --git a/arch/i386/kernel/traps.c b/arch/i386/kernel/traps.c
--- a/arch/i386/kernel/traps.c
+++ b/arch/i386/kernel/traps.c
@@ -803,15 +803,14 @@ void math_error(void __user *eip)
 	 */
 	cwd = get_fpu_cwd(task);
 	swd = get_fpu_swd(task);
-	switch (((~cwd) & swd & 0x3f) | (swd & 0x240)) {
+	switch (swd & ~cwd & 0x3f) {
 		case 0x000:
 		default:
 			break;
 		case 0x001: /* Invalid Op */
-		case 0x041: /* Stack Fault */
-		case 0x241: /* Stack Fault | Direction */
+			/* swd & 0x240 == 0x040: Stack Fault */
+			/* swd & 0x240 == 0x240: Stack Fault | Direction */
 			info.si_code = FPE_FLTINV;
-			/* Should we clear the SF or let user space do it ???? */
 			break;
 		case 0x002: /* Denormalize */
 		case 0x010: /* Underflow */

Re: [patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[Chuck Ebbert]

On Tue, 23 Aug 2005 02:20:07 +0200, Andi Kleen wrote:

> every reviewer has to look up all the bits in the manual?

 I fixed the test program too:

 Before patch:

$ ./fpsig
handler: signum = 8, errno = 0, code = 0 [unknown]
handler: fpu cwd = 0xb40, fpu swd = 0xbaa0
handler: i387 unmasked precision exception, rounded up

 After:

$ ./fpsig
handler: signum = 8, errno = 0, code = 6 [inexact result]
handler: fpu cwd = 0xb40, fpu swd = 0xbaa0
handler: i387 unmasked precision exception, rounded up

/* i387 fp signal test */

#define _GNU_SOURCE
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <signal.h>
#include <errno.h>

__attribute__ ((aligned(4096))) unsigned char altstack[4096];
unsigned short cw = 0x0b40; /* unmask all exceptions, round up */
struct sigaction sa;
stack_t ss = {
	.ss_sp   = &altstack[2047],
	.ss_size = sizeof(altstack)/2,
};

static void handler(int nr, siginfo_t *si, void *uc)
{
	char *decode;
	int code = si->si_code;
	unsigned short cwd = *(unsigned short *)&altstack[0xd84];
	unsigned short swd = *(unsigned short *)&altstack[0xd88];

	switch (code) {
		case FPE_INTDIV:
			decode = "divide by zero";
			break;
		case FPE_FLTRES:
			decode = "inexact result";
			break;
		case FPE_FLTINV:
			decode = "invalid operation";
			break;
		default:
			decode = "unknown";
			break;
	}
	printf("handler: signum = %d, errno = %d, code = %d [%s]\n",
		si->si_signo, si->si_errno, code, decode);
	printf("handler: fpu cwd = 0x%hx, fpu swd = 0x%hx\n", cwd, swd);
	if (swd & 0x20 & ~cwd)
		printf("handler: i387 unmasked precision exception, rounded %s\n",
			swd & 0x200 ? "up" : "down");
	exit(1);
}

int main(int argc, char * const argv[])
{
	sa.sa_sigaction = handler;
	sa.sa_flags     = SA_ONSTACK | SA_SIGINFO;

	if (sigaltstack(&ss, 0))
		perror("sigaltstack");
	if (sigaction(SIGFPE, &sa, NULL))
		perror("sigaction");

	asm volatile ("fnclex ; fldcw %0" : : "m" (cw));
	asm volatile ( /*  st(1) = 3.0, st = 1.0  */
	    "fld1 ; fld1 ; faddp ; fld1 ; faddp ; fld1");
	asm volatile (
	    "fdivp ; fwait");  /*  1.0 / 3.0  */

	return 0;
}
__
Chuck

Re: [patch 2.6.13-rc6] i386: fix incorrect FP signal delivery

[Chuck Ebbert]

On Tue, 23 Aug 2005 02:20:07 +0200, Andi Kleen wrote:

> How about you describe what you actually changed and why so that not 
> every reviewer has to look up all the bits in the manual?


 The patch had a bug anyway, so here's another try.  *** Replace
the previous patch with this one. ***


i386 floating-point exception handling has a bug that can cause error
code 0 to be sent instead of the proper code during signal delivery.
This is caused by unconditionally checking the IS and c1 bits from
the FPU status word when they are not always relevant.  The IS bit
tells whether an exception is a stack fault and is only relevant
when the exception is IE (invalid operation.)  The C1 bit determines
whether a stack fault is overflow or underflow and is only relevant
when IS and IE are set.

This bug also exists in the 2.4 kernel and appears to be in the 2.6
x86_64 code as well.

Patch applies with offsets to 2.4.31 and 2.6.13-rc6-mm1.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

 arch/i386/kernel/traps.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

--- 2.6.13-rc6a.orig/arch/i386/kernel/traps.c
+++ 2.6.13-rc6a/arch/i386/kernel/traps.c
@@ -778,7 +778,7 @@ void math_error(void __user *eip)
 {
 	struct task_struct * task;
 	siginfo_t info;
-	unsigned short cwd, swd;
+	unsigned short cwd, swd, wd;
 
 	/*
 	 * Save the info for the exception handler and clear the error.
@@ -803,7 +803,21 @@ void math_error(void __user *eip)
 	 */
 	cwd = get_fpu_cwd(task);
 	swd = get_fpu_swd(task);
-	switch (((~cwd) & swd & 0x3f) | (swd & 0x240)) {
+	wd = swd & 0x3f & ~cwd;
+	/*
+	 * If the exception is invalid operation, the IS bit is needed
+	 * to see if it's a stack fault.
+	 */
+	if (wd & 1)
+		wd |= swd & 0x40;
+	/*
+	 * If it's a stack fault, C1 is needed to see if it's overflow or
+	 * underflow.
+	 */
+	if (wd & 0x40)
+		wd |= swd & 0x200;
+
+	switch (wd) {
 		case 0x000:
 		default:
 			break;
__
Chuck