[PATCH] x86: Unify printk strings in fault_32|64.c

[PATCH] x86: Unify printk strings in fault_32|64.c

[Harvey Harrison]

Adding the address of the faulting library missed removing a
line ending from X86_32.

Also update the shorter printk format for X86_32 in fault_64.c
to make it easier to se the remaining differences.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Ingo, trivial printk update after Andi's patches.

 arch/x86/mm/fault_32.c |    2 +-
 arch/x86/mm/fault_64.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/fault_32.c b/arch/x86/mm/fault_32.c
index 793e830..0bd2417 100644
--- a/arch/x86/mm/fault_32.c
+++ b/arch/x86/mm/fault_32.c
@@ -589,7 +589,7 @@ bad_area_nosemaphore:
 		    printk_ratelimit()) {
 			printk(
 #ifdef CONFIG_X86_32
-			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx\n",
+			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx",
 #else
 			"%s%s[%d]: segfault at %lx ip %lx sp %lx error %lx",
 #endif
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index 9270a7d..9ac449e 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -591,7 +591,7 @@ bad_area_nosemaphore:
 		    printk_ratelimit()) {
 			printk(
 #ifdef CONFIG_X86_32
-			"%s%s[%d]: segfault at %08lx ip %08lx sp %08lx error %lx\n",
+			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx",
 #else
 			"%s%s[%d]: segfault at %lx ip %lx sp %lx error %lx",
 #endif
-- 
1.5.4.rc3.1118.gf6754c

Re: [PATCH] x86: Unify printk strings in fault_32|64.c

[Andi Kleen]

On Saturday 19 January 2008 05:22:29 Harvey Harrison wrote:
> Adding the address of the faulting library missed removing a
> line ending from X86_32.
> 
> Also update the shorter printk format for X86_32 in fault_64.c
> to make it easier to se the remaining differences.

Thanks. I think it was correct initially, but one of the merge steps
with the changing git-x86 caused some hunks to be dropped and the patch 
never quite recovered from that.

-Andi

Re: [PATCH] x86: Unify printk strings in fault_32|64.c

[Harvey Harrison]

On Sat, 2008-01-19 at 06:08 +0100, Andi Kleen wrote:
> On Saturday 19 January 2008 05:22:29 Harvey Harrison wrote:
> > Adding the address of the faulting library missed removing a
> > line ending from X86_32.
> > 
> > Also update the shorter printk format for X86_32 in fault_64.c
> > to make it easier to se the remaining differences.
> 
> Thanks. I think it was correct initially, but one of the merge steps
> with the changing git-x86 caused some hunks to be dropped and the patch 
> never quite recovered from that.

No worries, hoping to get them unified this weekend, should make this
easier going forward.

Harvey

Re: [PATCH] x86: Unify printk strings in fault_32|64.c

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

>  #ifdef CONFIG_X86_32
> -			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx\n",
> +			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx",
>  #else

>  #ifdef CONFIG_X86_32
> -			"%s%s[%d]: segfault at %08lx ip %08lx sp %08lx error %lx\n",
> +			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx",
>  #else

thanks, applied.

	Ingo

Re: [PATCH] x86: Unify printk strings in fault_32|64.c

[Harvey Harrison]

On Sat, 2008-01-19 at 09:13 +0100, Ingo Molnar wrote:
> * Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> >  #ifdef CONFIG_X86_32
> > -			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx\n",
> > +			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx",
> >  #else
> 
> >  #ifdef CONFIG_X86_32
> > -			"%s%s[%d]: segfault at %08lx ip %08lx sp %08lx error %lx\n",
> > +			"%s%s[%d]: segfault at %lx ip %08lx sp %08lx error %lx",
> >  #else
> 
> thanks, applied.
> 
> 	Ingo

I'm just looking at some of the other printks in do+page_fault right
now in the oops code.

What do you prefer:
	"Unable to handle kernel NULL pointer dereference"
or
	"BUG: unable to handle kernel NULL pointer dereference"

Other than that, I think for the most part, the shorter X86_64 strings
are better.

This does change user-visible output however, but would be nice to have
a similar format.

Harvey

[RFC PATCH] x86: Pull out oops printing helper from do_page_fault

[Harvey Harrison]

Pull out an show_fault_oops() helper out from the oops printing
code in do_page_fault.

Try to make 32/64 bit oops traces have more similar format.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Ingo, for discussion about how the oops traces should look,
pull out a helper function.  This changes the existing format.

 arch/x86/mm/fault_32.c |   53 ++++++++++++++++++++++++++---------------------
 arch/x86/mm/fault_64.c |   24 +++++++++++++++------
 2 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/arch/x86/mm/fault_32.c b/arch/x86/mm/fault_32.c
index 0bd2417..e15b80b 100644
--- a/arch/x86/mm/fault_32.c
+++ b/arch/x86/mm/fault_32.c
@@ -396,6 +396,34 @@ static inline int vmalloc_fault(unsigned long address)
 #endif
 }
 
+static void show_fault_oops(struct pt_regs *regs, unsigned long error_code
+			    unsigned long address)
+{
+	if (!oops_may_print())
+		return;
+
+#ifdef CONFIG_X86_PAE
+	if (error_code & PF_INSTR) {
+		int level;
+		pte_t *pte = lookup_address(address, &level);
+
+		if (pte && pte_present(*pte) && !pte_exec(*pte))
+			printk(KERN_CRIT "kernel tried to execute "
+				"NX-protected page - exploit attempt? "
+				"(uid: %d)\n", current->uid);
+	}
+#endif
+	printk(KERN_ALERT "BUG: unable to handle kernel ");
+	if (address < PAGE_SIZE)
+		printk(KERN_CONT "NULL pointer dereference");
+	else
+		printk(KERN_CONT "paging request");
+
+	printk(KERN_CONT " at: %08lx IP: %08lx\n", address, regs->ip);
+
+	dump_pagetable(address);
+}
+
 int show_unhandled_signals = 1;
 
 /*
@@ -633,30 +661,7 @@ no_context:
 
 	bust_spinlocks(1);
 
-	if (oops_may_print()) {
-
-#ifdef CONFIG_X86_PAE
-		if (error_code & PF_INSTR) {
-			int level;
-			pte_t *pte = lookup_address(address, &level);
-
-			if (pte && pte_present(*pte) && !pte_exec(*pte))
-				printk(KERN_CRIT "kernel tried to execute "
-					"NX-protected page - exploit attempt? "
-					"(uid: %d)\n", current->uid);
-		}
-#endif
-		if (address < PAGE_SIZE)
-			printk(KERN_ALERT "BUG: unable to handle kernel NULL "
-					"pointer dereference");
-		else
-			printk(KERN_ALERT "BUG: unable to handle kernel paging"
-					" request");
-		printk(" at virtual address %08lx\n", address);
-		printk(KERN_ALERT "printing ip: %08lx ", regs->ip);
-
-		dump_pagetable(address);
-	}
+	show_fault_oops(regs, error_code, address);
 
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index ccbb8e3..dce2893 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -384,6 +384,21 @@ static int vmalloc_fault(unsigned long address)
 #endif
 }
 
+static void show_fault_oops(struct pt_regs *regs, unsigned long error_code
+			    unsigned long address)
+{
+	printk(KERN_ALERT "BUG: unable to handle kernel ");
+	if (address < PAGE_SIZE)
+		printk(KERN_CONT "NULL pointer dereference");
+	else
+		printk(KERN_CONT "paging request");
+
+	printk(KERN_CONT " at %016lx IP: \n", address);
+	printk_address(regs->ip, 1);
+
+	dump_pagetable(address);
+}
+
 int show_unhandled_signals = 1;
 
 /*
@@ -636,13 +651,8 @@ no_context:
 
 	flags = oops_begin();
 
-	if (address < PAGE_SIZE)
-		printk(KERN_ALERT "Unable to handle kernel NULL pointer dereference");
-	else
-		printk(KERN_ALERT "Unable to handle kernel paging request");
-	printk(" at %016lx RIP: \n" KERN_ALERT, address);
-	printk_address(regs->ip, 1);
-	dump_pagetable(address);
+	show_fault_oops(regs, error_code, address);
+
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
-- 
1.5.4.rc3.1118.gf6754c

Re: [RFC PATCHv2] x86: Pull out oops printing helper from do_page_fault

[Harvey Harrison]

Pull out an show_fault_oops() helper out from the oops printing
code in do_page_fault.

Try to make 32/64 bit oops traces have more similar format.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Clearly that last patch was for discussion only, he's one that
actually compiles (missing comma on args in show_fault_oops)

 arch/x86/mm/fault_32.c |   53 ++++++++++++++++++++++++++---------------------
 arch/x86/mm/fault_64.c |   24 +++++++++++++++------
 2 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/arch/x86/mm/fault_32.c b/arch/x86/mm/fault_32.c
index 0bd2417..83b4c1a 100644
--- a/arch/x86/mm/fault_32.c
+++ b/arch/x86/mm/fault_32.c
@@ -396,6 +396,34 @@ static inline int vmalloc_fault(unsigned long address)
 #endif
 }
 
+static void show_fault_oops(struct pt_regs *regs, unsigned long error_code,
+			    unsigned long address)
+{
+	if (!oops_may_print())
+		return;
+
+#ifdef CONFIG_X86_PAE
+	if (error_code & PF_INSTR) {
+		int level;
+		pte_t *pte = lookup_address(address, &level);
+
+		if (pte && pte_present(*pte) && !pte_exec(*pte))
+			printk(KERN_CRIT "kernel tried to execute "
+				"NX-protected page - exploit attempt? "
+				"(uid: %d)\n", current->uid);
+	}
+#endif
+	printk(KERN_ALERT "BUG: unable to handle kernel ");
+	if (address < PAGE_SIZE)
+		printk(KERN_CONT "NULL pointer dereference");
+	else
+		printk(KERN_CONT "paging request");
+
+	printk(KERN_CONT " at: %08lx IP: %08lx\n", address, regs->ip);
+
+	dump_pagetable(address);
+}
+
 int show_unhandled_signals = 1;
 
 /*
@@ -633,30 +661,7 @@ no_context:
 
 	bust_spinlocks(1);
 
-	if (oops_may_print()) {
-
-#ifdef CONFIG_X86_PAE
-		if (error_code & PF_INSTR) {
-			int level;
-			pte_t *pte = lookup_address(address, &level);
-
-			if (pte && pte_present(*pte) && !pte_exec(*pte))
-				printk(KERN_CRIT "kernel tried to execute "
-					"NX-protected page - exploit attempt? "
-					"(uid: %d)\n", current->uid);
-		}
-#endif
-		if (address < PAGE_SIZE)
-			printk(KERN_ALERT "BUG: unable to handle kernel NULL "
-					"pointer dereference");
-		else
-			printk(KERN_ALERT "BUG: unable to handle kernel paging"
-					" request");
-		printk(" at virtual address %08lx\n", address);
-		printk(KERN_ALERT "printing ip: %08lx ", regs->ip);
-
-		dump_pagetable(address);
-	}
+	show_fault_oops(regs, error_code, address);
 
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index ccbb8e3..34f50f6 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -384,6 +384,21 @@ static int vmalloc_fault(unsigned long address)
 #endif
 }
 
+static void show_fault_oops(struct pt_regs *regs, unsigned long error_code,
+			    unsigned long address)
+{
+	printk(KERN_ALERT "BUG: unable to handle kernel ");
+	if (address < PAGE_SIZE)
+		printk(KERN_CONT "NULL pointer dereference");
+	else
+		printk(KERN_CONT "paging request");
+
+	printk(KERN_CONT " at %016lx IP: \n", address);
+	printk_address(regs->ip, 1);
+
+	dump_pagetable(address);
+}
+
 int show_unhandled_signals = 1;
 
 /*
@@ -636,13 +651,8 @@ no_context:
 
 	flags = oops_begin();
 
-	if (address < PAGE_SIZE)
-		printk(KERN_ALERT "Unable to handle kernel NULL pointer dereference");
-	else
-		printk(KERN_ALERT "Unable to handle kernel paging request");
-	printk(" at %016lx RIP: \n" KERN_ALERT, address);
-	printk_address(regs->ip, 1);
-	dump_pagetable(address);
+	show_fault_oops(regs, error_code, address);
+
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
-- 
1.5.4.rc3.1118.gf6754c