Re: [RFC 1/6] pstore: map pstore types to names

From: Kees Cook
Date: Fri Oct 26 2018 - 15:04:32 EST

On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> ---
> 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