2020-03-31 19:34:18

by Marco Elver

Subject: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h

Both affect access checks, and should therefore be in kcsan-checks.h.
This is in preparation to use these in compiler.h.

Signed-off-by: Marco Elver <[email protected]>
---
include/linux/kcsan-checks.h | 16 ++++++++++++++++
include/linux/kcsan.h | 16 ----------------
2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/kcsan-checks.h b/include/linux/kcsan-checks.h
index 101df7f46d89..ef95ddc49182 100644
--- a/include/linux/kcsan-checks.h
+++ b/include/linux/kcsan-checks.h
@@ -36,6 +36,20 @@
*/
void __kcsan_check_access(const volatile void *ptr, size_t size, int type);

+/**
+ * kcsan_disable_current - disable KCSAN for the current context
+ *
+ * Supports nesting.
+ */
+void kcsan_disable_current(void);
+
+/**
+ * kcsan_enable_current - re-enable KCSAN for the current context
+ *
+ * Supports nesting.
+ */
+void kcsan_enable_current(void);
+
/**
* kcsan_nestable_atomic_begin - begin nestable atomic region
*
@@ -133,6 +147,8 @@ void kcsan_end_scoped_access(struct kcsan_scoped_access *sa);
static inline void __kcsan_check_access(const volatile void *ptr, size_t size,
int type) { }

+static inline void kcsan_disable_current(void) { }
+static inline void kcsan_enable_current(void) { }
static inline void kcsan_nestable_atomic_begin(void) { }
static inline void kcsan_nestable_atomic_end(void) { }
static inline void kcsan_flat_atomic_begin(void) { }
diff --git a/include/linux/kcsan.h b/include/linux/kcsan.h
index 17ae59e4b685..53340d8789f9 100644
--- a/include/linux/kcsan.h
+++ b/include/linux/kcsan.h
@@ -50,25 +50,9 @@ struct kcsan_ctx {
*/
void kcsan_init(void);

-/**
- * kcsan_disable_current - disable KCSAN for the current context
- *
- * Supports nesting.
- */
-void kcsan_disable_current(void);
-
-/**
- * kcsan_enable_current - re-enable KCSAN for the current context
- *
- * Supports nesting.
- */
-void kcsan_enable_current(void);
-
#else /* CONFIG_KCSAN */

static inline void kcsan_init(void) { }
-static inline void kcsan_disable_current(void) { }
-static inline void kcsan_enable_current(void) { }

#endif /* CONFIG_KCSAN */

--
2.26.0.rc2.310.g2932bb562d-goog

2020-03-31 19:36:07

by Marco Elver

Subject: [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses

Thus far, accesses marked with data_race() would still require the
racing access to be marked in some way (be it with READ_ONCE(),
WRITE_ONCE(), or data_race() itself), as otherwise KCSAN would still
report a data race. This requirement, however, seems to be unintuitive,
and some valid use-cases demand *not* marking other accesses, as it
might hide more serious bugs (e.g. diagnostic reads).

Therefore, this commit changes data_race() to no longer require marking
racing accesses (although it's still recommended if possible).

The alternative would have been introducing another variant of
data_race(), however, since usage of data_race() already needs to be
carefully reasoned about, distinguishing between these cases likely adds
more complexity in the wrong place.

Link: https://lkml.kernel.org/r/20200331131002.GA30975@willie-the-truck
Signed-off-by: Marco Elver <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Qian Cai <[email protected]>
---
include/linux/compiler.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f504edebd5d7..1729bd17e9b7 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -326,9 +326,9 @@ unsigned long read_word_at_a_time(const void *addr)
#define data_race(expr) \
({ \
typeof(({ expr; })) __val; \
- kcsan_nestable_atomic_begin(); \
+ kcsan_disable_current(); \
__val = ({ expr; }); \
- kcsan_nestable_atomic_end(); \
+ kcsan_enable_current(); \
__val; \
})
#else
--
2.26.0.rc2.310.g2932bb562d-goog

2020-03-31 22:57:02

by Paul E. McKenney

Subject: Re: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h

On Tue, Mar 31, 2020 at 09:32:32PM +0200, Marco Elver wrote:
> Both affect access checks, and should therefore be in kcsan-checks.h.
> This is in preparation to use these in compiler.h.
>
> Signed-off-by: Marco Elver <[email protected]>

The two of these do indeed make data_race() act more like one would
expect, thank you! I have queued them for further testing and review.

Thanx, Paul

> ---
> include/linux/kcsan-checks.h | 16 ++++++++++++++++
> include/linux/kcsan.h | 16 ----------------
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/kcsan-checks.h b/include/linux/kcsan-checks.h
> index 101df7f46d89..ef95ddc49182 100644
> --- a/include/linux/kcsan-checks.h
> +++ b/include/linux/kcsan-checks.h
> @@ -36,6 +36,20 @@
> */
> void __kcsan_check_access(const volatile void *ptr, size_t size, int type);
>
> +/**
> + * kcsan_disable_current - disable KCSAN for the current context
> + *
> + * Supports nesting.
> + */
> +void kcsan_disable_current(void);
> +
> +/**
> + * kcsan_enable_current - re-enable KCSAN for the current context
> + *
> + * Supports nesting.
> + */
> +void kcsan_enable_current(void);
> +
> /**
> * kcsan_nestable_atomic_begin - begin nestable atomic region
> *
> @@ -133,6 +147,8 @@ void kcsan_end_scoped_access(struct kcsan_scoped_access *sa);
> static inline void __kcsan_check_access(const volatile void *ptr, size_t size,
> int type) { }
>
> +static inline void kcsan_disable_current(void) { }
> +static inline void kcsan_enable_current(void) { }
> static inline void kcsan_nestable_atomic_begin(void) { }
> static inline void kcsan_nestable_atomic_end(void) { }
> static inline void kcsan_flat_atomic_begin(void) { }
> diff --git a/include/linux/kcsan.h b/include/linux/kcsan.h
> index 17ae59e4b685..53340d8789f9 100644
> --- a/include/linux/kcsan.h
> +++ b/include/linux/kcsan.h
> @@ -50,25 +50,9 @@ struct kcsan_ctx {
> */
> void kcsan_init(void);
>
> -/**
> - * kcsan_disable_current - disable KCSAN for the current context
> - *
> - * Supports nesting.
> - */
> -void kcsan_disable_current(void);
> -
> -/**
> - * kcsan_enable_current - re-enable KCSAN for the current context
> - *
> - * Supports nesting.
> - */
> -void kcsan_enable_current(void);
> -
> #else /* CONFIG_KCSAN */
>
> static inline void kcsan_init(void) { }
> -static inline void kcsan_disable_current(void) { }
> -static inline void kcsan_enable_current(void) { }
>
> #endif /* CONFIG_KCSAN */
>
> --
> 2.26.0.rc2.310.g2932bb562d-goog
>

2020-04-01 08:39:13

by Will Deacon

Subject: Re: [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses

On Tue, Mar 31, 2020 at 09:32:33PM +0200, Marco Elver wrote:
> Thus far, accesses marked with data_race() would still require the
> racing access to be marked in some way (be it with READ_ONCE(),
> WRITE_ONCE(), or data_race() itself), as otherwise KCSAN would still
> report a data race. This requirement, however, seems to be unintuitive,
> and some valid use-cases demand *not* marking other accesses, as it
> might hide more serious bugs (e.g. diagnostic reads).
>
> Therefore, this commit changes data_race() to no longer require marking
> racing accesses (although it's still recommended if possible).
>
> The alternative would have been introducing another variant of
> data_race(), however, since usage of data_race() already needs to be
> carefully reasoned about, distinguishing between these cases likely adds
> more complexity in the wrong place.

Just a thought, but perhaps worth extending scripts/checkpatch.pl to
check for use of data_race() without a comment? We already have that for
memory barriers, so should be easy enough to extend with any luck.

> Link: https://lkml.kernel.org/r/20200331131002.GA30975@willie-the-truck
> Signed-off-by: Marco Elver <[email protected]>
> Cc: Paul E. McKenney <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Qian Cai <[email protected]>
> ---
> include/linux/compiler.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index f504edebd5d7..1729bd17e9b7 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -326,9 +326,9 @@ unsigned long read_word_at_a_time(const void *addr)
> #define data_race(expr) \
> ({ \
> typeof(({ expr; })) __val; \
> - kcsan_nestable_atomic_begin(); \
> + kcsan_disable_current(); \
> __val = ({ expr; }); \
> - kcsan_nestable_atomic_end(); \
> + kcsan_enable_current(); \
> __val; \
> })
> #else

Acked-by: Will Deacon <[email protected]>

Will

2020-04-01 08:42:20

by Will Deacon

Subject: Re: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h

On Tue, Mar 31, 2020 at 09:32:32PM +0200, Marco Elver wrote:
> Both affect access checks, and should therefore be in kcsan-checks.h.
> This is in preparation to use these in compiler.h.
>
> Signed-off-by: Marco Elver <[email protected]>
> ---
> include/linux/kcsan-checks.h | 16 ++++++++++++++++
> include/linux/kcsan.h | 16 ----------------
> 2 files changed, 16 insertions(+), 16 deletions(-)

Acked-by: Will Deacon <[email protected]>

Will

2020-04-01 16:39:07

by Paul E. McKenney

Subject: Re: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h

On Wed, Apr 01, 2020 at 09:40:02AM +0100, Will Deacon wrote:
> On Tue, Mar 31, 2020 at 09:32:32PM +0200, Marco Elver wrote:
> > Both affect access checks, and should therefore be in kcsan-checks.h.
> > This is in preparation to use these in compiler.h.
> >
> > Signed-off-by: Marco Elver <[email protected]>
> > ---
> > include/linux/kcsan-checks.h | 16 ++++++++++++++++
> > include/linux/kcsan.h | 16 ----------------
> > 2 files changed, 16 insertions(+), 16 deletions(-)
>
> Acked-by: Will Deacon <[email protected]>

Applied both acks, thank you!

Thanx, Paul