* [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h
@ 2020-03-31 19:32 Marco Elver
2020-03-31 19:32 ` [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses Marco Elver
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Marco Elver @ 2020-03-31 19:32 UTC (permalink / raw)
To: elver; +Cc: paulmck, dvyukov, glider, andreyknvl, cai, kasan-dev, linux-kernel
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 <elver@google.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses
2020-03-31 19:32 [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Marco Elver
@ 2020-03-31 19:32 ` Marco Elver
2020-04-01 8:38 ` Will Deacon
2020-03-31 22:55 ` [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Paul E. McKenney
2020-04-01 8:40 ` Will Deacon
2 siblings, 1 reply; 6+ messages in thread
From: Marco Elver @ 2020-03-31 19:32 UTC (permalink / raw)
To: elver
Cc: paulmck, dvyukov, glider, andreyknvl, cai, kasan-dev,
linux-kernel, Will Deacon
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 <elver@google.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Qian Cai <cai@lca.pw>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h
2020-03-31 19:32 [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Marco Elver
2020-03-31 19:32 ` [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses Marco Elver
@ 2020-03-31 22:55 ` Paul E. McKenney
2020-04-01 8:40 ` Will Deacon
2 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2020-03-31 22:55 UTC (permalink / raw)
To: Marco Elver; +Cc: dvyukov, glider, andreyknvl, cai, kasan-dev, linux-kernel
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 <elver@google.com>
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses
2020-03-31 19:32 ` [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses Marco Elver
@ 2020-04-01 8:38 ` Will Deacon
0 siblings, 0 replies; 6+ messages in thread
From: Will Deacon @ 2020-04-01 8:38 UTC (permalink / raw)
To: Marco Elver
Cc: paulmck, dvyukov, glider, andreyknvl, cai, kasan-dev, linux-kernel
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 <elver@google.com>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Qian Cai <cai@lca.pw>
> ---
> 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 <will@kernel.org>
Will
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h
2020-03-31 19:32 [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Marco Elver
2020-03-31 19:32 ` [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses Marco Elver
2020-03-31 22:55 ` [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Paul E. McKenney
@ 2020-04-01 8:40 ` Will Deacon
2020-04-01 16:37 ` Paul E. McKenney
2 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2020-04-01 8:40 UTC (permalink / raw)
To: Marco Elver
Cc: paulmck, dvyukov, glider, andreyknvl, cai, kasan-dev, linux-kernel
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 <elver@google.com>
> ---
> include/linux/kcsan-checks.h | 16 ++++++++++++++++
> include/linux/kcsan.h | 16 ----------------
> 2 files changed, 16 insertions(+), 16 deletions(-)
Acked-by: Will Deacon <will@kernel.org>
Will
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h
2020-04-01 8:40 ` Will Deacon
@ 2020-04-01 16:37 ` Paul E. McKenney
0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2020-04-01 16:37 UTC (permalink / raw)
To: Will Deacon
Cc: Marco Elver, dvyukov, glider, andreyknvl, cai, kasan-dev, linux-kernel
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 <elver@google.com>
> > ---
> > include/linux/kcsan-checks.h | 16 ++++++++++++++++
> > include/linux/kcsan.h | 16 ----------------
> > 2 files changed, 16 insertions(+), 16 deletions(-)
>
> Acked-by: Will Deacon <will@kernel.org>
Applied both acks, thank you!
Thanx, Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-01 16:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31 19:32 [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Marco Elver
2020-03-31 19:32 ` [PATCH 2/2] kcsan: Change data_race() to no longer require marking racing accesses Marco Elver
2020-04-01 8:38 ` Will Deacon
2020-03-31 22:55 ` [PATCH 1/2] kcsan: Move kcsan_{disable,enable}_current() to kcsan-checks.h Paul E. McKenney
2020-04-01 8:40 ` Will Deacon
2020-04-01 16:37 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).