Messages in this thread | | | From | Marco Elver <> | Date | Thu, 6 Feb 2020 16:55:03 +0100 | Subject | Re: [PATCH 2/3] kcsan: Introduce ASSERT_EXCLUSIVE_* macros |
| |
On Wed, 5 Feb 2020 at 23:04, Paul E. McKenney <paulmck@kernel.org> wrote: > > On Wed, Feb 05, 2020 at 10:48:14PM +0100, Marco Elver wrote: > > On Wed, 5 Feb 2020 at 22:33, Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > 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@google.com> > > > > Suggested-by: Paul E. McKenney <paulmck@kernel.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. ;-) > > > > Happy to update the example -- I'll send it tomorrow. > > Sounds great!
v2 sent: http://lkml.kernel.org/r/20200206154626.243230-1-elver@google.com
Thanks, -- Marco
> > > I am queueing these as-is for review and testing, but please feel free > > > to send updated versions. Easy to do the replacement! > > > > Thank you! > > > > > 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? > > > > That works just fine, and will check for races on the whole struct. > > Nice!!! > > Thanx, Paul > > > Thanks, > > -- Marco > > > > > 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 > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > > > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > > > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200205213302.GA2935%40paulmck-ThinkPad-P72.
| |