i386: add option to show more code in oops reports

PATCH In review Chuck Ebbert 3 participants 6 messages Jan 24, 2007 – Jan 29, 2007 Documentation

lore

Share & tools

lore

Chuck Ebbert

author

2007-01-24 20:22 UTC #0 lore raw

PATCH 1/1 i386: add option to show more code in oops reports

Sometimes we may need to see more code than the default in an oops, so add an option for that.

Signed-off-by Chuck Ebbert <[email protected]>

 Documentation/kernel-parameters.txt |    5 +++++
 arch/i386/kernel/traps.c            |   17 +++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

Documentation/kernel-parameters.txt +5 -0

@@ -361,6 +361,11 @@ and is between 256 and 4096 characters. 
 			clocksource is not available, it defaults to PIT.
 			Format: { pit | tsc | cyclone | pmtmr }
 
+	code_bytes	[IA32] How many bytes of object code to print in an
+			oops report.
+			Range: 64 - 1024
+			Default: 64
+
 	disable_8254_timer
 	enable_8254_timer
 			[IA32/X86_64] Disable/Enable interrupt 0 timer routing

arch/i386/kernel/traps.c +15 -2

@@ -94,6 +94,7 @@ asmlinkage void spurious_interrupt_bug(v
 asmlinkage void machine_check(void);
 
 int kstack_depth_to_print = 24;
+int code_bytes = 64;
 ATOMIC_NOTIFIER_HEAD(i386die_chain);
 
 int register_die_notifier(struct notifier_block *nb)

@@ -324,7 +325,7 @@ void show_registers(struct pt_regs *regs
 	 */
 	if (in_kernel) {
 		u8 *eip;
-		int code_bytes = 64;
+		int code_prologue = code_bytes * 43 / 64;
 		unsigned char c;
 
 		printk("\n" KERN_EMERG "Stack: ");

@@ -332,7 +333,7 @@ void show_registers(struct pt_regs *regs
 
 		printk(KERN_EMERG "Code: ");
 
-		eip = (u8 *)regs->eip - 43;
+		eip = (u8 *)regs->eip - code_prologue;
 		if (eip < (u8 *)PAGE_OFFSET ||
 			probe_kernel_address(eip, c)) {
 			/* try starting at EIP */

@@ -1191,3 +1192,15 @@ static int __init kstack_setup(char *s)
 	return 1;
 }
 __setup("kstack=", kstack_setup);
+
+static int __init code_bytes_setup(char *s)
+{
+	code_bytes = simple_strtoul(s, NULL, 0);
+	if (code_bytes < 64)
+		code_bytes = 64;
+	if (code_bytes > 1024)
+		code_bytes = 1024;
+
+	return 1;
+}
+__setup("code_bytes=", code_bytes_setup);

Andrew Morton

2007-01-25 22:55 UTC #1 lore raw

Quoted 2 lines · On Wed, 24 Jan 2007 15:22:49 -0500 Chuck Ebbert <[email protected]> wrote:

Sometimes we may need to see more code than the default in an oops, so add an option for that.

spose so, but some more justification would be nice. As would an x86_64 version?

Quoted 1 line

Signed-off-by Chuck Ebbert <[email protected]>

ooh, congrats.

arch/i386/kernel/traps.c asmlinkage void spurious_interrupt_bug(v · line 97

Quoted 7 lines

arch/i386/kernel/traps.c +1 -0

@@ -94,6 +94,7 @@ asmlinkage void spurious_interrupt_bug(v
 asmlinkage void machine_check(void);
 
 int kstack_depth_to_print = 24;
+int code_bytes = 64;

static scope, please. And I think it should be unsigned.

Quoted 13 lines

arch/i386/kernel/traps.c +1 -1

 ATOMIC_NOTIFIER_HEAD(i386die_chain);
 
 int register_die_notifier(struct notifier_block *nb)

@@ -324,7 +325,7 @@ void show_registers(struct pt_regs *regs
 	 */
 	if (in_kernel) {
 		u8 *eip;
-		int code_bytes = 64;
+		int code_prologue = code_bytes * 43 / 64;
 		unsigned char c;
 
 		printk("\n" KERN_EMERG "Stack: ");

arch/i386/kernel/traps.c void show_registers(struct pt_regs *regs · line 339

Quoted 8 lines

arch/i386/kernel/traps.c +1 -1

@@ -332,7 +333,7 @@ void show_registers(struct pt_regs *regs
 
 		printk(KERN_EMERG "Code: ");
 
-		eip = (u8 *)regs->eip - 43;
+		eip = (u8 *)regs->eip - code_prologue;
 		if (eip < (u8 *)PAGE_OFFSET ||
 			probe_kernel_address(eip, c)) {
 			/* try starting at EIP */

You missed this bit:

		if (eip < (u8 *)PAGE_OFFSET ||
			probe_kernel_address(eip, c)) {
			/* try starting at EIP */
			eip = (u8 *)regs->eip;
			code_bytes = 32;
		}

Do we really want to be modifying the global variable here?

Quoted 6 lines

arch/i386/kernel/traps.c +2 -0

@@ -1191,3 +1192,15 @@ static int __init kstack_setup(char *s)
 	return 1;
 }
 __setup("kstack=", kstack_setup);
+
+static int __init code_bytes_setup(char *s)

arch/i386/kernel/traps.c static int __init kstack_setup(char *s) · line 1206

Quoted 10 lines

arch/i386/kernel/traps.c +10 -0

+{
+	code_bytes = simple_strtoul(s, NULL, 0);
+	if (code_bytes < 64)
+		code_bytes = 64;
+	if (code_bytes > 1024)
+		code_bytes = 1024;
+
+	return 1;
+}
+__setup("code_bytes=", code_bytes_setup);

I'm OK with the upper limit, but I'd sugegst that we remove the lower
limit: someone might _want_ to be able to set code_bytes=0, who knows?

And if code_bytes is unsigned, the single comparison with 1024 will suffice.

OTOH, why have any checks at all in there? If the user sets code_bytes=0xfffffff0 and things break, he gets to own both pieces...

Chuck Ebbert

author

2007-01-25 23:56 UTC #2 lore raw

Quoted 11 lines · Andrew Morton

Quoted 3 lines · On Wed, 24 Jan 2007 15:22:49 -0500 Chuck Ebbert <[email protected]> wrote:
Sometimes we may need to see more code than the default in an oops, so add an option for that.
spose so, but some more justification would be nice. As would an x86_64 version?

Can't think of a way to word the justification, but I've wanted to see more code a few times.

As for x86_64, Andi doesn't want to print any code before the failure and just
printing more afterwards didn't seem to make much sense.

Quoted 6 lines

Quoted 3 lines
Signed-off-by Chuck Ebbert <[email protected]>
ooh, congrats.

Thanks. Looks like I'll have plenty to do here...

Quoted 38 lines

arch/i386/kernel/traps.c asmlinkage void spurious_interrupt_bug(v · line 98
Quoted 9 lines
arch/i386/kernel/traps.c +1 -0
@@ -94,6 +94,7 @@ asmlinkage void spurious_interrupt_bug(v
 asmlinkage void machine_check(void);
 
 int kstack_depth_to_print = 24;
+int code_bytes = 64;
 
static scope, please. And I think it should be unsigned.
Quoted 14 lines
arch/i386/kernel/traps.c +0 -0
 ATOMIC_NOTIFIER_HEAD(i386die_chain);
 
int register_die_notifier(struct notifier_block *nb)
arch/i386/kernel/traps.c +1 -1
@@ -324,7 +325,7 @@ void show_registers(struct pt_regs *regs
 	 */
 	if (in_kernel) {
 		u8 *eip;
-		int code_bytes = 64;
+		int code_prologue = code_bytes * 43 / 64;
 		unsigned char c;
 
 		printk("\n" KERN_EMERG "Stack: ");
arch/i386/kernel/traps.c void show_registers(struct pt_regs *regs · line 339
Quoted 8 lines
arch/i386/kernel/traps.c +1 -1
@@ -332,7 +333,7 @@ void show_registers(struct pt_regs *regs
 
 		printk(KERN_EMERG "Code: ");
 
-		eip = (u8 *)regs->eip - 43;
+		eip = (u8 *)regs->eip - code_prologue;
 		if (eip < (u8 *)PAGE_OFFSET ||
 			probe_kernel_address(eip, c)) {
 			/* try starting at EIP */
You missed this bit:

Quoted 9 lines

		if (eip < (u8 *)PAGE_OFFSET ||
			probe_kernel_address(eip, c)) {
			/* try starting at EIP */
			eip = (u8 *)regs->eip;
			code_bytes = 32;
		}

Do we really want to be modifying the global variable here?

Oops.

Quoted 22 lines

Quoted 8 lines
arch/i386/kernel/traps.c +2 -0
@@ -1191,3 +1192,15 @@ static int __init kstack_setup(char *s)
 	return 1;
 }
 __setup("kstack=", kstack_setup);
+
+static int __init code_bytes_setup(char *s)
arch/i386/kernel/traps.c static int __init kstack_setup(char *s) · line 1206
Quoted 10 lines
arch/i386/kernel/traps.c +10 -0
+{
+	code_bytes = simple_strtoul(s, NULL, 0);
+	if (code_bytes < 64)
+		code_bytes = 64;
+	if (code_bytes > 1024)
+		code_bytes = 1024;
+
+	return 1;
+}
+__setup("code_bytes=", code_bytes_setup);
I'm OK with the upper limit, but I'd sugegst that we remove the lower
limit: someone might _want_ to be able to set code_bytes=0, who knows?

Quoted 6 lines

And if code_bytes is unsigned, the single comparison with 1024 will suffice.
OTOH, why have any checks at all in there? If the user sets code_bytes=0xfffffff0 and things break, he gets to own both pieces...

It's multiplying the number by 43 and dividing by 64, so we need to avoid overflow.
(I couldn't think of an easy way to preserve current behavior.)

Andi Kleen

2007-01-29 08:47 UTC #3 lore raw

Quoted 2 lines · Chuck Ebbert

Can't think of a way to word the justification, but I've wanted to see more code a few times.

Hmm, not sure I see the point. The Code line is just that you can make sense of random mailing list oopses where you don't
have a vmlinux. But as long as you don't make the option
the default you would need to ask people to set it for you -- and when you ask you could always as well ask about the vmlinux and get
as much code as you ever wanted.

So unless it's default it's likely useless and I don't think it is a good idea to make it default because oops screen estate is so precious.

-Andi

Chuck Ebbert

author

2007-01-29 15:39 UTC #4 lore raw

Quoted 7 lines · Andi Kleen

Quoted 3 lines · Chuck Ebbert
Can't think of a way to word the justification, but I've wanted to see more code a few times.

Quoted 7 lines

Hmm, not sure I see the point. The Code line is just that you can make sense of random mailing list oopses where you don't
have a vmlinux. But as long as you don't make the option
the default you would need to ask people to set it for you -- and when you ask you could always as well ask about the vmlinux and get
as much code as you ever wanted.

This was patch inspired by my finding out that code in the running kernel might have
been modified at runtime by some strange bug, and looking at vmlinux might not be
helpful.

See:

    http://lkml.org/lkml/2007/1/24/143

Quoted 4 lines

So unless it's default it's likely useless and I don't think it is a good idea to make it default because oops screen estate is so precious.

Yeah, there's no way it could be the default. But I'd like to see if Alistair John Strachan's
running kernel matches the vmlinux he posted with that strange unexplained oops.

Andi Kleen

2007-01-29 18:03 UTC #5 lore raw

Quoted 6 lines

This was patch inspired by my finding out that code in the running kernel might have
been modified at runtime by some strange bug, and looking at vmlinux might not be
helpful.

Hmm ok, although I still suspect in such a case you'll be better off with a full kcrash dump.

-Andi