linux-kernel - Re: [PATCH] x86/tlb: Revert: Align TLB invalidation info

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-=whgXyXCy9vHY2hKJ9Pa8PnwdwKo9eOQ3jDbCs_FkdDvkQ@mail.gmail.com>
Date:   Tue, 16 Apr 2019 10:45:05 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Nadav Amit <namit@...are.com>,
        Dave Hansen <dave.hansen@...el.com>
Subject: Re: [PATCH] x86/tlb: Revert: Align TLB invalidation info

On Tue, Apr 16, 2019 at 1:03 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> Using 320 bytes of stack space for a 40 byte structure is ludicrous and
> clearly not right.

Ack.

That said, I wish we didn't have these stack structures at all. Or at
least were more careful about them. For example, another case of this
struct on the stack looks really iffy too:

                struct flush_tlb_info info;
                info.start = start;
                info.end = end;
                on_each_cpu(do_kernel_range_flush, &info, 1);

note how it only initializes two of the fields, and leaves the others
entirely randomly initialized with garbage?

Yeah, yeah, "do_kernel_range_flush()" only uses those two fields, but
it still makes my skin crawl how we basically pass a largely
uninitialized structure and have other CPU's look at it.

And in another case we do have a nicely initialized structure

    void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
    {
        struct flush_tlb_info info = {
                .mm = NULL,
                .start = 0UL,
                .end = TLB_FLUSH_ALL,
        };

but it looks like it shouldn't have been on the stack in the first
place, because as far as I can tell it's entirely constant, and it
should just be a "static const" structure initialized at compile time.

So as far as I can tell, we could do something like

-static void flush_tlb_func_local(void *info, enum tlb_flush_reason reason)
+static void flush_tlb_func_local(const void *info, enum
tlb_flush_reason reason)
-       struct flush_tlb_info info = {
+       static const struct flush_tlb_info info = {

for that case.

End result: it looks like we have three of these stack things, and all
three had something odd in them.

So very much Ack on that patch, but maybe we could do a bit more cleanup here?

                   Linus