linux-kernel - Re: [GIT PULL] ftrace: Check if pages were allocated before calling free

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: <CAHk-=wjt9b7kxQ2J=aDNKbR1QBMB3Hiqb_hYcZbKsxGRSEb+gQ@mail.gmail.com>
Date:   Wed, 31 Mar 2021 10:45:01 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [GIT PULL] ftrace: Check if pages were allocated before calling free_pages()

On Wed, Mar 31, 2021 at 6:27 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
>                 order = get_count_order(pg->size / ENTRIES_PER_PAGE);
> -               free_pages((unsigned long)pg->records, order);
> +               if (order >= 0)
> +                       free_pages((unsigned long)pg->records, order);

Honestly, looking at that code, every single use of
"get_count_order()" seems really really confusing.

And really confused.

The comments are garbage, the code is odd, it's just all very very strange.

See here in ftrace_allocate_records():

        pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
        order = get_count_order(pages);

        /*
         * We want to fill as much as possible. No more than a page
         * may be empty.
         */
        if (!is_power_of_2(pages))
                order--;

can you actually explain what the logic is here?

The 'get_count_order()' function will return the smallest order that
the value fits in. But then if the value wasn't a power of two, you
subtract 1. If I understand the code correctly, that's just the same
as "what is the highest bit set", isn't it?

So afaik, what you *actually* want is just

        pages = DIV_ROUND_UP(count, ENTRIES_PER_PAGE);
        order = fls(pages)-1;

isn't it?

Did I misunderstand what the code wants to do? The "No more than a
page may be empty" seems wrong - you really mean "no empty pages".

I dunno. Maybe I'm wrong, but that code is really confusing (which is
why I may be wrong). It doesn't seem to make any sense at all to use
"get_count_order()" and then modify the end result.

             Linus