linux-kernel - Re: [RFC 4/6] pstore: further reduce ramoops_get_next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181026193637.GA122104@joelaf.mtv.corp.google.com>
Date:   Fri, 26 Oct 2018 12:36:37 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, kernel-team@...roid.com,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments
 by passing record

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@...lfernandes.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@...lfernandes.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