Skip to contentSkip to conversation

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

PATCH In review Chuck Ebbert 5 participants 7 messages Jul 10, 2006 – Jul 11, 2006

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 i386: handle_BUG(): don't print garbage if debug info unavailable

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.

---

Compile tested only, with/without CONFIG_DEBUG_BUGVERBOSE.

 arch/i386/kernel/traps.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)
arch/i386/kernel/traps.c +13 -7
@@ -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
Signature · 2 lines
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush
#1 lore raw
Quoted 54 lines · On Mon, 10 Jul 2006 10:30:55 -0400 Chuck Ebbert <[email protected]>…

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.

---

Compile tested only, with/without CONFIG_DEBUG_BUGVERBOSE.

 arch/i386/kernel/traps.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)
arch/i386/kernel/traps.c +11 -5
@@ -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
 

arch/i386/kernel/traps.c static void handle_BUG(struct pt_regs *r · line 363

Quoted 10 lines
arch/i386/kernel/traps.c +2 -2
 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?

#2 lore raw
Quoted 36 lines · Andrew Morton

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");
}
Quoted 1 line

OK?

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

Signature · 2 lines
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 
#3 lore raw
Quoted 2 lines · Nick Piggin

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

#4 lore raw
Quoted 9 lines · Linus Torvalds
Quoted 2 lines · Nick Piggin

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

#5 lore raw
In reply to Linus Torvalds
Quoted 5 lines · Linus Torvalds

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?

include/linux/uaccess.h renamed from include/linux/uaccess.h~add-probe_kernel_address +22 -0
@@ -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__ */

_