diff mbox

fix backtrace on PA-RISC

Message ID alpine.DEB.2.00.1106290043170.32272@artax.karlin.mff.cuni.cz (mailing list archive)
State Accepted, archived
Delegated to: Helge Deller
Headers show

Commit Message

Mikulas Patocka June 28, 2011, 10:48 p.m. UTC
Hi

This patch fixes frame unwind on PA-RISC.

Prior to the patch, stack backtraces often caused crashes and this 
resulted in recursive page faults.

Mikulas

---

This patch fixes backtrace on PA-RISC

There were several problems:

1) The code that decodes instructions handles instructions that subtract
from the stack pointer incorrectly. If the instruction subtracts the number X
from the stack pointer the code increases the frame size by (0x100000000-X).
This results in invalid accesses to memory and recursive page faults.

2) Becuase gcc reorders blocks, handling instructions that subtract from the
frame pointer is incorrect. For example, this function
"int f(int a)
{
	if (__builtin_expect(a, 1))
		return a;
	g();
	return a;
}"
is compiled in such a way, that the code that decreases the stack pointer for
the first "return a" is placed before the code for "g" call. If we recognize
this decrement, we mistakenly believe that the frame size for the "g" call
is zero.

To fix problems 1) and 2), the patch doesn't recognize instructions that
decrease the stack pointer at all. To further safeguard the unwind code
against nonsense values, we don't allow frame size larger than
Total_frame_size.

3) The backtrace is not locked. If stack dump races with module unload,
invalid table can be accessed.

This patch adds a spinlock when processing module tables.

Note, that for correct backtrace, you need recent binutils.
Binutils 2.18 from Debian 5 produce garbage unwind tables.
Binutils 2.21 work better (it sometimes forgets function frames, but at 
least it doesn't generate garbage).

Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>

---
 arch/parisc/kernel/unwind.c |   22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

John David Anglin June 29, 2011, 1:25 a.m. UTC | #1
On Tue, 28 Jun 2011, John David Anglin wrote:

> > @@ -305,18 +310,16 @@ static void unwind_frame_regs(struct unw
> >  
> >  			insn = *(unsigned int *)npc;
> >  
> > -			if ((insn & 0xffffc000) == 0x37de0000 ||
> > -			    (insn & 0xffe00000) == 0x6fc00000) {
> > +			if ((insn & 0xffffc001) == 0x37de0000 ||
> > +			    (insn & 0xffe00001) == 0x6fc00000) {
> >  				/* ldo X(sp), sp, or stwm X,D(sp) */
> > -				frame_size += (insn & 0x1 ? -1 << 13 : 0) | 
> > -					((insn & 0x3fff) >> 1);
> > +				frame_size += (insn & 0x3fff) >> 1;
> 
> This doesn't look correct to me.  Look at the disassembly code
> in binutils.  In wide mode, an extract_16 operation needs to be
> used for ldo.  In non wide mode and for stwm, extract_14 needs
> to be used.  extract_14 is a simple 14-bit extraction followed 
> low sign extension.
> 
> I believe the problem with the original code is that it doesn't
> handle wide mode (64-bit kernels) for ldo.

Here are the sign extension routines from binutils:

static inline int
sign_extend (int x, int len)
{
  int signbit = (1 << (len - 1));
  int mask = (signbit << 1) - 1;
  return ((x & mask) ^ signbit) - signbit;
}

static inline int
low_sign_extend (int x, int len)
{
  return (x >> 1) - ((x & 1) << (len - 1));
}

It probably doesn't matter too much but they are written in a manner
to avoid conditional tests.

Dave
diff mbox

Patch

Index: linux-2.6.39-fast/arch/parisc/kernel/unwind.c
===================================================================
--- linux-2.6.39-fast.orig/arch/parisc/kernel/unwind.c	2011-06-28 18:15:14.000000000 +0200
+++ linux-2.6.39-fast/arch/parisc/kernel/unwind.c	2011-06-29 00:24:43.000000000 +0200
@@ -75,7 +75,10 @@  find_unwind_entry(unsigned long addr)
 	if (addr >= kernel_unwind_table.start && 
 	    addr <= kernel_unwind_table.end)
 		e = find_unwind_entry_in_table(&kernel_unwind_table, addr);
-	else 
+	else {
+		unsigned long flags;
+
+		spin_lock_irqsave(&unwind_lock, flags);
 		list_for_each_entry(table, &unwind_tables, list) {
 			if (addr >= table->start && 
 			    addr <= table->end)
@@ -86,6 +89,8 @@  find_unwind_entry(unsigned long addr)
 				break;
 			}
 		}
+		spin_unlock_irqrestore(&unwind_lock, flags);
+	}
 
 	return e;
 }
@@ -305,18 +310,16 @@  static void unwind_frame_regs(struct unw
 
 			insn = *(unsigned int *)npc;
 
-			if ((insn & 0xffffc000) == 0x37de0000 ||
-			    (insn & 0xffe00000) == 0x6fc00000) {
+			if ((insn & 0xffffc001) == 0x37de0000 ||
+			    (insn & 0xffe00001) == 0x6fc00000) {
 				/* ldo X(sp), sp, or stwm X,D(sp) */
-				frame_size += (insn & 0x1 ? -1 << 13 : 0) | 
-					((insn & 0x3fff) >> 1);
+				frame_size += (insn & 0x3fff) >> 1;
 				dbg("analyzing func @ %lx, insn=%08x @ "
 				    "%lx, frame_size = %ld\n", info->ip,
 				    insn, npc, frame_size);
-			} else if ((insn & 0xffe00008) == 0x73c00008) {
+			} else if ((insn & 0xffe00009) == 0x73c00008) {
 				/* std,ma X,D(sp) */
-				frame_size += (insn & 0x1 ? -1 << 13 : 0) | 
-					(((insn >> 4) & 0x3ff) << 3);
+				frame_size += ((insn >> 4) & 0x3ff) << 3;
 				dbg("analyzing func @ %lx, insn=%08x @ "
 				    "%lx, frame_size = %ld\n", info->ip,
 				    insn, npc, frame_size);
@@ -335,6 +338,9 @@  static void unwind_frame_regs(struct unw
 			}
 		}
 
+		if (frame_size > e->Total_frame_size << 3)
+			frame_size = e->Total_frame_size << 3;
+
 		if (!unwind_special(info, e->region_start, frame_size)) {
 			info->prev_sp = info->sp - frame_size;
 			if (e->Millicode)