Date:   Tue, 11 Feb 2020 17:07:53 +0100
From:   Marco Elver <elver@...gle.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     "Paul E. McKenney" <paulmck@...nel.org>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>, Jan Kara <jack@...e.cz>,
        Qian Cai <cai@....pw>
Subject: Re: [PATCH 5/5] kcsan: Introduce ASSERT_EXCLUSIVE_BITS(var, mask)

v2: https://lore.kernel.org/lkml/20200211160423.138870-5-elver@google.com/

On Mon, 10 Feb 2020 at 22:07, John Hubbard <jhubbard@...dia.com> wrote:
>
> On 2/10/20 10:43 AM, Marco Elver wrote:
> > This introduces ASSERT_EXCLUSIVE_BITS(var, mask).
> > ASSERT_EXCLUSIVE_BITS(var, mask) will cause KCSAN to assume that the
> > following access is safe w.r.t. data races (however, please see the
> > docbook comment for disclaimer here).
> >
> > For more context on why this was considered necessary, please see:
> >   http://lkml.kernel.org/r/1580995070-25139-1-git-send-email-cai@lca.pw
> >
> > In particular, data races between reads (that use @mask bits of an
> > access that should not be modified concurrently) and writes (that change
> > ~@...k bits not used by the read) should ordinarily be marked. After
> > marking these, we would no longer be able to detect harmful races
> > between reads to @mask bits and writes to @mask bits.
>
> I know this is "just" the commit log, but as long as I'm reviewing the
> whole thing...to make the above a little clearer, see if you like this
> revised wording:
>
> In particular, before this patch, data races between reads (that use
> @mask bits of an access that should not be modified concurrently) and
> writes (that change ~@...k bits not used by the readers) would have
> been annotated with "data_race()". However, doing so would then hide
> real problems: we would no longer be able to detect harmful races
> between reads to @mask bits and writes to @mask bits.

Thanks, applied.

> >
> > Therefore, by using ASSERT_EXCLUSIVE_BITS(var, mask), we accomplish:
> >
> >   1. No new macros introduced elsewhere; since there are numerous ways in
> >      which we can extract the same bits, a one-size-fits-all macro is
> >      less preferred.
>
> This somehow confuses me a lot. Maybe say it like this:
>
> 1. Avoid a proliferation of specific macros at the call sites: by including a
>    mask in the argument list, we can use the same macro in a wide variety of
>    call sites, regardless of which bits in a field each call site uses.
>
> ?

Thanks, I took that mostly as-is.

> >
> >   2. The existing code does not need to be modified (although READ_ONCE()
> >      may still be advisable if we cannot prove that the data race is
> >      always safe).
> >
> >   3. We catch bugs where the exclusive bits are modified concurrently.
> >
> >   4. We document properties of the current code.
> >
> > Signed-off-by: Marco Elver <elver@...gle.com>
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > Cc: David Hildenbrand <david@...hat.com>
> > Cc: Jan Kara <jack@...e.cz>
> > Cc: John Hubbard <jhubbard@...dia.com>
> > Cc: Paul E. McKenney <paulmck@...nel.org>
> > Cc: Qian Cai <cai@....pw>
> > ---
> >  include/linux/kcsan-checks.h | 57 ++++++++++++++++++++++++++++++++----
> >  kernel/kcsan/debugfs.c       | 15 +++++++++-
> >  2 files changed, 65 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/kcsan-checks.h b/include/linux/kcsan-checks.h
> > index 4ef5233ff3f04..eae6030cd4348 100644
> > --- a/include/linux/kcsan-checks.h
> > +++ b/include/linux/kcsan-checks.h
> > @@ -152,9 +152,9 @@ static inline void kcsan_check_access(const volatile void *ptr, size_t size,
> >  #endif
> >
> >  /**
> > - * ASSERT_EXCLUSIVE_WRITER - assert no other threads are writing @var
> > + * ASSERT_EXCLUSIVE_WRITER - assert no concurrent writes to @var
> >   *
> > - * Assert that there are no other threads writing @var; other readers are
> > + * Assert that there are no concurrent writes to @var; other readers are
> >   * allowed. This assertion can be used to specify properties of concurrent code,
> >   * where violation cannot be detected as a normal data race.
> >   *
> > @@ -171,11 +171,11 @@ static inline void kcsan_check_access(const volatile void *ptr, size_t size,
> >       __kcsan_check_access(&(var), sizeof(var), KCSAN_ACCESS_ASSERT)
> >
> >  /**
> > - * ASSERT_EXCLUSIVE_ACCESS - assert no other threads are accessing @var
> > + * ASSERT_EXCLUSIVE_ACCESS - assert no concurrent accesses to @var
> >   *
> > - * Assert that no other thread is accessing @var (no readers nor writers). This
> > - * assertion can be used to specify properties of concurrent code, where
> > - * violation cannot be detected as a normal data race.
> > + * Assert that there are no concurrent accesses to @var (no readers nor
> > + * writers). This assertion can be used to specify properties of concurrent
> > + * code, where violation cannot be detected as a normal data race.
> >   *
> >   * For example, in a reference-counting algorithm where exclusive access is
> >   * expected after the refcount reaches 0. We can check that this property
> > @@ -191,4 +191,49 @@ static inline void kcsan_check_access(const volatile void *ptr, size_t size,
> >  #define ASSERT_EXCLUSIVE_ACCESS(var)                                           \
> >       __kcsan_check_access(&(var), sizeof(var), KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT)
> >
> > +/**
> > + * ASSERT_EXCLUSIVE_BITS - assert no concurrent writes to subset of bits in @var
> > + *
> > + * [Bit-granular variant of ASSERT_EXCLUSIVE_WRITER(var)]
>
>
> No need for the square brackets, unless that's some emerging convention in the
> documentation world.

Done.

>
> > + *
> > + * Assert that there are no concurrent writes to a subset of bits in @var;
> > + * concurrent readers are permitted. Concurrent writes (or reads) to ~@...k bits
> > + * are ignored. This assertion can be used to specify properties of concurrent
> > + * code, where marked accesses imply violations cannot be detected as a normal
> > + * data race.
>
>
> How about this wording:
>
> /*
>  * Assert that there are no concurrent writes to a subset of bits in @var;
>  * concurrent readers are permitted. Concurrent writes (or reads) to ~@...k bits
>  * are ignored. This assertion provides more detailed, bit-level information to
>  * the KCSAN system than most of the other (word granularity) annotations. As
>  * such, it allows KCSAN to safely overlook some bits while still continuing to
>  * check the remaining bits for unsafe access patterns.
>  *
>  * Use this if you have some bits that are read-only, and other bits that are
>  * not, within a variable.
>  */
>
> ?

I've updated it based on the information you want to convey here. I've
removed mention to KCSAN in the first paragraph, since KCSAN is an
implementation of this, but a user of the API shouldn't care too much
about that.

Hopefully it makes more sense in v2.

>
> > + *
> > + * For example, this may be used when certain bits of @var may only be modified
> > + * when holding the appropriate lock, but other bits may still be modified
> > + * concurrently. Writers, where other bits may change concurrently, could use
> > + * the assertion as follows:
> > + *
> > + *   spin_lock(&foo_lock);
> > + *   ASSERT_EXCLUSIVE_BITS(flags, FOO_MASK);
> > + *   old_flags = READ_ONCE(flags);
> > + *   new_flags = (old_flags & ~FOO_MASK) | (new_foo << FOO_SHIFT);
> > + *   if (cmpxchg(&flags, old_flags, new_flags) != old_flags) { ... }
> > + *   spin_unlock(&foo_lock);
> > + *
> > + * Readers, could use it as follows:
> > + *
> > + *   ASSERT_EXCLUSIVE_BITS(flags, FOO_MASK);
> > + *   foo = (READ_ONCE(flags) & FOO_MASK) >> FOO_SHIFT;
>
>
> In the general case (which is what this documentation covers), the
> READ_ONCE() is not required. So this should either leave it out, or
> explain that it's not necessarily required.

I've updated the example to lead to the fact you can omit the
READ_ONCE. However, I want to be very careful here, since I still
can't prove to myself no compiler will mess this up. In the general
case, we likely won't need the READ_ONCE, because you'd need a pretty
unfortunate compiler + architecture combo to mess this up for you. But
you never know.

Thanks,
-- Marco

>
> > + *
> > + * NOTE: The access that immediately follows is assumed to access the masked
> > + * bits only, and safe w.r.t. data races. While marking this access is optional
> > + * from KCSAN's point-of-view, it may still be advisable to do so, since we
> > + * cannot reason about all possible compiler optimizations when it comes to bit
> > + * manipulations (on the reader and writer side).
> > + *
> > + * @var variable to assert on
> > + * @mask only check for modifications to bits set in @mask
> > + */
> > +#define ASSERT_EXCLUSIVE_BITS(var, mask)                                       \
>
>
> This API looks good to me.
>
>
> > +     do {                                                                   \
> > +             kcsan_set_access_mask(mask);                                   \
> > +             __kcsan_check_access(&(var), sizeof(var), KCSAN_ACCESS_ASSERT);\
> > +             kcsan_set_access_mask(0);                                      \
> > +             kcsan_atomic_next(1);                                          \
> > +     } while (0)
> > +
> >  #endif /* _LINUX_KCSAN_CHECKS_H */
> > diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> > index 9bbba0e57c9b3..2ff1961239778 100644
> > --- a/kernel/kcsan/debugfs.c
> > +++ b/kernel/kcsan/debugfs.c
> > @@ -100,8 +100,10 @@ static noinline void microbenchmark(unsigned long iters)
> >   * debugfs file from multiple tasks to generate real conflicts and show reports.
> >   */
> >  static long test_dummy;
> > +static long test_flags;
> >  static noinline void test_thread(unsigned long iters)
> >  {
> > +     const long CHANGE_BITS = 0xff00ff00ff00ff00L;
> >       const struct kcsan_ctx ctx_save = current->kcsan_ctx;
> >       cycles_t cycles;
> >
> > @@ -109,16 +111,27 @@ static noinline void test_thread(unsigned long iters)
> >       memset(&current->kcsan_ctx, 0, sizeof(current->kcsan_ctx));
> >
> >       pr_info("KCSAN: %s begin | iters: %lu\n", __func__, iters);
> > +     pr_info("test_dummy@%px, test_flags@%px\n", &test_dummy, &test_flags);
> >
> >       cycles = get_cycles();
> >       while (iters--) {
> > +             /* These all should generate reports. */
> >               __kcsan_check_read(&test_dummy, sizeof(test_dummy));
> > -             __kcsan_check_write(&test_dummy, sizeof(test_dummy));
> >               ASSERT_EXCLUSIVE_WRITER(test_dummy);
> >               ASSERT_EXCLUSIVE_ACCESS(test_dummy);
> >
> > +             ASSERT_EXCLUSIVE_BITS(test_flags, ~CHANGE_BITS); /* no report */
> > +             __kcsan_check_read(&test_flags, sizeof(test_flags)); /* no report */
> > +
> > +             ASSERT_EXCLUSIVE_BITS(test_flags, CHANGE_BITS); /* report */
> > +             __kcsan_check_read(&test_flags, sizeof(test_flags)); /* no report */
> > +
> >               /* not actually instrumented */
> >               WRITE_ONCE(test_dummy, iters);  /* to observe value-change */
> > +             __kcsan_check_write(&test_dummy, sizeof(test_dummy));
> > +
> > +             test_flags ^= CHANGE_BITS; /* generate value-change */
> > +             __kcsan_check_write(&test_flags, sizeof(test_flags));
> >       }
> >       cycles = get_cycles() - cycles;
> >
> >
>
>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Powered by blists - more mailing lists