[PATCH] kmsg_dump: Don't run on non-error paths by default

[PATCH] kmsg_dump: Don't run on non-error paths by default

[Matthew Garrett]

Since 04c6862c055fb687c90d9652f32c11a063df15cf kmsg_dump() gets run on
normal paths including poweroff and reboot. This is less than ideal given
pstore implementations that can only represent single backtraces, since a
reboot may overwrite a stored oops before it's been picked up by userspace.
In addition, some pstore backends may have low performance and provide a
significant delay in reboot as a result.

This patch adds a printk.always_kmsg_dump kernel parameter (which can also
be changed from userspace). Without it, the code will only be run on
failure paths rather than on normal paths. The option can be enabled in
environments where there's a desire to attempt to audit whether or not
a reboot was cleanly requested or not.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Acked-by: Seiji Aguchi <seiji.aguchi@hds.com>
---
 Documentation/kernel-parameters.txt |    6 ++++++
 include/linux/kmsg_dump.h           |    9 +++++++--
 kernel/printk.c                     |    6 ++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 033d4e6..d99fd9c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2211,6 +2211,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 			default: off.
 
+	printk.always_kmsg_dump=
+			Trigger kmsg_dump for cases other than kernel oops or
+			panics
+			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
+			default: disabled
+
 	printk.time=	Show timing data prefixed to each printk message line
 			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
 
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index fee6631..35f7237 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -15,13 +15,18 @@
 #include <linux/errno.h>
 #include <linux/list.h>
 
+/*
+ * Keep this list arranged in rough order of priority. Anything listed after
+ * KMSG_DUMP_OOPS will not be logged by default unless printk.always_kmsg_dump
+ * is passed to the kernel.
+ */
 enum kmsg_dump_reason {
-	KMSG_DUMP_OOPS,
 	KMSG_DUMP_PANIC,
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_EMERG,
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
-	KMSG_DUMP_EMERG,
 };
 
 /**
diff --git a/kernel/printk.c b/kernel/printk.c
index 13c0a11..e7d07f5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -702,6 +702,9 @@ static bool printk_time = 0;
 #endif
 module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
 
+static bool always_kmsg_dump;
+module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
+
 /* Check if we have any console registered that can be called early in boot. */
 static int have_callable_console(void)
 {
@@ -1732,6 +1735,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	unsigned long l1, l2;
 	unsigned long flags;
 
+	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
+		return;
+
 	/* Theoretically, the log could move on after we do this, but
 	   there's not a lot we can do about that. The new messages
 	   will overwrite the start of what we dump. */
-- 
1.7.7.6

Re: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Jack Stone]

On 10/02/12 15:11, Matthew Garrett wrote:
> +/*
> + * Keep this list arranged in rough order of priority. Anything listed after
> + * KMSG_DUMP_OOPS will not be logged by default unless printk.always_kmsg_dump
> + * is passed to the kernel.
> + */
>  enum kmsg_dump_reason {
> -	KMSG_DUMP_OOPS,
>  	KMSG_DUMP_PANIC,
> +	KMSG_DUMP_OOPS,
> +	KMSG_DUMP_EMERG,
>  	KMSG_DUMP_RESTART,
>  	KMSG_DUMP_HALT,
>  	KMSG_DUMP_POWEROFF,
> -	KMSG_DUMP_EMERG,
>  };

...

> +	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
> +		return;
> +

Did you mean KMSG_DUMP_OOPS here?

Re: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Matthew Garrett]

Sigh. Is there any way to get git send-email to warn when you have a 
dirty tree?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Vivek Goyal]

On Fri, Feb 10, 2012 at 10:11:33AM -0500, Matthew Garrett wrote:

[..]
>  static int have_callable_console(void)
>  {
> @@ -1732,6 +1735,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
>  	unsigned long l1, l2;
>  	unsigned long flags;
>  
> +	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
> +		return;
> +

reason > KMSG_DUMP_OOPS?

Thanks
Vivek

[PATCH] kmsg_dump: Don't run on non-error paths by default

[Matthew Garrett]

Since 04c6862c055fb687c90d9652f32c11a063df15cf kmsg_dump() gets run on
normal paths including poweroff and reboot. This is less than ideal given
pstore implementations that can only represent single backtraces, since a
reboot may overwrite a stored oops before it's been picked up by userspace.
In addition, some pstore backends may have low performance and provide a
significant delay in reboot as a result.

This patch adds a printk.always_kmsg_dump kernel parameter (which can also
be changed from userspace). Without it, the code will only be run on
failure paths rather than on normal paths. The option can be enabled in
environments where there's a desire to attempt to audit whether or not
a reboot was cleanly requested or not.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 include/linux/kmsg_dump.h |    9 +++++++--
 kernel/printk.c           |    6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index fee6631..35f7237 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -15,13 +15,18 @@
 #include <linux/errno.h>
 #include <linux/list.h>
 
+/*
+ * Keep this list arranged in rough order of priority. Anything listed after
+ * KMSG_DUMP_OOPS will not be logged by default unless printk.always_kmsg_dump
+ * is passed to the kernel.
+ */
 enum kmsg_dump_reason {
-	KMSG_DUMP_OOPS,
 	KMSG_DUMP_PANIC,
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_EMERG,
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
-	KMSG_DUMP_EMERG,
 };
 
 /**
diff --git a/kernel/printk.c b/kernel/printk.c
index 13c0a11..e7d07f5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -702,6 +702,9 @@ static bool printk_time = 0;
 #endif
 module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
 
+static bool always_kmsg_dump;
+module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
+
 /* Check if we have any console registered that can be called early in boot. */
 static int have_callable_console(void)
 {
@@ -1732,6 +1735,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	unsigned long l1, l2;
 	unsigned long flags;
 
+	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
+		return;
+
 	/* Theoretically, the log could move on after we do this, but
 	   there's not a lot we can do about that. The new messages
 	   will overwrite the start of what we dump. */
-- 
1.7.7.6

Re: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Vivek Goyal]

On Wed, Feb 08, 2012 at 11:14:06AM -0500, Matthew Garrett wrote:

[..]
> +/*
> + * Keep this list arranged in rough order of priority. Anything listed after
> + * KMSG_DUMP_OOPS will not be logged by default unless printk.always_kmsg_dump
> + * is passed to the kernel.
> + */
>  enum kmsg_dump_reason {
> -	KMSG_DUMP_OOPS,
>  	KMSG_DUMP_PANIC,
> +	KMSG_DUMP_OOPS,
> +	KMSG_DUMP_EMERG,
>  	KMSG_DUMP_RESTART,
>  	KMSG_DUMP_HALT,
>  	KMSG_DUMP_POWEROFF,
> -	KMSG_DUMP_EMERG,
>  };
>  
>  /**
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 13c0a11..e7d07f5 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -702,6 +702,9 @@ static bool printk_time = 0;
>  #endif
>  module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
>  
> +static bool always_kmsg_dump;
> +module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | S_IWUSR);
> +
>  /* Check if we have any console registered that can be called early in boot. */
>  static int have_callable_console(void)
>  {
> @@ -1732,6 +1735,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
>  	unsigned long l1, l2;
>  	unsigned long flags;
>  
> +	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
> +		return;

Did you mean reason > KMSG_DUMP_OOPS to enable oops logging by default?
 
Given the fact that not everybody likes kmsg_dump() and it is not known
how stable it with various backends, will it make sense to keep it disabled
by default and provide a knob to enable it (instead of always_kmsg_dump).

Thanks
Vivek

Re: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Matthew Garrett]

On Wed, Feb 08, 2012 at 11:30:07AM -0500, Vivek Goyal wrote:

> > +	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
> > +		return;
> 
> Did you mean reason > KMSG_DUMP_OOPS to enable oops logging by default?

Bah. Yes, I think my test system was set to panic on oops so I didn't 
catch that.

> Given the fact that not everybody likes kmsg_dump() and it is not known
> how stable it with various backends, will it make sense to keep it disabled
> by default and provide a knob to enable it (instead of always_kmsg_dump).

I think logging panics and oopses by default is a useful feature. It may 
be desirable to have a mechanism to disable it if some other way of 
handling that (serial console, kdump) is available.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Vivek Goyal]

On Wed, Feb 08, 2012 at 04:37:20PM +0000, Matthew Garrett wrote:
> On Wed, Feb 08, 2012 at 11:30:07AM -0500, Vivek Goyal wrote:
> 
> > > +	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
> > > +		return;
> > 
> > Did you mean reason > KMSG_DUMP_OOPS to enable oops logging by default?
> 
> Bah. Yes, I think my test system was set to panic on oops so I didn't 
> catch that.
> 
> > Given the fact that not everybody likes kmsg_dump() and it is not known
> > how stable it with various backends, will it make sense to keep it disabled
> > by default and provide a knob to enable it (instead of always_kmsg_dump).
> 
> I think logging panics and oopses by default is a useful feature. It may 
> be desirable to have a mechanism to disable it if some other way of 
> handling that (serial console, kdump) is available.

Ok, keeping it enabled by default for panic and oops might be better.

Thinking more about it. This problem sounds similar to different kernel log
levels where we enable desired level of logging. May be instead of having
a boolean knob, we can have multivalue knob.

Thanks
Vivek

RE: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Seiji Aguchi]

This patch, introducing tunable parameter, is reasonable to me.

Acked-by: Seiji Aguchi <seiji.aguchi.hds.com>

-----Original Message-----
From: Matthew Garrett [mailto:mjg@redhat.com] 
Sent: Wednesday, February 08, 2012 11:14 AM
To: linux-kernel@vger.kernel.org
Cc: Seiji Aguchi; dzickus@redhat.com; vgoyal@redhat.com; Matthew Garrett
Subject: [PATCH] kmsg_dump: Don't run on non-error paths by default

Since 04c6862c055fb687c90d9652f32c11a063df15cf kmsg_dump() gets run on normal paths including poweroff and reboot. This is less than ideal given pstore implementations that can only represent single backtraces, since a reboot may overwrite a stored oops before it's been picked up by userspace.
In addition, some pstore backends may have low performance and provide a significant delay in reboot as a result.

This patch adds a printk.always_kmsg_dump kernel parameter (which can also be changed from userspace). Without it, the code will only be run on failure paths rather than on normal paths. The option can be enabled in environments where there's a desire to attempt to audit whether or not a reboot was cleanly requested or not.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 include/linux/kmsg_dump.h |    9 +++++++--
 kernel/printk.c           |    6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h index fee6631..35f7237 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -15,13 +15,18 @@
 #include <linux/errno.h>
 #include <linux/list.h>
 
+/*
+ * Keep this list arranged in rough order of priority. Anything listed 
+after
+ * KMSG_DUMP_OOPS will not be logged by default unless 
+printk.always_kmsg_dump
+ * is passed to the kernel.
+ */
 enum kmsg_dump_reason {
-	KMSG_DUMP_OOPS,
 	KMSG_DUMP_PANIC,
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_EMERG,
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
-	KMSG_DUMP_EMERG,
 };
 
 /**
diff --git a/kernel/printk.c b/kernel/printk.c index 13c0a11..e7d07f5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -702,6 +702,9 @@ static bool printk_time = 0;  #endif  module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
 
+static bool always_kmsg_dump;
+module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | 
+S_IWUSR);
+
 /* Check if we have any console registered that can be called early in boot. */  static int have_callable_console(void)  { @@ -1732,6 +1735,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	unsigned long l1, l2;
 	unsigned long flags;
 
+	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
+		return;
+
 	/* Theoretically, the log could move on after we do this, but
 	   there's not a lot we can do about that. The new messages
 	   will overwrite the start of what we dump. */
--
1.7.7.6

RE: [PATCH] kmsg_dump: Don't run on non-error paths by default

[Seiji Aguchi]

There was a mistake in writing for my e-mail address.

Acked-by: Seiji Aguchi <seiji.aguchi@hds.com>


-----Original Message-----
From: Seiji Aguchi 
Sent: Wednesday, February 08, 2012 3:24 PM
To: 'Matthew Garrett'; linux-kernel@vger.kernel.org
Cc: dzickus@redhat.com; vgoyal@redhat.com
Subject: RE: [PATCH] kmsg_dump: Don't run on non-error paths by default

This patch, introducing tunable parameter, is reasonable to me.

Acked-by: Seiji Aguchi <seiji.aguchi.hds.com>

-----Original Message-----
From: Matthew Garrett [mailto:mjg@redhat.com]
Sent: Wednesday, February 08, 2012 11:14 AM
To: linux-kernel@vger.kernel.org
Cc: Seiji Aguchi; dzickus@redhat.com; vgoyal@redhat.com; Matthew Garrett
Subject: [PATCH] kmsg_dump: Don't run on non-error paths by default

Since 04c6862c055fb687c90d9652f32c11a063df15cf kmsg_dump() gets run on normal paths including poweroff and reboot. This is less than ideal given pstore implementations that can only represent single backtraces, since a reboot may overwrite a stored oops before it's been picked up by userspace.
In addition, some pstore backends may have low performance and provide a significant delay in reboot as a result.

This patch adds a printk.always_kmsg_dump kernel parameter (which can also be changed from userspace). Without it, the code will only be run on failure paths rather than on normal paths. The option can be enabled in environments where there's a desire to attempt to audit whether or not a reboot was cleanly requested or not.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 include/linux/kmsg_dump.h |    9 +++++++--
 kernel/printk.c           |    6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h index fee6631..35f7237 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -15,13 +15,18 @@
 #include <linux/errno.h>
 #include <linux/list.h>
 
+/*
+ * Keep this list arranged in rough order of priority. Anything listed 
+after
+ * KMSG_DUMP_OOPS will not be logged by default unless 
+printk.always_kmsg_dump
+ * is passed to the kernel.
+ */
 enum kmsg_dump_reason {
-	KMSG_DUMP_OOPS,
 	KMSG_DUMP_PANIC,
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_EMERG,
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
-	KMSG_DUMP_EMERG,
 };
 
 /**
diff --git a/kernel/printk.c b/kernel/printk.c index 13c0a11..e7d07f5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -702,6 +702,9 @@ static bool printk_time = 0;  #endif  module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
 
+static bool always_kmsg_dump;
+module_param_named(always_kmsg_dump, always_kmsg_dump, bool, S_IRUGO | 
+S_IWUSR);
+
 /* Check if we have any console registered that can be called early in boot. */  static int have_callable_console(void)  { @@ -1732,6 +1735,9 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	unsigned long l1, l2;
 	unsigned long flags;
 
+	if (reason > KMSG_DUMP_PANIC && !always_kmsg_dump)
+		return;
+
 	/* Theoretically, the log could move on after we do this, but
 	   there's not a lot we can do about that. The new messages
 	   will overwrite the start of what we dump. */
--
1.7.7.6