do_wp_page: BUG on invalid pfn

PATCH In review Ed L Cashin 3 participants 8 messages Aug 15, 2003 1 message not in the archive

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ed L Cashin

author

2003-08-15 21:15 UTC #1 lore raw

PATCH 1/1 do_wp_page: BUG on invalid pfn

Quoted 6 lines · Rusty Russell

In message <[email protected]> you write:
Quoted 1 line
This patch just does what the comment says should be done.
Hi Ed!
Not trivial I'm afraid. Send to Linus and lkml.

This patch just does what the comment says should be done. I thought it was a trivial patch, but Rusty Russell has informed me otherwise. (Thanks, RR).

mm/memory.c renamed from mm/memory.c.orig +2 -8

@@ -990,15 +990,10 @@
 	int ret;
 
 	if (unlikely(!pfn_valid(pfn))) {
-		/*
-		 * This should really halt the system so it can be debugged or
-		 * at least the kernel stops what it's doing before it corrupts
-		 * data, but for the moment just pretend this is OOM.
-		 */
-		pte_unmap(page_table);
 		printk(KERN_ERR "do_wp_page: bogus page at address %08lx\n",
 				address);
-		goto oom;
+		dump_stack();
+		BUG();
 	}
 	old_page = pfn_to_page(pfn);

@@ -1054,7 +1049,6 @@
 
 no_mem:
 	page_cache_release(old_page);
-oom:
 	ret = VM_FAULT_OOM;
 out:
 	spin_unlock(&mm->page_table_lock);

Signature · 2 lines

--Ed L Cashin            |   PGP public key:
  [email protected]        |   http://noserose.net/e/pgp/

Mike Fedyk

2003-08-15 21:22 UTC #2 lore raw

Quoted 22 lines · Ed L Cashin

Quoted 6 lines · Rusty Russell
In message <[email protected]> you write:
Quoted 1 line
This patch just does what the comment says should be done.
Hi Ed!
Not trivial I'm afraid. Send to Linus and lkml.
This patch just does what the comment says should be done. I thought it was a trivial patch, but Rusty Russell has informed me otherwise. (Thanks, RR).
mm/memory.c renamed from mm/memory.c.orig +0 -1
@@ -990,15 +990,10 @@
 	int ret;
 
 	if (unlikely(!pfn_valid(pfn))) {
-		/*

mm/memory.c line 996

Quoted 10 lines

mm/memory.c renamed from mm/memory.c.orig +2 -6

-		 * This should really halt the system so it can be debugged or
-		 * at least the kernel stops what it's doing before it corrupts
-		 * data, but for the moment just pretend this is OOM.
-		 */
-		pte_unmap(page_table);
 		printk(KERN_ERR "do_wp_page: bogus page at address %08lx\n",
 				address);
-		goto oom;
+		dump_stack();
+		BUG();

You're not unmapping the pte I guess to not interfere with the dump_stack, but what about the printk? Will that affect the dump_stack also?

Ed L Cashin

author

2003-08-15 21:52 UTC #3 lore raw

Quoted 21 lines · Mike Fedyk

Quoted 20 lines · Ed L Cashin
Quoted 6 lines · Rusty Russell
In message <[email protected]> you write:
Quoted 1 line
This patch just does what the comment says should be done.
Hi Ed!
Not trivial I'm afraid. Send to Linus and lkml.
This patch just does what the comment says should be done. I thought it was a trivial patch, but Rusty Russell has informed me otherwise. (Thanks, RR).
mm/memory.c renamed from mm/memory.c.orig +0 -0
@@ -990,15 +990,10 @@
 	int ret;
 

Quoted 14 lines

mm/memory.c line 996

Quoted 12 lines

mm/memory.c renamed from mm/memory.c.orig +2 -7

 	if (unlikely(!pfn_valid(pfn))) {
-		/*
-		 * This should really halt the system so it can be debugged or
-		 * at least the kernel stops what it's doing before it corrupts
-		 * data, but for the moment just pretend this is OOM.
-		 */
-		pte_unmap(page_table);
 		printk(KERN_ERR "do_wp_page: bogus page at address %08lx\n",
 				address);
-		goto oom;
+		dump_stack();
+		BUG();

You're not unmapping the pte I guess to not interfere with the dump_stack,

This patch changes the logic from "pretend it's out of memory" to "announce something's very wrong and bail out right away." Unmapping the pte seems like a precursor to carrying on business as usual, but there must be some subtleties here that I am unaware of, or Rusty Russell wouldn't have called this patch non-trivial.

Quoted 1 line

but what about the printk? Will that affect the dump_stack also?

It seems like you'd return from the printk before dumping the stack, so I wouldn't think so.

Signature · 2 lines

--Ed L Cashin            |   PGP public key:
  [email protected]        |   http://noserose.net/e/pgp/

Mike Fedyk

2003-08-15 22:06 UTC #4 lore raw

Quoted 38 lines · Ed L Cashin

Quoted 21 lines · Mike Fedyk
Quoted 20 lines · Ed L Cashin
Quoted 6 lines · Rusty Russell
In message <[email protected]> you write:
Quoted 1 line
This patch just does what the comment says should be done.
Hi Ed!
Not trivial I'm afraid. Send to Linus and lkml.
This patch just does what the comment says should be done. I thought it was a trivial patch, but Rusty Russell has informed me otherwise. (Thanks, RR).
mm/memory.c renamed from mm/memory.c.orig +0 -0
@@ -990,15 +990,10 @@
 	int ret;
 
Quoted 14 lines
mm/memory.c line 996
Quoted 12 lines
mm/memory.c renamed from mm/memory.c.orig +2 -7
 	if (unlikely(!pfn_valid(pfn))) {
-		/*
-		 * This should really halt the system so it can be debugged or
-		 * at least the kernel stops what it's doing before it corrupts
-		 * data, but for the moment just pretend this is OOM.
-		 */
-		pte_unmap(page_table);
 		printk(KERN_ERR "do_wp_page: bogus page at address %08lx\n",
 				address);
-		goto oom;
+		dump_stack();
+		BUG();
You're not unmapping the pte I guess to not interfere with the dump_stack,

Quoted 5 lines

This patch changes the logic from "pretend it's out of memory" to "announce something's very wrong and bail out right away." Unmapping the pte seems like a precursor to carrying on business as usual, but there must be some subtleties here that I am unaware of, or Rusty Russell wouldn't have called this patch non-trivial.

So does show_stack() halt the kernel? If not, then you probably want the pte_unmap since you'll have a working/semi-working system after the bug() call.

And if show_stack() does halt the kernel, what's the point of bug() then?

Ed L Cashin

author

2003-08-15 22:18 UTC #5 lore raw

Quoted 1 line · Mike Fedyk

On Fri, Aug 15, 2003 at 05:52:09PM -0400, Ed L Cashin wrote:

...

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
mm/memory.c renamed from mm/memory.c.orig +2 -0
+		dump_stack();
+		BUG();

...

Quoted 1 line

So does show_stack() halt the kernel?

I'm not calling show_stack but dump_stack. For i386 it looks like handle_BUG in arch/i386/kernel/traps.c is the function that gets run when BUG raises an exception.

BUG() does halt the system, though.

Signature · 2 lines

--Ed L Cashin            |   PGP public key:
  [email protected]        |   http://noserose.net/e/pgp/

Russell King

2003-08-15 21:39 UTC #6 lore raw

In reply to Ed L Cashin

mm/memory.c line 996

Quoted 2 lines · Ed L Cashin

mm/memory.c renamed from mm/memory.c.orig +2 -0
+		dump_stack();
+		BUG();

Is there much point to both dump_stack and BUG() - BUG is supposed to provide a calltrace, which dump_stack also does. Do we really need two copies?

Signature · 2 lines

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Ed L Cashin

author

2003-08-15 21:50 UTC #7 lore raw

Quoted 7 lines · Russell King

mm/memory.c line 996
Quoted 2 lines · Ed L Cashin
mm/memory.c renamed from mm/memory.c.orig +2 -0
+		dump_stack();
+		BUG();
Is there much point to both dump_stack and BUG() - BUG is supposed to provide a calltrace, which dump_stack also does. Do we really need two copies?

On i386 WARN_ON calls dump_stack, but BUG just prints some minimal helpful info on the console, like this:

------------[ cut here ]------------
kernel BUG at kernel/any.c:36!

invalid operand: 0000 [#1]

Signature · 2 lines

--Ed L Cashin            |   PGP public key:
  [email protected]        |   http://noserose.net/e/pgp/

Russell King

2003-08-15 22:11 UTC #8 lore raw

Quoted 6 lines · Ed L Cashin

On i386 WARN_ON calls dump_stack, but BUG just prints some minimal helpful info on the console, like this:
------------[ cut here ]------------
kernel BUG at kernel/any.c:36!
invalid operand: 0000 [#1]

BUG causes an exception, which calls die(), which in turn calls handle_BUG(), and this indeed does print the first two lines of the above. die() goes on to print the 3rd line, but it also goes on to call show_registers() which should print the registers and calltrace as well.

Maybe you've found a bug in show_registers() ?

Signature · 2 lines

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html