linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Will Deacon <will@kernel.org>
To: Marco Elver <elver@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH v5 00/18] Rework READ_ONCE() to improve codegen
Date: Wed, 13 May 2020 22:25:21 +0100	[thread overview]
Message-ID: <20200513212520.GC28594@willie-the-truck> (raw)
In-Reply-To: <CANpmjNNOpJk0tprXKB_deiNAv_UmmORf1-2uajLhnLWQQ1hvoA@mail.gmail.com>

On Wed, May 13, 2020 at 08:54:03PM +0200, Marco Elver wrote:
> On Wed, 13 May 2020 at 19:47, Will Deacon <will@kernel.org> wrote:
> > On Wed, May 13, 2020 at 07:32:58PM +0200, Marco Elver wrote:
> > > - We do *not* want the call to __read_once_word_nocheck if we have
> > > __no_sanitize_or_inline. AFAIK that's the main problem -- this applies
> > > to both KASAN and KCSAN.
> >
> > Sorry, I should've been more explicit. The code above, with KASAN enabled,
> > compiles to:
> >
> > ffffffff810a2d50 <foo>:
> > ffffffff810a2d50:       48 8b 07                mov    (%rdi),%rax
> > ffffffff810a2d53:       c3                      retq
> >
> > but with KCSAN enabled, compiles to:
> >
> > ffffffff8109ecd0 <foo>:
> > ffffffff8109ecd0:       53                      push   %rbx
> > ffffffff8109ecd1:       48 89 fb                mov    %rdi,%rbx
> > ffffffff8109ecd4:       48 8b 7c 24 08          mov    0x8(%rsp),%rdi
> > ffffffff8109ecd9:       e8 52 9c 1a 00          callq  ffffffff81248930 <__tsan_func_entry>
> > ffffffff8109ecde:       48 89 df                mov    %rbx,%rdi
> > ffffffff8109ece1:       e8 1a 00 00 00          callq  ffffffff8109ed00 <__read_once_word_nocheck>
> > ffffffff8109ece6:       48 89 c3                mov    %rax,%rbx
> > ffffffff8109ece9:       e8 52 9c 1a 00          callq  ffffffff81248940 <__tsan_func_exit>
> > ffffffff8109ecee:       48 89 d8                mov    %rbx,%rax
> > ffffffff8109ecf1:       5b                      pop    %rbx
> > ffffffff8109ecf2:       c3                      retq
> >
> > Is that expected? There don't appear to be any more annotations to throw
> > at it.
> 
> Right, so this is expected.

Fair enough, I just found it weird since it's different to the usual
"disable instrumentation/trace" function annotations.

> We can definitely make __tsan_func_entry/exit disappear with Clang, with
> GCC it's going to be a while if we want to fix it.
> 
> If we remove 'noinline' from __no_kcsan_or_inline, we no longer get
> the call to __read_once_word_nocheck above! But...
> 
> For KCSAN we force 'noinline' because older compilers still inline and
> then instrument small functions even if we just have the no_sanitize
> attribute (without inline mentioned). The same is actually true for
> KASAN, so KASAN's READ_ONCE_NOCHECK might be broken in a few places,
> but nobody seems to have noticed [1]. KASAN's __no_kasan_or_inline
> should also have a 'noinline' I think. I just tested
> __no_kcsan_or_inline without 'noinline', and yes, GCC 9 still decided
> to inline a small function and then instrument the accesses.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600
> 
> The good news is that Clang does the right thing when removing
> 'noinline' from __no_kcsan_or_inline:
> 1. doesn't inline into functions that are instrumented, and
> 2. your above example doesn't do the call to __read_once_word_nocheck.
> 
> The obvious solution to this is: restrict which compiler we want to support?

I would be in favour of that, but I defer to the x86 folks since this
affects them much more than it does me. On the arm64 side, we've got patches
queued for 5.8 that require GCC 10.0.1 or later, and that thing is only a
week old. I think it's reasonable to require a recent toolchain for optional
features like this that inherently rely on compiler support.

> > > From what I gather, we want to just compile the function as if the
> > > sanitizer was never enabled. One reason for why this doesn't quite
> > > work is because of the preprocessor.
> > >
> > > Note that the sanitizers won't complain about these accesses, which
> > > unfortunately is all these attributes ever were documented to do. So
> > > the attributes aren't completely useless. Why doesn't
> > > K[AC]SAN_SANITIZE := n work?
> >
> > I just don't get the point in having a function annotation if you then have to
> > pass flags at the per-object level. That also then necessitates either weird
> > refactoring and grouping of code into "noinstrument.c" type files, or blanket
> > disabling of instrumentation for things like arch/x86/
> 
> If you want a solution now, here is one way to get us closer to where
> we want to be:
> 
> 1. Peter's patch to add data_race around __READ_ONCE/__WRITE_ONCE.
> 2. Patch to make __tsan_func_entry/exit disappear with Clang.
> 3. Remove 'noinline' from __no_kcsan_or_inline.
> 4. Patch to warn users that KCSAN may have problems with GCC and
> should use Clang >= 7.
> 
> But this is probably only half a solution.

At this point, I think that if READ_ONCE_NOCHECK() works as expected, and
calling __{READ,WRITE}_ONCE from functions tagged with __no_sanitize doesn't
result in instrumentation, then we're good.

Will

  reply	other threads:[~2020-05-13 21:25 UTC|newest]

Thread overview: 127+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-11 20:41 Will Deacon
2020-05-11 20:41 ` [PATCH v5 01/18] sparc32: mm: Fix argument checking in __srmmu_get_nocache() Will Deacon
2020-05-12 14:37   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 02/18] sparc32: mm: Restructure sparc32 MMU page-table layout Will Deacon
2020-05-12 14:37   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 03/18] sparc32: mm: Change pgtable_t type to pte_t * instead of struct page * Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 04/18] sparc32: mm: Reduce allocation size for PMD and PTE tables Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-17  0:00   ` [PATCH v5 04/18] " Guenter Roeck
2020-05-17  0:07     ` Guenter Roeck
2020-05-18  8:37       ` Will Deacon
2020-05-18  9:18         ` Mike Rapoport
2020-05-18  9:48         ` Guenter Roeck
2020-05-18 14:23           ` Mike Rapoport
2020-05-18 16:08             ` Guenter Roeck
2020-05-18 18:11               ` Ira Weiny
2020-05-18 18:14               ` Ira Weiny
2020-05-18 18:09             ` Guenter Roeck
2020-05-18 18:21               ` Ira Weiny
2020-05-18 19:15               ` Mike Rapoport
2020-05-19 16:40                 ` Guenter Roeck
2020-05-20 17:03         ` Mike Rapoport
2020-05-20 19:03           ` Guenter Roeck
2020-05-20 19:51             ` Mike Rapoport
2020-05-21 23:02               ` Guenter Roeck
2020-05-24 12:32                 ` Mike Rapoport
2020-05-24 14:01                   ` Guenter Roeck
2020-05-26 13:26                   ` Will Deacon
2020-05-26 14:01                     ` Will Deacon
2020-05-26 15:21                       ` Mike Rapoport
2020-05-26 16:18                       ` Guenter Roeck
2020-05-26 16:29                         ` Mike Rapoport
2020-05-26 17:15                           ` Guenter Roeck
2020-05-11 20:41 ` [PATCH v5 05/18] compiler/gcc: Raise minimum GCC version for kernel builds to 4.8 Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 06/18] netfilter: Avoid assigning 'const' pointer to non-const pointer Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 07/18] net: tls: " Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 08/18] fault_inject: Don't rely on "return value" from WRITE_ONCE() Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 09/18] arm64: csum: Disable KASAN for do_csum() Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 10/18] READ_ONCE: Simplify implementations of {READ,WRITE}_ONCE() Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 11/18] READ_ONCE: Enforce atomicity for {READ,WRITE}_ONCE() memory accesses Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 12/18] READ_ONCE: Drop pointer qualifiers when reading from scalar types Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 13/18] locking/barriers: Use '__unqual_scalar_typeof' for load-acquire macros Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 14/18] arm64: barrier: Use '__unqual_scalar_typeof' for acquire/release macros Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 15/18] gcov: Remove old GCC 3.4 support Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 16/18] kcsan: Rework data_race() so that it can be used by READ_ONCE() Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-11 20:41 ` [PATCH v5 17/18] READ_ONCE: Use data_race() to avoid KCSAN instrumentation Will Deacon
2020-05-12  8:23   ` Peter Zijlstra
2020-05-12  9:49     ` Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-20 22:17     ` Borislav Petkov
2020-05-20 22:30       ` Marco Elver
2020-05-21  7:25         ` Borislav Petkov
2020-05-21  9:37           ` Marco Elver
2020-05-21  3:30       ` Nathan Chancellor
2020-05-22 16:08       ` [tip: locking/kcsan] compiler.h: Avoid nested statement expression in data_race() tip-bot2 for Marco Elver
2020-05-11 20:41 ` [PATCH v5 18/18] linux/compiler.h: Remove redundant '#else' Will Deacon
2020-05-12 14:36   ` [tip: locking/kcsan] " tip-bot2 for Will Deacon
2020-05-12  8:18 ` [PATCH v5 00/18] Rework READ_ONCE() to improve codegen Peter Zijlstra
2020-05-12 17:53   ` Marco Elver
2020-05-12 18:55     ` Marco Elver
2020-05-12 19:07     ` Peter Zijlstra
2020-05-12 20:31       ` Marco Elver
2020-05-13 11:10         ` Peter Zijlstra
2020-05-13 11:14           ` Peter Zijlstra
2020-05-13 11:48           ` Marco Elver
2020-05-13 12:32             ` Peter Zijlstra
2020-05-13 12:40               ` Will Deacon
2020-05-13 13:15                 ` Marco Elver
2020-05-13 13:24                   ` Peter Zijlstra
2020-05-13 13:58                     ` Marco Elver
2020-05-14 11:21                       ` Peter Zijlstra
2020-05-14 11:24                         ` Peter Zijlstra
2020-05-14 11:35                         ` Peter Zijlstra
2020-05-14 12:01                         ` Will Deacon
2020-05-14 12:27                           ` Peter Zijlstra
2020-05-14 13:07                             ` Marco Elver
2020-05-14 13:14                               ` Peter Zijlstra
2020-05-14 12:20                         ` Peter Zijlstra
2020-05-14 14:13                       ` Peter Zijlstra
2020-05-14 14:20                         ` Marco Elver
2020-05-15  9:20                           ` Peter Zijlstra
2020-05-13 16:50                   ` Will Deacon
2020-05-13 17:32                     ` Marco Elver
2020-05-13 17:47                       ` Will Deacon
2020-05-13 18:54                         ` Marco Elver
2020-05-13 21:25                           ` Will Deacon [this message]
2020-05-14  7:31                             ` Marco Elver
2020-05-14 11:05                               ` Will Deacon
2020-05-14 13:35                                 ` Marco Elver
2020-05-14 13:47                                   ` Peter Zijlstra
2020-05-14 13:50                                   ` Peter Zijlstra
2020-05-14 13:56                                   ` Peter Zijlstra
2020-05-14 14:24                                   ` Peter Zijlstra
2020-05-14 15:09                                     ` Thomas Gleixner
2020-05-14 15:29                                       ` Marco Elver
2020-05-14 19:37                                         ` Thomas Gleixner
2020-05-15 13:55                                     ` David Laight
2020-05-15 14:04                                       ` Marco Elver
2020-05-15 14:07                                       ` Peter Zijlstra
2020-05-14 15:38                                   ` Paul E. McKenney
2020-05-22 16:08                                   ` [tip: locking/kcsan] kcsan: Restrict supported compilers tip-bot2 for Marco Elver
2020-06-03 18:52                                 ` [PATCH v5 00/18] Rework READ_ONCE() to improve codegen Borislav Petkov
2020-06-03 19:23                                   ` Marco Elver
2020-06-03 22:05                                     ` Borislav Petkov
2020-06-08 17:32                                     ` Martin Liška
2020-06-08 19:56                                       ` Marco Elver
2020-06-09 11:55                                         ` Martin Liška
2020-06-09 12:36                                           ` Martin Liška
2020-06-09 13:45                                             ` Marco Elver
2020-05-22 16:08                           ` [tip: locking/kcsan] kcsan: Remove 'noinline' from __no_kcsan_or_inline tip-bot2 for Marco Elver
2020-05-13 13:21                 ` [PATCH v5 00/18] Rework READ_ONCE() to improve codegen David Laight
2020-05-13 16:32                   ` Thomas Gleixner
2020-05-12 21:14       ` Will Deacon
2020-05-12 22:00         ` Marco Elver

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=20200513212520.GC28594@willie-the-truck \
    --to=will@kernel.org \
    --cc=dvyukov@google.com \
    --cc=elver@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH v5 00/18] Rework READ_ONCE() to improve codegen' \
    /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
on how to clone and mirror all data and code used for this inbox