* [RFC 1/6] pstore: map pstore types to names
@ 2018-10-26 18:00 Joel Fernandes (Google)
2018-10-26 18:00 ` [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz Joel Fernandes (Google)
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2018-10-26 18:00 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Joel Fernandes (Google),
Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
In later patches we will need to map types to names, so create a table
for that which can also be used and reused in different parts of old and
new code. Also use it to save the type in the PRZ which will be useful
in later patches.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/pstore/inode.c | 44 ++++++++++++++++++++------------------
fs/pstore/ram.c | 4 +++-
include/linux/pstore.h | 29 +++++++++++++++++++++++++
include/linux/pstore_ram.h | 2 ++
4 files changed, 57 insertions(+), 22 deletions(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 5fcb845b9fec..43757049d384 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -304,6 +304,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
struct dentry *dentry;
struct inode *inode;
int rc = 0;
+ enum pstore_type_id type;
char name[PSTORE_NAMELEN];
struct pstore_private *private, *pos;
unsigned long flags;
@@ -335,43 +336,44 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
goto fail_alloc;
private->record = record;
- switch (record->type) {
+ type = record->type;
+ switch (type) {
case PSTORE_TYPE_DMESG:
- scnprintf(name, sizeof(name), "dmesg-%s-%llu%s",
- record->psi->name, record->id,
- record->compressed ? ".enc.z" : "");
+ scnprintf(name, sizeof(name), "%s-%s-%llu%s",
+ pstore_names[type], record->psi->name, record->id,
+ record->compressed ? ".enc.z" : "");
break;
case PSTORE_TYPE_CONSOLE:
- scnprintf(name, sizeof(name), "console-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_FTRACE:
- scnprintf(name, sizeof(name), "ftrace-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_MCE:
- scnprintf(name, sizeof(name), "mce-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_RTAS:
- scnprintf(name, sizeof(name), "rtas-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_OF:
- scnprintf(name, sizeof(name), "powerpc-ofw-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_COMMON:
- scnprintf(name, sizeof(name), "powerpc-common-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_PMSG:
- scnprintf(name, sizeof(name), "pmsg-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_OPAL:
- scnprintf(name, sizeof(name), "powerpc-opal-%s-%llu",
- record->psi->name, record->id);
+ scnprintf(name, sizeof(name), "%s-%s-%llu",
+ pstore_names[type], record->psi->name, record->id);
break;
case PSTORE_TYPE_UNKNOWN:
scnprintf(name, sizeof(name), "unknown-%s-%llu",
@@ -379,7 +381,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
break;
default:
scnprintf(name, sizeof(name), "type%d-%s-%llu",
- record->type, record->psi->name, record->id);
+ type, record->psi->name, record->id);
break;
}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index f4fd2e72add4..c7cd858adce7 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,6 +604,7 @@ static int ramoops_init_przs(const char *name,
goto fail;
}
*paddr += zone_sz;
+ prz_ar[i]->type = pstore_name_to_type(name);
}
*przs = prz_ar;
@@ -642,6 +643,7 @@ static int ramoops_init_prz(const char *name,
persistent_ram_zap(*prz);
*paddr += sz;
+ (*prz)->type = pstore_name_to_type(name);
return 0;
}
@@ -777,7 +779,7 @@ static int ramoops_probe(struct platform_device *pdev)
dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
- cxt->pmsg_size;
- err = ramoops_init_przs("dump", dev, cxt, &cxt->dprzs, &paddr,
+ err = ramoops_init_przs("dmesg", dev, cxt, &cxt->dprzs, &paddr,
dump_mem_sz, cxt->record_size,
&cxt->max_dump_cnt, 0, 0);
if (err)
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index a15bc4d48752..4a3dbdffd8d3 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -47,6 +47,21 @@ enum pstore_type_id {
PSTORE_TYPE_UNKNOWN = 255
};
+/* names should be in the same order as the above table */
+static char __maybe_unused *pstore_names[] = {
+ "dmesg",
+ "mce",
+ "console",
+ "ftrace",
+ "rtas",
+ "powerpc-ofw",
+ "powerpc-common",
+ "pmsg",
+ "powerpc-opal",
+ "event",
+ "unknown", /* unknown should be the last string */
+};
+
struct pstore_info;
/**
* struct pstore_record - details of a pstore record entry
@@ -274,4 +289,18 @@ pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val)
}
#endif
+static inline enum pstore_type_id pstore_name_to_type(const char *name)
+{
+ char *str;
+ int i = 0;
+
+ for (; strncmp(pstore_names[i], "unknown", 7); i++) {
+ str = pstore_names[i];
+ if (!strncmp(name, str, strlen(str)))
+ return (enum pstore_type_id)i;
+ }
+
+ return PSTORE_TYPE_UNKNOWN;
+}
+
#endif /*_LINUX_PSTORE_H*/
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index e6d226464838..ee0f493254dd 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -22,6 +22,7 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
+#include <linux/pstore.h>
#include <linux/types.h>
/*
@@ -47,6 +48,7 @@ struct persistent_ram_zone {
size_t size;
void *vaddr;
struct persistent_ram_buffer *buffer;
+ enum pstore_type_id type;
size_t buffer_size;
u32 flags;
raw_spinlock_t buffer_lock;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
@ 2018-10-26 18:00 ` Joel Fernandes (Google)
2018-10-26 19:05 ` Kees Cook
2018-10-26 18:00 ` [RFC 3/6] pstore: remove max " Joel Fernandes (Google)
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes (Google) @ 2018-10-26 18:00 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Joel Fernandes (Google),
Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Since we store the type of the prz when we initialize it, we no longer
need to pass it again in ramoops_get_next_prz since we can just use that
to setup the pstore record. So lets remove it from the argument list.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/pstore/ram.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index c7cd858adce7..cbfdf4b8e89d 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -125,9 +125,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
static struct persistent_ram_zone *
ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
- u64 *id,
- enum pstore_type_id *typep, enum pstore_type_id type,
- bool update)
+ u64 *id, enum pstore_type_id *typep, bool update)
{
struct persistent_ram_zone *prz;
int i = (*c)++;
@@ -147,7 +145,7 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
if (!persistent_ram_old_size(prz))
return NULL;
- *typep = type;
+ *typep = prz->type;
*id = i;
return prz;
@@ -257,8 +255,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
cxt->max_dump_cnt, &record->id,
- &record->type,
- PSTORE_TYPE_DMESG, 1);
+ &record->type, 1);
if (!prz_ok(prz))
continue;
header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
@@ -274,20 +271,18 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
- 1, &record->id, &record->type,
- PSTORE_TYPE_CONSOLE, 0);
+ 1, &record->id, &record->type, 0);
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
- 1, &record->id, &record->type,
- PSTORE_TYPE_PMSG, 0);
+ 1, &record->id, &record->type, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
&cxt->ftrace_read_cnt, 1, &record->id,
- &record->type, PSTORE_TYPE_FTRACE, 0);
+ &record->type, 0);
} else {
/*
* Build a new dummy record which combines all the
@@ -306,8 +301,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
&cxt->ftrace_read_cnt,
cxt->max_ftrace_cnt,
&record->id,
- &record->type,
- PSTORE_TYPE_FTRACE, 0);
+ &record->type, 0);
if (!prz_ok(prz_next))
continue;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
2018-10-26 18:00 ` [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz Joel Fernandes (Google)
@ 2018-10-26 18:00 ` Joel Fernandes (Google)
2018-10-26 19:22 ` Joel Fernandes
2018-10-26 19:22 ` Kees Cook
2018-10-26 18:00 ` [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record Joel Fernandes (Google)
` (3 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2018-10-26 18:00 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Joel Fernandes (Google),
Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
From the code flow, the 'max' checks are already being done on the prz
passed to ramoops_get_next_prz. Lets remove it to simplify this function
and reduce its arguments.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/pstore/ram.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index cbfdf4b8e89d..3055e05acab1 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
}
static struct persistent_ram_zone *
-ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
+ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
u64 *id, enum pstore_type_id *typep, bool update)
{
struct persistent_ram_zone *prz;
int i = (*c)++;
/* Give up if we never existed or have hit the end. */
- if (!przs || i >= max)
+ if (!przs)
return NULL;
prz = przs[i];
@@ -254,8 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
/* Find the next valid persistent_ram_zone for DMESG */
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
- cxt->max_dump_cnt, &record->id,
- &record->type, 1);
+ &record->id, &record->type, 1);
if (!prz_ok(prz))
continue;
header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
@@ -271,17 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
- 1, &record->id, &record->type, 0);
+ &record->id, &record->type, 0);
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
- 1, &record->id, &record->type, 0);
+ &record->id, &record->type, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
- &cxt->ftrace_read_cnt, 1, &record->id,
+ &cxt->ftrace_read_cnt, &record->id,
&record->type, 0);
} else {
/*
@@ -299,7 +298,6 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
prz_next = ramoops_get_next_prz(cxt->fprzs,
&cxt->ftrace_read_cnt,
- cxt->max_ftrace_cnt,
&record->id,
&record->type, 0);
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
2018-10-26 18:00 ` [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz Joel Fernandes (Google)
2018-10-26 18:00 ` [RFC 3/6] pstore: remove max " Joel Fernandes (Google)
@ 2018-10-26 18:00 ` Joel Fernandes (Google)
2018-10-26 19:32 ` Kees Cook
2018-10-26 18:00 ` [RFC 5/6] pstore: donot treat empty buffers as valid Joel Fernandes (Google)
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes (Google) @ 2018-10-26 18:00 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Joel Fernandes (Google),
Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Both the id and type fields of a pstore_record are set by
ramoops_get_next_prz. So we can just pass a pointer to the pstore_record
instead of passing individual elements. This results in cleaner more
readable code and fewer lines.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/pstore/ram.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 3055e05acab1..710c3d30bac0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -125,7 +125,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
static struct persistent_ram_zone *
ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
- u64 *id, enum pstore_type_id *typep, bool update)
+ struct pstore_record *record, bool update)
{
struct persistent_ram_zone *prz;
int i = (*c)++;
@@ -145,8 +145,8 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
if (!persistent_ram_old_size(prz))
return NULL;
- *typep = prz->type;
- *id = i;
+ record->type = prz->type;
+ record->id = i;
return prz;
}
@@ -254,7 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
/* Find the next valid persistent_ram_zone for DMESG */
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
- &record->id, &record->type, 1);
+ record, 1);
if (!prz_ok(prz))
continue;
header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
@@ -270,18 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
- &record->id, &record->type, 0);
+ record, 0);
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
- &record->id, &record->type, 0);
+ record, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
- &cxt->ftrace_read_cnt, &record->id,
- &record->type, 0);
+ &cxt->ftrace_read_cnt, record, 0);
} else {
/*
* Build a new dummy record which combines all the
@@ -298,8 +297,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
prz_next = ramoops_get_next_prz(cxt->fprzs,
&cxt->ftrace_read_cnt,
- &record->id,
- &record->type, 0);
+ record, 0);
if (!prz_ok(prz_next))
continue;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 5/6] pstore: donot treat empty buffers as valid
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
` (2 preceding siblings ...)
2018-10-26 18:00 ` [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record Joel Fernandes (Google)
@ 2018-10-26 18:00 ` Joel Fernandes (Google)
2018-10-26 19:39 ` Kees Cook
2018-10-26 18:00 ` [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size" Joel Fernandes (Google)
2018-10-26 19:04 ` [RFC 1/6] pstore: map pstore types to names Kees Cook
5 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes (Google) @ 2018-10-26 18:00 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Joel Fernandes (Google),
Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
pstore currently calls persistent_ram_save_old even if a buffer is
empty. While this appears to work, it is simply not the right thing to
do and could lead to bugs so lets avoid that. It also prevent misleading
prints in the logs which claim the buffer is valid.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/pstore/ram_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0792595ebcfb..1299aa3ea734 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -495,7 +495,7 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
sig ^= PERSISTENT_RAM_SIG;
- if (prz->buffer->sig == sig) {
+ if (prz->buffer->sig == sig && buffer_size(prz)) {
if (buffer_size(prz) > prz->buffer_size ||
buffer_start(prz) > buffer_size(prz))
pr_info("found existing invalid buffer, size %zu, start %zu\n",
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
` (3 preceding siblings ...)
2018-10-26 18:00 ` [RFC 5/6] pstore: donot treat empty buffers as valid Joel Fernandes (Google)
@ 2018-10-26 18:00 ` Joel Fernandes (Google)
2018-10-26 18:16 ` Kees Cook
2018-10-26 19:04 ` [RFC 1/6] pstore: map pstore types to names Kees Cook
5 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes (Google) @ 2018-10-26 18:00 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Joel Fernandes (Google),
Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
Due to the commit which is being reverted here, it is not possible to
know if pstore's messages were from a previous boot, or from very old
boots. This creates an awkard situation where its unclear if crash or
other logs are from the previous boot or from very old boots. Also
typically we dump the pstore buffers after one reboot and are interested
in only the previous boot's crash so let us reset the buffer after we
save them.
Lastly, if we don't zap them, then I think it is possible that part of
the buffer will be from this boot and the other parts will be from
previous boots. So this revert fixes all of this by calling
persistent_ram_zap always.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/pstore/ram_core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 1299aa3ea734..67d74dd97da1 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -504,7 +504,6 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
pr_debug("found existing buffer, size %zu, start %zu\n",
buffer_size(prz), buffer_start(prz));
persistent_ram_save_old(prz);
- return 0;
}
} else {
pr_debug("no valid data in buffer (sig = 0x%08x)\n",
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
2018-10-26 18:00 ` [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size" Joel Fernandes (Google)
@ 2018-10-26 18:16 ` Kees Cook
2018-10-26 18:22 ` Joel Fernandes
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2018-10-26 18:16 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
>
> Due to the commit which is being reverted here, it is not possible to
> know if pstore's messages were from a previous boot, or from very old
> boots. This creates an awkard situation where its unclear if crash or
> other logs are from the previous boot or from very old boots. Also
> typically we dump the pstore buffers after one reboot and are interested
> in only the previous boot's crash so let us reset the buffer after we
> save them.
>
> Lastly, if we don't zap them, then I think it is possible that part of
> the buffer will be from this boot and the other parts will be from
> previous boots. So this revert fixes all of this by calling
> persistent_ram_zap always.
I like the other patches (comments coming), but not this one: it's
very intentional to keep all crashes around until they're explicitly
unlinked from the pstore filesystem from userspace. Especially true
for catching chains of kernel crashes, or a failed log collection,
etc. Surviving multiple reboots is the expected behavior on Chrome OS
too.
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
2018-10-26 18:16 ` Kees Cook
@ 2018-10-26 18:22 ` Joel Fernandes
2018-10-26 19:42 ` Kees Cook
0 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 18:22 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
> >
> > Due to the commit which is being reverted here, it is not possible to
> > know if pstore's messages were from a previous boot, or from very old
> > boots. This creates an awkard situation where its unclear if crash or
> > other logs are from the previous boot or from very old boots. Also
> > typically we dump the pstore buffers after one reboot and are interested
> > in only the previous boot's crash so let us reset the buffer after we
> > save them.
> >
> > Lastly, if we don't zap them, then I think it is possible that part of
> > the buffer will be from this boot and the other parts will be from
> > previous boots. So this revert fixes all of this by calling
> > persistent_ram_zap always.
>
> I like the other patches (comments coming), but not this one: it's
> very intentional to keep all crashes around until they're explicitly
> unlinked from the pstore filesystem from userspace. Especially true
> for catching chains of kernel crashes, or a failed log collection,
> etc. Surviving multiple reboots is the expected behavior on Chrome OS
> too.
Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that
unlinking was another way to clear the logs.
thanks!
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/6] pstore: map pstore types to names
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
` (4 preceding siblings ...)
2018-10-26 18:00 ` [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size" Joel Fernandes (Google)
@ 2018-10-26 19:04 ` Kees Cook
2018-10-26 20:35 ` Joel Fernandes
5 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:04 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> In later patches we will need to map types to names, so create a table
> for that which can also be used and reused in different parts of old and
> new code. Also use it to save the type in the PRZ which will be useful
> in later patches.
Yes, I like it. :) Comments below...
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> fs/pstore/inode.c | 44 ++++++++++++++++++++------------------
> fs/pstore/ram.c | 4 +++-
> include/linux/pstore.h | 29 +++++++++++++++++++++++++
> include/linux/pstore_ram.h | 2 ++
> 4 files changed, 57 insertions(+), 22 deletions(-)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 5fcb845b9fec..43757049d384 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -304,6 +304,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> struct dentry *dentry;
> struct inode *inode;
> int rc = 0;
> + enum pstore_type_id type;
> char name[PSTORE_NAMELEN];
> struct pstore_private *private, *pos;
> unsigned long flags;
> @@ -335,43 +336,44 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> goto fail_alloc;
> private->record = record;
>
> - switch (record->type) {
> + type = record->type;
Let's rename PSTORE_TYPE_UNKNOWN in the enum to be PSTORE_TYPE_MAX and
!= 255 (just leave it at the end). The value is never exposed to
userspace (nor to backend storage), so we can instead use it as the
bounds-check for doing type -> name mappings. (The one use in erst can
just be renamed.)
Then we can add a function to do the bounds checking and mapping
(instead of using a bare array lookup).
> + switch (type) {
> case PSTORE_TYPE_DMESG:
> - scnprintf(name, sizeof(name), "dmesg-%s-%llu%s",
> - record->psi->name, record->id,
> - record->compressed ? ".enc.z" : "");
> + scnprintf(name, sizeof(name), "%s-%s-%llu%s",
> + pstore_names[type], record->psi->name, record->id,
> + record->compressed ? ".enc.z" : "");
> break;
> case PSTORE_TYPE_CONSOLE:
> - scnprintf(name, sizeof(name), "console-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_FTRACE:
> - scnprintf(name, sizeof(name), "ftrace-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_MCE:
> - scnprintf(name, sizeof(name), "mce-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_PPC_RTAS:
> - scnprintf(name, sizeof(name), "rtas-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_PPC_OF:
> - scnprintf(name, sizeof(name), "powerpc-ofw-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_PPC_COMMON:
> - scnprintf(name, sizeof(name), "powerpc-common-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_PMSG:
> - scnprintf(name, sizeof(name), "pmsg-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_PPC_OPAL:
> - scnprintf(name, sizeof(name), "powerpc-opal-%s-%llu",
> - record->psi->name, record->id);
> + scnprintf(name, sizeof(name), "%s-%s-%llu",
> + pstore_names[type], record->psi->name, record->id);
> break;
> case PSTORE_TYPE_UNKNOWN:
> scnprintf(name, sizeof(name), "unknown-%s-%llu",
All of these, including PSTORE_TYPE_UNKNOWN are now identical (you can
include the .enc.z logic in for all of them. I think the entire switch
can be dropped, yes?
scnprintf(name, sizeof(name), "%s-%s-%llu%s",
pstore_name(record->type), record->psi->name, record->id,
record->compressed ? ".enc.z" : "")
> @@ -379,7 +381,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> break;
> default:
> scnprintf(name, sizeof(name), "type%d-%s-%llu",
> - record->type, record->psi->name, record->id);
> + type, record->psi->name, record->id);
> break;
> }
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index f4fd2e72add4..c7cd858adce7 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -604,6 +604,7 @@ static int ramoops_init_przs(const char *name,
> goto fail;
> }
> *paddr += zone_sz;
> + prz_ar[i]->type = pstore_name_to_type(name);
> }
>
> *przs = prz_ar;
> @@ -642,6 +643,7 @@ static int ramoops_init_prz(const char *name,
> persistent_ram_zap(*prz);
>
> *paddr += sz;
> + (*prz)->type = pstore_name_to_type(name);
>
> return 0;
> }
> @@ -777,7 +779,7 @@ static int ramoops_probe(struct platform_device *pdev)
>
> dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> - cxt->pmsg_size;
> - err = ramoops_init_przs("dump", dev, cxt, &cxt->dprzs, &paddr,
> + err = ramoops_init_przs("dmesg", dev, cxt, &cxt->dprzs, &paddr,
Yup. That's a funny mismatch. Not exposed to userspace in a released kernel. ;)
> dump_mem_sz, cxt->record_size,
> &cxt->max_dump_cnt, 0, 0);
> if (err)
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index a15bc4d48752..4a3dbdffd8d3 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -47,6 +47,21 @@ enum pstore_type_id {
> PSTORE_TYPE_UNKNOWN = 255
> };
>
> +/* names should be in the same order as the above table */
> +static char __maybe_unused *pstore_names[] = {
This should be static const char * const pstore_names[], I think?
> + "dmesg",
> + "mce",
> + "console",
> + "ftrace",
> + "rtas",
> + "powerpc-ofw",
> + "powerpc-common",
> + "pmsg",
> + "powerpc-opal",
> + "event",
> + "unknown", /* unknown should be the last string */
End this with a NULL for a cheaper compare below.
> +};
> +
> struct pstore_info;
> /**
> * struct pstore_record - details of a pstore record entry
> @@ -274,4 +289,18 @@ pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val)
> }
> #endif
>
> +static inline enum pstore_type_id pstore_name_to_type(const char *name)
> +{
> + char *str;
> + int i = 0;
> +
> + for (; strncmp(pstore_names[i], "unknown", 7); i++) {
char **ptr;
for (ptr = pstores_names; *ptr; ptr++) {
> + str = pstore_names[i];
> + if (!strncmp(name, str, strlen(str)))
"n" version not needed: the LHS is controlled and we want full matching:
if (!strcmp(*ptr, name))
> + return (enum pstore_type_id)i;
I don't think this cast is needed?
return (ptr - pstores_names);
> + }
> +
> + return PSTORE_TYPE_UNKNOWN;
> +}
> +
> #endif /*_LINUX_PSTORE_H*/
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index e6d226464838..ee0f493254dd 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -22,6 +22,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> +#include <linux/pstore.h>
> #include <linux/types.h>
>
> /*
> @@ -47,6 +48,7 @@ struct persistent_ram_zone {
> size_t size;
> void *vaddr;
> struct persistent_ram_buffer *buffer;
> + enum pstore_type_id type;
> size_t buffer_size;
> u32 flags;
> raw_spinlock_t buffer_lock;
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz
2018-10-26 18:00 ` [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz Joel Fernandes (Google)
@ 2018-10-26 19:05 ` Kees Cook
0 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:05 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> Since we store the type of the prz when we initialize it, we no longer
> need to pass it again in ramoops_get_next_prz since we can just use that
> to setup the pstore record. So lets remove it from the argument list.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Yes yes. I like where this is going. :)
-Kees
> ---
> fs/pstore/ram.c | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index c7cd858adce7..cbfdf4b8e89d 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -125,9 +125,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>
> static struct persistent_ram_zone *
> ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> - u64 *id,
> - enum pstore_type_id *typep, enum pstore_type_id type,
> - bool update)
> + u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
> @@ -147,7 +145,7 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> if (!persistent_ram_old_size(prz))
> return NULL;
>
> - *typep = type;
> + *typep = prz->type;
> *id = i;
>
> return prz;
> @@ -257,8 +255,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
> cxt->max_dump_cnt, &record->id,
> - &record->type,
> - PSTORE_TYPE_DMESG, 1);
> + &record->type, 1);
> if (!prz_ok(prz))
> continue;
> header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
> @@ -274,20 +271,18 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> - 1, &record->id, &record->type,
> - PSTORE_TYPE_CONSOLE, 0);
> + 1, &record->id, &record->type, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> - 1, &record->id, &record->type,
> - PSTORE_TYPE_PMSG, 0);
> + 1, &record->id, &record->type, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> &cxt->ftrace_read_cnt, 1, &record->id,
> - &record->type, PSTORE_TYPE_FTRACE, 0);
> + &record->type, 0);
> } else {
> /*
> * Build a new dummy record which combines all the
> @@ -306,8 +301,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> &cxt->ftrace_read_cnt,
> cxt->max_ftrace_cnt,
> &record->id,
> - &record->type,
> - PSTORE_TYPE_FTRACE, 0);
> + &record->type, 0);
>
> if (!prz_ok(prz_next))
> continue;
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
2018-10-26 18:00 ` [RFC 3/6] pstore: remove max " Joel Fernandes (Google)
@ 2018-10-26 19:22 ` Joel Fernandes
2018-10-26 19:27 ` Kees Cook
2018-10-26 19:22 ` Kees Cook
1 sibling, 1 reply; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 19:22 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> fs/pstore/ram.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> }
>
> static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
>
> /* Give up if we never existed or have hit the end. */
> - if (!przs || i >= max)
> + if (!przs)
> return NULL;
>
> prz = przs[i];
Ah, looks like I may have introduced an issue here since 'i' isn't checked by
the caller for the single prz case, its only checked for the multiple prz
cases, so something like below could be folded in. I still feel its better
than passing the max argument.
Another thought is, even better we could have a different function when
there's only one prz and not have to pass an array, just pass the first
element? Something like...
ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
enum pstore_type_id *typep, bool update)
And for the _single case, we also wouldn't need to pass id so that's another
argument less.
Let me know what you think, otherwise something like the below will need to
be folded in to fix this patch... thanks.
----8<---
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 5702b692bdb9..061d2af2485b 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
}
}
- if (!prz_ok(prz))
+ if (!prz_ok(prz) && !cxt->console_read_cnt) {
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
record, 0);
+ }
- if (!prz_ok(prz))
+ if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
record, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
- if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
+ if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
+ !cxt->ftrace_read_cnt) {
prz = ramoops_get_next_prz(cxt->fprzs,
&cxt->ftrace_read_cnt, record, 0);
} else {
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
2018-10-26 18:00 ` [RFC 3/6] pstore: remove max " Joel Fernandes (Google)
2018-10-26 19:22 ` Joel Fernandes
@ 2018-10-26 19:22 ` Kees Cook
1 sibling, 0 replies; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:22 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> fs/pstore/ram.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> }
>
> static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
>
> /* Give up if we never existed or have hit the end. */
> - if (!przs || i >= max)
> + if (!przs)
> return NULL;
>
> prz = przs[i];
> @@ -254,8 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> /* Find the next valid persistent_ram_zone for DMESG */
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
> - cxt->max_dump_cnt, &record->id,
> - &record->type, 1);
> + &record->id, &record->type, 1);
> if (!prz_ok(prz))
> continue;
> header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
> @@ -271,17 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> - 1, &record->id, &record->type, 0);
> + &record->id, &record->type, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> - 1, &record->id, &record->type, 0);
> + &record->id, &record->type, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> - &cxt->ftrace_read_cnt, 1, &record->id,
> + &cxt->ftrace_read_cnt, &record->id,
> &record->type, 0);
> } else {
> /*
> @@ -299,7 +298,6 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
> prz_next = ramoops_get_next_prz(cxt->fprzs,
> &cxt->ftrace_read_cnt,
> - cxt->max_ftrace_cnt,
> &record->id,
> &record->type, 0);
>
> --
> 2.19.1.568.g152ad8e336-goog
>
Yup, looks good.
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
2018-10-26 19:22 ` Joel Fernandes
@ 2018-10-26 19:27 ` Kees Cook
2018-10-26 19:40 ` Joel Fernandes
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:27 UTC (permalink / raw)
To: Joel Fernandes; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
>> From the code flow, the 'max' checks are already being done on the prz
>> passed to ramoops_get_next_prz. Lets remove it to simplify this function
>> and reduce its arguments.
>>
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> ---
>> fs/pstore/ram.c | 14 ++++++--------
>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index cbfdf4b8e89d..3055e05acab1 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>> }
>>
>> static struct persistent_ram_zone *
>> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
>> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
>> u64 *id, enum pstore_type_id *typep, bool update)
>> {
>> struct persistent_ram_zone *prz;
>> int i = (*c)++;
>>
>> /* Give up if we never existed or have hit the end. */
>> - if (!przs || i >= max)
>> + if (!przs)
>> return NULL;
>>
>> prz = przs[i];
>
> Ah, looks like I may have introduced an issue here since 'i' isn't checked by
> the caller for the single prz case, its only checked for the multiple prz
> cases, so something like below could be folded in. I still feel its better
> than passing the max argument.
>
> Another thought is, even better we could have a different function when
> there's only one prz and not have to pass an array, just pass the first
> element? Something like...
>
> ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
> enum pstore_type_id *typep, bool update)
> And for the _single case, we also wouldn't need to pass id so that's another
> argument less.
>
> Let me know what you think, otherwise something like the below will need to
> be folded in to fix this patch... thanks.
>
> ----8<---
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 5702b692bdb9..061d2af2485b 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> }
> }
>
> - if (!prz_ok(prz))
> + if (!prz_ok(prz) && !cxt->console_read_cnt) {
> prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> record, 0);
> + }
>
> - if (!prz_ok(prz))
> + if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
> prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> record, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> + !cxt->ftrace_read_cnt) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> &cxt->ftrace_read_cnt, record, 0);
> } else {
Ah yeah, good catch! I think your added fix is right. I was pondering
asking you to remove the & on the *_read_cnt and having the caller do
the increment:
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++,
&record->id,
&record->type,
PSTORE_TYPE_DMESG, 1);
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record
2018-10-26 18:00 ` [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record Joel Fernandes (Google)
@ 2018-10-26 19:32 ` Kees Cook
2018-10-26 19:36 ` Joel Fernandes
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:32 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> Both the id and type fields of a pstore_record are set by
> ramoops_get_next_prz. So we can just pass a pointer to the pstore_record
> instead of passing individual elements. This results in cleaner more
> readable code and fewer lines.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> fs/pstore/ram.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 3055e05acab1..710c3d30bac0 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -125,7 +125,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>
> static struct persistent_ram_zone *
> ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> - u64 *id, enum pstore_type_id *typep, bool update)
> + struct pstore_record *record, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
> @@ -145,8 +145,8 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> if (!persistent_ram_old_size(prz))
> return NULL;
>
> - *typep = prz->type;
> - *id = i;
> + record->type = prz->type;
> + record->id = i;
Yes yes. I've been meaning to get all this cleaned up after I
refactored everything to actually HAVE record at all. :P
>
> return prz;
> }
> @@ -254,7 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> /* Find the next valid persistent_ram_zone for DMESG */
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
> - &record->id, &record->type, 1);
> + record, 1);
In another patch, I think you could drop the "update" field too, and
use the record->type instead to determine if update is needed. Like:
static struct persistent_ram_zone *
ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint c,
struct pstore_record *record)
{
bool update = (record->type == PSTORE_TYPE_DMESG);
...
> if (!prz_ok(prz))
> continue;
> header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
> @@ -270,18 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> - &record->id, &record->type, 0);
> + record, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> - &record->id, &record->type, 0);
> + record, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> - &cxt->ftrace_read_cnt, &record->id,
> - &record->type, 0);
> + &cxt->ftrace_read_cnt, record, 0);
> } else {
> /*
> * Build a new dummy record which combines all the
> @@ -298,8 +297,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
> prz_next = ramoops_get_next_prz(cxt->fprzs,
> &cxt->ftrace_read_cnt,
> - &record->id,
> - &record->type, 0);
> + record, 0);
>
> if (!prz_ok(prz_next))
> continue;
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record
2018-10-26 19:32 ` Kees Cook
@ 2018-10-26 19:36 ` Joel Fernandes
0 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 19:36 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 08:32:16PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > Both the id and type fields of a pstore_record are set by
> > ramoops_get_next_prz. So we can just pass a pointer to the pstore_record
> > instead of passing individual elements. This results in cleaner more
> > readable code and fewer lines.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > fs/pstore/ram.c | 18 ++++++++----------
> > 1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 3055e05acab1..710c3d30bac0 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -125,7 +125,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> >
> > static struct persistent_ram_zone *
> > ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> > - u64 *id, enum pstore_type_id *typep, bool update)
> > + struct pstore_record *record, bool update)
> > {
> > struct persistent_ram_zone *prz;
> > int i = (*c)++;
> > @@ -145,8 +145,8 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> > if (!persistent_ram_old_size(prz))
> > return NULL;
> >
> > - *typep = prz->type;
> > - *id = i;
> > + record->type = prz->type;
> > + record->id = i;
>
> Yes yes. I've been meaning to get all this cleaned up after I
> refactored everything to actually HAVE record at all. :P
>
> >
> > return prz;
> > }
> > @@ -254,7 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> > /* Find the next valid persistent_ram_zone for DMESG */
> > while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> > prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
> > - &record->id, &record->type, 1);
> > + record, 1);
>
> In another patch, I think you could drop the "update" field too, and
> use the record->type instead to determine if update is needed. Like:
>
> static struct persistent_ram_zone *
> ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint c,
> struct pstore_record *record)
> {
> bool update = (record->type == PSTORE_TYPE_DMESG);
> ...
Yes, I agree, I'll do that :)
thanks!
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 5/6] pstore: donot treat empty buffers as valid
2018-10-26 18:00 ` [RFC 5/6] pstore: donot treat empty buffers as valid Joel Fernandes (Google)
@ 2018-10-26 19:39 ` Kees Cook
2018-10-26 20:22 ` Joel Fernandes
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:39 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> pstore currently calls persistent_ram_save_old even if a buffer is
> empty. While this appears to work, it is simply not the right thing to
> do and could lead to bugs so lets avoid that. It also prevent misleading
> prints in the logs which claim the buffer is valid.
I need to be better convinced that a present zero length record is the
same as a non-present record. This seems true, but there is
potentially still metadata available from a backend. What were the
misleading prints in logs?
-Kees
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> fs/pstore/ram_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 0792595ebcfb..1299aa3ea734 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -495,7 +495,7 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
>
> sig ^= PERSISTENT_RAM_SIG;
>
> - if (prz->buffer->sig == sig) {
> + if (prz->buffer->sig == sig && buffer_size(prz)) {
> if (buffer_size(prz) > prz->buffer_size ||
> buffer_start(prz) > buffer_size(prz))
> pr_info("found existing invalid buffer, size %zu, start %zu\n",
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
2018-10-26 19:27 ` Kees Cook
@ 2018-10-26 19:40 ` Joel Fernandes
0 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 19:40 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 08:27:49PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> >> From the code flow, the 'max' checks are already being done on the prz
> >> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> >> and reduce its arguments.
> >>
> >> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >> ---
> >> fs/pstore/ram.c | 14 ++++++--------
> >> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> >> index cbfdf4b8e89d..3055e05acab1 100644
> >> --- a/fs/pstore/ram.c
> >> +++ b/fs/pstore/ram.c
> >> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> >> }
> >>
> >> static struct persistent_ram_zone *
> >> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> >> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> >> u64 *id, enum pstore_type_id *typep, bool update)
> >> {
> >> struct persistent_ram_zone *prz;
> >> int i = (*c)++;
> >>
> >> /* Give up if we never existed or have hit the end. */
> >> - if (!przs || i >= max)
> >> + if (!przs)
> >> return NULL;
> >>
> >> prz = przs[i];
> >
> > Ah, looks like I may have introduced an issue here since 'i' isn't checked by
> > the caller for the single prz case, its only checked for the multiple prz
> > cases, so something like below could be folded in. I still feel its better
> > than passing the max argument.
> >
> > Another thought is, even better we could have a different function when
> > there's only one prz and not have to pass an array, just pass the first
> > element? Something like...
> >
> > ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
> > enum pstore_type_id *typep, bool update)
> > And for the _single case, we also wouldn't need to pass id so that's another
> > argument less.
> >
> > Let me know what you think, otherwise something like the below will need to
> > be folded in to fix this patch... thanks.
> >
> > ----8<---
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 5702b692bdb9..061d2af2485b 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> > }
> > }
> >
> > - if (!prz_ok(prz))
> > + if (!prz_ok(prz) && !cxt->console_read_cnt) {
> > prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> > record, 0);
> > + }
> >
> > - if (!prz_ok(prz))
> > + if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
> > prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> > record, 0);
> >
> > /* ftrace is last since it may want to dynamically allocate memory. */
> > if (!prz_ok(prz)) {
> > - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> > + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> > + !cxt->ftrace_read_cnt) {
> > prz = ramoops_get_next_prz(cxt->fprzs,
> > &cxt->ftrace_read_cnt, record, 0);
> > } else {
>
> Ah yeah, good catch! I think your added fix is right. I was pondering
> asking you to remove the & on the *_read_cnt and having the caller do
> the increment:
>
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++,
> &record->id,
> &record->type,
> PSTORE_TYPE_DMESG, 1);
Sure, that's better, I'll do that. That we don't have to pass a pointer, the
caller knows about the increment, and its a local variable less. thanks!
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
2018-10-26 18:22 ` Joel Fernandes
@ 2018-10-26 19:42 ` Kees Cook
2018-10-26 20:09 ` Joel Fernandes
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2018-10-26 19:42 UTC (permalink / raw)
To: Joel Fernandes; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 7:22 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote:
>> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
>> <joel@joelfernandes.org> wrote:
>> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
>> >
>> > Due to the commit which is being reverted here, it is not possible to
>> > know if pstore's messages were from a previous boot, or from very old
>> > boots. This creates an awkard situation where its unclear if crash or
>> > other logs are from the previous boot or from very old boots. Also
>> > typically we dump the pstore buffers after one reboot and are interested
>> > in only the previous boot's crash so let us reset the buffer after we
>> > save them.
>> >
>> > Lastly, if we don't zap them, then I think it is possible that part of
>> > the buffer will be from this boot and the other parts will be from
>> > previous boots. So this revert fixes all of this by calling
>> > persistent_ram_zap always.
>>
>> I like the other patches (comments coming), but not this one: it's
>> very intentional to keep all crashes around until they're explicitly
>> unlinked from the pstore filesystem from userspace. Especially true
>> for catching chains of kernel crashes, or a failed log collection,
>> etc. Surviving multiple reboots is the expected behavior on Chrome OS
>> too.
>
> Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that
> unlinking was another way to clear the logs.
In another thread I discovered that the "single prz" ones actually
_are_ zapped at boot. I didn't realize, but it explains why pmsg would
vanish on me sometimes. ;) I always thought I was just doing something
wrong with it. (And I wonder if it's actually a bug that pmsg is
zapped -- console doesn't matter: it's overwritten every boot by
design.)
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
2018-10-26 19:42 ` Kees Cook
@ 2018-10-26 20:09 ` Joel Fernandes
0 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 20:09 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 08:42:12PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:22 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote:
> >> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> >> <joel@joelfernandes.org> wrote:
> >> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
> >> >
> >> > Due to the commit which is being reverted here, it is not possible to
> >> > know if pstore's messages were from a previous boot, or from very old
> >> > boots. This creates an awkard situation where its unclear if crash or
> >> > other logs are from the previous boot or from very old boots. Also
> >> > typically we dump the pstore buffers after one reboot and are interested
> >> > in only the previous boot's crash so let us reset the buffer after we
> >> > save them.
> >> >
> >> > Lastly, if we don't zap them, then I think it is possible that part of
> >> > the buffer will be from this boot and the other parts will be from
> >> > previous boots. So this revert fixes all of this by calling
> >> > persistent_ram_zap always.
> >>
> >> I like the other patches (comments coming), but not this one: it's
> >> very intentional to keep all crashes around until they're explicitly
> >> unlinked from the pstore filesystem from userspace. Especially true
> >> for catching chains of kernel crashes, or a failed log collection,
> >> etc. Surviving multiple reboots is the expected behavior on Chrome OS
> >> too.
> >
> > Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that
> > unlinking was another way to clear the logs.
>
> In another thread I discovered that the "single prz" ones actually
> _are_ zapped at boot. I didn't realize, but it explains why pmsg would
> vanish on me sometimes. ;) I always thought I was just doing something
> wrong with it. (And I wonder if it's actually a bug that pmsg is
> zapped -- console doesn't matter: it's overwritten every boot by
> design.)
Oh yeah they are. So seems like some are zapped on boot and some aren't then.
Hmm, I would think it makes sense not to boot-zap dmesg ever, since that's
crash logs someone may want to see after many reboots. But console and pmsg
should be since those just "what happened on the last boot". I guess it
should be made clear in some structure or something which types are zapped on
boot, and which ones aren't. That'll make it clear when adding a new type
about that behavior, instead of relying on the assumption that single prz are
zapped and multiple ones aren't. Like for ftrace, since the per-cpu
configuration was added, it will now be zapped on boot if it is using a
per-cpu configuration and not zapped on boot if it isn't right? That would
seem a bit inconsistent.
thanks!
-Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 5/6] pstore: donot treat empty buffers as valid
2018-10-26 19:39 ` Kees Cook
@ 2018-10-26 20:22 ` Joel Fernandes
0 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 20:22 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 08:39:13PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > pstore currently calls persistent_ram_save_old even if a buffer is
> > empty. While this appears to work, it is simply not the right thing to
> > do and could lead to bugs so lets avoid that. It also prevent misleading
> > prints in the logs which claim the buffer is valid.
>
> I need to be better convinced that a present zero length record is the
> same as a non-present record. This seems true, but there is
> potentially still metadata available from a backend. What were the
> misleading prints in logs?
I got something like:
found existing buffer, size 0, start 0
When I was expecting:
no valid data in buffer (sig = ...)
The other thing is a call to persistent_ram_zap is also prevented on the
buffer, which prevent zero-initialize prz->ecc_info.par. Since we are
dropping patch 6/6, the zap will not happen. But I'm not super familiar with
the ecc bits of this code to say that if that's an issue.
About the present zero-length record, I would argue that it should not be
"present" at all. When the system first boots, the record is not present but
the signatures are initialized, then on reboots because the signatures were
initialized, the buffer appears as valid even if it was unused. So for dmesg,
all the max_dump_cnt number of buffers would appear as if they are valid
which is a bit strange because there was no crash at all so it should be in
the same state on reboot, as if there was no crash. That could be a matter of
perspective though so I leave it you how you prefer to do it :)
thanks,
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/6] pstore: map pstore types to names
2018-10-26 19:04 ` [RFC 1/6] pstore: map pstore types to names Kees Cook
@ 2018-10-26 20:35 ` Joel Fernandes
2018-10-26 20:41 ` Kees Cook
0 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes @ 2018-10-26 20:35 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 08:04:24PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > In later patches we will need to map types to names, so create a table
> > for that which can also be used and reused in different parts of old and
> > new code. Also use it to save the type in the PRZ which will be useful
> > in later patches.
>
> Yes, I like it. :) Comments below...
I'm glad, thanks, my replies are below:
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > fs/pstore/inode.c | 44 ++++++++++++++++++++------------------
> > fs/pstore/ram.c | 4 +++-
> > include/linux/pstore.h | 29 +++++++++++++++++++++++++
> > include/linux/pstore_ram.h | 2 ++
> > 4 files changed, 57 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> > index 5fcb845b9fec..43757049d384 100644
> > --- a/fs/pstore/inode.c
> > +++ b/fs/pstore/inode.c
> > @@ -304,6 +304,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> > struct dentry *dentry;
> > struct inode *inode;
> > int rc = 0;
> > + enum pstore_type_id type;
> > char name[PSTORE_NAMELEN];
> > struct pstore_private *private, *pos;
> > unsigned long flags;
> > @@ -335,43 +336,44 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> > goto fail_alloc;
> > private->record = record;
> >
> > - switch (record->type) {
> > + type = record->type;
>
> Let's rename PSTORE_TYPE_UNKNOWN in the enum to be PSTORE_TYPE_MAX and
> != 255 (just leave it at the end). The value is never exposed to
> userspace (nor to backend storage), so we can instead use it as the
> bounds-check for doing type -> name mappings. (The one use in erst can
> just be renamed.)
>
> Then we can add a function to do the bounds checking and mapping
> (instead of using a bare array lookup).
>
> > + switch (type) {
> > case PSTORE_TYPE_DMESG:
> > - scnprintf(name, sizeof(name), "dmesg-%s-%llu%s",
> > - record->psi->name, record->id,
> > - record->compressed ? ".enc.z" : "");
> > + scnprintf(name, sizeof(name), "%s-%s-%llu%s",
> > + pstore_names[type], record->psi->name, record->id,
> > + record->compressed ? ".enc.z" : "");
> > break;
> > case PSTORE_TYPE_CONSOLE:
> > - scnprintf(name, sizeof(name), "console-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_FTRACE:
> > - scnprintf(name, sizeof(name), "ftrace-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_MCE:
> > - scnprintf(name, sizeof(name), "mce-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_PPC_RTAS:
> > - scnprintf(name, sizeof(name), "rtas-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_PPC_OF:
> > - scnprintf(name, sizeof(name), "powerpc-ofw-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_PPC_COMMON:
> > - scnprintf(name, sizeof(name), "powerpc-common-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_PMSG:
> > - scnprintf(name, sizeof(name), "pmsg-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_PPC_OPAL:
> > - scnprintf(name, sizeof(name), "powerpc-opal-%s-%llu",
> > - record->psi->name, record->id);
> > + scnprintf(name, sizeof(name), "%s-%s-%llu",
> > + pstore_names[type], record->psi->name, record->id);
> > break;
> > case PSTORE_TYPE_UNKNOWN:
> > scnprintf(name, sizeof(name), "unknown-%s-%llu",
>
> All of these, including PSTORE_TYPE_UNKNOWN are now identical (you can
> include the .enc.z logic in for all of them. I think the entire switch
> can be dropped, yes?
>
> scnprintf(name, sizeof(name), "%s-%s-%llu%s",
> pstore_name(record->type), record->psi->name, record->id,
> record->compressed ? ".enc.z" : "")
True! I'll just do that. Sounds good! The PSTORE_TYPE_UNKNOWN and the below
"default:" case could be combined I guessed. Its a great idea and lesser
lines, thanks!
> > dump_mem_sz, cxt->record_size,
> > &cxt->max_dump_cnt, 0, 0);
> > if (err)
> > diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> > index a15bc4d48752..4a3dbdffd8d3 100644
> > --- a/include/linux/pstore.h
> > +++ b/include/linux/pstore.h
> > @@ -47,6 +47,21 @@ enum pstore_type_id {
> > PSTORE_TYPE_UNKNOWN = 255
> > };
> >
> > +/* names should be in the same order as the above table */
> > +static char __maybe_unused *pstore_names[] = {
>
> This should be static const char * const pstore_names[], I think?
Sure, I'll add the const(s) to it.
> > + "dmesg",
> > + "mce",
> > + "console",
> > + "ftrace",
> > + "rtas",
> > + "powerpc-ofw",
> > + "powerpc-common",
> > + "pmsg",
> > + "powerpc-opal",
> > + "event",
> > + "unknown", /* unknown should be the last string */
>
> End this with a NULL for a cheaper compare below.
Agreed, I am wondering if we need the unknown string though if we
terminate the type enum table with a MAX. I'll look more into that.
> > +};
> > +
> > struct pstore_info;
> > /**
> > * struct pstore_record - details of a pstore record entry
> > @@ -274,4 +289,18 @@ pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val)
> > }
> > #endif
> >
> > +static inline enum pstore_type_id pstore_name_to_type(const char *name)
> > +{
> > + char *str;
> > + int i = 0;
> > +
> > + for (; strncmp(pstore_names[i], "unknown", 7); i++) {
> char **ptr;
>
> for (ptr = pstores_names; *ptr; ptr++) {
>
> > + str = pstore_names[i];
> > + if (!strncmp(name, str, strlen(str)))
>
> "n" version not needed: the LHS is controlled and we want full matching:
>
> if (!strcmp(*ptr, name))
Sounds good, but now that we are adding PSTORE_TYPE_MAX, I think it would be
cleaner and safer if I iterated until PSTORE_TYPE_MAX, so I'll use that to
terminate the loop?
> > + return (enum pstore_type_id)i;
>
> I don't think this cast is needed?
>
> return (ptr - pstores_names);
But I have the index variable, so it would be cleaner to just return that? I
believe I can just drop the cast and do that.
thanks,
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/6] pstore: map pstore types to names
2018-10-26 20:35 ` Joel Fernandes
@ 2018-10-26 20:41 ` Kees Cook
0 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2018-10-26 20:41 UTC (permalink / raw)
To: Joel Fernandes; +Cc: LKML, kernel-team, Anton Vorontsov, Colin Cross, Tony Luck
On Fri, Oct 26, 2018 at 9:35 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> But I have the index variable, so it would be cleaner to just return that? I
> believe I can just drop the cast and do that.
Yeah, that should be fine.
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-10-26 20:41 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-26 18:00 [RFC 1/6] pstore: map pstore types to names Joel Fernandes (Google)
2018-10-26 18:00 ` [RFC 2/6] pstore: remove type argument from ramoops_get_next_prz Joel Fernandes (Google)
2018-10-26 19:05 ` Kees Cook
2018-10-26 18:00 ` [RFC 3/6] pstore: remove max " Joel Fernandes (Google)
2018-10-26 19:22 ` Joel Fernandes
2018-10-26 19:27 ` Kees Cook
2018-10-26 19:40 ` Joel Fernandes
2018-10-26 19:22 ` Kees Cook
2018-10-26 18:00 ` [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record Joel Fernandes (Google)
2018-10-26 19:32 ` Kees Cook
2018-10-26 19:36 ` Joel Fernandes
2018-10-26 18:00 ` [RFC 5/6] pstore: donot treat empty buffers as valid Joel Fernandes (Google)
2018-10-26 19:39 ` Kees Cook
2018-10-26 20:22 ` Joel Fernandes
2018-10-26 18:00 ` [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size" Joel Fernandes (Google)
2018-10-26 18:16 ` Kees Cook
2018-10-26 18:22 ` Joel Fernandes
2018-10-26 19:42 ` Kees Cook
2018-10-26 20:09 ` Joel Fernandes
2018-10-26 19:04 ` [RFC 1/6] pstore: map pstore types to names Kees Cook
2018-10-26 20:35 ` Joel Fernandes
2018-10-26 20:41 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).