Re: Top kernel oopses/warnings this week

[Arjan van de Ven <arjan@linux.intel.com>]

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

Matt Mackall wrote:
> 
> Blech. Invoking the random pool machinery at oops time is moderately
> safe, but not very shiny. Going through all the sprintf ugliness to
> format it to an irrelevant UUID standard is not very shiny either. At
> least refactor it so it's not duplicating code.
> 
> And I'd much rather the static variable lived with its user, as
> random.c is already too miscellaneous:

ok so something like this?


From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [patch] Print end-of-oops marker with UUID

Right now, it's nearly impossible for parsers to detect the end-of-oops
condition; for example this is a problem for www.kerneloops.org.
In addition, it's not currently possible to detect whether or not
2 oopses that look alike are actually the same oops reported twice,
or truely 2 unique oopses.

This patch factors out the "sprintf a UUID into a string" code from
random.c into a separate function (using snprintf as suggested by
Randy). So far I left the %02x in place instead of using Linus'
"improvement"; if someone really hates the %02x's he/she can do that
later.

It also reduces the stack footprint of proc_do_uuid(); it
was using 64 bytes for the string where 37 is sufficient.
With these random.c changes, the oops_exit() function can print an
end-of-oops marker from the oops_exit() function.

Normally, the UUID used for oopses is calculated as late_initcall
(in the hope that at that time there is enough entropy to get a
unique enough UUID); however for early oopses the oops_exit() function
needs to generate the UUID on the fly.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
CC: Matt
CC: Ted
CC: Randy

--- linux-2.6.24-rc5/drivers/char/random.c.org	2007-12-18 11:37:22.000000000 -0800
+++ linux-2.6.24-rc5/drivers/char/random.c	2007-12-18 12:20:48.000000000 -0800
@@ -1176,8 +1175,34 @@ static int max_read_thresh = INPUT_POOL_
  static int max_write_thresh = INPUT_POOL_WORDS * 32;
  static char sysctl_bootid[16];

+
+/**
+ * snprintf_uuid - Convert a 16 byte UUID into string format
+ * @string: buffer to store the UUID into
+ * @len:    size of @string
+ * @uuid:   the UUID to convert
+ *
+ * This function converts a 16 byte binary UUID into canonical
+ * ASCII form. This ASCII form needs 37 bytes of storage space,
+ * allocated and provided by the caller.
+ *
+ * Returns: pointer to @string
+ *
+ * Locking: none
+ */
+const char *snprintf_uuid(char *string, int len, unsigned char *uuid)
+{
+	snprintf(string, len,  "%02x%02x%02x%02x-%02x%02x-%02x%02x-"
+		"%02x%02x-%02x%02x%02x%02x%02x%02x",
+		uuid[0],  uuid[1],  uuid[2],  uuid[3],
+		uuid[4],  uuid[5],  uuid[6],  uuid[7],
+		uuid[8],  uuid[9],  uuid[10], uuid[11],
+		uuid[12], uuid[13], uuid[14], uuid[15]);
+	return string;
+}
+
  /*
- * These functions is used to return both the bootid UUID, and random
+ * These functions are used to return both the bootid UUID, and random
   * UUID.  The difference is in whether table->data is NULL; if it is,
   * then a new UUID is generated and returned to the user.
   *
@@ -1189,7 +1214,7 @@ static int proc_do_uuid(ctl_table *table
  			void __user *buffer, size_t *lenp, loff_t *ppos)
  {
  	ctl_table fake_table;
-	unsigned char buf[64], tmp_uuid[16], *uuid;
+	unsigned char buf[37], tmp_uuid[16], *uuid;

  	uuid = table->data;
  	if (!uuid) {
@@ -1199,12 +1224,7 @@ static int proc_do_uuid(ctl_table *table
  	if (uuid[8] == 0)
  		generate_random_uuid(uuid);

-	sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-"
-		"%02x%02x%02x%02x%02x%02x",
-		uuid[0],  uuid[1],  uuid[2],  uuid[3],
-		uuid[4],  uuid[5],  uuid[6],  uuid[7],
-		uuid[8],  uuid[9],  uuid[10], uuid[11],
-		uuid[12], uuid[13], uuid[14], uuid[15]);
+	snprintf_uuid(buf, sizeof(buf), uuid);
  	fake_table.data = buf;
  	fake_table.maxlen = sizeof(buf);

--- linux-2.6.24-rc5/include/linux/random.h.org	2007-12-18 12:22:49.000000000 -0800
+++ linux-2.6.24-rc5/include/linux/random.h	2007-12-18 12:22:57.000000000 -0800
@@ -71,6 +71,7 @@ unsigned long randomize_range(unsigned l

  u32 random32(void);
  void srandom32(u32 seed);
+const char *snprintf_uuid(char *string, int len, unsigned char *uuid);

  #endif /* __KERNEL___ */

--- linux-2.6.24-rc5/kernel/panic.c.org	2007-12-18 12:23:19.000000000 -0800
+++ linux-2.6.24-rc5/kernel/panic.c	2007-12-18 12:35:46.000000000 -0800
@@ -19,6 +19,7 @@
  #include <linux/nmi.h>
  #include <linux/kexec.h>
  #include <linux/debug_locks.h>
+#include <linux/random.h>

  int panic_on_oops;
  int tainted;
@@ -32,6 +33,8 @@ ATOMIC_NOTIFIER_HEAD(panic_notifier_list

  EXPORT_SYMBOL(panic_notifier_list);

+static unsigned char oops_uuid[16];
+
  static int __init panic_setup(char *str)
  {
  	panic_timeout = simple_strtoul(str, NULL, 0);
@@ -265,15 +268,32 @@ void oops_enter(void)
  	do_oops_enter_exit();
  }

+static int prime_oops_uuid(void)
+{
+	if (oops_uuid[8] == 0)
+		generate_random_uuid(oops_uuid);
+	return 0;
+}
+
  /*
   * Called when the architecture exits its oops handler, after printing
   * everything.
   */
  void oops_exit(void)
  {
+	char uuid_string[37];
  	do_oops_enter_exit();
+
+	/*
+	 * normally the oops_uid is already calculated, but if we oops during
+	 * really early boot, it may not be. In that case, calculate it here.
+	 */
+	prime_oops_uuid();
+	printk("---[ end trace %s ]---\n",
+		snprintf_uuid(uuid_string, sizeof(uuid_string), oops_uuid));
  }

+late_initcall(prime_oops_uuid);
  #ifdef CONFIG_CC_STACKPROTECTOR
  /*
   * Called when gcc's -fstack-protector feature is used, and

[-- Attachment #2: oopsend2.patch --]
[-- Type: text/x-patch, Size: 5063 bytes --]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [patch] Print end-of-oops marker with UUID

Right now, it's nearly impossible for parsers to detect the end-of-oops
condition; for example this is a problem for www.kerneloops.org.
In addition, it's not currently possible to detect whether or not
2 oopses that look alike are actually the same oops reported twice,
or truely 2 unique oopses. 

This patch factors out the "sprintf a UUID into a string" code from
random.c into a separate function (using snprintf as suggested by
Randy). So far I left the %02x in place instead of using Linus' 
"improvement"; if someone really hates the %02x's he/she can do that
later.

It also reduces the stack footprint of proc_do_uuid(); it
was using 64 bytes for the string where 37 is sufficient.
With these random.c changes, the oops_exit() function can print an
end-of-oops marker from the oops_exit() function.

Normally, the UUID used for oopses is calculated as late_initcall 
(in the hope that at that time there is enough entropy to get a 
unique enough UUID); however for early oopses the oops_exit() function
needs to generate the UUID on the fly.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
CC: Matt
CC: Ted
CC: Randy

--- linux-2.6.24-rc5/drivers/char/random.c.org	2007-12-18 11:37:22.000000000 -0800
+++ linux-2.6.24-rc5/drivers/char/random.c	2007-12-18 12:20:48.000000000 -0800
@@ -1176,8 +1175,34 @@ static int max_read_thresh = INPUT_POOL_
 static int max_write_thresh = INPUT_POOL_WORDS * 32;
 static char sysctl_bootid[16];
 
+
+/**
+ * snprintf_uuid - Convert a 16 byte UUID into string format
+ * @string: buffer to store the UUID into
+ * @len:    size of @string
+ * @uuid:   the UUID to convert
+ *
+ * This function converts a 16 byte binary UUID into canonical
+ * ASCII form. This ASCII form needs 37 bytes of storage space,
+ * allocated and provided by the caller.
+ *
+ * Returns: pointer to @string
+ *
+ * Locking: none
+ */
+const char *snprintf_uuid(char *string, int len, unsigned char *uuid)
+{
+	snprintf(string, len,  "%02x%02x%02x%02x-%02x%02x-%02x%02x-"
+		"%02x%02x-%02x%02x%02x%02x%02x%02x",
+		uuid[0],  uuid[1],  uuid[2],  uuid[3],
+		uuid[4],  uuid[5],  uuid[6],  uuid[7],
+		uuid[8],  uuid[9],  uuid[10], uuid[11],
+		uuid[12], uuid[13], uuid[14], uuid[15]);
+	return string;
+}
+
 /*
- * These functions is used to return both the bootid UUID, and random
+ * These functions are used to return both the bootid UUID, and random
  * UUID.  The difference is in whether table->data is NULL; if it is,
  * then a new UUID is generated and returned to the user.
  *
@@ -1189,7 +1214,7 @@ static int proc_do_uuid(ctl_table *table
 			void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	ctl_table fake_table;
-	unsigned char buf[64], tmp_uuid[16], *uuid;
+	unsigned char buf[37], tmp_uuid[16], *uuid;
 
 	uuid = table->data;
 	if (!uuid) {
@@ -1199,12 +1224,7 @@ static int proc_do_uuid(ctl_table *table
 	if (uuid[8] == 0)
 		generate_random_uuid(uuid);
 
-	sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-"
-		"%02x%02x%02x%02x%02x%02x",
-		uuid[0],  uuid[1],  uuid[2],  uuid[3],
-		uuid[4],  uuid[5],  uuid[6],  uuid[7],
-		uuid[8],  uuid[9],  uuid[10], uuid[11],
-		uuid[12], uuid[13], uuid[14], uuid[15]);
+	snprintf_uuid(buf, sizeof(buf), uuid);
 	fake_table.data = buf;
 	fake_table.maxlen = sizeof(buf);
 
--- linux-2.6.24-rc5/include/linux/random.h.org	2007-12-18 12:22:49.000000000 -0800
+++ linux-2.6.24-rc5/include/linux/random.h	2007-12-18 12:22:57.000000000 -0800
@@ -71,6 +71,7 @@ unsigned long randomize_range(unsigned l
 
 u32 random32(void);
 void srandom32(u32 seed);
+const char *snprintf_uuid(char *string, int len, unsigned char *uuid);
 
 #endif /* __KERNEL___ */
 
--- linux-2.6.24-rc5/kernel/panic.c.org	2007-12-18 12:23:19.000000000 -0800
+++ linux-2.6.24-rc5/kernel/panic.c	2007-12-18 12:35:46.000000000 -0800
@@ -19,6 +19,7 @@
 #include <linux/nmi.h>
 #include <linux/kexec.h>
 #include <linux/debug_locks.h>
+#include <linux/random.h>
 
 int panic_on_oops;
 int tainted;
@@ -32,6 +33,8 @@ ATOMIC_NOTIFIER_HEAD(panic_notifier_list
 
 EXPORT_SYMBOL(panic_notifier_list);
 
+static unsigned char oops_uuid[16];
+
 static int __init panic_setup(char *str)
 {
 	panic_timeout = simple_strtoul(str, NULL, 0);
@@ -265,15 +268,32 @@ void oops_enter(void)
 	do_oops_enter_exit();
 }
 
+static int prime_oops_uuid(void)
+{
+	if (oops_uuid[8] == 0)
+		generate_random_uuid(oops_uuid);
+	return 0;
+}
+
 /*
  * Called when the architecture exits its oops handler, after printing
  * everything.
  */
 void oops_exit(void)
 {
+	char uuid_string[37];
 	do_oops_enter_exit();
+
+	/*
+	 * normally the oops_uid is already calculated, but if we oops during
+	 * really early boot, it may not be. In that case, calculate it here.
+	 */
+	prime_oops_uuid();
+	printk("---[ end trace %s ]---\n",
+		snprintf_uuid(uuid_string, sizeof(uuid_string), oops_uuid));
 }
 
+late_initcall(prime_oops_uuid);
 #ifdef CONFIG_CC_STACKPROTECTOR
 /*
  * Called when gcc's -fstack-protector feature is used, and