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

[Boqun Feng]

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, paulmck@xxxxxxxxxx wrote:
> From: Marco Elver <elver@xxxxxxxxxx>
> 
> 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 <dvyukov@xxxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Daniel Axtens <dja@xxxxxxxxxx>
> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> Cc: <linux-arch@xxxxxxxxxxxxxxx>
> Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> ---
>  .../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
>