[1/6] pstore: Add console log messages support

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>
---
 fs/pstore/Kconfig      |    7 +++++++
 fs/pstore/inode.c      |    3 +++
 fs/pstore/platform.c   |   25 +++++++++++++++++++++++++
 include/linux/pstore.h |    1 +
 4 files changed, 36 insertions(+)

On Wed, May 16, 2012 at 5:56 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> 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>
> ---
>  fs/pstore/Kconfig      |    7 +++++++
>  fs/pstore/inode.c      |    3 +++
>  fs/pstore/platform.c   |   25 +++++++++++++++++++++++++
>  include/linux/pstore.h |    1 +
>  4 files changed, 36 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 1950788..97d35ee 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..3a384c8 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,28 @@ 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)
> +{
> +       strncpy(psinfo->buf, s, c);

The size of psinfo->buf needs to be the length argument to strncpy,
not the size of "s". If "s" isn't NULL terminated, then the min of c
and buf's size should be used. And if this should be NULL terminated
in buf, that needs to be added manually since strncpy won't do it if
it hits the max length argument.

-Kees

On Wed, May 16, 2012 at 09:49:11AM -0700, Kees Cook wrote:
[...]
> > +#ifdef CONFIG_PSTORE_CONSOLE
> > +static void pstore_console_write(struct console *con, const char *s, unsigned c)
> > +{
> > +       strncpy(psinfo->buf, s, c);
> 
> The size of psinfo->buf needs to be the length argument to strncpy,
> not the size of "s". If "s" isn't NULL terminated, then the min of c
> and buf's size should be used. And if this should be NULL terminated
> in buf, that needs to be added manually since strncpy won't do it if
> it hits the max length argument.

Whoops. Will fix it, thanks!

On Wed, May 16, 2012 at 03:33:45PM -0700, Anton Vorontsov wrote:
> On Wed, May 16, 2012 at 09:49:11AM -0700, Kees Cook wrote:
> [...]
> > > +#ifdef CONFIG_PSTORE_CONSOLE
> > > +static void pstore_console_write(struct console *con, const char *s, unsigned c)
> > > +{
> > > +       strncpy(psinfo->buf, s, c);
> > 
> > The size of psinfo->buf needs to be the length argument to strncpy,
> > not the size of "s". If "s" isn't NULL terminated, then the min of c
> > and buf's size should be used. And if this should be NULL terminated
> > in buf, that needs to be added manually since strncpy won't do it if
> > it hits the max length argument.
> 
> Whoops. Will fix it, thanks!

Actually, we shouldn't bother with NUL-termination at all. We should
do, is just carefully copy all 'c' elements of 's' into pstore in
'pstore->bufsize' chunks.

So, the code should be something along these lines:

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;
        }
}

Here, in case of c > bufsize, we can't just copy bufsize bytes once and
exit, since 'bufsize' doesn't tell anything about TYPE_CONSOLE's internal
storage size (today console's persistent ram zone size and pstore's
bufsize are equal, but this is something we may change soon, i.e. make
TYPE_DMESG and TYPE_CONSOLE record sizes configurable).

Thanks,