All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic
@ 2020-02-04 14:03 Marco Elver
  2020-02-04 14:03 ` [PATCH 2/3] kcsan: Clarify Kconfig option KCSAN_IGNORE_ATOMICS Marco Elver
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Marco Elver @ 2020-02-04 14:03 UTC (permalink / raw)
  To: elver; +Cc: paulmck, andreyknvl, glider, dvyukov, kasan-dev, linux-kernel

This adds option KCSAN_ASSUME_PLAIN_WRITES_ATOMIC. If enabled, plain
writes up to word size are also assumed to be atomic, and also not
subject to other unsafe compiler optimizations resulting in data races.

This option has been enabled by default to reflect current kernel-wide
preferences.

Signed-off-by: Marco Elver <elver@google.com>
---
 kernel/kcsan/core.c | 20 +++++++++++++++-----
 lib/Kconfig.kcsan   | 26 +++++++++++++++++++-------
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
index 64b30f7716a12..3bd1bf8d6bfeb 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -169,10 +169,19 @@ static __always_inline struct kcsan_ctx *get_ctx(void)
 	return in_task() ? &current->kcsan_ctx : raw_cpu_ptr(&kcsan_cpu_ctx);
 }
 
-static __always_inline bool is_atomic(const volatile void *ptr)
+static __always_inline bool
+is_atomic(const volatile void *ptr, size_t size, int type)
 {
-	struct kcsan_ctx *ctx = get_ctx();
+	struct kcsan_ctx *ctx;
+
+	if ((type & KCSAN_ACCESS_ATOMIC) != 0)
+		return true;
 
+	if (IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC) &&
+	    (type & KCSAN_ACCESS_WRITE) != 0 && size <= sizeof(long))
+		return true; /* Assume all writes up to word size are atomic. */
+
+	ctx = get_ctx();
 	if (unlikely(ctx->atomic_next > 0)) {
 		/*
 		 * Because we do not have separate contexts for nested
@@ -193,7 +202,8 @@ static __always_inline bool is_atomic(const volatile void *ptr)
 	return kcsan_is_atomic(ptr);
 }
 
-static __always_inline bool should_watch(const volatile void *ptr, int type)
+static __always_inline bool
+should_watch(const volatile void *ptr, size_t size, int type)
 {
 	/*
 	 * Never set up watchpoints when memory operations are atomic.
@@ -202,7 +212,7 @@ static __always_inline bool should_watch(const volatile void *ptr, int type)
 	 * should not count towards skipped instructions, and (2) to actually
 	 * decrement kcsan_atomic_next for consecutive instruction stream.
 	 */
-	if ((type & KCSAN_ACCESS_ATOMIC) != 0 || is_atomic(ptr))
+	if (is_atomic(ptr, size, type))
 		return false;
 
 	if (this_cpu_dec_return(kcsan_skip) >= 0)
@@ -460,7 +470,7 @@ static __always_inline void check_access(const volatile void *ptr, size_t size,
 	if (unlikely(watchpoint != NULL))
 		kcsan_found_watchpoint(ptr, size, type, watchpoint,
 				       encoded_watchpoint);
-	else if (unlikely(should_watch(ptr, type)))
+	else if (unlikely(should_watch(ptr, size, type)))
 		kcsan_setup_watchpoint(ptr, size, type);
 }
 
diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 3552990abcfe5..08972376f0454 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -91,13 +91,13 @@ config KCSAN_REPORT_ONCE_IN_MS
 	  limiting reporting to avoid flooding the console with reports.
 	  Setting this to 0 disables rate limiting.
 
-# Note that, while some of the below options could be turned into boot
-# parameters, to optimize for the common use-case, we avoid this because: (a)
-# it would impact performance (and we want to avoid static branch for all
-# {READ,WRITE}_ONCE, atomic_*, bitops, etc.), and (b) complicate the design
-# without real benefit. The main purpose of the below options is for use in
-# fuzzer configs to control reported data races, and they are not expected
-# to be switched frequently by a user.
+# The main purpose of the below options is to control reported data races (e.g.
+# in fuzzer configs), and are not expected to be switched frequently by other
+# users. We could turn some of them into boot parameters, but given they should
+# not be switched normally, let's keep them here to simplify configuration.
+#
+# The defaults below are chosen to be very conservative, and may miss certain
+# bugs.
 
 config KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
 	bool "Report races of unknown origin"
@@ -116,6 +116,18 @@ config KCSAN_REPORT_VALUE_CHANGE_ONLY
 	  the data value of the memory location was observed to remain
 	  unchanged, do not report the data race.
 
+config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
+	bool "Assume that plain writes up to word size are atomic"
+	default y
+	help
+	  Assume that plain writes up to word size are atomic by default, and
+	  also not subject to other unsafe compiler optimizations resulting in
+	  data races. This will cause KCSAN to not report data races due to
+	  conflicts where the only plain accesses are writes up to word size:
+	  conflicts between marked reads and plain writes up to word size will
+	  not be reported as data races; notice that data races between two
+	  conflicting plain writes will also not be reported.
+
 config KCSAN_IGNORE_ATOMICS
 	bool "Do not instrument marked atomic accesses"
 	help
-- 
2.25.0.341.g760bfbb309-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/3] kcsan: Clarify Kconfig option KCSAN_IGNORE_ATOMICS
  2020-02-04 14:03 [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
@ 2020-02-04 14:03 ` Marco Elver
  2020-02-04 14:03 ` [PATCH 3/3] kcsan: Cleanup of main KCSAN Kconfig option Marco Elver
  2020-02-04 15:28 ` [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
  2 siblings, 0 replies; 6+ messages in thread
From: Marco Elver @ 2020-02-04 14:03 UTC (permalink / raw)
  To: elver; +Cc: paulmck, andreyknvl, glider, dvyukov, kasan-dev, linux-kernel

Clarify difference between options KCSAN_IGNORE_ATOMICS and
KCSAN_ASSUME_PLAIN_WRITES_ATOMIC in help text.

Signed-off-by: Marco Elver <elver@google.com>
---
 lib/Kconfig.kcsan | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 08972376f0454..35fab63111d75 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -131,8 +131,17 @@ config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
 config KCSAN_IGNORE_ATOMICS
 	bool "Do not instrument marked atomic accesses"
 	help
-	  If enabled, never instruments marked atomic accesses. This results in
-	  not reporting data races where one access is atomic and the other is
-	  a plain access.
+	  Never instrument marked atomic accesses. This option can be used for
+	  more advanced filtering. Conflicting marked atomic reads and plain
+	  writes will never be reported as a data race, however, will cause
+	  plain reads and marked writes to result in "unknown origin" reports.
+	  If combined with CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN=n, data
+	  races where at least one access is marked atomic will never be
+	  reported.
+
+	  Like KCSAN_ASSUME_PLAIN_WRITES_ATOMIC, conflicting marked atomic
+	  reads and plain writes will not be reported as data races, however,
+	  unlike that option, data races due to two conflicting plain writes
+	  will be reported (if CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n).
 
 endif # KCSAN
-- 
2.25.0.341.g760bfbb309-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/3] kcsan: Cleanup of main KCSAN Kconfig option
  2020-02-04 14:03 [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
  2020-02-04 14:03 ` [PATCH 2/3] kcsan: Clarify Kconfig option KCSAN_IGNORE_ATOMICS Marco Elver
@ 2020-02-04 14:03 ` Marco Elver
  2020-02-04 15:28 ` [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
  2 siblings, 0 replies; 6+ messages in thread
From: Marco Elver @ 2020-02-04 14:03 UTC (permalink / raw)
  To: elver; +Cc: paulmck, andreyknvl, glider, dvyukov, kasan-dev, linux-kernel

This patch cleans up the rules of the 'KCSAN' Kconfig option by:
  1. implicitly selecting 'STACKTRACE' instead of depending on it;
  2. depending on DEBUG_KERNEL, to avoid accidentally turning KCSAN on if
     the kernel is not meant to be a debug kernel;
  3. updating the short and long summaries.

Signed-off-by: Marco Elver <elver@google.com>
---
 lib/Kconfig.kcsan | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 35fab63111d75..0af6301061c03 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -4,12 +4,15 @@ config HAVE_ARCH_KCSAN
 	bool
 
 menuconfig KCSAN
-	bool "KCSAN: watchpoint-based dynamic data race detector"
-	depends on HAVE_ARCH_KCSAN && !KASAN && STACKTRACE
+	bool "KCSAN: dynamic data race detector"
+	depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN
+	select STACKTRACE
 	help
-	  Kernel Concurrency Sanitizer is a dynamic data race detector, which
-	  uses a watchpoint-based sampling approach to detect races. See
-	  <file:Documentation/dev-tools/kcsan.rst> for more details.
+	  The Kernel Concurrency Sanitizer (KCSAN) is a dynamic data race
+	  detector, which relies on compile-time instrumentation, and uses a
+	  watchpoint-based sampling approach to detect data races.
+
+	  See <file:Documentation/dev-tools/kcsan.rst> for more details.
 
 if KCSAN
 
-- 
2.25.0.341.g760bfbb309-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic
  2020-02-04 14:03 [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
  2020-02-04 14:03 ` [PATCH 2/3] kcsan: Clarify Kconfig option KCSAN_IGNORE_ATOMICS Marco Elver
  2020-02-04 14:03 ` [PATCH 3/3] kcsan: Cleanup of main KCSAN Kconfig option Marco Elver
@ 2020-02-04 15:28 ` Marco Elver
  2020-02-04 15:40   ` Paul E. McKenney
  2 siblings, 1 reply; 6+ messages in thread
From: Marco Elver @ 2020-02-04 15:28 UTC (permalink / raw)
  To: Marco Elver
  Cc: Paul E. McKenney, Andrey Konovalov, Alexander Potapenko,
	Dmitry Vyukov, kasan-dev, LKML

On Tue, 4 Feb 2020 at 15:04, Marco Elver <elver@google.com> wrote:
>
> This adds option KCSAN_ASSUME_PLAIN_WRITES_ATOMIC. If enabled, plain
> writes up to word size are also assumed to be atomic, and also not
> subject to other unsafe compiler optimizations resulting in data races.

I just realized we should probably also check for alignedness. Would
this be fair to add as an additional constraint? It would be my
preference.

Thanks,
-- Marco

> This option has been enabled by default to reflect current kernel-wide
> preferences.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  kernel/kcsan/core.c | 20 +++++++++++++++-----
>  lib/Kconfig.kcsan   | 26 +++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> index 64b30f7716a12..3bd1bf8d6bfeb 100644
> --- a/kernel/kcsan/core.c
> +++ b/kernel/kcsan/core.c
> @@ -169,10 +169,19 @@ static __always_inline struct kcsan_ctx *get_ctx(void)
>         return in_task() ? &current->kcsan_ctx : raw_cpu_ptr(&kcsan_cpu_ctx);
>  }
>
> -static __always_inline bool is_atomic(const volatile void *ptr)
> +static __always_inline bool
> +is_atomic(const volatile void *ptr, size_t size, int type)
>  {
> -       struct kcsan_ctx *ctx = get_ctx();
> +       struct kcsan_ctx *ctx;
> +
> +       if ((type & KCSAN_ACCESS_ATOMIC) != 0)
> +               return true;
>
> +       if (IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC) &&
> +           (type & KCSAN_ACCESS_WRITE) != 0 && size <= sizeof(long))
> +               return true; /* Assume all writes up to word size are atomic. */
> +
> +       ctx = get_ctx();
>         if (unlikely(ctx->atomic_next > 0)) {
>                 /*
>                  * Because we do not have separate contexts for nested
> @@ -193,7 +202,8 @@ static __always_inline bool is_atomic(const volatile void *ptr)
>         return kcsan_is_atomic(ptr);
>  }
>
> -static __always_inline bool should_watch(const volatile void *ptr, int type)
> +static __always_inline bool
> +should_watch(const volatile void *ptr, size_t size, int type)
>  {
>         /*
>          * Never set up watchpoints when memory operations are atomic.
> @@ -202,7 +212,7 @@ static __always_inline bool should_watch(const volatile void *ptr, int type)
>          * should not count towards skipped instructions, and (2) to actually
>          * decrement kcsan_atomic_next for consecutive instruction stream.
>          */
> -       if ((type & KCSAN_ACCESS_ATOMIC) != 0 || is_atomic(ptr))
> +       if (is_atomic(ptr, size, type))
>                 return false;
>
>         if (this_cpu_dec_return(kcsan_skip) >= 0)
> @@ -460,7 +470,7 @@ static __always_inline void check_access(const volatile void *ptr, size_t size,
>         if (unlikely(watchpoint != NULL))
>                 kcsan_found_watchpoint(ptr, size, type, watchpoint,
>                                        encoded_watchpoint);
> -       else if (unlikely(should_watch(ptr, type)))
> +       else if (unlikely(should_watch(ptr, size, type)))
>                 kcsan_setup_watchpoint(ptr, size, type);
>  }
>
> diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
> index 3552990abcfe5..08972376f0454 100644
> --- a/lib/Kconfig.kcsan
> +++ b/lib/Kconfig.kcsan
> @@ -91,13 +91,13 @@ config KCSAN_REPORT_ONCE_IN_MS
>           limiting reporting to avoid flooding the console with reports.
>           Setting this to 0 disables rate limiting.
>
> -# Note that, while some of the below options could be turned into boot
> -# parameters, to optimize for the common use-case, we avoid this because: (a)
> -# it would impact performance (and we want to avoid static branch for all
> -# {READ,WRITE}_ONCE, atomic_*, bitops, etc.), and (b) complicate the design
> -# without real benefit. The main purpose of the below options is for use in
> -# fuzzer configs to control reported data races, and they are not expected
> -# to be switched frequently by a user.
> +# The main purpose of the below options is to control reported data races (e.g.
> +# in fuzzer configs), and are not expected to be switched frequently by other
> +# users. We could turn some of them into boot parameters, but given they should
> +# not be switched normally, let's keep them here to simplify configuration.
> +#
> +# The defaults below are chosen to be very conservative, and may miss certain
> +# bugs.
>
>  config KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
>         bool "Report races of unknown origin"
> @@ -116,6 +116,18 @@ config KCSAN_REPORT_VALUE_CHANGE_ONLY
>           the data value of the memory location was observed to remain
>           unchanged, do not report the data race.
>
> +config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
> +       bool "Assume that plain writes up to word size are atomic"
> +       default y
> +       help
> +         Assume that plain writes up to word size are atomic by default, and
> +         also not subject to other unsafe compiler optimizations resulting in
> +         data races. This will cause KCSAN to not report data races due to
> +         conflicts where the only plain accesses are writes up to word size:
> +         conflicts between marked reads and plain writes up to word size will
> +         not be reported as data races; notice that data races between two
> +         conflicting plain writes will also not be reported.
> +
>  config KCSAN_IGNORE_ATOMICS
>         bool "Do not instrument marked atomic accesses"
>         help
> --
> 2.25.0.341.g760bfbb309-goog
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic
  2020-02-04 15:28 ` [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
@ 2020-02-04 15:40   ` Paul E. McKenney
  2020-02-04 17:22     ` Marco Elver
  0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2020-02-04 15:40 UTC (permalink / raw)
  To: Marco Elver
  Cc: Andrey Konovalov, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML

On Tue, Feb 04, 2020 at 04:28:47PM +0100, Marco Elver wrote:
> On Tue, 4 Feb 2020 at 15:04, Marco Elver <elver@google.com> wrote:
> >
> > This adds option KCSAN_ASSUME_PLAIN_WRITES_ATOMIC. If enabled, plain
> > writes up to word size are also assumed to be atomic, and also not
> > subject to other unsafe compiler optimizations resulting in data races.
> 
> I just realized we should probably also check for alignedness. Would
> this be fair to add as an additional constraint? It would be my
> preference.

Checking for alignment makes a lot of sense to me!  Otherwise, write
tearing is expected behavior on some systems.

							Thanx, Paul

> Thanks,
> -- Marco
> 
> > This option has been enabled by default to reflect current kernel-wide
> > preferences.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> >  kernel/kcsan/core.c | 20 +++++++++++++++-----
> >  lib/Kconfig.kcsan   | 26 +++++++++++++++++++-------
> >  2 files changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> > index 64b30f7716a12..3bd1bf8d6bfeb 100644
> > --- a/kernel/kcsan/core.c
> > +++ b/kernel/kcsan/core.c
> > @@ -169,10 +169,19 @@ static __always_inline struct kcsan_ctx *get_ctx(void)
> >         return in_task() ? &current->kcsan_ctx : raw_cpu_ptr(&kcsan_cpu_ctx);
> >  }
> >
> > -static __always_inline bool is_atomic(const volatile void *ptr)
> > +static __always_inline bool
> > +is_atomic(const volatile void *ptr, size_t size, int type)
> >  {
> > -       struct kcsan_ctx *ctx = get_ctx();
> > +       struct kcsan_ctx *ctx;
> > +
> > +       if ((type & KCSAN_ACCESS_ATOMIC) != 0)
> > +               return true;
> >
> > +       if (IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC) &&
> > +           (type & KCSAN_ACCESS_WRITE) != 0 && size <= sizeof(long))
> > +               return true; /* Assume all writes up to word size are atomic. */
> > +
> > +       ctx = get_ctx();
> >         if (unlikely(ctx->atomic_next > 0)) {
> >                 /*
> >                  * Because we do not have separate contexts for nested
> > @@ -193,7 +202,8 @@ static __always_inline bool is_atomic(const volatile void *ptr)
> >         return kcsan_is_atomic(ptr);
> >  }
> >
> > -static __always_inline bool should_watch(const volatile void *ptr, int type)
> > +static __always_inline bool
> > +should_watch(const volatile void *ptr, size_t size, int type)
> >  {
> >         /*
> >          * Never set up watchpoints when memory operations are atomic.
> > @@ -202,7 +212,7 @@ static __always_inline bool should_watch(const volatile void *ptr, int type)
> >          * should not count towards skipped instructions, and (2) to actually
> >          * decrement kcsan_atomic_next for consecutive instruction stream.
> >          */
> > -       if ((type & KCSAN_ACCESS_ATOMIC) != 0 || is_atomic(ptr))
> > +       if (is_atomic(ptr, size, type))
> >                 return false;
> >
> >         if (this_cpu_dec_return(kcsan_skip) >= 0)
> > @@ -460,7 +470,7 @@ static __always_inline void check_access(const volatile void *ptr, size_t size,
> >         if (unlikely(watchpoint != NULL))
> >                 kcsan_found_watchpoint(ptr, size, type, watchpoint,
> >                                        encoded_watchpoint);
> > -       else if (unlikely(should_watch(ptr, type)))
> > +       else if (unlikely(should_watch(ptr, size, type)))
> >                 kcsan_setup_watchpoint(ptr, size, type);
> >  }
> >
> > diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
> > index 3552990abcfe5..08972376f0454 100644
> > --- a/lib/Kconfig.kcsan
> > +++ b/lib/Kconfig.kcsan
> > @@ -91,13 +91,13 @@ config KCSAN_REPORT_ONCE_IN_MS
> >           limiting reporting to avoid flooding the console with reports.
> >           Setting this to 0 disables rate limiting.
> >
> > -# Note that, while some of the below options could be turned into boot
> > -# parameters, to optimize for the common use-case, we avoid this because: (a)
> > -# it would impact performance (and we want to avoid static branch for all
> > -# {READ,WRITE}_ONCE, atomic_*, bitops, etc.), and (b) complicate the design
> > -# without real benefit. The main purpose of the below options is for use in
> > -# fuzzer configs to control reported data races, and they are not expected
> > -# to be switched frequently by a user.
> > +# The main purpose of the below options is to control reported data races (e.g.
> > +# in fuzzer configs), and are not expected to be switched frequently by other
> > +# users. We could turn some of them into boot parameters, but given they should
> > +# not be switched normally, let's keep them here to simplify configuration.
> > +#
> > +# The defaults below are chosen to be very conservative, and may miss certain
> > +# bugs.
> >
> >  config KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
> >         bool "Report races of unknown origin"
> > @@ -116,6 +116,18 @@ config KCSAN_REPORT_VALUE_CHANGE_ONLY
> >           the data value of the memory location was observed to remain
> >           unchanged, do not report the data race.
> >
> > +config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
> > +       bool "Assume that plain writes up to word size are atomic"
> > +       default y
> > +       help
> > +         Assume that plain writes up to word size are atomic by default, and
> > +         also not subject to other unsafe compiler optimizations resulting in
> > +         data races. This will cause KCSAN to not report data races due to
> > +         conflicts where the only plain accesses are writes up to word size:
> > +         conflicts between marked reads and plain writes up to word size will
> > +         not be reported as data races; notice that data races between two
> > +         conflicting plain writes will also not be reported.
> > +
> >  config KCSAN_IGNORE_ATOMICS
> >         bool "Do not instrument marked atomic accesses"
> >         help
> > --
> > 2.25.0.341.g760bfbb309-goog
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic
  2020-02-04 15:40   ` Paul E. McKenney
@ 2020-02-04 17:22     ` Marco Elver
  0 siblings, 0 replies; 6+ messages in thread
From: Marco Elver @ 2020-02-04 17:22 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Andrey Konovalov, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML

On Tue, 4 Feb 2020 at 16:40, Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Tue, Feb 04, 2020 at 04:28:47PM +0100, Marco Elver wrote:
> > On Tue, 4 Feb 2020 at 15:04, Marco Elver <elver@google.com> wrote:
> > >
> > > This adds option KCSAN_ASSUME_PLAIN_WRITES_ATOMIC. If enabled, plain
> > > writes up to word size are also assumed to be atomic, and also not
> > > subject to other unsafe compiler optimizations resulting in data races.
> >
> > I just realized we should probably also check for alignedness. Would
> > this be fair to add as an additional constraint? It would be my
> > preference.
>
> Checking for alignment makes a lot of sense to me!  Otherwise, write
> tearing is expected behavior on some systems.

Sent v2: http://lkml.kernel.org/r/20200204172112.234455-1-elver@google.com

Thanks,
-- Marco

>                                                         Thanx, Paul
>
> > Thanks,
> > -- Marco
> >
> > > This option has been enabled by default to reflect current kernel-wide
> > > preferences.
> > >
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > ---
> > >  kernel/kcsan/core.c | 20 +++++++++++++++-----
> > >  lib/Kconfig.kcsan   | 26 +++++++++++++++++++-------
> > >  2 files changed, 34 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> > > index 64b30f7716a12..3bd1bf8d6bfeb 100644
> > > --- a/kernel/kcsan/core.c
> > > +++ b/kernel/kcsan/core.c
> > > @@ -169,10 +169,19 @@ static __always_inline struct kcsan_ctx *get_ctx(void)
> > >         return in_task() ? &current->kcsan_ctx : raw_cpu_ptr(&kcsan_cpu_ctx);
> > >  }
> > >
> > > -static __always_inline bool is_atomic(const volatile void *ptr)
> > > +static __always_inline bool
> > > +is_atomic(const volatile void *ptr, size_t size, int type)
> > >  {
> > > -       struct kcsan_ctx *ctx = get_ctx();
> > > +       struct kcsan_ctx *ctx;
> > > +
> > > +       if ((type & KCSAN_ACCESS_ATOMIC) != 0)
> > > +               return true;
> > >
> > > +       if (IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC) &&
> > > +           (type & KCSAN_ACCESS_WRITE) != 0 && size <= sizeof(long))
> > > +               return true; /* Assume all writes up to word size are atomic. */
> > > +
> > > +       ctx = get_ctx();
> > >         if (unlikely(ctx->atomic_next > 0)) {
> > >                 /*
> > >                  * Because we do not have separate contexts for nested
> > > @@ -193,7 +202,8 @@ static __always_inline bool is_atomic(const volatile void *ptr)
> > >         return kcsan_is_atomic(ptr);
> > >  }
> > >
> > > -static __always_inline bool should_watch(const volatile void *ptr, int type)
> > > +static __always_inline bool
> > > +should_watch(const volatile void *ptr, size_t size, int type)
> > >  {
> > >         /*
> > >          * Never set up watchpoints when memory operations are atomic.
> > > @@ -202,7 +212,7 @@ static __always_inline bool should_watch(const volatile void *ptr, int type)
> > >          * should not count towards skipped instructions, and (2) to actually
> > >          * decrement kcsan_atomic_next for consecutive instruction stream.
> > >          */
> > > -       if ((type & KCSAN_ACCESS_ATOMIC) != 0 || is_atomic(ptr))
> > > +       if (is_atomic(ptr, size, type))
> > >                 return false;
> > >
> > >         if (this_cpu_dec_return(kcsan_skip) >= 0)
> > > @@ -460,7 +470,7 @@ static __always_inline void check_access(const volatile void *ptr, size_t size,
> > >         if (unlikely(watchpoint != NULL))
> > >                 kcsan_found_watchpoint(ptr, size, type, watchpoint,
> > >                                        encoded_watchpoint);
> > > -       else if (unlikely(should_watch(ptr, type)))
> > > +       else if (unlikely(should_watch(ptr, size, type)))
> > >                 kcsan_setup_watchpoint(ptr, size, type);
> > >  }
> > >
> > > diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
> > > index 3552990abcfe5..08972376f0454 100644
> > > --- a/lib/Kconfig.kcsan
> > > +++ b/lib/Kconfig.kcsan
> > > @@ -91,13 +91,13 @@ config KCSAN_REPORT_ONCE_IN_MS
> > >           limiting reporting to avoid flooding the console with reports.
> > >           Setting this to 0 disables rate limiting.
> > >
> > > -# Note that, while some of the below options could be turned into boot
> > > -# parameters, to optimize for the common use-case, we avoid this because: (a)
> > > -# it would impact performance (and we want to avoid static branch for all
> > > -# {READ,WRITE}_ONCE, atomic_*, bitops, etc.), and (b) complicate the design
> > > -# without real benefit. The main purpose of the below options is for use in
> > > -# fuzzer configs to control reported data races, and they are not expected
> > > -# to be switched frequently by a user.
> > > +# The main purpose of the below options is to control reported data races (e.g.
> > > +# in fuzzer configs), and are not expected to be switched frequently by other
> > > +# users. We could turn some of them into boot parameters, but given they should
> > > +# not be switched normally, let's keep them here to simplify configuration.
> > > +#
> > > +# The defaults below are chosen to be very conservative, and may miss certain
> > > +# bugs.
> > >
> > >  config KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
> > >         bool "Report races of unknown origin"
> > > @@ -116,6 +116,18 @@ config KCSAN_REPORT_VALUE_CHANGE_ONLY
> > >           the data value of the memory location was observed to remain
> > >           unchanged, do not report the data race.
> > >
> > > +config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
> > > +       bool "Assume that plain writes up to word size are atomic"
> > > +       default y
> > > +       help
> > > +         Assume that plain writes up to word size are atomic by default, and
> > > +         also not subject to other unsafe compiler optimizations resulting in
> > > +         data races. This will cause KCSAN to not report data races due to
> > > +         conflicts where the only plain accesses are writes up to word size:
> > > +         conflicts between marked reads and plain writes up to word size will
> > > +         not be reported as data races; notice that data races between two
> > > +         conflicting plain writes will also not be reported.
> > > +
> > >  config KCSAN_IGNORE_ATOMICS
> > >         bool "Do not instrument marked atomic accesses"
> > >         help
> > > --
> > > 2.25.0.341.g760bfbb309-goog
> > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-02-04 17:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 14:03 [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
2020-02-04 14:03 ` [PATCH 2/3] kcsan: Clarify Kconfig option KCSAN_IGNORE_ATOMICS Marco Elver
2020-02-04 14:03 ` [PATCH 3/3] kcsan: Cleanup of main KCSAN Kconfig option Marco Elver
2020-02-04 15:28 ` [PATCH 1/3] kcsan: Add option to assume plain writes up to word size are atomic Marco Elver
2020-02-04 15:40   ` Paul E. McKenney
2020-02-04 17:22     ` Marco Elver

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.