Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 26 Oct 2018 13:35:14 -0700
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, kernel-team@android.com,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Subject: Re: [RFC 1/6] pstore: map pstore types to names
Message-ID: <20181026203514.GC129228@joelaf.mtv.corp.google.com>
References: <20181026180042.52199-1-joel@joelfernandes.org>
 <CAGXu5jLKLKDwVgRoMx57Y76V9FVJqQ+EeTHB2CnxAN5CnC2Jmw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGXu5jLKLKDwVgRoMx57Y76V9FVJqQ+EeTHB2CnxAN5CnC2Jmw@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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