Re: [PATCH v2 2/2] rust: kcsan: add KUnit test for data race detection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kunit: rust: KCSAN test stalls and spuriously fails on UP/PREEMPT_NONE
--

commit 8335c11e7a640669968685ad30c1312fe11f4a68
Author: Marco Elver <elver@google.com>

rust: kcsan: add KUnit test for data race detection

Add a minimal KUnit test that checks that KCSAN detects data races in
Rust code: two threads concurrently increment a shared counter without
synchronization, and a probe on the console tracepoint waits for the
KCSAN report.

> diff --git a/rust/kernel/Kconfig.test b/rust/kernel/Kconfig.test
> index 3bf4194f28c7c..26bcd91a48211 100644
> --- a/rust/kernel/Kconfig.test
> +++ b/rust/kernel/Kconfig.test
> @@ -104,4 +104,15 @@ config RUST_TIME_KUNIT_TEST
>  
>  	  If unsure, say N.
>  
> +config RUST_KCSAN_KUNIT_TEST
> +	bool "KUnit test for KCSAN instrumentation of Rust code" if !KUNIT_ALL_TESTS
> +	depends on KCSAN && HAVE_KCSAN_RUSTC && TRACEPOINTS

[Severity: High]
Should the RUST_KCSAN_KUNIT_TEST configuration in rust/kernel/Kconfig.test
include a depends on SMP requirement?

The help text notes that the test requires at least 2 CPUs to reliably
observe a data race. Without an SMP dependency, the test can be built
and may spuriously fail on UP configurations.

> +	default KUNIT_ALL_TESTS
> +	help
> +	  This option enables a KUnit test checking that KCSAN detects data
> +	  races in Rust code. The test needs at least 2 CPUs to reliably
> +	  observe a data race.
> +
> +	  If unsure, say N.
> +
>  endif

> diff --git a/rust/kernel/kcsan_test.rs b/rust/kernel/kcsan_test.rs
> new file mode 100644
> index 0000000000000..24810cf2970a0
> --- /dev/null
> +++ b/rust/kernel/kcsan_test.rs
[ ... ]
> +    #[test]
> +    fn data_race() -> Result {
[ ... ]
> +        let worker = ctx.clone();
> +        workqueue::system_unbound().try_spawn(GFP_KERNEL, move || {
> +            while !worker.stop.load(Relaxed) {
> +                racy_inc();
> +            }
> +            worker.done.complete_all();
> +        })?;
> +
> +        let start = Instant::<Monotonic>::now();
> +        while !OBSERVED.load(Relaxed) && start.elapsed() < Delta::from_secs(10) {
> +            racy_inc();
> +        }

[Severity: High]
Could this loop in data_race() lead to a CPU stall or spurious test failure
on uniprocessor systems or when PREEMPT_NONE is set?

The main thread spins tightly in kernel mode for up to 10 seconds without
yielding. If the worker thread is scheduled on the same CPU, it is prevented
from executing.

After 10 seconds, the main thread exits the spin loop, sets the stop flag,
and waits for completion. The worker finally runs, sees the stop flag
immediately, and exits without performing any concurrent increments. The
assert!(OBSERVED.load(Relaxed)) assertion will then spuriously fail.

Would it make sense to yield the CPU during this loop, for example, using
cond_resched()?

> +
> +        ctx.stop.store(true, Relaxed);
> +        ctx.done.wait_for_completion();
> +        drop(_unregister);
> +
> +        assert!(OBSERVED.load(Relaxed));
> +        Ok(())
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015145714.3327543-1-elver@google.com?part=2