* [PATCH v2 3/3] pstore: add pmsg
@ 2015-01-06 17:49 Mark Salyzyn
2015-01-13 17:58 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Mark Salyzyn @ 2015-01-06 17:49 UTC (permalink / raw)
To: linux-kernel
Cc: joe, Mark Salyzyn, Anton Vorontsov, Colin Cross, Kees Cook,
Tony Luck
A secured user-space accessible pstore object. Writes
to /dev/pmsg0 are appended to the buffer, on reboot
the persistent contents are available in
/sys/fs/pstore/pmsg-ramoops-[ID].
One possible use is syslogd, or other daemon, can
write messages, then on reboot provides a means to
triage user-space activities leading up to a panic
as a companion to the pstore dmesg or console logs.
Signed-off-by: Mark Salyzyn <salyzyn@android.com>
---
fs/pstore/Kconfig | 10 +++++
fs/pstore/Makefile | 2 +
fs/pstore/inode.c | 3 ++
fs/pstore/internal.h | 6 +++
fs/pstore/platform.c | 1 +
fs/pstore/pmsg.c | 101 +++++++++++++++++++++++++++++++++++++++++++++
fs/pstore/ram.c | 46 ++++++++++++++++++---
include/linux/pstore.h | 1 +
include/linux/pstore_ram.h | 1 +
9 files changed, 166 insertions(+), 5 deletions(-)
create mode 100644 fs/pstore/pmsg.c
diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 983d951..916b8e2 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -21,6 +21,16 @@ config PSTORE_CONSOLE
When the option is enabled, pstore will log all kernel
messages, even if no oops or panic happened.
+config PSTORE_PMSG
+ bool "Log user space messages"
+ depends on PSTORE
+ help
+ When the option is enabled, pstore will export a character
+ interface /dev/pmsg0 to log user space messages. On reboot
+ data can be retrieved from /sys/fs/pstore/pmsg-ramoops-[ID].
+
+ If unsure, say N.
+
config PSTORE_FTRACE
bool "Persistent function tracer"
depends on PSTORE
diff --git a/fs/pstore/Makefile b/fs/pstore/Makefile
index 4c9095c..e647d8e 100644
--- a/fs/pstore/Makefile
+++ b/fs/pstore/Makefile
@@ -7,5 +7,7 @@ obj-y += pstore.o
pstore-objs += inode.o platform.o
obj-$(CONFIG_PSTORE_FTRACE) += ftrace.o
+obj-$(CONFIG_PSTORE_PMSG) += pmsg.o
+
ramoops-objs += ram.o ram_core.o
obj-$(CONFIG_PSTORE_RAM) += ramoops.o
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1948567..a241d3e 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -361,6 +361,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
scnprintf(name, sizeof(name), "powerpc-common-%s-%lld",
psname, id);
break;
+ case PSTORE_TYPE_PMSG:
+ scnprintf(name, sizeof(name), "pmsg-%s-%lld", psname, id);
+ break;
case PSTORE_TYPE_UNKNOWN:
scnprintf(name, sizeof(name), "unknown-%s-%lld", psname, id);
break;
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 3b3d305..c36ba2c 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -45,6 +45,12 @@ extern void pstore_register_ftrace(void);
static inline void pstore_register_ftrace(void) {}
#endif
+#ifdef CONFIG_PSTORE_PMSG
+extern void pstore_register_pmsg(void);
+#else
+static inline void pstore_register_pmsg(void) {}
+#endif
+
extern struct pstore_info *psinfo;
extern void pstore_set_kmsg_bytes(int);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 0a9b72c..15ee78c 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -447,6 +447,7 @@ int pstore_register(struct pstore_info *psi)
if ((psi->flags & PSTORE_FLAGS_FRAGILE) == 0) {
pstore_register_console();
pstore_register_ftrace();
+ pstore_register_pmsg();
}
if (pstore_update_ms >= 0) {
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
new file mode 100644
index 0000000..d50d818
--- /dev/null
+++ b/fs/pstore/pmsg.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright 2014 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/cdev.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include "internal.h"
+
+static loff_t pmsg_lseek(struct file *file, loff_t offset, int orig)
+{
+ return file->f_pos = 0;
+}
+
+static ssize_t write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ size_t i;
+
+ if (!count)
+ return 0;
+
+ if (!access_ok(VERIFY_READ, buf, count))
+ return -EFAULT;
+
+ for (i = 0; i < count; ) {
+ char buffer[512];
+ size_t c = min(count - i, sizeof(buffer));
+ u64 id;
+ long ret;
+
+ ret = __copy_from_user(buffer, buf + i, c);
+ if (ret != 0)
+ return -EFAULT;
+ psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
+ psinfo);
+
+ i += c;
+ }
+ return count;
+}
+
+static const struct file_operations pmsg_fops = {
+ .owner = THIS_MODULE,
+ .llseek = pmsg_lseek,
+ .write = write_pmsg,
+};
+
+static struct class *pmsg_class;
+static int pmsg_major;
+#define PMSG_NAME "pmsg"
+
+static char *pmsg_devnode(struct device *dev, umode_t *mode)
+{
+ if (mode)
+ *mode = 0220;
+ return NULL;
+}
+
+void pstore_register_pmsg(void)
+{
+ struct device *pmsg_device;
+
+ pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
+ if (pmsg_major < 0) {
+ pr_err("register_chrdev failed for pmsg\n");
+ goto err;
+ }
+
+ pmsg_class = class_create(THIS_MODULE, PMSG_NAME);
+ if (IS_ERR(pmsg_class)) {
+ pr_err("pmsg: device class file already in use\n");
+ goto err_class;
+ }
+ pmsg_class->devnode = pmsg_devnode;
+
+ pmsg_device = device_create(pmsg_class, NULL, MKDEV(pmsg_major, 0),
+ NULL, "%s%d", PMSG_NAME, 0);
+ if (IS_ERR(pmsg_device)) {
+ pr_err("pmsg: failed to create device\n");
+ goto err_device;
+ }
+ return;
+
+err_device:
+ class_destroy(pmsg_class);
+err_class:
+ unregister_chrdev(pmsg_major, PMSG_NAME);
+err:
+ return;
+}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 34ed8f8..39d1373 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -51,6 +51,10 @@ static ulong ramoops_ftrace_size = MIN_MEM_SIZE;
module_param_named(ftrace_size, ramoops_ftrace_size, ulong, 0400);
MODULE_PARM_DESC(ftrace_size, "size of ftrace log");
+static ulong ramoops_pmsg_size = MIN_MEM_SIZE;
+module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
+MODULE_PARM_DESC(pmsg_size, "size of user space message log");
+
static ulong mem_address;
module_param(mem_address, ulong, 0400);
MODULE_PARM_DESC(mem_address,
@@ -82,12 +86,14 @@ struct ramoops_context {
struct persistent_ram_zone **przs;
struct persistent_ram_zone *cprz;
struct persistent_ram_zone *fprz;
+ struct persistent_ram_zone *mprz;
phys_addr_t phys_addr;
unsigned long size;
unsigned int memtype;
size_t record_size;
size_t console_size;
size_t ftrace_size;
+ size_t pmsg_size;
int dump_oops;
struct persistent_ram_ecc_info ecc_info;
unsigned int max_dump_cnt;
@@ -96,6 +102,7 @@ struct ramoops_context {
unsigned int dump_read_cnt;
unsigned int console_read_cnt;
unsigned int ftrace_read_cnt;
+ unsigned int pmsg_read_cnt;
struct pstore_info pstore;
};
@@ -109,6 +116,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
cxt->dump_read_cnt = 0;
cxt->console_read_cnt = 0;
cxt->ftrace_read_cnt = 0;
+ cxt->pmsg_read_cnt = 0;
return 0;
}
@@ -164,6 +172,12 @@ static int ramoops_read_kmsg_hdr(char *buffer, struct timespec *time,
return header_length;
}
+static bool prz_ok(struct persistent_ram_zone *prz)
+{
+ return !!prz && !!(persistent_ram_old_size(prz) +
+ persistent_ram_ecc_string(prz, NULL, 0));
+}
+
static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
int *count, struct timespec *time,
char **buf, bool *compressed,
@@ -178,13 +192,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
prz = ramoops_get_next_prz(cxt->przs, &cxt->dump_read_cnt,
cxt->max_dump_cnt, id, type,
PSTORE_TYPE_DMESG, 1);
- if (!prz)
+ if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
1, id, type, PSTORE_TYPE_CONSOLE, 0);
- if (!prz)
+ if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->fprz, &cxt->ftrace_read_cnt,
1, id, type, PSTORE_TYPE_FTRACE, 0);
- if (!prz)
+ if (!prz_ok(prz))
+ prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
+ 1, id, type, PSTORE_TYPE_PMSG, 0);
+ if (!prz_ok(prz))
return 0;
if (!persistent_ram_old(prz))
@@ -252,6 +269,11 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
return -ENOMEM;
persistent_ram_write(cxt->fprz, buf, size);
return 0;
+ } else if (type == PSTORE_TYPE_PMSG) {
+ if (!cxt->mprz)
+ return -ENOMEM;
+ persistent_ram_write(cxt->mprz, buf, size);
+ return 0;
}
if (type != PSTORE_TYPE_DMESG)
@@ -309,6 +331,9 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
case PSTORE_TYPE_FTRACE:
prz = cxt->fprz;
break;
+ case PSTORE_TYPE_PMSG:
+ prz = cxt->mprz;
+ break;
default:
return -EINVAL;
}
@@ -435,7 +460,7 @@ static int ramoops_probe(struct platform_device *pdev)
goto fail_out;
if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
- !pdata->ftrace_size)) {
+ !pdata->ftrace_size && !pdata->pmsg_size)) {
pr_err("The memory size and the record/console size must be "
"non-zero\n");
goto fail_out;
@@ -447,6 +472,8 @@ static int ramoops_probe(struct platform_device *pdev)
pdata->console_size = rounddown_pow_of_two(pdata->console_size);
if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
+ if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
+ pdata->pmsg_size = rounddown_pow_of_two(pdata->pmsg_size);
cxt->size = pdata->mem_size;
cxt->phys_addr = pdata->mem_address;
@@ -454,12 +481,14 @@ static int ramoops_probe(struct platform_device *pdev)
cxt->record_size = pdata->record_size;
cxt->console_size = pdata->console_size;
cxt->ftrace_size = pdata->ftrace_size;
+ cxt->pmsg_size = pdata->pmsg_size;
cxt->dump_oops = pdata->dump_oops;
cxt->ecc_info = pdata->ecc_info;
paddr = cxt->phys_addr;
- dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size;
+ dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
+ - cxt->pmsg_size;
err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
if (err)
goto fail_out;
@@ -474,6 +503,10 @@ static int ramoops_probe(struct platform_device *pdev)
if (err)
goto fail_init_fprz;
+ err = ramoops_init_prz(dev, cxt, &cxt->mprz, &paddr, cxt->pmsg_size, 0);
+ if (err)
+ goto fail_init_mprz;
+
cxt->pstore.data = cxt;
/*
* Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
@@ -517,6 +550,8 @@ fail_buf:
kfree(cxt->pstore.buf);
fail_clear:
cxt->pstore.bufsize = 0;
+ kfree(cxt->mprz);
+fail_init_mprz:
kfree(cxt->fprz);
fail_init_fprz:
kfree(cxt->cprz);
@@ -574,6 +609,7 @@ static void ramoops_register_dummy(void)
dummy_data->record_size = record_size;
dummy_data->console_size = ramoops_console_size;
dummy_data->ftrace_size = ramoops_ftrace_size;
+ dummy_data->pmsg_size = ramoops_pmsg_size;
dummy_data->dump_oops = dump_oops;
/*
* For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index ece0c6b..8884f6e 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -39,6 +39,7 @@ enum pstore_type_id {
PSTORE_TYPE_PPC_RTAS = 4,
PSTORE_TYPE_PPC_OF = 5,
PSTORE_TYPE_PPC_COMMON = 6,
+ PSTORE_TYPE_PMSG = 7,
PSTORE_TYPE_UNKNOWN = 255
};
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 4af3fdc..9c9d6c1 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -81,6 +81,7 @@ struct ramoops_platform_data {
unsigned long record_size;
unsigned long console_size;
unsigned long ftrace_size;
+ unsigned long pmsg_size;
int dump_oops;
struct persistent_ram_ecc_info ecc_info;
};
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 3/3] pstore: add pmsg
2015-01-06 17:49 [PATCH v2 3/3] pstore: add pmsg Mark Salyzyn
@ 2015-01-13 17:58 ` Kees Cook
2015-01-13 18:54 ` Mark Salyzyn
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2015-01-13 17:58 UTC (permalink / raw)
To: Mark Salyzyn; +Cc: LKML, Joe Perches, Anton Vorontsov, Colin Cross, Tony Luck
On Tue, Jan 6, 2015 at 9:49 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> A secured user-space accessible pstore object. Writes
> to /dev/pmsg0 are appended to the buffer, on reboot
> the persistent contents are available in
> /sys/fs/pstore/pmsg-ramoops-[ID].
>
> One possible use is syslogd, or other daemon, can
> write messages, then on reboot provides a means to
> triage user-space activities leading up to a panic
> as a companion to the pstore dmesg or console logs.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> ---
> fs/pstore/Kconfig | 10 +++++
> fs/pstore/Makefile | 2 +
> fs/pstore/inode.c | 3 ++
> fs/pstore/internal.h | 6 +++
> fs/pstore/platform.c | 1 +
> fs/pstore/pmsg.c | 101 +++++++++++++++++++++++++++++++++++++++++++++
> fs/pstore/ram.c | 46 ++++++++++++++++++---
> include/linux/pstore.h | 1 +
> include/linux/pstore_ram.h | 1 +
> 9 files changed, 166 insertions(+), 5 deletions(-)
> create mode 100644 fs/pstore/pmsg.c
>
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index 983d951..916b8e2 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -21,6 +21,16 @@ config PSTORE_CONSOLE
> When the option is enabled, pstore will log all kernel
> messages, even if no oops or panic happened.
>
> +config PSTORE_PMSG
> + bool "Log user space messages"
> + depends on PSTORE
> + help
> + When the option is enabled, pstore will export a character
> + interface /dev/pmsg0 to log user space messages. On reboot
> + data can be retrieved from /sys/fs/pstore/pmsg-ramoops-[ID].
> +
> + If unsure, say N.
> +
> config PSTORE_FTRACE
> bool "Persistent function tracer"
> depends on PSTORE
> diff --git a/fs/pstore/Makefile b/fs/pstore/Makefile
> index 4c9095c..e647d8e 100644
> --- a/fs/pstore/Makefile
> +++ b/fs/pstore/Makefile
> @@ -7,5 +7,7 @@ obj-y += pstore.o
> pstore-objs += inode.o platform.o
> obj-$(CONFIG_PSTORE_FTRACE) += ftrace.o
>
> +obj-$(CONFIG_PSTORE_PMSG) += pmsg.o
> +
> ramoops-objs += ram.o ram_core.o
> obj-$(CONFIG_PSTORE_RAM) += ramoops.o
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 1948567..a241d3e 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -361,6 +361,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
> scnprintf(name, sizeof(name), "powerpc-common-%s-%lld",
> psname, id);
> break;
> + case PSTORE_TYPE_PMSG:
> + scnprintf(name, sizeof(name), "pmsg-%s-%lld", psname, id);
> + break;
> case PSTORE_TYPE_UNKNOWN:
> scnprintf(name, sizeof(name), "unknown-%s-%lld", psname, id);
> break;
> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> index 3b3d305..c36ba2c 100644
> --- a/fs/pstore/internal.h
> +++ b/fs/pstore/internal.h
> @@ -45,6 +45,12 @@ extern void pstore_register_ftrace(void);
> static inline void pstore_register_ftrace(void) {}
> #endif
>
> +#ifdef CONFIG_PSTORE_PMSG
> +extern void pstore_register_pmsg(void);
> +#else
> +static inline void pstore_register_pmsg(void) {}
> +#endif
> +
> extern struct pstore_info *psinfo;
>
> extern void pstore_set_kmsg_bytes(int);
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 0a9b72c..15ee78c 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -447,6 +447,7 @@ int pstore_register(struct pstore_info *psi)
> if ((psi->flags & PSTORE_FLAGS_FRAGILE) == 0) {
> pstore_register_console();
> pstore_register_ftrace();
> + pstore_register_pmsg();
> }
>
> if (pstore_update_ms >= 0) {
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> new file mode 100644
> index 0000000..d50d818
> --- /dev/null
> +++ b/fs/pstore/pmsg.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright 2014 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/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include "internal.h"
> +
> +static loff_t pmsg_lseek(struct file *file, loff_t offset, int orig)
> +{
> + return file->f_pos = 0;
> +}
> +
> +static ssize_t write_pmsg(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + size_t i;
> +
> + if (!count)
> + return 0;
> +
> + if (!access_ok(VERIFY_READ, buf, count))
> + return -EFAULT;
> +
> + for (i = 0; i < count; ) {
> + char buffer[512];
This feels like a lot of stack space to use for a buffer. Would it
maybe be better to either reduce the copy size or use a larger kmalloc
or vmalloc buffer to act as the bounce buffer to pass to write_buf?
> + size_t c = min(count - i, sizeof(buffer));
> + u64 id;
> + long ret;
> +
> + ret = __copy_from_user(buffer, buf + i, c);
> + if (ret != 0)
> + return -EFAULT;
> + psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
> + psinfo);
> +
> + i += c;
> + }
> + return count;
> +}
> +
> +static const struct file_operations pmsg_fops = {
> + .owner = THIS_MODULE,
> + .llseek = pmsg_lseek,
> + .write = write_pmsg,
> +};
> +
> +static struct class *pmsg_class;
> +static int pmsg_major;
> +#define PMSG_NAME "pmsg"
> +
> +static char *pmsg_devnode(struct device *dev, umode_t *mode)
> +{
> + if (mode)
> + *mode = 0220;
> + return NULL;
> +}
> +
> +void pstore_register_pmsg(void)
> +{
> + struct device *pmsg_device;
> +
> + pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
> + if (pmsg_major < 0) {
> + pr_err("register_chrdev failed for pmsg\n");
> + goto err;
> + }
> +
> + pmsg_class = class_create(THIS_MODULE, PMSG_NAME);
> + if (IS_ERR(pmsg_class)) {
> + pr_err("pmsg: device class file already in use\n");
> + goto err_class;
> + }
> + pmsg_class->devnode = pmsg_devnode;
> +
> + pmsg_device = device_create(pmsg_class, NULL, MKDEV(pmsg_major, 0),
> + NULL, "%s%d", PMSG_NAME, 0);
> + if (IS_ERR(pmsg_device)) {
> + pr_err("pmsg: failed to create device\n");
Instead of mentioning pmsg directly here and in the pr_err()s above,
maybe set it via the pr_* defines:
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
And maybe use KBUILD_MODNAME instead of PMSG_NAME or vice versa?
> + goto err_device;
> + }
> + return;
> +
> +err_device:
> + class_destroy(pmsg_class);
> +err_class:
> + unregister_chrdev(pmsg_major, PMSG_NAME);
> +err:
> + return;
> +}
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 34ed8f8..39d1373 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -51,6 +51,10 @@ static ulong ramoops_ftrace_size = MIN_MEM_SIZE;
> module_param_named(ftrace_size, ramoops_ftrace_size, ulong, 0400);
> MODULE_PARM_DESC(ftrace_size, "size of ftrace log");
>
> +static ulong ramoops_pmsg_size = MIN_MEM_SIZE;
> +module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
> +MODULE_PARM_DESC(pmsg_size, "size of user space message log");
> +
> static ulong mem_address;
> module_param(mem_address, ulong, 0400);
> MODULE_PARM_DESC(mem_address,
> @@ -82,12 +86,14 @@ struct ramoops_context {
> struct persistent_ram_zone **przs;
> struct persistent_ram_zone *cprz;
> struct persistent_ram_zone *fprz;
> + struct persistent_ram_zone *mprz;
> phys_addr_t phys_addr;
> unsigned long size;
> unsigned int memtype;
> size_t record_size;
> size_t console_size;
> size_t ftrace_size;
> + size_t pmsg_size;
> int dump_oops;
> struct persistent_ram_ecc_info ecc_info;
> unsigned int max_dump_cnt;
> @@ -96,6 +102,7 @@ struct ramoops_context {
> unsigned int dump_read_cnt;
> unsigned int console_read_cnt;
> unsigned int ftrace_read_cnt;
> + unsigned int pmsg_read_cnt;
> struct pstore_info pstore;
> };
>
> @@ -109,6 +116,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> cxt->dump_read_cnt = 0;
> cxt->console_read_cnt = 0;
> cxt->ftrace_read_cnt = 0;
> + cxt->pmsg_read_cnt = 0;
> return 0;
> }
>
> @@ -164,6 +172,12 @@ static int ramoops_read_kmsg_hdr(char *buffer, struct timespec *time,
> return header_length;
> }
>
> +static bool prz_ok(struct persistent_ram_zone *prz)
> +{
> + return !!prz && !!(persistent_ram_old_size(prz) +
> + persistent_ram_ecc_string(prz, NULL, 0));
> +}
The addition of prz_ok() seems like a separate change?
> +
> static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
> int *count, struct timespec *time,
> char **buf, bool *compressed,
> @@ -178,13 +192,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
> prz = ramoops_get_next_prz(cxt->przs, &cxt->dump_read_cnt,
> cxt->max_dump_cnt, id, type,
> PSTORE_TYPE_DMESG, 1);
> - if (!prz)
> + if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> 1, id, type, PSTORE_TYPE_CONSOLE, 0);
> - if (!prz)
> + if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->fprz, &cxt->ftrace_read_cnt,
> 1, id, type, PSTORE_TYPE_FTRACE, 0);
> - if (!prz)
> + if (!prz_ok(prz))
> + prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> + 1, id, type, PSTORE_TYPE_PMSG, 0);
> + if (!prz_ok(prz))
> return 0;
>
> if (!persistent_ram_old(prz))
> @@ -252,6 +269,11 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
> return -ENOMEM;
> persistent_ram_write(cxt->fprz, buf, size);
> return 0;
> + } else if (type == PSTORE_TYPE_PMSG) {
> + if (!cxt->mprz)
> + return -ENOMEM;
> + persistent_ram_write(cxt->mprz, buf, size);
> + return 0;
> }
>
> if (type != PSTORE_TYPE_DMESG)
> @@ -309,6 +331,9 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
> case PSTORE_TYPE_FTRACE:
> prz = cxt->fprz;
> break;
> + case PSTORE_TYPE_PMSG:
> + prz = cxt->mprz;
> + break;
> default:
> return -EINVAL;
> }
> @@ -435,7 +460,7 @@ static int ramoops_probe(struct platform_device *pdev)
> goto fail_out;
>
> if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
> - !pdata->ftrace_size)) {
> + !pdata->ftrace_size && !pdata->pmsg_size)) {
> pr_err("The memory size and the record/console size must be "
> "non-zero\n");
> goto fail_out;
> @@ -447,6 +472,8 @@ static int ramoops_probe(struct platform_device *pdev)
> pdata->console_size = rounddown_pow_of_two(pdata->console_size);
> if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
> pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
> + if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
> + pdata->pmsg_size = rounddown_pow_of_two(pdata->pmsg_size);
>
> cxt->size = pdata->mem_size;
> cxt->phys_addr = pdata->mem_address;
> @@ -454,12 +481,14 @@ static int ramoops_probe(struct platform_device *pdev)
> cxt->record_size = pdata->record_size;
> cxt->console_size = pdata->console_size;
> cxt->ftrace_size = pdata->ftrace_size;
> + cxt->pmsg_size = pdata->pmsg_size;
> cxt->dump_oops = pdata->dump_oops;
> cxt->ecc_info = pdata->ecc_info;
>
> paddr = cxt->phys_addr;
>
> - dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size;
> + dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> + - cxt->pmsg_size;
> err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
> if (err)
> goto fail_out;
> @@ -474,6 +503,10 @@ static int ramoops_probe(struct platform_device *pdev)
> if (err)
> goto fail_init_fprz;
>
> + err = ramoops_init_prz(dev, cxt, &cxt->mprz, &paddr, cxt->pmsg_size, 0);
> + if (err)
> + goto fail_init_mprz;
> +
> cxt->pstore.data = cxt;
> /*
> * Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
> @@ -517,6 +550,8 @@ fail_buf:
> kfree(cxt->pstore.buf);
> fail_clear:
> cxt->pstore.bufsize = 0;
> + kfree(cxt->mprz);
> +fail_init_mprz:
> kfree(cxt->fprz);
> fail_init_fprz:
> kfree(cxt->cprz);
> @@ -574,6 +609,7 @@ static void ramoops_register_dummy(void)
> dummy_data->record_size = record_size;
> dummy_data->console_size = ramoops_console_size;
> dummy_data->ftrace_size = ramoops_ftrace_size;
> + dummy_data->pmsg_size = ramoops_pmsg_size;
> dummy_data->dump_oops = dump_oops;
> /*
> * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index ece0c6b..8884f6e 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -39,6 +39,7 @@ enum pstore_type_id {
> PSTORE_TYPE_PPC_RTAS = 4,
> PSTORE_TYPE_PPC_OF = 5,
> PSTORE_TYPE_PPC_COMMON = 6,
> + PSTORE_TYPE_PMSG = 7,
> PSTORE_TYPE_UNKNOWN = 255
> };
>
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 4af3fdc..9c9d6c1 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -81,6 +81,7 @@ struct ramoops_platform_data {
> unsigned long record_size;
> unsigned long console_size;
> unsigned long ftrace_size;
> + unsigned long pmsg_size;
> int dump_oops;
> struct persistent_ram_ecc_info ecc_info;
> };
> --
> 2.2.0.rc0.207.ga3a616c
>
Some nits above. Overall, this seems like a fine idea. Would it make
sense to enforce a single opener for these writes to avoid
interleaving?
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 3/3] pstore: add pmsg
2015-01-13 17:58 ` Kees Cook
@ 2015-01-13 18:54 ` Mark Salyzyn
0 siblings, 0 replies; 3+ messages in thread
From: Mark Salyzyn @ 2015-01-13 18:54 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, Joe Perches, Anton Vorontsov, Colin Cross, Tony Luck
On 01/13/2015 09:58 AM, Kees Cook wrote:
> On Tue, Jan 6, 2015 at 9:49 AM, Mark Salyzyn <salyzyn@android.com> wrote:
>> A secured user-space accessible pstore object. Writes
>> to /dev/pmsg0 are appended to the buffer, on reboot
>> the persistent contents are available in
>> /sys/fs/pstore/pmsg-ramoops-[ID].
>>
>> One possible use is syslogd, or other daemon, can
>> write messages, then on reboot provides a means to
>> triage user-space activities leading up to a panic
>> as a companion to the pstore dmesg or console logs.
>>
>> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
>> ---
>> fs/pstore/Kconfig | 10 +++++
>> fs/pstore/Makefile | 2 +
>> fs/pstore/inode.c | 3 ++
>> fs/pstore/internal.h | 6 +++
>> fs/pstore/platform.c | 1 +
>> fs/pstore/pmsg.c | 101 +++++++++++++++++++++++++++++++++++++++++++++
>> fs/pstore/ram.c | 46 ++++++++++++++++++---
>> include/linux/pstore.h | 1 +
>> include/linux/pstore_ram.h | 1 +
>> 9 files changed, 166 insertions(+), 5 deletions(-)
>> create mode 100644 fs/pstore/pmsg.c
>>
. . .
>> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
>> new file mode 100644
>> index 0000000..d50d818
>> --- /dev/null
>> +++ b/fs/pstore/pmsg.c
. . .
>> +static ssize_t write_pmsg(struct file *file, const char __user *buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + size_t i;
>> +
>> + if (!count)
>> + return 0;
>> +
>> + if (!access_ok(VERIFY_READ, buf, count))
>> + return -EFAULT;
>> +
>> + for (i = 0; i < count; ) {
>> + char buffer[512];
> This feels like a lot of stack space to use for a buffer. Would it
> maybe be better to either reduce the copy size or use a larger kmalloc
> or vmalloc buffer to act as the bounce buffer to pass to write_buf?
I am taking a page(sic) from kernel/printk/printk.c and countless
drivers that have settled on 512 byte on-stack bounce buffers as an
acceptable median. I will test vmalloc though since it would offer us
the promise of a more atomic operation (see below).
. . .
>> + if (IS_ERR(pmsg_device)) {
>> + pr_err("pmsg: failed to create device\n");
> Instead of mentioning pmsg directly here and in the pr_err()s above,
> maybe set it via the pr_* defines:
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> And maybe use KBUILD_MODNAME instead of PMSG_NAME or vice versa?
Thanks :-)
> +static bool prz_ok(struct persistent_ram_zone *prz)
> +{
> + return !!prz && !!(persistent_ram_old_size(prz) +
> + persistent_ram_ecc_string(prz, NULL, 0));
> +}
> The addition of prz_ok() seems like a separate change?
Will split out.
. . .
> Some nits above. Overall, this seems like a fine idea. Would it make
> sense to enforce a single opener for these writes to avoid
> interleaving?
I expect logging would favour lowest overhead. A reduced syscall
approach would be for multiple writers to hold on to an open file
descriptor. We can avoid the problem by switching to spinlock of the
write handler to guarantee atomic. This makes a switch from stack bounce
buffer to vmalloc advised.
>
> -Kees
Thanks. Expect a respin of the entire pstore pmsg patch-set story after
testing.
Sincerely -- Mark Salyzyn
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-01-13 18:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-06 17:49 [PATCH v2 3/3] pstore: add pmsg Mark Salyzyn
2015-01-13 17:58 ` Kees Cook
2015-01-13 18:54 ` Mark Salyzyn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox