All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Potapenko <glider@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>
Subject: Re: [GIT PULL] meminit fix for v5.3-rc2
Date: Wed, 31 Jul 2019 13:30:19 +0200	[thread overview]
Message-ID: <CAG_fn=WzcGvS7=SpigawnPDQgY13nFiqhmjFPaOH8LeuoU7Fhw@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wgTM+cN7zyUZacGQDv3DuuoA4LORNPWgb1Y_Z1p4iedNQ@mail.gmail.com>

On Tue, Jul 30, 2019 at 9:54 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jul 30, 2019 at 6:53 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > I wonder how hard it should be to make a zero-filling GCC plugin?
> > I'm not a big fan of hacking GCC, but it shouldn't differ much from
> > the existing GCC plugins that initialize locals.
>
> The thing is, as long as it's a plugin, I don't think we can rely on
> it. The gcc people will rightly just laugh at us if we were to report
> a bug with some kernel plugin.
>
> So I'd like the zeroing of local variables to be a native compiler
> option, so that we can (_eventually_ - these things take a long time)
> just start saying "ok, we simply consider stack variables to be always
> initialized".
>
> > I've some stale data collected on an x86 QEMU instance.
> > For 0x00 stack initialization:
> >  - hackbench, netperf and parallel Linux build were virtually free
> > (slowdown within stdev)
> >  - for af_inet_loopback the slowdown was ~4%
> > For 0xAA stack initialization:
> >  - netperf and parallel Linux build were free
> >  - for hackbench the slowdown was ~1.5%
> >  - for af_inet_loopback the slowdown was ~7%
>
> So I would expect that we have some special cases where we end up
> having arrays (or big structures) on the stack that end up being
> critical, and where initializing them is clearly  abad idea.
>
> Then we can verify manually are very much initialized, and that we
> could then mark and say "this is uninitialized".
>
> So when a compiler has an option to initialize stack variables, it
> would probably _also_ be a very good idea for that compiler to then
> support a variable attribute that says "don't initialize _this_
> variable, I will do that manually".
FWIW Clang has __attribute((uninitialized)) already:
https://reviews.llvm.org/rL349442
I agree that making it in GCC is easier if initialization itself is
implemented as part of GCC.

> But if we in ten years had a kernel model where only allocations and
> variables that were _explicitly_ uninitialized, that would be lovely.
>
> Then you can grep for those and verify that "yes, this is safe".
>
> We've historically had the reverse model - things are uninitialized by
> default, and you have to explicitly initialize them. Turning that on
> its head is what I would like to do long-term.
>
> (For normal allocations that wouldn't be too bad: get rid of
> __GFP_ZERO and friends, and instead do __GFP_UNINITIALIZED).
There've been concerns about such flags easily going out of control
(my original proposal for heap initialization contained such a flag).
To some extent their spread can be prevented by build-time checks, but
a simple grep can be insufficient, as people will start creating
helper functions to allocate with __GFP_UNINITIALIZED.

> Again - I don't think we want a world where everything is
> force-initialized. There _are_ going to be situations where that just
> hurts too much. But if we get to a place where we are zero-initialized
> by default, and have to explicitly mark the unsafe things (and we'll
> have comments not just about how they get initialized, but also about
> why that particular thing is so performance-critical), that would be a
> good place to be.
>
> This, btw, is why I also think that the "initialize with poison" is
> pointless and wrong. Yes, it can find bugs, but it doesn't really help
> improve the general situation, and people see it as a debugging tool,
> not a "improve code quality and improve the life of kernel developers"
> tool.
This sure makes sense. If this policy is adopted kernel-wide, we won't
need any debugging tools for uninit variables, so it's natural to
initialize them to zero.
>                 Linus



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

  reply	other threads:[~2019-07-31 11:30 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-28 19:21 [GIT PULL] meminit fix for v5.3-rc2 Kees Cook
2019-07-28 19:43 ` Linus Torvalds
2019-07-28 19:50   ` Linus Torvalds
2019-07-28 22:16   ` Kees Cook
2019-07-30 13:53     ` Alexander Potapenko
2019-07-30 19:53       ` Linus Torvalds
2019-07-31 11:30         ` Alexander Potapenko [this message]
2019-07-28 19:50 ` pr-tracker-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAG_fn=WzcGvS7=SpigawnPDQgY13nFiqhmjFPaOH8LeuoU7Fhw@mail.gmail.com' \
    --to=glider@google.com \
    --cc=arnd@arndb.de \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.