Subject: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

From: Thiemo Seufer <[email-masked]>
Date: Tue, 24 Jul 2001 08:04:11 +0200

Hi All,

somebody made wrong assumptions about how compute_return_epc() works.
It does not return the epc but stores it in the register struct.
Return value is -EFAULT or zero.

I've speculated below how the right solution might look, but I
don't know enough about signal handling to be sure.

Thiemo

diff -BurPX /bigdisk/src/dontdiff linux-orig/arch/mips/kernel/traps.c linux/arch/mips/kernel/traps.c
--- linux-orig/arch/mips/kernel/traps.c	Sat Jul 14 18:49:46 2001
+++ linux/arch/mips/kernel/traps.c	Sun Jul 22 08:44:57 2001
@@ -378,8 +378,11 @@
 		else
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
-		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_errno = compute_return_epc(regs);
+		if (info.si_errno)
+			info.si_addr = NULL;
+		else
+			info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
@@ -418,8 +421,11 @@
 		else
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
-		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_errno = compute_return_epc(regs);
+		if (info.si_errno)
+			info.si_addr = NULL;
+		else
+			info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 30 Jul 2001 20:46:07 +0200 (MET DST)

> somebody made wrong assumptions about how compute_return_epc() works.

 It was me, I admit...  Thanks for pointing it out. 

> don't know enough about signal handling to be sure.

 I think the following fix is sufficient -- let's just pass EPC and let
the userland handle it.  You don't normally want a "break" in a branch
delay slot -- such a sequence is of questionable utility.  But if it is to
be handled, the KISS approach gives the userland a chance to handle an
exception gracefully.  One may want to emulate overflows somehow, for
example.  Also the code is shorter. 

 Ralf, please apply it.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email-masked], PGP key available        +

patch-mips-2.4.5-20010730-break-1
diff -up --recursive --new-file linux.macro/arch/mips/kernel/traps.c linux/arch/mips/kernel/traps.c
--- linux.macro/arch/mips/kernel/traps.c	Tue Jul 24 04:26:34 2001
+++ linux/arch/mips/kernel/traps.c	Mon Jul 30 18:26:03 2001
@@ -378,7 +378,7 @@ asmlinkage void do_bp(struct pt_regs *re
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
@@ -418,7 +418,7 @@ asmlinkage void do_tr(struct pt_regs *re
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:

---

From: "Kevin D. Kissell" <[email-masked]>
Date: Mon, 30 Jul 2001 22:13:55 +0200

Many years ago, I went to a lot of effort to see to it that
FP exceptions on the Fairchild Clipper passed enough
information up through SIGFPE to allow full userland
emulation or fixup and replay-on-return of the operation
on floating point faults (entertaining because there was
a seperate FP PC). And of course no one ever used it!   ;-)

I missed the beginning of this thread, but it looks from the
patch as if this is really about handling integer overflow
exceptions, not FP exceptions.  That's unfortunate.
To begin with you are correct in that we should be
passing the EPC value at the exception (and certainly not
the result  of invoking compute_return_epc(), much less
its side effects!), and the state of the BD bit from the
Cause register, either abstracted into a variable in the
info structure or as a flat-out copy of the Cause register.
I would recommend the former as being less of a security
hole.  Yes, we could blow that off and make the user
decode for branches himself, but it's tacky. But what's
more pernicious is the fact that, being an integer exception,
it could have resulted from an overflow of any of the GPRs,
including those that will be used by the low-level library
code dispatching the signal to the user, and in the generated
code of the user's own signal handler.  If we actually want
to allow the user to fix the operation and saturate (or whatever)
the destination register, enough of the trap context needs
to be passed up to the signal handler *and* back down
on the signal return to allow that manipulation, followed
by a resumption of execution at the instruction following
the one that generated the trap.

            Been there, done that, got the coffee mug,

            Kevin K.

----- Original Message -----
From: "Maciej W. Rozycki" <[email-masked]>
To: "Ralf Baechle" <[email-masked]>; "Thiemo Seufer"
<[email-masked]>
Cc: <[email-masked]>
Sent: Monday, July 30, 2001 8:46 PM
Subject: Re: [PATCH] wrong use of compute_return_epc() in
/mips/kernel/traps.c

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 30 Jul 2001 22:41:05 +0200 (MET DST)

> a seperate FP PC). And of course no one ever used it!   ;-)

 That's why I took the KISS approach.  Current code is cheap and satisfies
the specs I use, i.e. glibc's info pages.  If anyone needs anything more
one is free to improve handlers. 

> exceptions, not FP exceptions.  That's unfortunate.

 The patch is about break and trap instruction handlers, indeed. 

> info structure or as a flat-out copy of the Cause register.

 I don't think passing BD is needed here.  Enough information is
available.  A SIGFPE due to a branch or a jump instruction is impossible. 
Unless you have a chip on fire, in which case problems with signal
handlers are insignificant.

> the one that generated the trap.

 Let's just leave it as an excercise to one writing some emulation code. 
A general-purpose handler in the kernel or libc (which no one will
probably use) is an overkill and by defining own strict coding rules (be
it a calling convention, shadow registers or whatever) one can create
software that suits one's specific needs.

---

From: "Kevin D. Kissell" <[email-masked]>
Date: Mon, 30 Jul 2001 23:48:35 +0200

> one is free to improve handlers.

The handlers cannot spontaneously generate information known
only to the kernel.

>  Let's just leave it as an excercise to one writing some emulation code.

An excercise much akin to squaring the circle.  Think
about it.  Your program has overflowed on an operation
on a0.  The signal is sent, and your signal routine is called,
trashing a0 with the first argument.  For greater amusement,
assume that the instruction was "add a0, a0, a1" (which
is what could easily happen in a C function that begins
by summing its first two arguments).  I simply do not see
how user level code can determine whether the overflow
was postive or negative without knowlege of the inputs,
and those inputs may well have existed only in the registers.
And even assuming that one could deduce the correct value,
how would you propose patching it back into the execution
stream, particularly in the presence of delayed branches?

> software that suits one's specific needs.

Yeah.  Right.  Shadow registers will allow you to be certain
that you haven't trashed your inputs.  They won't help you
resume execution with a corrected value in the destination
register of the faulting instruction.  I have no problem with
declaring that signals  cannot be used to handle overflow
exceptions.  I do have a problem with falsely pretending to
support such handling. We can decide (for now) that the problem
doesn't need to  be solved.  We can decide to call it a "TODO"
for some time  when we're old and grey.   We can solve it now.
I really don't care.  But it's dishonest to say that the current code
solves it,  even  to a first order of approximation.  To trot out an
overused Einstein quote,

"Everything should be as simple as possible. But not simpler".

            Kevin K.

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 31 Jul 2001 00:31:24 +0200 (MET DST)

> The handlers cannot spontaneously generate information known

 I'm not sure what you mean here.  Could you please elaborate?

> stream, particularly in the presence of delayed branches?

 Sure, you are writing of the general case.  That's not solvable given
current interfaces.

> register of the faulting instruction.  I have no problem with

 It actually depends on how you specify your requirements.

> for some time  when we're old and grey.   We can solve it now.

 Si_addr in the siginfo struct is specified to contain a faulting
instruction's reference.  My patch fixes the code to fullfill the
specification.  The code was meant to do it already when I submitted it
last year.  Somehow I made a bug and now I'm only fixing it. 

 We do nowhere pretend to support handling of overflow exceptions to the
extent that permits emulating of faulting instructions.  Check the docs,
namely the "Program Error Signals" node of glibc's info pages.  It seems
my handler's example was a bit unfortunate, sigh...

> "Everything should be as simple as possible. But not simpler".

 Yes, sure, we may hardcode a zero here, but what the gain would be? 
Providing a real value instead makes us adhere to the spec, it's only
marginally slower and it may be useful for anything, not only extending
precision.  For example one may print the address involved for user's
(developer's) feedback and abort the affected function only as opposed to
dumping a core.