[patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Chuck Ebbert]

handle_BUG() tries to print file and line number even when
they're not available (CONFIG_DEBUG_BUGVERBOSE is not set.)
Change this to print a message stating info is unavailable
instead of printing a misleading message.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

---

 Compile tested only, with/without CONFIG_DEBUG_BUGVERBOSE.

 arch/i386/kernel/traps.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

--- 2.6.18-rc1-nb.orig/arch/i386/kernel/traps.c
+++ 2.6.18-rc1-nb/arch/i386/kernel/traps.c
@@ -324,13 +324,14 @@ void show_registers(struct pt_regs *regs
 
 static void handle_BUG(struct pt_regs *regs)
 {
+	unsigned long eip = regs->eip;
 	unsigned short ud2;
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
 	unsigned short line;
 	char *file;
 	char c;
-	unsigned long eip;
-
-	eip = regs->eip;
+#endif
 
 	if (eip < PAGE_OFFSET)
 		goto no_bug;
@@ -338,21 +339,26 @@ static void handle_BUG(struct pt_regs *r
 		goto no_bug;
 	if (ud2 != 0x0b0f)
 		goto no_bug;
+	printk(KERN_EMERG "------------[ cut here ]------------\n");
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
 	if (__get_user(line, (unsigned short __user *)(eip + 2)))
-		goto bug;
+		goto no_info;
 	if (__get_user(file, (char * __user *)(eip + 4)) ||
 		(unsigned long)file < PAGE_OFFSET || __get_user(c, file))
 		file = "<bad filename>";
 
-	printk(KERN_EMERG "------------[ cut here ]------------\n");
 	printk(KERN_EMERG "kernel BUG at %s:%d!\n", file, line);
+#else
+	goto no_info;
+#endif
 
 no_bug:
 	return;
 
 	/* Here we know it was a BUG but file-n-line is unavailable */
-bug:
-	printk(KERN_EMERG "Kernel BUG\n");
+no_info:
+	printk(KERN_EMERG "Kernel BUG at [verbose debug info unavailable]\n");
 }
 
 /* This is gone through when something in the kernel
-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: [patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Andrew Morton]

On Mon, 10 Jul 2006 10:30:55 -0400
Chuck Ebbert <76306.1226@compuserve.com> wrote:

> handle_BUG() tries to print file and line number even when
> they're not available (CONFIG_DEBUG_BUGVERBOSE is not set.)
> Change this to print a message stating info is unavailable
> instead of printing a misleading message.
> 
> Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>
> 
> ---
> 
>  Compile tested only, with/without CONFIG_DEBUG_BUGVERBOSE.
> 
>  arch/i386/kernel/traps.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
> 
> --- 2.6.18-rc1-nb.orig/arch/i386/kernel/traps.c
> +++ 2.6.18-rc1-nb/arch/i386/kernel/traps.c
> @@ -324,13 +324,14 @@ void show_registers(struct pt_regs *regs
>  
>  static void handle_BUG(struct pt_regs *regs)
>  {
> +	unsigned long eip = regs->eip;
>  	unsigned short ud2;
> +
> +#ifdef CONFIG_DEBUG_BUGVERBOSE
>  	unsigned short line;
>  	char *file;
>  	char c;
> -	unsigned long eip;
> -
> -	eip = regs->eip;
> +#endif
>  
>  	if (eip < PAGE_OFFSET)
>  		goto no_bug;
> @@ -338,21 +339,26 @@ static void handle_BUG(struct pt_regs *r
>  		goto no_bug;
>  	if (ud2 != 0x0b0f)
>  		goto no_bug;
> +	printk(KERN_EMERG "------------[ cut here ]------------\n");
> +
> +#ifdef CONFIG_DEBUG_BUGVERBOSE
>  	if (__get_user(line, (unsigned short __user *)(eip + 2)))
> -		goto bug;
> +		goto no_info;
>  	if (__get_user(file, (char * __user *)(eip + 4)) ||
>  		(unsigned long)file < PAGE_OFFSET || __get_user(c, file))
>  		file = "<bad filename>";
>  
> -	printk(KERN_EMERG "------------[ cut here ]------------\n");
>  	printk(KERN_EMERG "kernel BUG at %s:%d!\n", file, line);
> +#else
> +	goto no_info;
> +#endif
>  
>  no_bug:
>  	return;
>  
>  	/* Here we know it was a BUG but file-n-line is unavailable */
> -bug:
> -	printk(KERN_EMERG "Kernel BUG\n");
> +no_info:
> +	printk(KERN_EMERG "Kernel BUG at [verbose debug info unavailable]\n");
>  }
>  

I think we can do it a lot more tidily.

static void handle_BUG(struct pt_regs *regs)
{
	unsigned long eip = regs->eip;
	unsigned short ud2;

	if (eip < PAGE_OFFSET)
		return;
	if (__get_user(ud2, (unsigned short __user *)eip))
		return;
	if (ud2 != 0x0b0f)
		return;

	printk(KERN_EMERG "------------[ cut here ]------------\n");

#ifdef CONFIG_DEBUG_BUGVERBOSE
	do {
		unsigned short line;
		char *file;
		char c;

		if (__get_user(line, (unsigned short __user *)(eip + 2)))
			break;
		if (__get_user(file, (char * __user *)(eip + 4)) ||
		    (unsigned long)file < PAGE_OFFSET || __get_user(c, file))
			file = "<bad filename>";

		printk(KERN_EMERG "kernel BUG at %s:%d!\n", file, line);
		return;
	} while (0);
#endif
	printk(KERN_EMERG "Kernel BUG at [verbose debug info unavailable]\n");
}


OK?

Re: [patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Nick Piggin]

Andrew Morton wrote:

> I think we can do it a lot more tidily.
> 
> static void handle_BUG(struct pt_regs *regs)
> {
> 	unsigned long eip = regs->eip;
> 	unsigned short ud2;
> 
> 	if (eip < PAGE_OFFSET)
> 		return;
> 	if (__get_user(ud2, (unsigned short __user *)eip))
> 		return;
> 	if (ud2 != 0x0b0f)
> 		return;
> 
> 	printk(KERN_EMERG "------------[ cut here ]------------\n");
> 
> #ifdef CONFIG_DEBUG_BUGVERBOSE
> 	do {
> 		unsigned short line;
> 		char *file;
> 		char c;
> 
> 		if (__get_user(line, (unsigned short __user *)(eip + 2)))
> 			break;
> 		if (__get_user(file, (char * __user *)(eip + 4)) ||
> 		    (unsigned long)file < PAGE_OFFSET || __get_user(c, file))
> 			file = "<bad filename>";
> 
> 		printk(KERN_EMERG "kernel BUG at %s:%d!\n", file, line);
> 		return;
> 	} while (0);
> #endif
> 	printk(KERN_EMERG "Kernel BUG at [verbose debug info unavailable]\n");
> }
> 
> 
> OK?

OK but you don't need a do/while(0) here.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Linus Torvalds]

On Tue, 11 Jul 2006, Nick Piggin wrote:
> 
> OK but you don't need a do/while(0) here.

Actually, the way Andrew wrote it, it _is_ needed. It does two things:

 - it's the block scope that allows the private variables
 - if the "get_user()" fails, the "break" means that you don't have to 
   have a goto.

That said, I think it's wrong to use "__get_user()", which can hang on the 
MM semaphore if something is bogus. We should probably mark us as being 
"in_atomic()" to make sure that the page fault handler, if it is entered, 
will not try to get the semaphore or page anything in.

But that's a separate issue.

		Linus

Re: [patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Ray Lee]

On 7/11/06, Linus Torvalds <torvalds@osdl.org> wrote:
> On Tue, 11 Jul 2006, Nick Piggin wrote:
> >
> > OK but you don't need a do/while(0) here.
>
> Actually, the way Andrew wrote it, it _is_ needed. It does two things:
>
>  - it's the block scope that allows the private variables
>  - if the "get_user()" fails, the "break" means that you don't have to
>    have a goto.

<pedantry> The latter is true, but the former can also be done with
just bare braces:

  int a=4;
  {   int a=3;  printf("%d ",a); /* 3 */  }
  printf("%d ",a); /* 4 */

</pedantry, only useful to those who wish to write ugly code or source
code parsers>

Not being a person with actual *useful* skills, I can't comment on the
__get_user() issue :-).

Ray

Re: [patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> That said, I think it's wrong to use "__get_user()", which can hang on the 
> MM semaphore if something is bogus. We should probably mark us as being 
> "in_atomic()" to make sure that the page fault handler, if it is entered, 
> will not try to get the semaphore or page anything in.

This?


--- a/include/linux/uaccess.h~add-probe_kernel_address
+++ a/include/linux/uaccess.h
@@ -19,4 +19,26 @@ static inline unsigned long __copy_from_
 
 #endif		/* ARCH_HAS_NOCACHE_UACCESS */
 
+/**
+ * probe_kernel_address(): safely attempt to read from a location
+ * @addr: address to read from - its type is type typeof(retval)*
+ * @retval: read into this variable
+ *
+ * Safely read from address @addr into variable @revtal.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ * We ensure that the __get_user() is executed in atomic context so that
+ * do_page_fault() doesn't attempt to take mmap_sem.  This makes
+ * probe_kernel_address() suitable for use within regions where the caller
+ * already holds mmap_sem, or other locks which nest inside mmap_sem.
+ */
+#define probe_kernel_address(addr, retval)		\
+	({						\
+		long ret;				\
+							\
+		inc_preempt_count();			\
+		ret = __get_user(retval, addr);		\
+		dec_preempt_count();			\
+		ret;					\
+	})
+
 #endif		/* __LINUX_UACCESS_H__ */
_

Re: [patch] i386: handle_BUG(): don't print garbage if debug info unavailable

[Linus Torvalds]

On Tue, 11 Jul 2006, Andrew Morton wrote:
> 
> This?

Looks correct to me.

		Linus