[PATCH v4] ramoops: use pstore interface

[PATCH v4] ramoops: use pstore interface

[Kees Cook]

Instead of using /dev/mem directly, use the common pstore infrastructure
to handle Oops gathering and extraction.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v4:
 - rebased to Linus's tree. Still time to get into merge window?
v3:
 - Added more documentation about which memory regions it would be
   used with.
v2:
 - general reworking.
---
 Documentation/ramoops.txt |    8 +-
 drivers/char/Kconfig      |    1 +
 drivers/char/ramoops.c    |  218 +++++++++++++++++++++++++++++++++-----------
 3 files changed, 168 insertions(+), 59 deletions(-)

diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 8fb1ba7..a0b9d8e 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache <sergiu@chromium.org>
 
-Updated: 8 August 2011
+Updated: 17 November 2011
 
 0. Introduction
 
@@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with the actual data.
 
 4. Reading the data
 
-The dump data can be read from memory (through /dev/mem or other means).
-Getting the module parameters, which are needed in order to parse the data, can
-be done through /sys/module/ramoops/parameters/* .
+The dump data can be read from the pstore filesystem. The format for these
+files is "dmesg-ramoops-N", where N is the record number in memory. To delete
+a stored record from RAM, simply unlink the respective pstore file.
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 4364303..f166499 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -603,6 +603,7 @@ source "drivers/s390/char/Kconfig"
 config RAMOOPS
 	tristate "Log panic/oops to a RAM buffer"
 	depends on HAS_IOMEM
+	depends on PSTORE
 	default n
 	help
 	  This enables panic and oops messages to be logged to a circular
diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 7c7f42a1f8..51f4dcc 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -2,6 +2,7 @@
  * RAM Oops/Panic logger
  *
  * Copyright (C) 2010 Marco Stornelli <marco.stornelli@gmail.com>
+ * Copyright (C) 2011 Kees Cook <keescook@chromium.org>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -24,7 +25,7 @@
 #include <linux/kernel.h>
 #include <linux/err.h>
 #include <linux/module.h>
-#include <linux/kmsg_dump.h>
+#include <linux/pstore.h>
 #include <linux/time.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -56,74 +57,153 @@ module_param(dump_oops, int, 0600);
 MODULE_PARM_DESC(dump_oops,
 		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
 
-static struct ramoops_context {
-	struct kmsg_dumper dump;
+struct ramoops_context {
 	void *virt_addr;
 	phys_addr_t phys_addr;
 	unsigned long size;
-	unsigned long record_size;
+	size_t record_size;
 	int dump_oops;
-	int count;
-	int max_count;
-} oops_cxt;
+	unsigned int count;
+	unsigned int max_count;
+	unsigned int read_count;
+	struct pstore_info pstore;
+};
 
 static struct platform_device *dummy;
 static struct ramoops_platform_data *dummy_data;
 
-static void ramoops_do_dump(struct kmsg_dumper *dumper,
-		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
-		const char *s2, unsigned long l2)
+static int ramoops_pstore_open(struct pstore_info *psi)
+{
+	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
+
+	cxt->read_count = 0;
+	return 0;
+}
+
+static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
+				   struct timespec *time,
+				   char **buf,
+				   struct pstore_info *psi)
+{
+	ssize_t size;
+	char *rambuf;
+	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
+
+	if (cxt->read_count >= cxt->max_count)
+		return -EINVAL;
+	*id = cxt->read_count++;
+	/* Only supports dmesg output so far. */
+	*type = PSTORE_TYPE_DMESG;
+	/* TODO(kees): Bogus time for the moment. */
+	time->tv_sec = 0;
+	time->tv_nsec = 0;
+
+	rambuf = cxt->virt_addr + (*id * cxt->record_size);
+	size = strnlen(rambuf, cxt->record_size);
+	*buf = kmalloc(size, GFP_KERNEL);
+	if (*buf == NULL)
+		return -ENOMEM;
+	memcpy(*buf, rambuf, size);
+
+	return size;
+}
+
+static int ramoops_pstore_write(enum pstore_type_id type,
+				enum kmsg_dump_reason reason,
+				u64 *id,
+				unsigned int part,
+				size_t size, struct pstore_info *psi)
 {
-	struct ramoops_context *cxt = container_of(dumper,
-			struct ramoops_context, dump);
-	unsigned long s1_start, s2_start;
-	unsigned long l1_cpy, l2_cpy;
-	int res, hdr_size;
-	char *buf, *buf_orig;
+	char *buf;
+	size_t res;
 	struct timeval timestamp;
+	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
+	size_t available = cxt->record_size;
 
+	/* Only store dmesg dumps. */
+	if (type != PSTORE_TYPE_DMESG)
+		return -EINVAL;
+
+	/* Only store crash dumps. */
 	if (reason != KMSG_DUMP_OOPS &&
-	    reason != KMSG_DUMP_PANIC &&
-	    reason != KMSG_DUMP_KEXEC)
-		return;
+	    reason != KMSG_DUMP_PANIC)
+		return -EINVAL;
 
 	/* Only dump oopses if dump_oops is set */
 	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
-		return;
+		return -EINVAL;
+
+	/* Explicitly only take the first part of any new crash.
+	 * If our buffer is larger than kmsg_bytes, this can never happen,
+	 * and if our buffer is smaller than kmsg_bytes, we don't want the
+	 * report split across multiple records. */
+	if (part != 1)
+		return -ENOSPC;
 
 	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
-	buf_orig = buf;
 
-	memset(buf, '\0', cxt->record_size);
 	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
 	buf += res;
+	available -= res;
+
 	do_gettimeofday(&timestamp);
 	res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec);
 	buf += res;
+	available -= res;
 
-	hdr_size = buf - buf_orig;
-	l2_cpy = min(l2, cxt->record_size - hdr_size);
-	l1_cpy = min(l1, cxt->record_size - hdr_size - l2_cpy);
-
-	s2_start = l2 - l2_cpy;
-	s1_start = l1 - l1_cpy;
+	if (size > available)
+		size = available;
 
-	memcpy(buf, s1 + s1_start, l1_cpy);
-	memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
+	memcpy(buf, cxt->pstore.buf, size);
+	memset(buf + size, '\0', available - size);
 
 	cxt->count = (cxt->count + 1) % cxt->max_count;
+
+	return 0;
+}
+
+static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
+				struct pstore_info *psi)
+{
+	char *buf;
+	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
+
+	if (id >= cxt->max_count)
+		return -EINVAL;
+
+	buf = cxt->virt_addr + (id * cxt->record_size);
+	memset(buf, '\0', cxt->record_size);
+
+	return 0;
 }
 
+static struct ramoops_context oops_cxt = {
+	.pstore = {
+		.owner	= THIS_MODULE,
+		.name	= "ramoops",
+		.open	= ramoops_pstore_open,
+		.read	= ramoops_pstore_read,
+		.write	= ramoops_pstore_write,
+		.erase	= ramoops_pstore_erase,
+	},
+};
+
 static int __init ramoops_probe(struct platform_device *pdev)
 {
 	struct ramoops_platform_data *pdata = pdev->dev.platform_data;
 	struct ramoops_context *cxt = &oops_cxt;
 	int err = -EINVAL;
 
+	/* Only a single ramoops area allowed at a time, so fail extra
+	 * probes.
+	 */
+	if (cxt->max_count)
+		goto fail_out;
+
 	if (!pdata->mem_size || !pdata->record_size) {
 		pr_err("The memory size and the record size must be "
 			"non-zero\n");
-		goto fail3;
+		goto fail_out;
 	}
 
 	rounddown_pow_of_two(pdata->mem_size);
@@ -132,14 +212,15 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	/* 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, minium is %lu\n", MIN_MEM_SIZE);
-		goto fail3;
+		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 fail3;
+		goto fail_out;
 	}
 
 	cxt->max_count = pdata->mem_size / pdata->record_size;
@@ -148,54 +229,81 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	cxt->phys_addr = pdata->mem_address;
 	cxt->record_size = pdata->record_size;
 	cxt->dump_oops = pdata->dump_oops;
-	/*
-	 * Update the module parameter variables as well so they are visible
-	 * through /sys/module/ramoops/parameters/
-	 */
-	mem_size = pdata->mem_size;
-	mem_address = pdata->mem_address;
-	record_size = pdata->record_size;
-	dump_oops = pdata->dump_oops;
+
+	cxt->pstore.data = cxt;
+	cxt->pstore.bufsize = cxt->record_size;
+	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
+	spin_lock_init(&cxt->pstore.buf_lock);
+	if (!cxt->pstore.buf) {
+		pr_err("cannot allocate pstore buffer\n");
+		goto fail_clear;
+	}
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
-		pr_err("request mem region failed\n");
+		pr_err("request mem region (0x%lx@0x%llx) failed\n",
+			cxt->size, cxt->phys_addr);
 		err = -EINVAL;
-		goto fail3;
+		goto fail_buf;
 	}
 
 	cxt->virt_addr = ioremap(cxt->phys_addr,  cxt->size);
 	if (!cxt->virt_addr) {
 		pr_err("ioremap failed\n");
-		goto fail2;
+		goto fail_mem_region;
 	}
 
-	cxt->dump.dump = ramoops_do_dump;
-	err = kmsg_dump_register(&cxt->dump);
+	err = pstore_register(&cxt->pstore);
 	if (err) {
-		pr_err("registering kmsg dumper failed\n");
-		goto fail1;
+		pr_err("registering with pstore failed\n");
+		goto fail_iounmap;
 	}
 
+	/*
+	 * Update the module parameter variables as well so they are visible
+	 * through /sys/module/ramoops/parameters/
+	 */
+	mem_size = pdata->mem_size;
+	mem_address = pdata->mem_address;
+	record_size = pdata->record_size;
+	dump_oops = pdata->dump_oops;
+
+	pr_info("attached 0x%lx@0x%llx (%ux0x%zx)\n",
+		cxt->size, cxt->phys_addr, cxt->max_count, cxt->record_size);
+
 	return 0;
 
-fail1:
+fail_iounmap:
 	iounmap(cxt->virt_addr);
-fail2:
+fail_mem_region:
 	release_mem_region(cxt->phys_addr, cxt->size);
-fail3:
+fail_buf:
+	kfree(cxt->pstore.buf);
+fail_clear:
+	cxt->pstore.bufsize = 0;
+	cxt->max_count = 0;
+fail_out:
 	return err;
 }
 
 static int __exit ramoops_remove(struct platform_device *pdev)
 {
+/* TODO(kees): We cannot unload ramoops since pstore doesn't support
+ * unregistering yet.
+ */
+#if 0
 	struct ramoops_context *cxt = &oops_cxt;
 
-	if (kmsg_dump_unregister(&cxt->dump) < 0)
-		pr_warn("could not unregister kmsg_dumper\n");
-
 	iounmap(cxt->virt_addr);
 	release_mem_region(cxt->phys_addr, cxt->size);
+	cxt->max_count = 0;
+
+	/* TODO(kees): When pstore supports unregistering, call it here. */
+	kfree(cxt->pstore.buf);
+	cxt->pstore.bufsize = 0;
+
 	return 0;
+#endif
+	return -EBUSY;
 }
 
 static struct platform_driver ramoops_driver = {
-- 
1.7.4.1


-- 
Kees Cook
ChromeOS Security

Re: [PATCH v4] ramoops: use pstore interface

[Andrew Morton]

On Sat, 7 Jan 2012 09:15:16 -0800
Kees Cook <keescook@chromium.org> wrote:

> Instead of using /dev/mem directly, use the common pstore infrastructure
> to handle Oops gathering and extraction.

um, why?  This changelog provides no reason for anyone to apply the
patch!

>
> ...
>
> -static void ramoops_do_dump(struct kmsg_dumper *dumper,
> -		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
> -		const char *s2, unsigned long l2)
> +static int ramoops_pstore_open(struct pstore_info *psi)
> +{
> +	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;

Unneeded and undesirable cast of void*.  Multiple instances of this.

> +	cxt->read_count = 0;
> +	return 0;
> +}
> +
> +static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
> +				   struct timespec *time,
> +				   char **buf,
> +				   struct pstore_info *psi)
> +{
> +	ssize_t size;
> +	char *rambuf;
> +	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
> +
> +	if (cxt->read_count >= cxt->max_count)
> +		return -EINVAL;
> +	*id = cxt->read_count++;
> +	/* Only supports dmesg output so far. */
> +	*type = PSTORE_TYPE_DMESG;
> +	/* TODO(kees): Bogus time for the moment. */

Is this hard to fix now?

> +	time->tv_sec = 0;
> +	time->tv_nsec = 0;
> +
> +	rambuf = cxt->virt_addr + (*id * cxt->record_size);
> +	size = strnlen(rambuf, cxt->record_size);
> +	*buf = kmalloc(size, GFP_KERNEL);
> +	if (*buf == NULL)
> +		return -ENOMEM;
> +	memcpy(*buf, rambuf, size);
> +
> +	return size;
> +}

Note that pstore_get_records() will treat the -ve errno returns from
->read() in the same manner as EOF.  IOW, your error codes will be
dropped on the floor.  This appears to be a bug in pstore_get_records().

> +static int ramoops_pstore_write(enum pstore_type_id type,
> +				enum kmsg_dump_reason reason,
> +				u64 *id,
> +				unsigned int part,
> +				size_t size, struct pstore_info *psi)
>  {
> -	struct ramoops_context *cxt = container_of(dumper,
> -			struct ramoops_context, dump);
> -	unsigned long s1_start, s2_start;
> -	unsigned long l1_cpy, l2_cpy;
> -	int res, hdr_size;
> -	char *buf, *buf_orig;
> +	char *buf;
> +	size_t res;
>  	struct timeval timestamp;
> +	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
> +	size_t available = cxt->record_size;
>  
> +	/* Only store dmesg dumps. */
> +	if (type != PSTORE_TYPE_DMESG)
> +		return -EINVAL;
> +
> +	/* Only store crash dumps. */
>  	if (reason != KMSG_DUMP_OOPS &&
> -	    reason != KMSG_DUMP_PANIC &&
> -	    reason != KMSG_DUMP_KEXEC)
> -		return;
> +	    reason != KMSG_DUMP_PANIC)
> +		return -EINVAL;
>  
>  	/* Only dump oopses if dump_oops is set */
>  	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)

The above three comments describe what the code does, which was
obvious.  They failed to describe why it does this, which was
unobvious.  Sigh.

> -		return;
> +		return -EINVAL;
> +
> +	/* Explicitly only take the first part of any new crash.
> +	 * If our buffer is larger than kmsg_bytes, this can never happen,
> +	 * and if our buffer is smaller than kmsg_bytes, we don't want the
> +	 * report split across multiple records. */
> +	if (part != 1)
> +		return -ENOSPC;
>  
>  	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
> -	buf_orig = buf;
>  
> -	memset(buf, '\0', cxt->record_size);
>  	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>  	buf += res;
> +	available -= res;
> +
>  	do_gettimeofday(&timestamp);
>
> ...
>

Re: [PATCH v4] ramoops: use pstore interface

[Kees Cook]

On Tue, Jan 17, 2012 at 1:59 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sat, 7 Jan 2012 09:15:16 -0800
> Kees Cook <keescook@chromium.org> wrote:
>
>> Instead of using /dev/mem directly, use the common pstore infrastructure
>> to handle Oops gathering and extraction.
>
> um, why?  This changelog provides no reason for anyone to apply the
> patch!

Good point; this was lost as the patch cycled. I will expand this.

>> +     struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
>
> Unneeded and undesirable cast of void*.  Multiple instances of this.

Ah yes; I will fix this.

>> +     /* TODO(kees): Bogus time for the moment. */
>
> Is this hard to fix now?

I felt it was out of scope for the moment. There was enough change
happening for bolting it to pstore that adding a header with magic
values, etc, seemed like a logically separate task. As such, I left
this as TODO.

> Note that pstore_get_records() will treat the -ve errno returns from
> ->read() in the same manner as EOF.  IOW, your error codes will be
> dropped on the floor.  This appears to be a bug in pstore_get_records().

Well, IIUC, it just means the file doesn't get populated at all; there
is no userspace interface to finding out why a file didn't appear in
the pstore fliesystem. But yes, the specifics of the error are ignored
by pstore_get_records(). It didn't seem wrong to produce meaningful
codes in ramoops, though.

>> +     /* Only store dmesg dumps. */
>> +     if (type != PSTORE_TYPE_DMESG)
>> +             return -EINVAL;
>> +
>> +     /* Only store crash dumps. */
>>       if (reason != KMSG_DUMP_OOPS &&
>> -         reason != KMSG_DUMP_PANIC &&
>> -         reason != KMSG_DUMP_KEXEC)
>> -             return;
>> +         reason != KMSG_DUMP_PANIC)
>> +             return -EINVAL;
>>
>>       /* Only dump oopses if dump_oops is set */
>>       if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
>
> The above three comments describe what the code does, which was
> obvious.  They failed to describe why it does this, which was
> unobvious.  Sigh.

They were terse; I will attempt to expand on them.

Thanks for the review! I will send v5 shortly...

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v4] ramoops: use pstore interface

[Andrew Morton]

On Tue, 17 Jan 2012 15:48:25 -0800
Kees Cook <keescook@chromium.org> wrote:

> > Note that pstore_get_records() will treat the -ve errno returns from
> > ->read() in the same manner as EOF.  IOW, your error codes will be
> > dropped on the floor.  This appears to be a bug in pstore_get_records().
> 
> Well, IIUC, it just means the file doesn't get populated at all; there
> is no userspace interface to finding out why a file didn't appear in
> the pstore fliesystem. But yes, the specifics of the error are ignored
> by pstore_get_records(). It didn't seem wrong to produce meaningful
> codes in ramoops, though.
> 

Well, there's a printk() in there.  But it only comes out if it was
pstore_mkfile() which failed.