[PATCH] bitops, kcsan: Partially revert instrumentation for non-atomic bitops

[PATCH] bitops, kcsan: Partially revert instrumentation for non-atomic bitops

[Marco Elver]

Previous to the change to distinguish read-write accesses, when
CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y is set, KCSAN would consider
the non-atomic bitops as atomic. We want to partially revert to this
behaviour, but with one important distinction: report racing
modifications, since lost bits due to non-atomicity are certainly
possible.

Given the operations here only modify a single bit, assuming
non-atomicity of the writer is sufficient may be reasonable for certain
usage (and follows the permissible nature of the "assume plain writes
atomic" rule). In other words:

	1. We want non-atomic read-modify-write races to be reported;
	   this is accomplished by kcsan_check_read(), where any
	   concurrent write (atomic or not) will generate a report.

	2. We do not want to report races with marked readers, but -do-
	   want to report races with unmarked readers; this is
	   accomplished by the instrument_write() ("assume atomic
	   write" with Kconfig option set).

With the above rules, when KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is selected,
it is hoped that KCSAN's reporting behaviour is better aligned with
current expected permissible usage for non-atomic bitops.

Note that, a side-effect of not telling KCSAN that the accesses are
read-writes, is that this information is not displayed in the access
summary in the report. It is, however, visible in inline-expanded stack
traces. For now, it does not make sense to introduce yet another special
case to KCSAN's runtime, only to cater to the case here.

Signed-off-by: Marco Elver <elver@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
As discussed, partially reverting behaviour for non-atomic bitops when
KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is selected.

I'd like to avoid more special cases in KCSAN's runtime to cater to
cases like this, not only because it adds more complexity, but it
invites more special cases to be added. If there are other such
primitives, we likely have to do it on a case-by-case basis as well, and
justify carefully for each such case. But currently, as far as I can
tell, the bitops are truly special, simply because we do know each op
just touches a single bit.
---
 .../bitops/instrumented-non-atomic.h          | 30 +++++++++++++++++--
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/bitops/instrumented-non-atomic.h b/include/asm-generic/bitops/instrumented-non-atomic.h
index f86234c7c10c..37363d570b9b 100644
--- a/include/asm-generic/bitops/instrumented-non-atomic.h
+++ b/include/asm-generic/bitops/instrumented-non-atomic.h
@@ -58,6 +58,30 @@ static inline void __change_bit(long nr, volatile unsigned long *addr)
 	arch___change_bit(nr, addr);
 }
 
+static inline void __instrument_read_write_bitop(long nr, volatile unsigned long *addr)
+{
+	if (IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC)) {
+		/*
+		 * We treat non-atomic read-write bitops a little more special.
+		 * Given the operations here only modify a single bit, assuming
+		 * non-atomicity of the writer is sufficient may be reasonable
+		 * for certain usage (and follows the permissible nature of the
+		 * assume-plain-writes-atomic rule):
+		 * 1. report read-modify-write races -> check read;
+		 * 2. do not report races with marked readers, but do report
+		 *    races with unmarked readers -> check "atomic" write.
+		 */
+		kcsan_check_read(addr + BIT_WORD(nr), sizeof(long));
+		/*
+		 * Use generic write instrumentation, in case other sanitizers
+		 * or tools are enabled alongside KCSAN.
+		 */
+		instrument_write(addr + BIT_WORD(nr), sizeof(long));
+	} else {
+		instrument_read_write(addr + BIT_WORD(nr), sizeof(long));
+	}
+}
+
 /**
  * __test_and_set_bit - Set a bit and return its old value
  * @nr: Bit to set
@@ -68,7 +92,7 @@ static inline void __change_bit(long nr, volatile unsigned long *addr)
  */
 static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
 {
-	instrument_read_write(addr + BIT_WORD(nr), sizeof(long));
+	__instrument_read_write_bitop(nr, addr);
 	return arch___test_and_set_bit(nr, addr);
 }
 
@@ -82,7 +106,7 @@ static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
  */
 static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
 {
-	instrument_read_write(addr + BIT_WORD(nr), sizeof(long));
+	__instrument_read_write_bitop(nr, addr);
 	return arch___test_and_clear_bit(nr, addr);
 }
 
@@ -96,7 +120,7 @@ static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
  */
 static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
 {
-	instrument_read_write(addr + BIT_WORD(nr), sizeof(long));
+	__instrument_read_write_bitop(nr, addr);
 	return arch___test_and_change_bit(nr, addr);
 }
 
-- 
2.28.0.220.ged08abb693-goog

Re: [PATCH] bitops, kcsan: Partially revert instrumentation for non-atomic bitops

[Marco Elver]

On Thu, 13 Aug 2020 at 18:39, Marco Elver <elver@google.com> wrote:
> Previous to the change to distinguish read-write accesses, when
> CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y is set, KCSAN would consider
> the non-atomic bitops as atomic. We want to partially revert to this
> behaviour, but with one important distinction: report racing
> modifications, since lost bits due to non-atomicity are certainly
> possible.
>
> Given the operations here only modify a single bit, assuming
> non-atomicity of the writer is sufficient may be reasonable for certain
> usage (and follows the permissible nature of the "assume plain writes
> atomic" rule). In other words:
>
>         1. We want non-atomic read-modify-write races to be reported;
>            this is accomplished by kcsan_check_read(), where any
>            concurrent write (atomic or not) will generate a report.
>
>         2. We do not want to report races with marked readers, but -do-
>            want to report races with unmarked readers; this is
>            accomplished by the instrument_write() ("assume atomic
>            write" with Kconfig option set).
>
> With the above rules, when KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is selected,
> it is hoped that KCSAN's reporting behaviour is better aligned with
> current expected permissible usage for non-atomic bitops.
>
> Note that, a side-effect of not telling KCSAN that the accesses are
> read-writes, is that this information is not displayed in the access
> summary in the report. It is, however, visible in inline-expanded stack
> traces. For now, it does not make sense to introduce yet another special
> case to KCSAN's runtime, only to cater to the case here.
>
> Signed-off-by: Marco Elver <elver@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
> As discussed, partially reverting behaviour for non-atomic bitops when
> KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is selected.
>
> I'd like to avoid more special cases in KCSAN's runtime to cater to
> cases like this, not only because it adds more complexity, but it
> invites more special cases to be added. If there are other such
> primitives, we likely have to do it on a case-by-case basis as well, and
> justify carefully for each such case. But currently, as far as I can
> tell, the bitops are truly special, simply because we do know each op
> just touches a single bit.
> ---
>  .../bitops/instrumented-non-atomic.h          | 30 +++++++++++++++++--
>  1 file changed, 27 insertions(+), 3 deletions(-)

Paul, if it looks good to you, feel free to pick it up.

Thanks,
-- Marco

Re: [PATCH] bitops, kcsan: Partially revert instrumentation for non-atomic bitops

[Paul E. McKenney]

On Tue, Aug 18, 2020 at 10:34:28AM +0200, Marco Elver wrote:
> On Thu, 13 Aug 2020 at 18:39, Marco Elver <elver@google.com> wrote:
> > Previous to the change to distinguish read-write accesses, when
> > CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y is set, KCSAN would consider
> > the non-atomic bitops as atomic. We want to partially revert to this
> > behaviour, but with one important distinction: report racing
> > modifications, since lost bits due to non-atomicity are certainly
> > possible.
> >
> > Given the operations here only modify a single bit, assuming
> > non-atomicity of the writer is sufficient may be reasonable for certain
> > usage (and follows the permissible nature of the "assume plain writes
> > atomic" rule). In other words:
> >
> >         1. We want non-atomic read-modify-write races to be reported;
> >            this is accomplished by kcsan_check_read(), where any
> >            concurrent write (atomic or not) will generate a report.
> >
> >         2. We do not want to report races with marked readers, but -do-
> >            want to report races with unmarked readers; this is
> >            accomplished by the instrument_write() ("assume atomic
> >            write" with Kconfig option set).
> >
> > With the above rules, when KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is selected,
> > it is hoped that KCSAN's reporting behaviour is better aligned with
> > current expected permissible usage for non-atomic bitops.
> >
> > Note that, a side-effect of not telling KCSAN that the accesses are
> > read-writes, is that this information is not displayed in the access
> > summary in the report. It is, however, visible in inline-expanded stack
> > traces. For now, it does not make sense to introduce yet another special
> > case to KCSAN's runtime, only to cater to the case here.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> > As discussed, partially reverting behaviour for non-atomic bitops when
> > KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is selected.
> >
> > I'd like to avoid more special cases in KCSAN's runtime to cater to
> > cases like this, not only because it adds more complexity, but it
> > invites more special cases to be added. If there are other such
> > primitives, we likely have to do it on a case-by-case basis as well, and
> > justify carefully for each such case. But currently, as far as I can
> > tell, the bitops are truly special, simply because we do know each op
> > just touches a single bit.
> > ---
> >  .../bitops/instrumented-non-atomic.h          | 30 +++++++++++++++++--
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> Paul, if it looks good to you, feel free to pick it up.

Queued, thank you!

							Thanx, Paul