Re: [PATCH 5/7] ramoops: Split ftrace buffer space into per-CPU zones

[Kees Cook <keescook@chromium.org>]

On Sun, Oct 16, 2016 at 10:40 AM, Joel Fernandes <joelaf@google.com> wrote:
> Hi Kees,
>
> On Mon, Oct 10, 2016 at 4:59 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Sun, Oct 9, 2016 at 10:15 AM, Joel Fernandes <joelaf@google.com> wrote:
>>> On Fri, Oct 7, 2016 at 10:28 PM, Joel Fernandes <joelaf@google.com> wrote:
>>>> If FTRACE_PER_CPU flag is passed to ramoops pdata, split the space into
>>>> multiple zones depending on the number of CPUs.
>>>>
>>>> This speeds up the performance of function tracing by about 280% in my tests as
>>>> we avoid the locking. The trade off being lesser space available per CPU.  Let
>>>> the ramoops user decide which option they want based on pdata flag.
>>>>
>>>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>>>> ---
>>>>  fs/pstore/ram.c            | 70 +++++++++++++++++++++++++++++++++++-----------
>>>>  include/linux/pstore_ram.h |  3 ++
>>>>  2 files changed, 56 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>>> [..]
>>>> @@ -391,14 +418,21 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
>>>>  {
>>>>         int i;
>>>>
>>>> -       if (!cxt->przs)
>>>> -               return;
>>>> +       /* Free dump PRZs */
>>>> +       if (cxt->przs) {
>>>> +               for (i = 0; i < cxt->max_dump_cnt; i++)
>>>> +                       persistent_ram_free(cxt->przs[i]);
>>>>
>>>> -       for (i = 0; i < cxt->max_dump_cnt; i++)
>>>> -               persistent_ram_free(cxt->przs[i]);
>>>> +               kfree(cxt->przs);
>>>> +               cxt->max_dump_cnt = 0;
>>>> +       }
>>>>
>>>> -       kfree(cxt->przs);
>>>> -       cxt->max_dump_cnt = 0;
>>>> +       /* Free ftrace PRZs */
>>>> +       if (cxt->fprzs) {
>>>> +               for (i = 0; i < nr_cpu_ids; i++)
>>>> +                       persistent_ram_free(cxt->przs[i]);
>>>
>>> I am supposed to free fprzs[i] here, instead of przs[i]. Also need to
>>> check flag and free correct number of zones. Will fix for v2, sorry
>>> about that.
>>
>> I think the cpu id needs to be bounds-checked against the size of the
>> allocation. In theory, CPU hot-plug could grow the number of CPUs
>> after pstore is initialized.
>
> nr_cpu_ids is fixed to the number of possible CPUs regardless of if
> hotplug is being used or not. I did a hotplug test as well to confirm
> this. So if I boot on 4 CPU machine with only 2 CPUs online, then
> nr_cpu_ids is 4.

Ah-ha, okay. I wasn't sure if there was some way to grow nr_cpu_ids after boot.

Thanks for checking!

-Kees

-- 
Kees Cook
Nexus Security