LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Tobin C. Harding" <me@tobin.cc>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [GIT PULL] vsprintf: Replace memory barrier with static_key for random_ptr_key update
Date: Thu, 17 May 2018 07:40:53 +1000	[thread overview]
Message-ID: <20180516214053.GA25268@eros> (raw)
In-Reply-To: <20180516091241.2ea940d1@gandalf.local.home>

On Wed, May 16, 2018 at 09:12:41AM -0400, Steven Rostedt wrote:
> 
> Linus,
> 
> The memory barrier usage in updating the random ptr hash for %p in
> vsprintf is incorrect. Instead of adding the read memory barrier
> into vsprintf() which will cause a slight degradation to a commonly
> used function in the kernel just to solve a very unlikely race
> condition that can only happen at boot up, change the code from
> using a variable branch to a static_branch. Not only does this solve
> the race condition, it actually will improve the performance of
> vsprintf() by removing the conditional branch that is only needed
> at boot.
> 
> 
> Please pull the latest trace-v4.17-rc5-vsprintf tree, which can be found at:
> 
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> trace-v4.17-rc5-vsprintf
> 
> Tag SHA1: 3e2a2dfc8987e9a2b4e185b65a9b48c374c80791
> Head SHA1: 85f4f12d51397f1648e1f4350f77e24039b82d61
> 
> 
> Steven Rostedt (VMware) (1):
>       vsprintf: Replace memory barrier with static_key for random_ptr_key update
> 
> ----
>  lib/vsprintf.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> ---------------------------
> commit 85f4f12d51397f1648e1f4350f77e24039b82d61
> Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Date:   Tue May 15 22:24:52 2018 -0400
> 
>     vsprintf: Replace memory barrier with static_key for random_ptr_key update
>     
>     Reviewing Tobin's patches for getting pointers out early before
>     entropy has been established, I noticed that there's a lone smp_mb() in
>     the code. As with most lone memory barriers, this one appears to be
>     incorrectly used.
>     
>     We currently basically have this:
>     
>             get_random_bytes(&ptr_key, sizeof(ptr_key));
>             /*
>              * have_filled_random_ptr_key==true is dependent on get_random_bytes().
>              * ptr_to_id() needs to see have_filled_random_ptr_key==true
>              * after get_random_bytes() returns.
>              */
>             smp_mb();
>             WRITE_ONCE(have_filled_random_ptr_key, true);
>     
>     And later we have:
>     
>             if (unlikely(!have_filled_random_ptr_key))
>                     return string(buf, end, "(ptrval)", spec);
>     
>     /* Missing memory barrier here. */
>     
>             hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
>     
>     As the CPU can perform speculative loads, we could have a situation
>     with the following:
>     
>             CPU0                            CPU1
>             ----                            ----
>                                        load ptr_key = 0
>        store ptr_key = random
>        smp_mb()
>        store have_filled_random_ptr_key
>     
>                                        load have_filled_random_ptr_key = true
>     
>                                         BAD BAD BAD! (you're so bad!)

The additional clarification in this line, added for the pull request, is pure gold.


	Tobin

      reply	other threads:[~2018-05-16 21:41 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-16 13:12 Steven Rostedt
2018-05-16 21:40 ` Tobin C. Harding [this message]

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=20180516214053.GA25268@eros \
    --to=me@tobin.cc \
    --cc=akpm@linux-foundation.org \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [GIT PULL] vsprintf: Replace memory barrier with static_key for random_ptr_key update' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).