Hi Paul and Marco,
The whole update patchset looks good to me, just one question out of
curiosity fo this one, please see below:
On Mon, Aug 31, 2020 at 11:18:04AM -0700, paul...@kernel.org wrote:
> From: Marco Elver <el...@google.com>
>
> 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).
>
Is there any code in kernel using the above assumption (i.e.
non-atomicity of the writer is sufficient)? IOW, have you observed
anything bad (e.g. an anoying false positive) after applying the
read_write changes but without this patch?
Regards,
Boqun
> 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.
>
> Cc: Dmitry Vyukov <dvyu...@google.com>
> Cc: Paul E. McKenney <paul...@kernel.org>
> Cc: Will Deacon <w...@kernel.org>
> Cc: Arnd Bergmann <a...@arndb.de>
> Cc: Daniel Axtens <d...@axtens.net>
> Cc: Michael Ellerman <m...@ellerman.id.au>
> Cc: <linux-a...@vger.kernel.org>
> Signed-off-by: Marco Elver <el...@google.com>
> Signed-off-by: Paul E. McKenney <paul...@kernel.org>
> ---
> .../asm-generic/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 f86234c..37363d5 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.9.5
>
