Date:   Wed, 5 Feb 2020 13:33:02 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Marco Elver <elver@...gle.com>
Cc:     andreyknvl@...gle.com, glider@...gle.com, dvyukov@...gle.com,
        kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] kcsan: Introduce ASSERT_EXCLUSIVE_* macros

On Wed, Feb 05, 2020 at 09:43:32PM +0100, Marco Elver wrote:
> Introduces ASSERT_EXCLUSIVE_WRITER and ASSERT_EXCLUSIVE_ACCESS, which
> may be used to assert properties of synchronization logic, where
> violation cannot be detected as a normal data race.
> 
> Examples of the reports that may be generated:
> 
>     ==================================================================
>     BUG: KCSAN: data-race in test_thread / test_thread
> 
>     write to 0xffffffffab3d1540 of 8 bytes by task 466 on cpu 2:
>      test_thread+0x8d/0x111
>      debugfs_write.cold+0x32/0x44
>      ...
> 
>     assert no writes to 0xffffffffab3d1540 of 8 bytes by task 464 on cpu 0:
>      test_thread+0xa3/0x111
>      debugfs_write.cold+0x32/0x44
>      ...
>     ==================================================================
> 
>     ==================================================================
>     BUG: KCSAN: data-race in test_thread / test_thread
> 
>     assert no accesses to 0xffffffffab3d1540 of 8 bytes by task 465 on cpu 1:
>      test_thread+0xb9/0x111
>      debugfs_write.cold+0x32/0x44
>      ...
> 
>     read to 0xffffffffab3d1540 of 8 bytes by task 464 on cpu 0:
>      test_thread+0x77/0x111
>      debugfs_write.cold+0x32/0x44
>      ...
>     ==================================================================
> 
> Signed-off-by: Marco Elver <elver@...gle.com>
> Suggested-by: Paul E. McKenney <paulmck@...nel.org>
> ---
> 
> Please let me know if the names make sense, given they do not include a
> KCSAN_ prefix.

I am OK with this, but there might well be some bikeshedding later on.
Which should not be a real problem, irritating though it might be.

> The names are unique across the kernel. I wouldn't expect another macro
> with the same name but different semantics to pop up any time soon. If
> there is a dual use to these macros (e.g. another tool that could hook
> into it), we could also move it elsewhere (include/linux/compiler.h?).
> 
> We can also revisit the original suggestion of WRITE_ONCE_EXCLUSIVE(),
> if it is something that'd be used very widely. It'd be straightforward
> to add with the help of these macros, but would need to be added to
> include/linux/compiler.h.

A more definite use case for ASSERT_EXCLUSIVE_ACCESS() is a
reference-counting algorithm where exclusive access is expected after
a successful atomic_dec_and_test().  Any objection to making the
docbook header use that example?  I believe that a more familiar
example would help people see the point of all this.  ;-)

I am queueing these as-is for review and testing, but please feel free
to send updated versions.  Easy to do the replacement!

And you knew that this was coming...  It looks to me that I can
do something like this:

	struct foo {
		int a;
		char b;
		long c;
		atomic_t refctr;
	};

	void do_a_foo(struct foo *fp)
	{
		if (atomic_dec_and_test(&fp->refctr)) {
			ASSERT_EXCLUSIVE_ACCESS(*fp);
			safely_dispose_of(fp);
		}
	}

Does that work, or is it necessary to assert for each field separately?

							Thanx, Paul

> ---
>  include/linux/kcsan-checks.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/linux/kcsan-checks.h b/include/linux/kcsan-checks.h
> index 21b1d1f214ad5..1a7b51e516335 100644
> --- a/include/linux/kcsan-checks.h
> +++ b/include/linux/kcsan-checks.h
> @@ -96,4 +96,38 @@ static inline void kcsan_check_access(const volatile void *ptr, size_t size,
>  	kcsan_check_access(ptr, size, KCSAN_ACCESS_ATOMIC | KCSAN_ACCESS_WRITE)
>  #endif
>  
> +/**
> + * ASSERT_EXCLUSIVE_WRITER - assert no other threads are writing @var
> + *
> + * Assert that there are no other threads writing @var; other readers are
> + * allowed. This assertion can be used to specify properties of synchronization
> + * logic, where violation cannot be detected as a normal data race.
> + *
> + * For example, if a per-CPU variable is only meant to be written by a single
> + * CPU, but may be read from other CPUs; in this case, reads and writes must be
> + * marked properly, however, if an off-CPU WRITE_ONCE() races with the owning
> + * CPU's WRITE_ONCE(), would not constitute a data race but could be a harmful
> + * race condition. Using this macro allows specifying this property in the code
> + * and catch such bugs.
> + *
> + * @var variable to assert on
> + */
> +#define ASSERT_EXCLUSIVE_WRITER(var)                                           \
> +	__kcsan_check_access(&(var), sizeof(var), KCSAN_ACCESS_ASSERT)
> +
> +/**
> + * ASSERT_EXCLUSIVE_ACCESS - assert no other threads are accessing @var
> + *
> + * Assert that no other thread is accessing @var (no readers nor writers). This
> + * assertion can be used to specify properties of synchronization logic, where
> + * violation cannot be detected as a normal data race.
> + *
> + * For example, if a variable is not read nor written by the current thread, nor
> + * should it be touched by any other threads during the current execution phase.
> + *
> + * @var variable to assert on
> + */
> +#define ASSERT_EXCLUSIVE_ACCESS(var)                                           \
> +	__kcsan_check_access(&(var), sizeof(var), KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT)
> +
>  #endif /* _LINUX_KCSAN_CHECKS_H */
> -- 
> 2.25.0.341.g760bfbb309-goog
> 

Powered by blists - more mailing lists