On Fri, Oct 26, 2018 at 08:27:49PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes <j...@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) <j...@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
