Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz

[Kees Cook <keescook@chromium.org>]

On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes <joel@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) <joel@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);

-Kees

-- 
Kees Cook