| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Feb 2020 13:41:14 -0800
From: John Hubbard <jhubbard@...dia.com>
To: Marco Elver <elver@...gle.com>
CC: <paulmck@...nel.org>, <andreyknvl@...gle.com>, <glider@...gle.com>,
<dvyukov@...gle.com>, <kasan-dev@...glegroups.com>,
<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 v2 5/5] kcsan: Introduce ASSERT_EXCLUSIVE_BITS(var, mask)
On 2/11/20 8:04 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, 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()" (or "READ_ONCE()"). 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.
>
> Therefore, by using ASSERT_EXCLUSIVE_BITS(var, mask), we accomplish:
>
> 1. Avoid proliferation of specific macros at the call sites: by
> including a single mask in the argument list, we can use the same
> macro in a wide variety of call sites, regardless of how and which
> bits in a field each call site actually accesses.
>
> 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.
API looks good to me. (I'm not yet familiar enough with KCSAN to provide
any useful review of about the various kcsan*() calls that implement the
new macro.)
btw, it might be helpful for newcomers if you mentioned which tree this
is based on. I poked around briefly and failed several times to find one. :)
You can add:
Acked-by: John Hubbard <jhubbard@...dia.com>
thanks,
--
John Hubbard
NVIDIA
>
> 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>
> ---
> v2:
> * Update API documentation to be clearer about how this compares to the
> existing assertions, and update use-cases. [Based on suggestions from
> John Hubbard]
> * Update commit message. [Suggestions from John Hubbard]
> ---
> include/linux/kcsan-checks.h | 69 ++++++++++++++++++++++++++++++++----
> kernel/kcsan/debugfs.c | 15 +++++++-
> 2 files changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/kcsan-checks.h b/include/linux/kcsan-checks.h
> index 4ef5233ff3f04..1b8aac5d6a0b5 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,61 @@ 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).
> + *
> + * Assert that there are no concurrent writes to a subset of bits in @var;
> + * concurrent readers are permitted. This assertion captures more detailed
> + * bit-level properties, compared to the other (word granularity) assertions.
> + * Only the bits set in @mask are checked for concurrent modifications, while
> + * ignoring the remaining bits, i.e. concurrent writes (or reads) to ~@...k bits
> + * are ignored.
> + *
> + * Use this for variables, where some bits must not be modified concurrently,
> + * yet other bits are expected to be modified concurrently.
> + *
> + * For example, variables where, after initialization, some bits are read-only,
> + * but other bits may still be modified concurrently. A reader may wish to
> + * assert that this is true as follows:
> + *
> + * ASSERT_EXCLUSIVE_BITS(flags, READ_ONLY_MASK);
> + * foo = (READ_ONCE(flags) & READ_ONLY_MASK) >> READ_ONLY_SHIFT;
> + *
> + * Note: The access that immediately follows ASSERT_EXCLUSIVE_BITS() is
> + * assumed to access the masked bits only, and KCSAN optimistically assumes it
> + * is therefore safe, even in the presence of data races, and marking it with
> + * READ_ONCE() is optional from KCSAN's point-of-view. We caution, however,
> + * that it may still be advisable to do so, since we cannot reason about all
> + * compiler optimizations when it comes to bit manipulations (on the reader
> + * and writer side). If you are sure nothing can go wrong, we can write the
> + * above simply as:
> + *
> + * ASSERT_EXCLUSIVE_BITS(flags, READ_ONLY_MASK);
> + * foo = (flags & READ_ONLY_MASK) >> READ_ONLY_SHIFT;
> + *
> + * Another example, where this may be used, is 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);
> + *
> + * @var variable to assert on
> + * @mask only check for modifications to bits set in @mask
> + */
> +#define ASSERT_EXCLUSIVE_BITS(var, mask) \
> + 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(¤t->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;
>
>
Powered by blists - more mailing lists