[PATCH v4 0/16] Merge ram_console into pstore, and more

[PATCH v4 0/16] Merge ram_console into pstore, and more

[Anton Vorontsov]

Hi all,

A brand new v4:

- Per Kees Cook's comments, the patches no longer remove an automatic
  updates feature, but instead make the it configurable; plus disable
  it by default (in a separate patch);
- Fixed some bugs noticed by Colin Cross;
- Documented new continuous ramoops-console log behaviour (also
  noticed by Colin Cross).

In v3:

- Rebased on top of current staging-next;
- The series are getting bigger. This is partly because we now support
  different persistent zone sizes for oops records and console log,
  per Colin Cross' request.
  And I believe the code is now more manageable for further enhancements
  (e.g. if we'd want to add other message types, e.g. tracing);
- Addressed Kees Cook's comments on the unlinking matters;
- Removed automatic updates support. Please see the last patch
  description for rationale;
- A new fixup for pstore/inode, just getting rid of a sparse warning.

In v2:

- Updated documentation per Colin Cross' comments;
- Corrected return value in ramoops_pstore_write() (noticed by Kees Cook);
- Fixed large writes handling in pstore_console_write(), i.e. when
  log_buf write is larger than pstore bufsize. Also Noticed by Kees Cook.


And a boilerplate for the series:

Currently pstore doesn't support logging kernel messages in run-time,
it only dumps dmesg when kernel oopses/panics. This makes pstore
useless for debugging hangs caused by HW issues or improper use of HW
(e.g. weird device inserted -> driver tried to write reserved bits ->
SoC hanged. In that case we don't get any messages in the pstore.

This series add a new message type for pstore, i.e. PSTORE_TYPE_CONSOLE,
plus make pstore/ram.c handle the new messages.

The old ram_console driver is removed. This might probably cause
some pain for out-of-tree code, as it would need to be adjusted...
but "no pain, no gain"? :-) Though, if there's some serious resistance,
we can probably postpone the last two patches.

Thanks!

--- 
 Documentation/ramoops.txt             |   14 ++
 drivers/staging/android/Kconfig       |    5 -
 drivers/staging/android/Makefile      |    1 -
 drivers/staging/android/ram_console.c |  179 --------------------------
 fs/pstore/Kconfig                     |    7 +
 fs/pstore/inode.c                     |    5 +-
 fs/pstore/platform.c                  |   49 ++++++-
 fs/pstore/ram.c                       |  228 ++++++++++++++++++++++++++-------
 fs/pstore/ram_core.c                  |  108 +++-------------
 include/linux/pstore.h                |    1 +
 include/linux/pstore_ram.h            |   22 +---
 11 files changed, 272 insertions(+), 347 deletions(-)

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[PATCH 01/16] pstore/inode: Make pstore_fill_super() static

[Anton Vorontsov]

There's no reason to extern it. The patch fixes the annoying sparse
warning:

CHECK   fs/pstore/inode.c
fs/pstore/inode.c:264:5: warning: symbol 'pstore_fill_super' was not
declared. Should it be static?

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1950788..49b40ea 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -258,7 +258,7 @@ fail:
 	return rc;
 }
 
-int pstore_fill_super(struct super_block *sb, void *data, int silent)
+static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct inode *inode;
 
-- 
1.7.9.2

Re: [PATCH 01/16] pstore/inode: Make pstore_fill_super() static

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> There's no reason to extern it. The patch fixes the annoying sparse
> warning:
>
> CHECK   fs/pstore/inode.c
> fs/pstore/inode.c:264:5: warning: symbol 'pstore_fill_super' was not
> declared. Should it be static?
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 02/16] pstore/ram: Should update old dmesg buffer before reading

[Anton Vorontsov]

Without the update, we'll only see the new dmesg buffer after the
reboot, but previously we could see it right away. Making an oops
visible in pstore filesystem before reboot is a somewhat dubious
feature, but removing it wasn't an intentional change, so let's
restore it.

For this we have to make persistent_ram_save_old() safe for calling
multiple times, and also extern it.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c            |    2 ++
 fs/pstore/ram_core.c       |   15 ++++++++-------
 include/linux/pstore_ram.h |    1 +
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 9123cce..16ff733 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -106,6 +106,8 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 	time->tv_sec = 0;
 	time->tv_nsec = 0;
 
+	/* Update old/shadowed buffer. */
+	persistent_ram_save_old(prz);
 	size = persistent_ram_old_size(prz);
 	*buf = kmalloc(size, GFP_KERNEL);
 	if (*buf == NULL)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 31f8d18..235513c 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -250,23 +250,24 @@ static void notrace persistent_ram_update(struct persistent_ram_zone *prz,
 	persistent_ram_update_ecc(prz, start, count);
 }
 
-static void __init
-persistent_ram_save_old(struct persistent_ram_zone *prz)
+void persistent_ram_save_old(struct persistent_ram_zone *prz)
 {
 	struct persistent_ram_buffer *buffer = prz->buffer;
 	size_t size = buffer_size(prz);
 	size_t start = buffer_start(prz);
-	char *dest;
 
-	persistent_ram_ecc_old(prz);
+	if (!size)
+		return;
 
-	dest = kmalloc(size, GFP_KERNEL);
-	if (dest == NULL) {
+	if (!prz->old_log) {
+		persistent_ram_ecc_old(prz);
+		prz->old_log = kmalloc(size, GFP_KERNEL);
+	}
+	if (!prz->old_log) {
 		pr_err("persistent_ram: failed to allocate buffer\n");
 		return;
 	}
 
-	prz->old_log = dest;
 	prz->old_log_size = size;
 	memcpy(prz->old_log, &buffer->data[start], size - start);
 	memcpy(prz->old_log + size - start, &buffer->data[0], start);
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 7ed7fd4..4491e8f 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -75,6 +75,7 @@ struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev,
 int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
 	unsigned int count);
 
+void persistent_ram_save_old(struct persistent_ram_zone *prz);
 size_t persistent_ram_old_size(struct persistent_ram_zone *prz);
 void *persistent_ram_old(struct persistent_ram_zone *prz);
 void persistent_ram_free_old(struct persistent_ram_zone *prz);
-- 
1.7.9.2

Re: [PATCH 02/16] pstore/ram: Should update old dmesg buffer before reading

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> Without the update, we'll only see the new dmesg buffer after the
> reboot, but previously we could see it right away. Making an oops
> visible in pstore filesystem before reboot is a somewhat dubious
> feature, but removing it wasn't an intentional change, so let's
> restore it.
>
> For this we have to make persistent_ram_save_old() safe for calling
> multiple times, and also extern it.
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 03/16] pstore/ram_core: Do not reset restored zone's position and size

[Anton Vorontsov]

Otherwise, the files will survive just one reboot, and on a subsequent
boot they will disappear.

Also, as noticed by Colin Cross, this also causes an interesting behavior
change in the console logging. Before this change, the console log would
show only the messages from the last reboot.  After this change, the
console log will have logs from multiple boots appended to each other.

Now to get the only most recent messages we can do:

	tac ramoops-console | sed '/^Linux version.*(.*@.*)/ q' | tac

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram_core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 235513c..f6650d1 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -406,6 +406,7 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool
 				" size %zu, start %zu\n",
 			       buffer_size(prz), buffer_start(prz));
 			persistent_ram_save_old(prz);
+			return 0;
 		}
 	} else {
 		pr_info("persistent_ram: no valid data in buffer"
-- 
1.7.9.2

Re: [PATCH 03/16] pstore/ram_core: Do not reset restored zone's position and size

[Colin Cross]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> Otherwise, the files will survive just one reboot, and on a subsequent
> boot they will disappear.
>
> Also, as noticed by Colin Cross, this also causes an interesting behavior
> change in the console logging. Before this change, the console log would
> show only the messages from the last reboot.  After this change, the
> console log will have logs from multiple boots appended to each other.
>
> Now to get the only most recent messages we can do:
>
>        tac ramoops-console | sed '/^Linux version.*(.*@.*)/ q' | tac

Lots of problems with this.
"Linux version ..." is not the first line in the console log on my
devices, there are messages before it that shouldn't be dropped by
automated logs collectors using this regexp.
There is a timestamp before "Linux version", so the regexp never matches.
There is often no newline at the end of the old log, so if "Linux
version" was the first line in the log, it would still not get
matched.

Relying on the first line in the log to not change seems likely to
cause problems for scripts in the future.  Why not separate them where
the code knows for sure that the old log is ending and the new log is
starting?

> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
> ---
>  fs/pstore/ram_core.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 235513c..f6650d1 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -406,6 +406,7 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool
>                                " size %zu, start %zu\n",
>                               buffer_size(prz), buffer_start(prz));
>                        persistent_ram_save_old(prz);
> +                       return 0;
>                }
>        } else {
>                pr_info("persistent_ram: no valid data in buffer"
> --
> 1.7.9.2
>

[PATCH/RFC] Make sure linux_banner is the first message in log_buf

[Anton Vorontsov]

For scripting it is important to have a consistent header that tells
where the kernel log starts. At least to me it always seemed that
linux_banner served this purpose.

Though, an early kernel code may print things before the Linux banner.
When we parse logs from multiple boots grabbed from serial or pstore
consoles, we might miss these messages.

The only bulletproof, arch-independent and whatnot way to ensure that
we have the banner printed first is to print it from the printk itself.
The resulting code looks quite obvious.

Sure, this doesn't address bootloader or low-level very early arch-
dependent console output that goes to the HW directly, but there's
nothing we can do about it (in pstore console we don't see them anyway).

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---

On Tue, May 22, 2012 at 11:06:29AM -0700, Colin Cross wrote:
> >
> > Now to get the only most recent messages we can do:
> >
> >        tac ramoops-console | sed '/^Linux version.*(.*@.*)/ q' | tac
> 
> Lots of problems with this.
> "Linux version ..." is not the first line in the console log on my
> devices, there are messages before it that shouldn't be dropped by
> automated logs collectors using this regexp.

Ouch. This seems not right. And the same issue exist if we collect
logs from e.g. serial console, there (unlike to just reading /proc/kmsg)
we don't know where current kernel's messages start, and more than
that, in serial console case we don't know where the messages end
(unlike to pstore).

So it's a general problem, not only related to pstore.

But we can fix this. How about the patch below?

> There is a timestamp before "Linux version", so the regexp never matches.

This is also fixable.

tac ramoops-console | sed '/\(^\|] \)Linux version.*(.*@.*)/ q' | tac

> There is often no newline at the end of the old log, so if "Linux
> version" was the first line in the log, it would still not get
> matched.

Yeah, true. We will have to add at least a new line in pstore.

> Relying on the first line in the log to not change seems likely to
> cause problems for scripts in the future.  Why not separate them where
> the code knows for sure that the old log is ending and the new log is
> starting?

I'm not opposed to a header (or a footer) at all, and you're right,
one of them is obviously needed.

I just don't like introducing yet another ad-hoc header format that
we will only use for pstore/console. We have "start of the Linux
kernel log" header, so let's try to use it?..

If it is not suitable for us today, let's think how to fix that, and
if anything, we can fall-back to your plan, i.e. adding our own footer
(or zapping prz) at boot time.

But so far I can see this solution:

1. Make sure we printk linux_banner as the first string in the
   log_buf (this patch). This is an improvement not only for pstore;

2. In pstore, add a newline to previous' boot log (we'll add it
   unconditinally, so that it we won't loose the information about
   the last new line :-).

 init/main.c     |    1 -
 kernel/printk.c |    3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/init/main.c b/init/main.c
index 44b2433..df3711d 100644
--- a/init/main.c
+++ b/init/main.c
@@ -492,7 +492,6 @@ asmlinkage void __init start_kernel(void)
 	tick_init();
 	boot_cpu_init();
 	page_address_init();
-	printk(KERN_NOTICE "%s", linux_banner);
 	setup_arch(&command_line);
 	mm_init_owner(&init_mm, &init_task);
 	mm_init_cpumask(&init_mm);
diff --git a/kernel/printk.c b/kernel/printk.c
index b663c2c..1e1461b 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -749,6 +749,9 @@ asmlinkage int printk(const char *fmt, ...)
 	va_list args;
 	int r;
 
+	/* Make sure linux_banner is kernel's first message. */
+	printk_once(KERN_NOTICE "%s", linux_banner);
+
 #ifdef CONFIG_KGDB_KDB
 	if (unlikely(kdb_trap_printk)) {
 		va_start(args, fmt);
-- 
1.7.9.2

Re: [PATCH/RFC] Make sure linux_banner is the first message in log_buf

[Linus Torvalds]

On Tue, May 22, 2012 at 7:35 PM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> @@ -749,6 +749,9 @@ asmlinkage int printk(const char *fmt, ...)
>        va_list args;
>        int r;
>
> +       /* Make sure linux_banner is kernel's first message. */
> +       printk_once(KERN_NOTICE "%s", linux_banner);
> +

Ugh. No. That is too disgusting for words.

                  Linus

Re: [PATCH/RFC] Make sure linux_banner is the first message in log_buf

[David Miller]

From: Anton Vorontsov <anton.vorontsov@linaro.org>
Date: Tue, 22 May 2012 19:35:16 -0700

> For scripting it is important to have a consistent header that tells
> where the kernel log starts. At least to me it always seemed that
> linux_banner served this purpose.

Your change will not achieve this goal, many architectures
print things long before control passes to init/main.c

Re: [PATCH/RFC] Make sure linux_banner is the first message in log_buf

[Anton Vorontsov]

On Tue, May 22, 2012 at 10:55:12PM -0400, David Miller wrote:
> From: Anton Vorontsov <anton.vorontsov@linaro.org>
> Date: Tue, 22 May 2012 19:35:16 -0700
> 
> > For scripting it is important to have a consistent header that tells
> > where the kernel log starts. At least to me it always seemed that
> > linux_banner served this purpose.
> 
> Your change will not achieve this goal, many architectures
> print things long before control passes to init/main.c

If they use printk, we'll catch these messages, no? It is true that
if they just write to the HW directly, the messages are lost for us,
and there's nothing we can do about it (and we don't see these
messages from the kernel anyway, neither in dmesg, nor in pstore).

So, for serial (or arch-specific) console it is still cumbersome to
make proper parsing of all messages, but for pstore/ramconsole it is
OK. So, I dunno. I guess I'll have to add a custom header to pstore,
heh.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[PATCH 04/16] pstore/ram_core: Factor persistent_ram_zap() out of post_init()

[Anton Vorontsov]

A handy function that we will use outside of ram_core soon. But
so far just factor it out and start using it in post_init().

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram_core.c       |   11 ++++++++---
 include/linux/pstore_ram.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index f6650d1..c5fbdbb 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -320,6 +320,13 @@ void persistent_ram_free_old(struct persistent_ram_zone *prz)
 	prz->old_log_size = 0;
 }
 
+void persistent_ram_zap(struct persistent_ram_zone *prz)
+{
+	atomic_set(&prz->buffer->start, 0);
+	atomic_set(&prz->buffer->size, 0);
+	persistent_ram_update_header_ecc(prz);
+}
+
 static void *persistent_ram_vmap(phys_addr_t start, size_t size)
 {
 	struct page **pages;
@@ -414,8 +421,7 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool
 	}
 
 	prz->buffer->sig = PERSISTENT_RAM_SIG;
-	atomic_set(&prz->buffer->start, 0);
-	atomic_set(&prz->buffer->size, 0);
+	persistent_ram_zap(prz);
 
 	return 0;
 }
@@ -450,7 +456,6 @@ struct persistent_ram_zone * __init persistent_ram_new(phys_addr_t start,
 		goto err;
 
 	persistent_ram_post_init(prz, ecc);
-	persistent_ram_update_header_ecc(prz);
 
 	return prz;
 err:
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 4491e8f..3b823d4 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -69,6 +69,7 @@ struct persistent_ram_zone * __init persistent_ram_new(phys_addr_t start,
 						       size_t size,
 						       bool ecc);
 void persistent_ram_free(struct persistent_ram_zone *prz);
+void persistent_ram_zap(struct persistent_ram_zone *prz);
 struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev,
 		bool ecc);
 
-- 
1.7.9.2

Re: [PATCH 04/16] pstore/ram_core: Factor persistent_ram_zap() out of post_init()

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> A handy function that we will use outside of ram_core soon. But
> so far just factor it out and start using it in post_init().
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 05/16] pstore/ram: Should zap persistent zone on unlink

[Anton Vorontsov]

Otherwise, unlinked file will reappear on the next boot.

Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 16ff733..453030f 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -186,6 +186,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
 		return -EINVAL;
 
 	persistent_ram_free_old(cxt->przs[id]);
+	persistent_ram_zap(cxt->przs[id]);
 
 	return 0;
 }
-- 
1.7.9.2

Re: [PATCH 05/16] pstore/ram: Should zap persistent zone on unlink

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> Otherwise, unlinked file will reappear on the next boot.
>
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 06/16] pstore: Add console log messages support

[Anton Vorontsov]

Pstore doesn't support logging kernel messages in run-time, it only
dumps dmesg when kernel oopses/panics. This makes pstore useless for
debugging hangs caused by HW issues or improper use of HW (e.g.
weird device inserted -> driver tried to write a reserved bits ->
SoC hanged. In that case we don't get any messages in the pstore.

Therefore, let's add a runtime logging support: PSTORE_TYPE_CONSOLE.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Colin Cross <ccross@android.com>
---
 fs/pstore/Kconfig      |    7 +++++++
 fs/pstore/inode.c      |    3 +++
 fs/pstore/platform.c   |   33 +++++++++++++++++++++++++++++++++
 include/linux/pstore.h |    1 +
 4 files changed, 44 insertions(+)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 23ade26..d044de6 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -12,6 +12,13 @@ config PSTORE
 	   If you don't have a platform persistent store driver,
 	   say N.
 
+config PSTORE_CONSOLE
+	bool "Log kernel console messages"
+	depends on PSTORE
+	help
+	  When the option is enabled, pstore will log all kernel
+	  messages, even if no oops or panic happened.
+
 config PSTORE_RAM
 	tristate "Log panic/oops to a RAM buffer"
 	depends on PSTORE
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 49b40ea..fda1331 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -212,6 +212,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 	case PSTORE_TYPE_DMESG:
 		sprintf(name, "dmesg-%s-%lld", psname, id);
 		break;
+	case PSTORE_TYPE_CONSOLE:
+		sprintf(name, "console-%s", psname);
+		break;
 	case PSTORE_TYPE_MCE:
 		sprintf(name, "mce-%s-%lld", psname, id);
 		break;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 82c585f..a3f6d96 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -1,6 +1,7 @@
 /*
  * Persistent Storage - platform driver interface parts.
  *
+ * Copyright (C) 2007-2008 Google, Inc.
  * Copyright (C) 2010 Intel Corporation <tony.luck@intel.com>
  *
  *  This program is free software; you can redistribute it and/or modify
@@ -22,6 +23,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/kmsg_dump.h>
+#include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pstore.h>
 #include <linux/string.h>
@@ -156,6 +158,36 @@ static struct kmsg_dumper pstore_dumper = {
 	.dump = pstore_dump,
 };
 
+#ifdef CONFIG_PSTORE_CONSOLE
+static void pstore_console_write(struct console *con, const char *s, unsigned c)
+{
+	const char *e = s + c;
+
+	while (s < e) {
+		if (c > psinfo->bufsize)
+			c = psinfo->bufsize;
+		memcpy(psinfo->buf, s, c);
+		psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);
+		s += c;
+		c = e - s;
+	}
+}
+
+static struct console pstore_console = {
+	.name	= "pstore",
+	.write	= pstore_console_write,
+	.flags	= CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME,
+	.index	= -1,
+};
+
+static void pstore_register_console(void)
+{
+	register_console(&pstore_console);
+}
+#else
+static void pstore_register_console(void) {}
+#endif
+
 /*
  * platform specific persistent storage driver registers with
  * us here. If pstore is already mounted, call the platform
@@ -193,6 +225,7 @@ int pstore_register(struct pstore_info *psi)
 		pstore_get_records(0);
 
 	kmsg_dump_register(&pstore_dumper);
+	pstore_register_console();
 
 	pstore_timer.expires = jiffies + PSTORE_INTERVAL;
 	add_timer(&pstore_timer);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index e1461e1..1bd014b 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -29,6 +29,7 @@
 enum pstore_type_id {
 	PSTORE_TYPE_DMESG	= 0,
 	PSTORE_TYPE_MCE		= 1,
+	PSTORE_TYPE_CONSOLE	= 2,
 	PSTORE_TYPE_UNKNOWN	= 255
 };
 
-- 
1.7.9.2

[PATCH 07/16] pstore/ram: Introduce ramoops_context.max_dump_count

[Anton Vorontsov]

So far it is the same as max_count, but this will change when
we'll add support for other message types (e.g. console, mce,
tracing).

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 453030f..cdaeda9 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -70,6 +70,7 @@ struct ramoops_context {
 	bool ecc;
 	unsigned int count;
 	unsigned int max_count;
+	unsigned int max_dump_count;
 	unsigned int read_count;
 	struct pstore_info pstore;
 };
@@ -94,7 +95,7 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 	struct ramoops_context *cxt = psi->data;
 	struct persistent_ram_zone *prz;
 
-	if (cxt->read_count >= cxt->max_count)
+	if (cxt->read_count >= cxt->max_dump_count)
 		return -EINVAL;
 
 	*id = cxt->read_count++;
@@ -172,7 +173,7 @@ static int ramoops_pstore_write(enum pstore_type_id type,
 		size = prz->buffer_size - hlen;
 	persistent_ram_write(prz, cxt->pstore.buf, size);
 
-	cxt->count = (cxt->count + 1) % cxt->max_count;
+	cxt->count = (cxt->count + 1) % cxt->max_dump_count;
 
 	return 0;
 }
@@ -182,7 +183,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
 {
 	struct ramoops_context *cxt = psi->data;
 
-	if (id >= cxt->max_count)
+	if (id >= cxt->max_dump_count)
 		return -EINVAL;
 
 	persistent_ram_free_old(cxt->przs[id]);
@@ -213,7 +214,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	/* Only a single ramoops area allowed at a time, so fail extra
 	 * probes.
 	 */
-	if (cxt->max_count)
+	if (cxt->max_dump_count)
 		goto fail_out;
 
 	if (!pdata->mem_size || !pdata->record_size) {
@@ -240,6 +241,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	}
 
 	cxt->max_count = pdata->mem_size / pdata->record_size;
+	cxt->max_dump_count = cxt->max_count;
 	cxt->count = 0;
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
@@ -247,14 +249,14 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	cxt->dump_oops = pdata->dump_oops;
 	cxt->ecc = pdata->ecc;
 
-	cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_count, GFP_KERNEL);
+	cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_count, GFP_KERNEL);
 	if (!cxt->przs) {
 		err = -ENOMEM;
 		dev_err(dev, "failed to initialize a prz array\n");
 		goto fail_out;
 	}
 
-	for (i = 0; i < cxt->max_count; i++) {
+	for (i = 0; i < cxt->max_dump_count; i++) {
 		size_t sz = cxt->record_size;
 		phys_addr_t start = cxt->phys_addr + sz * i;
 
@@ -303,6 +305,7 @@ fail_buf:
 fail_clear:
 	cxt->pstore.bufsize = 0;
 	cxt->max_count = 0;
+	cxt->max_dump_count = 0;
 fail_przs:
 	for (i = 0; cxt->przs[i]; i++)
 		persistent_ram_free(cxt->przs[i]);
-- 
1.7.9.2

Re: [PATCH 07/16] pstore/ram: Introduce ramoops_context.max_dump_count

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> So far it is the same as max_count, but this will change when
> we'll add support for other message types (e.g. console, mce,
> tracing).
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 08/16] pstore/ram: Factor dmesg przs initialization out of probe()

[Anton Vorontsov]

This will help make code clearer when we'll add support for other
message types.

This also makes probe() much shorter and understandable, plus
makes mem/record size checking a bit easier.

Implementation detail: we now use a paddr pointer, this will
be used for allocating persistent ram zones for other message
types.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c |  105 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 39 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index cdaeda9..9785c84 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -203,13 +203,67 @@ static struct ramoops_context oops_cxt = {
 	},
 };
 
+static void ramoops_free_przs(struct ramoops_context *cxt)
+{
+	int i;
+
+	if (!cxt->przs)
+		return;
+
+	for (i = 0; cxt->przs[i]; i++)
+		persistent_ram_free(cxt->przs[i]);
+	kfree(cxt->przs);
+}
+
+static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
+			      phys_addr_t *paddr, size_t dump_mem_sz)
+{
+	int err = -ENOMEM;
+	int i;
+
+	if (!cxt->record_size)
+		return 0;
+
+	cxt->max_dump_count = dump_mem_sz / cxt->record_size;
+	if (!cxt->max_dump_count)
+		return -ENOMEM;
+
+	cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_count,
+			     GFP_KERNEL);
+	if (!cxt->przs) {
+		dev_err(dev, "failed to initialize a prz array for dumps\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < cxt->max_dump_count; i++) {
+		size_t sz = cxt->record_size;
+
+		cxt->przs[i] = persistent_ram_new(*paddr, sz, cxt->ecc);
+		if (IS_ERR(cxt->przs[i])) {
+			err = PTR_ERR(cxt->przs[i]);
+			dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
+				sz, (unsigned long long)*paddr, err);
+			goto fail_prz;
+		}
+		*paddr += sz;
+	}
+
+	cxt->max_count += cxt->max_dump_count;
+
+	return 0;
+fail_prz:
+	ramoops_free_przs(cxt);
+	return err;
+}
+
 static int __init ramoops_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct ramoops_platform_data *pdata = pdev->dev.platform_data;
 	struct ramoops_context *cxt = &oops_cxt;
+	size_t dump_mem_sz;
+	phys_addr_t paddr;
 	int err = -EINVAL;
-	int i;
 
 	/* Only a single ramoops area allowed at a time, so fail extra
 	 * probes.
@@ -226,22 +280,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
 	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
 
-	/* Check for the minimum memory size */
-	if (pdata->mem_size < MIN_MEM_SIZE &&
-			pdata->record_size < MIN_MEM_SIZE) {
-		pr_err("memory size too small, minimum is %lu\n",
-			MIN_MEM_SIZE);
-		goto fail_out;
-	}
-
-	if (pdata->mem_size < pdata->record_size) {
-		pr_err("The memory size must be larger than the "
-			"records size\n");
-		goto fail_out;
-	}
-
-	cxt->max_count = pdata->mem_size / pdata->record_size;
-	cxt->max_dump_count = cxt->max_count;
+	cxt->max_count = 0;
 	cxt->count = 0;
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
@@ -249,24 +288,14 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	cxt->dump_oops = pdata->dump_oops;
 	cxt->ecc = pdata->ecc;
 
-	cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_count, GFP_KERNEL);
-	if (!cxt->przs) {
-		err = -ENOMEM;
-		dev_err(dev, "failed to initialize a prz array\n");
-		goto fail_out;
-	}
-
-	for (i = 0; i < cxt->max_dump_count; i++) {
-		size_t sz = cxt->record_size;
-		phys_addr_t start = cxt->phys_addr + sz * i;
+	paddr = cxt->phys_addr;
 
-		cxt->przs[i] = persistent_ram_new(start, sz, cxt->ecc);
-		if (IS_ERR(cxt->przs[i])) {
-			err = PTR_ERR(cxt->przs[i]);
-			dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
-				sz, (unsigned long long)start, err);
-			goto fail_przs;
-		}
+	dump_mem_sz = cxt->size;
+	err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
+	if (err) {
+		pr_err("memory size too small, minimum is %lu\n",
+			cxt->record_size);
+		goto fail_count;
 	}
 
 	cxt->pstore.data = cxt;
@@ -279,7 +308,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	}
 
 	err = pstore_register(&cxt->pstore);
-	if (err) {
+	if (err || !cxt->max_count) {
 		pr_err("registering with pstore failed\n");
 		goto fail_buf;
 	}
@@ -306,10 +335,8 @@ fail_clear:
 	cxt->pstore.bufsize = 0;
 	cxt->max_count = 0;
 	cxt->max_dump_count = 0;
-fail_przs:
-	for (i = 0; cxt->przs[i]; i++)
-		persistent_ram_free(cxt->przs[i]);
-	kfree(cxt->przs);
+fail_count:
+	ramoops_free_przs(cxt);
 fail_out:
 	return err;
 }
-- 
1.7.9.2

Re: [PATCH 08/16] pstore/ram: Factor dmesg przs initialization out of probe()

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> This will help make code clearer when we'll add support for other
> message types.
>
> This also makes probe() much shorter and understandable, plus
> makes mem/record size checking a bit easier.
>
> Implementation detail: we now use a paddr pointer, this will
> be used for allocating persistent ram zones for other message
> types.
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 09/16] pstore/ram: Factor ramoops_get_dump_prz() out of ramoops_pstore_read()

[Anton Vorontsov]

This will help make code clearer when we'll add support for other
message types.

The patch also changes return value from -EINVAL to 0 in case of
end-of-records. The exact value doesn't matter for pstore (it should
be just <= 0), but 0 feels more correct.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 9785c84..6dc9e96 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -86,6 +86,24 @@ static int ramoops_pstore_open(struct pstore_info *psi)
 	return 0;
 }
 
+static struct persistent_ram_zone *
+ramoops_get_dump_prz(u64 id, enum pstore_type_id *type,
+		     struct ramoops_context *cxt)
+{
+	struct persistent_ram_zone *prz;
+
+	if (id > cxt->max_dump_count)
+		return NULL;
+
+	prz = cxt->przs[id];
+	if (!persistent_ram_old_size(prz))
+		return NULL;
+
+	*type = PSTORE_TYPE_DMESG;
+
+	return prz;
+}
+
 static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 				   struct timespec *time,
 				   char **buf,
@@ -95,14 +113,12 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 	struct ramoops_context *cxt = psi->data;
 	struct persistent_ram_zone *prz;
 
-	if (cxt->read_count >= cxt->max_dump_count)
-		return -EINVAL;
-
 	*id = cxt->read_count++;
-	prz = cxt->przs[*id];
 
-	/* Only supports dmesg output so far. */
-	*type = PSTORE_TYPE_DMESG;
+	prz = ramoops_get_dump_prz(*id, type, cxt);
+	if (!prz)
+		return 0;
+
 	/* TODO(kees): Bogus time for the moment. */
 	time->tv_sec = 0;
 	time->tv_nsec = 0;
-- 
1.7.9.2

Re: [PATCH 09/16] pstore/ram: Factor ramoops_get_dump_prz() out of ramoops_pstore_read()

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> This will help make code clearer when we'll add support for other
> message types.
>
> The patch also changes return value from -EINVAL to 0 in case of
> end-of-records. The exact value doesn't matter for pstore (it should
> be just <= 0), but 0 feels more correct.
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 10/16] pstore/ram: Add console messages handling

[Anton Vorontsov]

The console log size is configurable via ramoops.console_size
module option, and the log itself is available via
<pstore-mount>/console-ramoops file.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c            |  107 ++++++++++++++++++++++++++++++++++++++------
 include/linux/pstore_ram.h |    1 +
 2 files changed, 95 insertions(+), 13 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6dc9e96..4e1ba41 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -41,6 +41,10 @@ module_param(record_size, ulong, 0400);
 MODULE_PARM_DESC(record_size,
 		"size of each dump done on oops/panic");
 
+static ulong ramoops_console_size = MIN_MEM_SIZE;
+module_param_named(console_size, ramoops_console_size, ulong, 0400);
+MODULE_PARM_DESC(console_size, "size of kernel console log");
+
 static ulong mem_address;
 module_param(mem_address, ulong, 0400);
 MODULE_PARM_DESC(mem_address,
@@ -63,9 +67,11 @@ MODULE_PARM_DESC(ramoops_ecc,
 
 struct ramoops_context {
 	struct persistent_ram_zone **przs;
+	struct persistent_ram_zone *cprz;
 	phys_addr_t phys_addr;
 	unsigned long size;
 	size_t record_size;
+	size_t console_size;
 	int dump_oops;
 	bool ecc;
 	unsigned int count;
@@ -96,6 +102,9 @@ ramoops_get_dump_prz(u64 id, enum pstore_type_id *type,
 		return NULL;
 
 	prz = cxt->przs[id];
+
+	/* Update old/shadowed buffer. */
+	persistent_ram_save_old(prz);
 	if (!persistent_ram_old_size(prz))
 		return NULL;
 
@@ -104,6 +113,19 @@ ramoops_get_dump_prz(u64 id, enum pstore_type_id *type,
 	return prz;
 }
 
+static struct persistent_ram_zone *
+ramoops_get_console_prz(u64 id, enum pstore_type_id *type,
+			struct ramoops_context *cxt)
+{
+	if (id >= cxt->max_count)
+		return NULL;
+
+	*type = PSTORE_TYPE_CONSOLE;
+	cxt->read_count = cxt->max_count;
+
+	return cxt->cprz;
+}
+
 static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 				   struct timespec *time,
 				   char **buf,
@@ -117,14 +139,14 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 
 	prz = ramoops_get_dump_prz(*id, type, cxt);
 	if (!prz)
+		prz = ramoops_get_console_prz(*id, type, cxt);
+	if (!prz)
 		return 0;
 
 	/* TODO(kees): Bogus time for the moment. */
 	time->tv_sec = 0;
 	time->tv_nsec = 0;
 
-	/* Update old/shadowed buffer. */
-	persistent_ram_save_old(prz);
 	size = persistent_ram_old_size(prz);
 	*buf = kmalloc(size, GFP_KERNEL);
 	if (*buf == NULL)
@@ -161,7 +183,13 @@ static int ramoops_pstore_write(enum pstore_type_id type,
 	struct persistent_ram_zone *prz = cxt->przs[cxt->count];
 	size_t hlen;
 
-	/* Currently ramoops is designed to only store dmesg dumps. */
+	if (type == PSTORE_TYPE_CONSOLE) {
+		if (!cxt->cprz)
+			return -ENOMEM;
+		persistent_ram_write(cxt->cprz, cxt->pstore.buf, size);
+		return 0;
+	}
+
 	if (type != PSTORE_TYPE_DMESG)
 		return -EINVAL;
 
@@ -198,12 +226,17 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
 				struct pstore_info *psi)
 {
 	struct ramoops_context *cxt = psi->data;
+	struct persistent_ram_zone *prz;
 
-	if (id >= cxt->max_dump_count)
+	if (id >= cxt->max_dump_count && id < cxt->max_count)
+		prz = cxt->cprz;
+	else if (id < cxt->max_dump_count)
+		prz = cxt->przs[id];
+	else
 		return -EINVAL;
 
-	persistent_ram_free_old(cxt->przs[id]);
-	persistent_ram_zap(cxt->przs[id]);
+	persistent_ram_free_old(prz);
+	persistent_ram_zap(prz);
 
 	return 0;
 }
@@ -272,6 +305,35 @@ fail_prz:
 	return err;
 }
 
+static void ramoops_free_cprz(struct ramoops_context *cxt)
+{
+	kfree(cxt->cprz);
+}
+
+static int ramoops_init_cprz(struct device *dev, struct ramoops_context *cxt,
+			     phys_addr_t *paddr, size_t console_mem_sz)
+{
+	if (!console_mem_sz)
+		return 0;
+
+	if (*paddr + console_mem_sz > *paddr + cxt->size)
+		return -ENOMEM;
+
+	cxt->cprz = persistent_ram_new(*paddr, console_mem_sz, cxt->ecc);
+	if (IS_ERR(cxt->cprz)) {
+		int err = PTR_ERR(cxt->cprz);
+
+		dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
+			console_mem_sz, (unsigned long long)*paddr, err);
+		return err;
+	}
+
+	*paddr += console_mem_sz;
+	cxt->max_count++;
+
+	return 0;
+}
+
 static int __init ramoops_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -287,35 +349,51 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	if (cxt->max_dump_count)
 		goto fail_out;
 
-	if (!pdata->mem_size || !pdata->record_size) {
-		pr_err("The memory size and the record size must be "
+	if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size)) {
+		pr_err("The memory size and the record/console size must be "
 			"non-zero\n");
 		goto fail_out;
 	}
 
 	pdata->mem_size = rounddown_pow_of_two(pdata->mem_size);
 	pdata->record_size = rounddown_pow_of_two(pdata->record_size);
+	pdata->console_size = rounddown_pow_of_two(pdata->console_size);
 
 	cxt->max_count = 0;
 	cxt->count = 0;
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
 	cxt->record_size = pdata->record_size;
+	cxt->console_size = pdata->console_size;
 	cxt->dump_oops = pdata->dump_oops;
 	cxt->ecc = pdata->ecc;
 
 	paddr = cxt->phys_addr;
 
-	dump_mem_sz = cxt->size;
+	dump_mem_sz = cxt->size - cxt->console_size;
 	err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
-	if (err) {
+	if (err)
+		goto fail_out;
+
+	err = ramoops_init_cprz(dev, cxt, &paddr, cxt->console_size);
+	if (err)
+		goto fail_init_cprz;
+
+	if (!cxt->max_count) {
 		pr_err("memory size too small, minimum is %lu\n",
-			cxt->record_size);
+			cxt->console_size + cxt->record_size);
 		goto fail_count;
 	}
 
 	cxt->pstore.data = cxt;
-	cxt->pstore.bufsize = cxt->przs[0]->buffer_size;
+	/*
+	 * Console can handle any buffer size, so prefer dumps buffer
+	 * size since usually it is smaller.
+	 */
+	if (cxt->przs)
+		cxt->pstore.bufsize = cxt->przs[0]->buffer_size;
+	else
+		cxt->pstore.bufsize = cxt->cprz->buffer_size;
 	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
 	spin_lock_init(&cxt->pstore.buf_lock);
 	if (!cxt->pstore.buf) {
@@ -324,7 +402,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	}
 
 	err = pstore_register(&cxt->pstore);
-	if (err || !cxt->max_count) {
+	if (err) {
 		pr_err("registering with pstore failed\n");
 		goto fail_buf;
 	}
@@ -352,6 +430,8 @@ fail_clear:
 	cxt->max_count = 0;
 	cxt->max_dump_count = 0;
 fail_count:
+	ramoops_free_cprz(cxt);
+fail_init_cprz:
 	ramoops_free_przs(cxt);
 fail_out:
 	return err;
@@ -403,6 +483,7 @@ static int __init ramoops_init(void)
 		dummy_data->mem_size = mem_size;
 		dummy_data->mem_address = mem_address;
 		dummy_data->record_size = record_size;
+		dummy_data->console_size = ramoops_console_size;
 		dummy_data->dump_oops = dump_oops;
 		dummy_data->ecc = ramoops_ecc;
 		dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 3b823d4..9385d41 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -93,6 +93,7 @@ struct ramoops_platform_data {
 	unsigned long	mem_size;
 	unsigned long	mem_address;
 	unsigned long	record_size;
+	unsigned long	console_size;
 	int		dump_oops;
 	bool		ecc;
 };
-- 
1.7.9.2

Re: [PATCH 10/16] pstore/ram: Add console messages handling

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> The console log size is configurable via ramoops.console_size
> module option, and the log itself is available via
> <pstore-mount>/console-ramoops file.
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 11/16] pstore/ram_core: Silence some printks

[Anton Vorontsov]

Since we use multiple regions, the messages are somewhat annoying.
We do print total mapped memory already, so no need to print the
information for each region in the library routines.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Colin Cross <ccross@android.com>
---
 fs/pstore/ram_core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index c5fbdbb..78f6d4b 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -409,14 +409,14 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool
 				" size %zu, start %zu\n",
 			       buffer_size(prz), buffer_start(prz));
 		else {
-			pr_info("persistent_ram: found existing buffer,"
+			pr_debug("persistent_ram: found existing buffer,"
 				" size %zu, start %zu\n",
 			       buffer_size(prz), buffer_start(prz));
 			persistent_ram_save_old(prz);
 			return 0;
 		}
 	} else {
-		pr_info("persistent_ram: no valid data in buffer"
+		pr_debug("persistent_ram: no valid data in buffer"
 			" (sig = 0x%08x)\n", prz->buffer->sig);
 	}
 
-- 
1.7.9.2

[PATCH 12/16] pstore/ram: Add some more documentation and examples

[Anton Vorontsov]

Suggested-by: Shuah Khan <shuahkhan@gmail.com>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Colin Cross <ccross@android.com>
---
 Documentation/ramoops.txt |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 4ba7db2..59a74a8 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -40,6 +40,12 @@ corrupt, but usually it is restorable.
 Setting the ramoops parameters can be done in 2 different manners:
  1. Use the module parameters (which have the names of the variables described
  as before).
+ For quick debugging, you can also reserve parts of memory during boot
+ and then use the reserved memory for ramoops. For example, assuming a machine
+ with > 128 MB of memory, the following kernel command line will tell the
+ kernel to use only the first 128 MB of memory, and place ECC-protected ramoops
+ region at 128 MB boundary:
+ "mem=128M ramoops.mem_address=0x8000000 ramoops.ecc=1"
  2. Use a platform device and set the platform data. The parameters can then
  be set through that platform data. An example of doing that is:
 
@@ -70,6 +76,14 @@ if (ret) {
 	return ret;
 }
 
+You can specify either RAM memory or peripheral devices' memory. However, when
+specifying RAM, be sure to reserve the memory by issuing memblock_reserve()
+very early in the architecture code, e.g.:
+
+#include <linux/memblock.h>
+
+memblock_reserve(ramoops_data.mem_address, ramoops_data.mem_size);
+
 3. Dump format
 
 The data dump begins with a header, currently defined as "====" followed by a
-- 
1.7.9.2

[PATCH 13/16] staging/android: Remove ram_console driver

[Anton Vorontsov]

All the functionality is now supported by pstore and pstore_ram drivers.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Colin Cross <ccross@android.com>
---
 drivers/staging/android/Kconfig       |    5 -
 drivers/staging/android/Makefile      |    1 -
 drivers/staging/android/ram_console.c |  179 ---------------------------------
 3 files changed, 185 deletions(-)
 delete mode 100644 drivers/staging/android/ram_console.c

diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 63f9822..df42c6b 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -25,11 +25,6 @@ config ANDROID_LOGGER
 	tristate "Android log driver"
 	default n
 
-config ANDROID_RAM_CONSOLE
-	bool "Android RAM buffer console"
-	depends on !S390 && !UML && HAVE_MEMBLOCK && PSTORE_RAM=y
-	default n
-
 config ANDROID_TIMED_OUTPUT
 	bool "Timed output class driver"
 	default y
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 4677e7b..5f3ef2e 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -1,7 +1,6 @@
 obj-$(CONFIG_ANDROID_BINDER_IPC)	+= binder.o
 obj-$(CONFIG_ASHMEM)			+= ashmem.o
 obj-$(CONFIG_ANDROID_LOGGER)		+= logger.o
-obj-$(CONFIG_ANDROID_RAM_CONSOLE)	+= ram_console.o
 obj-$(CONFIG_ANDROID_TIMED_OUTPUT)	+= timed_output.o
 obj-$(CONFIG_ANDROID_TIMED_GPIO)	+= timed_gpio.o
 obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
diff --git a/drivers/staging/android/ram_console.c b/drivers/staging/android/ram_console.c
deleted file mode 100644
index 82323bb..0000000
--- a/drivers/staging/android/ram_console.c
+++ /dev/null
@@ -1,179 +0,0 @@
-/* drivers/android/ram_console.c
- *
- * Copyright (C) 2007-2008 Google, Inc.
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/console.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/proc_fs.h>
-#include <linux/string.h>
-#include <linux/uaccess.h>
-#include <linux/io.h>
-#include <linux/pstore_ram.h>
-#include "ram_console.h"
-
-static struct persistent_ram_zone *ram_console_zone;
-static const char *bootinfo;
-static size_t bootinfo_size;
-
-static void
-ram_console_write(struct console *console, const char *s, unsigned int count)
-{
-	struct persistent_ram_zone *prz = console->data;
-	persistent_ram_write(prz, s, count);
-}
-
-static struct console ram_console = {
-	.name	= "ram",
-	.write	= ram_console_write,
-	.flags	= CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME,
-	.index	= -1,
-};
-
-void ram_console_enable_console(int enabled)
-{
-	if (enabled)
-		ram_console.flags |= CON_ENABLED;
-	else
-		ram_console.flags &= ~CON_ENABLED;
-}
-
-static int __init ram_console_probe(struct platform_device *pdev)
-{
-	struct ram_console_platform_data *pdata = pdev->dev.platform_data;
-	struct persistent_ram_zone *prz;
-
-	prz = persistent_ram_init_ringbuffer(&pdev->dev, true);
-	if (IS_ERR(prz))
-		return PTR_ERR(prz);
-
-
-	if (pdata) {
-		bootinfo = kstrdup(pdata->bootinfo, GFP_KERNEL);
-		if (bootinfo)
-			bootinfo_size = strlen(bootinfo);
-	}
-
-	ram_console_zone = prz;
-	ram_console.data = prz;
-
-	register_console(&ram_console);
-
-	return 0;
-}
-
-static struct platform_driver ram_console_driver = {
-	.driver		= {
-		.name	= "ram_console",
-	},
-};
-
-static int __init ram_console_module_init(void)
-{
-	return platform_driver_probe(&ram_console_driver, ram_console_probe);
-}
-
-#ifndef CONFIG_PRINTK
-#define dmesg_restrict	0
-#endif
-
-static ssize_t ram_console_read_old(struct file *file, char __user *buf,
-				    size_t len, loff_t *offset)
-{
-	loff_t pos = *offset;
-	ssize_t count;
-	struct persistent_ram_zone *prz = ram_console_zone;
-	size_t old_log_size = persistent_ram_old_size(prz);
-	const char *old_log = persistent_ram_old(prz);
-	char *str;
-	int ret;
-
-	if (dmesg_restrict && !capable(CAP_SYSLOG))
-		return -EPERM;
-
-	/* Main last_kmsg log */
-	if (pos < old_log_size) {
-		count = min(len, (size_t)(old_log_size - pos));
-		if (copy_to_user(buf, old_log + pos, count))
-			return -EFAULT;
-		goto out;
-	}
-
-	/* ECC correction notice */
-	pos -= old_log_size;
-	count = persistent_ram_ecc_string(prz, NULL, 0);
-	if (pos < count) {
-		str = kmalloc(count, GFP_KERNEL);
-		if (!str)
-			return -ENOMEM;
-		persistent_ram_ecc_string(prz, str, count + 1);
-		count = min(len, (size_t)(count - pos));
-		ret = copy_to_user(buf, str + pos, count);
-		kfree(str);
-		if (ret)
-			return -EFAULT;
-		goto out;
-	}
-
-	/* Boot info passed through pdata */
-	pos -= count;
-	if (pos < bootinfo_size) {
-		count = min(len, (size_t)(bootinfo_size - pos));
-		if (copy_to_user(buf, bootinfo + pos, count))
-			return -EFAULT;
-		goto out;
-	}
-
-	/* EOF */
-	return 0;
-
-out:
-	*offset += count;
-	return count;
-}
-
-static const struct file_operations ram_console_file_ops = {
-	.owner = THIS_MODULE,
-	.read = ram_console_read_old,
-};
-
-static int __init ram_console_late_init(void)
-{
-	struct proc_dir_entry *entry;
-	struct persistent_ram_zone *prz = ram_console_zone;
-
-	if (!prz)
-		return 0;
-
-	if (persistent_ram_old_size(prz) == 0)
-		return 0;
-
-	entry = create_proc_entry("last_kmsg", S_IFREG | S_IRUGO, NULL);
-	if (!entry) {
-		printk(KERN_ERR "ram_console: failed to create proc entry\n");
-		persistent_ram_free_old(prz);
-		return 0;
-	}
-
-	entry->proc_fops = &ram_console_file_ops;
-	entry->size = persistent_ram_old_size(prz) +
-		persistent_ram_ecc_string(prz, NULL, 0) +
-		bootinfo_size;
-
-	return 0;
-}
-
-late_initcall(ram_console_late_init);
-postcore_initcall(ram_console_module_init);
-- 
1.7.9.2

[PATCH 14/16] pstore/ram_core: Remove now unused code

[Anton Vorontsov]

The code tried to maintain the global list of persistent ram zones,
which isn't a great idea overall, plus since Android's ram_console
is no longer there, we can remove some unused functions.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Colin Cross <ccross@android.com>
---
 fs/pstore/ram_core.c       |   77 --------------------------------------------
 include/linux/pstore_ram.h |   19 -----------
 2 files changed, 96 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 78f6d4b..0fd8161 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -35,8 +35,6 @@ struct persistent_ram_buffer {
 
 #define PERSISTENT_RAM_SIG (0x43474244) /* DBGC */
 
-static __initdata LIST_HEAD(persistent_ram_list);
-
 static inline size_t buffer_size(struct persistent_ram_zone *prz)
 {
 	return atomic_read(&prz->buffer->size);
@@ -462,78 +460,3 @@ err:
 	kfree(prz);
 	return ERR_PTR(ret);
 }
-
-#ifndef MODULE
-static int __init persistent_ram_buffer_init(const char *name,
-		struct persistent_ram_zone *prz)
-{
-	int i;
-	struct persistent_ram *ram;
-	struct persistent_ram_descriptor *desc;
-	phys_addr_t start;
-
-	list_for_each_entry(ram, &persistent_ram_list, node) {
-		start = ram->start;
-		for (i = 0; i < ram->num_descs; i++) {
-			desc = &ram->descs[i];
-			if (!strcmp(desc->name, name))
-				return persistent_ram_buffer_map(start,
-						desc->size, prz);
-			start += desc->size;
-		}
-	}
-
-	return -EINVAL;
-}
-
-static  __init
-struct persistent_ram_zone *__persistent_ram_init(struct device *dev, bool ecc)
-{
-	struct persistent_ram_zone *prz;
-	int ret = -ENOMEM;
-
-	prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
-	if (!prz) {
-		pr_err("persistent_ram: failed to allocate persistent ram zone\n");
-		goto err;
-	}
-
-	ret = persistent_ram_buffer_init(dev_name(dev), prz);
-	if (ret) {
-		pr_err("persistent_ram: failed to initialize buffer\n");
-		goto err;
-	}
-
-	persistent_ram_post_init(prz, ecc);
-
-	return prz;
-err:
-	kfree(prz);
-	return ERR_PTR(ret);
-}
-
-struct persistent_ram_zone * __init
-persistent_ram_init_ringbuffer(struct device *dev, bool ecc)
-{
-	return __persistent_ram_init(dev, ecc);
-}
-
-int __init persistent_ram_early_init(struct persistent_ram *ram)
-{
-	int ret;
-
-	ret = memblock_reserve(ram->start, ram->size);
-	if (ret) {
-		pr_err("Failed to reserve persistent memory from %08lx-%08lx\n",
-			(long)ram->start, (long)(ram->start + ram->size - 1));
-		return ret;
-	}
-
-	list_add_tail(&ram->node, &persistent_ram_list);
-
-	pr_info("Initialized persistent memory from %08lx-%08lx\n",
-		(long)ram->start, (long)(ram->start + ram->size - 1));
-
-	return 0;
-}
-#endif
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9385d41..2470bb5 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -25,21 +25,6 @@
 
 struct persistent_ram_buffer;
 
-struct persistent_ram_descriptor {
-	const char	*name;
-	phys_addr_t	size;
-};
-
-struct persistent_ram {
-	phys_addr_t	start;
-	phys_addr_t	size;
-
-	int					num_descs;
-	struct persistent_ram_descriptor	*descs;
-
-	struct list_head node;
-};
-
 struct persistent_ram_zone {
 	phys_addr_t paddr;
 	size_t size;
@@ -63,15 +48,11 @@ struct persistent_ram_zone {
 	size_t old_log_size;
 };
 
-int persistent_ram_early_init(struct persistent_ram *ram);
-
 struct persistent_ram_zone * __init persistent_ram_new(phys_addr_t start,
 						       size_t size,
 						       bool ecc);
 void persistent_ram_free(struct persistent_ram_zone *prz);
 void persistent_ram_zap(struct persistent_ram_zone *prz);
-struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev,
-		bool ecc);
 
 int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
 	unsigned int count);
-- 
1.7.9.2

[PATCH 15/16] pstore/platform: Make automatic updates interval configurable

[Anton Vorontsov]

There is no behavioural change, the default value is still 60 seconds.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/platform.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index a3f6d96..4f49bb4 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -31,6 +31,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/hardirq.h>
+#include <linux/jiffies.h>
 #include <linux/workqueue.h>
 
 #include "internal.h"
@@ -40,7 +41,10 @@
  * whether the system is actually still running well enough
  * to let someone see the entry
  */
-#define	PSTORE_INTERVAL	(60 * HZ)
+static int pstore_update_ms = 60000;
+module_param_named(update_ms, pstore_update_ms, int, 0600);
+MODULE_PARM_DESC(update_ms, "milliseconds before pstore updates its content "
+		 "(default is 60000; -1 means runtime updates are disabled)");
 
 static int pstore_new_entry;
 
@@ -227,8 +231,11 @@ int pstore_register(struct pstore_info *psi)
 	kmsg_dump_register(&pstore_dumper);
 	pstore_register_console();
 
-	pstore_timer.expires = jiffies + PSTORE_INTERVAL;
-	add_timer(&pstore_timer);
+	if (pstore_update_ms >= 0) {
+		pstore_timer.expires = jiffies +
+			msecs_to_jiffies(pstore_update_ms);
+		add_timer(&pstore_timer);
+	}
 
 	return 0;
 }
@@ -287,7 +294,7 @@ static void pstore_timefunc(unsigned long dummy)
 		schedule_work(&pstore_work);
 	}
 
-	mod_timer(&pstore_timer, jiffies + PSTORE_INTERVAL);
+	mod_timer(&pstore_timer, jiffies + msecs_to_jiffies(pstore_update_ms));
 }
 
 module_param(backend, charp, 0444);
-- 
1.7.9.2

Re: [PATCH 15/16] pstore/platform: Make automatic updates interval configurable

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> There is no behavioural change, the default value is still 60 seconds.
>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

[PATCH 16/16] pstore/platform: Disable automatic updates by default

[Anton Vorontsov]

Having automatic updates seems pointless for production system, and
even dangerous and thus counter-productive:

1. If we can mount pstore, or read files, we can as well read
   /proc/kmsg. So, there's little point in duplicating the
   functionality and present the same information but via another
   userland ABI;

2. Expecting the kernel to behave sanely after oops/panic is naive.
   It might work, but you'd rather not try it. Screwed up kernel
   can do rather bad things, like recursive faults[1]; and pstore
   rather provoking bad things to happen. It uses:

   1. Timers (assumes sane interrupts state);
   2. Workqueues and mutexes (assumes scheduler in a sane state);
   3. kzalloc (a working slab allocator);

   That's too much for a dead kernel, so the debugging facility
   itself might just make debugging harder, which is not what
   we want.

Maybe for non-oops message types it would make sense to re-enable
automatic updates, but so far I don't see any use case for this.
Even for tracing, it has its own run-time/normal ABI, so we're
only interested in pstore upon next boot, to retrieve what has
gone wrong with HW or SW.

So, let's disable the updates by default.

[1]
BUG: unable to handle kernel paging request at fffffffffffffff8
IP: [<ffffffff8104801b>] kthread_data+0xb/0x20
[...]
Process kworker/0:1 (pid: 14, threadinfo ffff8800072c0000, task ffff88000725b100)
[...
Call Trace:
 [<ffffffff81043710>] wq_worker_sleeping+0x10/0xa0
 [<ffffffff813687a8>] __schedule+0x568/0x7d0
 [<ffffffff8106c24d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81087e22>] ? call_rcu_sched+0x12/0x20
 [<ffffffff8102b596>] ? release_task+0x156/0x2d0
 [<ffffffff8102b45e>] ? release_task+0x1e/0x2d0
 [<ffffffff8106c24d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81368ac4>] schedule+0x24/0x70
 [<ffffffff8102cba8>] do_exit+0x1f8/0x370
 [<ffffffff810051e7>] oops_end+0x77/0xb0
 [<ffffffff8135c301>] no_context+0x1a6/0x1b5
 [<ffffffff8135c4de>] __bad_area_nosemaphore+0x1ce/0x1ed
 [<ffffffff81053156>] ? ttwu_queue+0xc6/0xe0
 [<ffffffff8135c50b>] bad_area_nosemaphore+0xe/0x10
 [<ffffffff8101fa47>] do_page_fault+0x2c7/0x450
 [<ffffffff8106e34b>] ? __lock_release+0x6b/0xe0
 [<ffffffff8106bf21>] ? mark_held_locks+0x61/0x140
 [<ffffffff810502fe>] ? __wake_up+0x4e/0x70
 [<ffffffff81185f7d>] ? trace_hardirqs_off_thunk+0x3a/0x3c
 [<ffffffff81158970>] ? pstore_register+0x120/0x120
 [<ffffffff8136a37f>] page_fault+0x1f/0x30
 [<ffffffff81158970>] ? pstore_register+0x120/0x120
 [<ffffffff81185ab8>] ? memcpy+0x68/0x110
 [<ffffffff8115875a>] ? pstore_get_records+0x3a/0x130
 [<ffffffff811590f4>] ? persistent_ram_copy_old+0x64/0x90
 [<ffffffff81158bf4>] ramoops_pstore_read+0x84/0x130
 [<ffffffff81158799>] pstore_get_records+0x79/0x130
 [<ffffffff81042536>] ? process_one_work+0x116/0x450
 [<ffffffff81158970>] ? pstore_register+0x120/0x120
 [<ffffffff8115897e>] pstore_dowork+0xe/0x10
 [<ffffffff81042594>] process_one_work+0x174/0x450
 [<ffffffff81042536>] ? process_one_work+0x116/0x450
 [<ffffffff81042e13>] worker_thread+0x123/0x2d0
 [<ffffffff81042cf0>] ? manage_workers.isra.28+0x120/0x120
 [<ffffffff81047d8e>] kthread+0x8e/0xa0
 [<ffffffff8136ba74>] kernel_thread_helper+0x4/0x10
 [<ffffffff8136a199>] ? retint_restore_args+0xe/0xe
 [<ffffffff81047d00>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff8136ba70>] ? gs_change+0xb/0xb
Code: be e2 00 00 00 48 c7 c7 d1 2a 4e 81 e8 bf fb fd ff 48 8b 5d f0 4c 8b 65 f8 c9 c3 0f 1f 44 00 00 48 8b 87 08 02 00 00 55 48 89 e5 <48> 8b 40 f8 5d c3 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00
RIP  [<ffffffff8104801b>] kthread_data+0xb/0x20
 RSP <ffff8800072c1888>
CR2: fffffffffffffff8
---[ end trace 996a332dc399111d ]---
Fixing recursive fault but reboot is needed!

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/platform.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 4f49bb4..1dbf49d 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -41,10 +41,11 @@
  * whether the system is actually still running well enough
  * to let someone see the entry
  */
-static int pstore_update_ms = 60000;
+static int pstore_update_ms = -1;
 module_param_named(update_ms, pstore_update_ms, int, 0600);
 MODULE_PARM_DESC(update_ms, "milliseconds before pstore updates its content "
-		 "(default is 60000; -1 means runtime updates are disabled)");
+		 "(default is -1, which means runtime updates are disabled; "
+		 "enabling this option is not safe)");
 
 static int pstore_new_entry;
 
-- 
1.7.9.2

Re: [PATCH 16/16] pstore/platform: Disable automatic updates by default

[Kees Cook]

On Tue, May 22, 2012 at 7:17 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> Having automatic updates seems pointless for production system, and
> even dangerous and thus counter-productive:
>
> 1. If we can mount pstore, or read files, we can as well read
>   /proc/kmsg. So, there's little point in duplicating the
>   functionality and present the same information but via another
>   userland ABI;
>
> 2. Expecting the kernel to behave sanely after oops/panic is naive.
>   It might work, but you'd rather not try it. Screwed up kernel
>   can do rather bad things, like recursive faults[1]; and pstore
>   rather provoking bad things to happen. It uses:
>
>   1. Timers (assumes sane interrupts state);
>   2. Workqueues and mutexes (assumes scheduler in a sane state);
>   3. kzalloc (a working slab allocator);
>
>   That's too much for a dead kernel, so the debugging facility
>   itself might just make debugging harder, which is not what
>   we want.
>
> Maybe for non-oops message types it would make sense to re-enable
> automatic updates, but so far I don't see any use case for this.
> Even for tracing, it has its own run-time/normal ABI, so we're
> only interested in pstore upon next boot, to retrieve what has
> gone wrong with HW or SW.
>
> So, let's disable the updates by default.

I'll let Tony ack this, but I'm fine with it -- making this
configurable is sufficient for my needs. :)

> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 4f49bb4..1dbf49d 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -41,10 +41,11 @@
>  * whether the system is actually still running well enough
>  * to let someone see the entry
>  */
> -static int pstore_update_ms = 60000;
> +static int pstore_update_ms = -1;
>  module_param_named(update_ms, pstore_update_ms, int, 0600);
>  MODULE_PARM_DESC(update_ms, "milliseconds before pstore updates its content "
> -                "(default is 60000; -1 means runtime updates are disabled)");
> +                "(default is -1, which means runtime updates are disabled; "
> +                "enabling this option is not safe)");

Perhaps "enabling this option may lead to further corruption on
Oopses" or something more specific?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v4 0/16] Merge ram_console into pstore, and more

[Greg Kroah-Hartman]

On Tue, May 22, 2012 at 07:17:17AM -0700, Anton Vorontsov wrote:
> Hi all,
> 
> A brand new v4:
> 
> - Per Kees Cook's comments, the patches no longer remove an automatic
>   updates feature, but instead make the it configurable; plus disable
>   it by default (in a separate patch);
> - Fixed some bugs noticed by Colin Cross;
> - Documented new continuous ramoops-console log behaviour (also
>   noticed by Colin Cross).

In case you were wondering, all of this is going to have to wait for
3.6, as the 3.5 merge window with my trees have been closed.  I'll be
able to start accepting patches once 3.5-rc1 is out, so take your time
in getting this all working properly, there's no rush here.

greg k-h

Re: [PATCH v4 0/16] Merge ram_console into pstore, and more

[Kees Cook]

On Tue, May 22, 2012 at 11:19 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, May 22, 2012 at 07:17:17AM -0700, Anton Vorontsov wrote:
>> Hi all,
>>
>> A brand new v4:
>>
>> - Per Kees Cook's comments, the patches no longer remove an automatic
>>   updates feature, but instead make the it configurable; plus disable
>>   it by default (in a separate patch);
>> - Fixed some bugs noticed by Colin Cross;
>> - Documented new continuous ramoops-console log behaviour (also
>>   noticed by Colin Cross).
>
> In case you were wondering, all of this is going to have to wait for
> 3.6, as the 3.5 merge window with my trees have been closed.  I'll be
> able to start accepting patches once 3.5-rc1 is out, so take your time
> in getting this all working properly, there's no rush here.

Will the ramoops stuff that was in -next (via -mm) that now landed in
-staging make it sanely into 3.5? Specifically the two patches at the
top of:
http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/ramoops

Thanks,

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v4 0/16] Merge ram_console into pstore, and more

[Greg Kroah-Hartman]

On Tue, May 22, 2012 at 11:33:33AM -0700, Kees Cook wrote:
> On Tue, May 22, 2012 at 11:19 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, May 22, 2012 at 07:17:17AM -0700, Anton Vorontsov wrote:
> >> Hi all,
> >>
> >> A brand new v4:
> >>
> >> - Per Kees Cook's comments, the patches no longer remove an automatic
> >>   updates feature, but instead make the it configurable; plus disable
> >>   it by default (in a separate patch);
> >> - Fixed some bugs noticed by Colin Cross;
> >> - Documented new continuous ramoops-console log behaviour (also
> >>   noticed by Colin Cross).
> >
> > In case you were wondering, all of this is going to have to wait for
> > 3.6, as the 3.5 merge window with my trees have been closed.  I'll be
> > able to start accepting patches once 3.5-rc1 is out, so take your time
> > in getting this all working properly, there's no rush here.
> 
> Will the ramoops stuff that was in -next (via -mm) that now landed in
> -staging make it sanely into 3.5? Specifically the two patches at the
> top of:
> http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/ramoops

Anything that was in staging and linux-next already, yes, will go in for
3.5, I've already sent Linus the pull request for that.

Is that a problem?

thanks,

greg k-h

Re: [PATCH v4 0/16] Merge ram_console into pstore, and more

[Kees Cook]

On Tue, May 22, 2012 at 12:04 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, May 22, 2012 at 11:33:33AM -0700, Kees Cook wrote:
>> On Tue, May 22, 2012 at 11:19 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Tue, May 22, 2012 at 07:17:17AM -0700, Anton Vorontsov wrote:
>> >> Hi all,
>> >>
>> >> A brand new v4:
>> >>
>> >> - Per Kees Cook's comments, the patches no longer remove an automatic
>> >>   updates feature, but instead make the it configurable; plus disable
>> >>   it by default (in a separate patch);
>> >> - Fixed some bugs noticed by Colin Cross;
>> >> - Documented new continuous ramoops-console log behaviour (also
>> >>   noticed by Colin Cross).
>> >
>> > In case you were wondering, all of this is going to have to wait for
>> > 3.6, as the 3.5 merge window with my trees have been closed.  I'll be
>> > able to start accepting patches once 3.5-rc1 is out, so take your time
>> > in getting this all working properly, there's no rush here.
>>
>> Will the ramoops stuff that was in -next (via -mm) that now landed in
>> -staging make it sanely into 3.5? Specifically the two patches at the
>> top of:
>> http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/ramoops
>
> Anything that was in staging and linux-next already, yes, will go in for
> 3.5, I've already sent Linus the pull request for that.
>
> Is that a problem?

No problem at all -- it missed 3.4 on a technicality, and I didn't
want it to miss 3.5 too. :)

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security