by Joel Fernandes

[permalink] [raw]

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

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)
> <[email protected]> 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) <[email protected]>
> > ---
> > 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

2018-10-26 20:42:39

by Kees Cook

[permalink] [raw]

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

On Fri, Oct 26, 2018 at 9:35 PM, Joel Fernandes <[email protected]> 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