[patch] i386: add option to show more code in oops reports

[patch] i386: add option to show more code in oops reports

[Chuck Ebbert]

[-- Attachment #1: Type: text/plain, Size: 146 bytes --]

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 <cebbert@redhat.com>


[-- Attachment #2: i386_show_more_code.patch --]
[-- Type: text/plain, Size: 1852 bytes --]

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

--- 2.6.20-rc5-32.orig/Documentation/kernel-parameters.txt
+++ 2.6.20-rc5-32/Documentation/kernel-parameters.txt
@@ -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
--- 2.6.20-rc5-32.orig/arch/i386/kernel/traps.c
+++ 2.6.20-rc5-32/arch/i386/kernel/traps.c
@@ -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);

Re: [patch] i386: add option to show more code in oops reports

[Andrew Morton]

On Wed, 24 Jan 2007 15:22:49 -0500
Chuck Ebbert <cebbert@redhat.com> 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?

> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>

ooh, congrats.

> --- 2.6.20-rc5-32.orig/arch/i386/kernel/traps.c
> +++ 2.6.20-rc5-32/arch/i386/kernel/traps.c
> @@ -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.

>  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 */

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?

> @@ -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);

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...

Re: [patch] i386: add option to show more code in oops reports

[Chuck Ebbert]

Andrew Morton wrote:
> On Wed, 24 Jan 2007 15:22:49 -0500
> Chuck Ebbert <cebbert@redhat.com> 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.
>   
>> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
>>     
>
> ooh, congrats.
>   
Thanks. Looks like I'll have plenty to do here...
>   
>> --- 2.6.20-rc5-32.orig/arch/i386/kernel/traps.c
>> +++ 2.6.20-rc5-32/arch/i386/kernel/traps.c
>> @@ -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.
>
>   
>>  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 */
>>     
>
> 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?
>   
Oops.
>   
>> @@ -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);
>>     
>
> 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...
>
>   
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.)

Re: [patch] i386: add option to show more code in oops reports

[Andi Kleen]

On Friday 26 January 2007 00:56, Chuck Ebbert wrote:

> 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

Re: [patch] i386: add option to show more code in oops reports

[Chuck Ebbert]

Andi Kleen wrote:
> On Friday 26 January 2007 00:56, Chuck Ebbert wrote:
>
>   
>> 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.
>   
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
> 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.

Re: [patch] i386: add option to show more code in oops reports

[Andi Kleen]

> 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

[patch] i386: add option to show more code in oops reports

[Chuck Ebbert]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

Sometimes developers need to see more object code in an oops report,
e.g. when kernel may be corrupted at runtime.

Add the "code_bytes" option for this.

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>



[-- Attachment #2: i386_show_more_code.patch --]
[-- Type: text/plain, Size: 2187 bytes --]

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

--- 2.6.20-rc6.1-32smp.orig/Documentation/kernel-parameters.txt
+++ 2.6.20-rc6.1-32smp/Documentation/kernel-parameters.txt
@@ -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: 0 - 8192
+			Default: 64
+
 	disable_8254_timer
 	enable_8254_timer
 			[IA32/X86_64] Disable/Enable interrupt 0 timer routing
--- 2.6.20-rc6.1-32smp.orig/arch/i386/kernel/traps.c
+++ 2.6.20-rc6.1-32smp/arch/i386/kernel/traps.c
@@ -94,6 +94,7 @@ asmlinkage void spurious_interrupt_bug(v
 asmlinkage void machine_check(void);
 
 int kstack_depth_to_print = 24;
+static unsigned int code_bytes = 64;
 ATOMIC_NOTIFIER_HEAD(i386die_chain);
 
 int register_die_notifier(struct notifier_block *nb)
@@ -324,7 +325,8 @@ void show_registers(struct pt_regs *regs
 	 */
 	if (in_kernel) {
 		u8 *eip;
-		int code_bytes = 64;
+		unsigned int code_prologue = code_bytes * 43 / 64;
+		unsigned int code_len = code_bytes;
 		unsigned char c;
 
 		printk("\n" KERN_EMERG "Stack: ");
@@ -332,14 +334,14 @@ 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 */
 			eip = (u8 *)regs->eip;
-			code_bytes = 32;
+			code_len = code_len - code_prologue + 1;
 		}
-		for (i = 0; i < code_bytes; i++, eip++) {
+		for (i = 0; i < code_len; i++, eip++) {
 			if (eip < (u8 *)PAGE_OFFSET ||
 				probe_kernel_address(eip, c)) {
 				printk(" Bad EIP value.");
@@ -1191,3 +1193,13 @@ 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 > 8192)
+		code_bytes = 8192;
+
+	return 1;
+}
+__setup("code_bytes=", code_bytes_setup);