leds: KCSAN report

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 7428 bytes --]

On Fri, 2026-10-09 at 13:08 -0700, Boqun Feng wrote:
> On Fri, Oct 09, 2026 at 07:59:02PM +0000, Markus Probst wrote:
> [..]
> > > > > > +/// The multicolor sub led info representation.
> > > > > > +///
> > > > > > +/// This structure represents the Rust abstraction for a C `struct mc_subled`.
> > > > > > +#[repr(C)]
> > > > > > +#[derive(Debug)]
> > > > > > +#[non_exhaustive]
> > > > > > +pub struct MultiColorSubLed {
> > > > > > +    /// The color of the sub led
> > > > > > +    pub color: Color,
> > > > > > +    brightness: UnsafeCell<u32>,
> > > > > > +    intensity: UnsafeCell<u32>,
> > > > > 
> > > > > These should be `Atomic<u32>`, or am I missing something here? Using
> > > > > `Atomic<u32>` should resolve sashiko's comment on this patch.
> > > > Snippet of the `Atomic::from_ptr` rustdoc:
> > > > 
> > > > "
> > > > For the duration of 'a, other accesses to *ptr must not cause data
> > > > races (defined by LKMM) against atomic operations on the returned
> > > > reference. Note that if all other accesses are atomic, then this safety
> > > > requirement is trivially fulfilled.
> > > > "
> > > > 
> > > > This safety requirement is likely not met if I see this correctly,
> > > > because the led subsystem does not use atomic accesses.
> > > > 
> > > 
> > > Then the C side has a data race that needs some fix (or they use
> > > READ_ONCE() or WRITE_ONCE() which are *atomic* to avoid the data race).
> > They don't use READ_ONCE or WRITE_ONCE.
> > 
> > Writes to "intensity" can happen at anytime by `multi_intensity_store`.
> > It does lock the `led_access` mutex on write. It is not locked on read
> > and `grep "READ_ONCE" -r drivers/leds/` has no matches in drivers
> > either.
> > 
> 
> I wonder whether KCSAN will report an issue of this (w/o
> CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC).
First of all, thats a pretty noticable performance impact on desktop
here. Gotta recompile my kernel very soon.

Second of all, it does.
(Also like every second reports with something else, e.g. vfs, tty,
_find_next_bit, btrfs and more).

Reproduced with:
- Software to write random values to "multi_intensity":
https://gist.github.com/0xIO32/2ec10e7521fc80e96f700e1f6fe219c4
- Self-written low-quality multicolor led driver (to not overload real
led hardware for this test)
https://gist.github.com/0xIO32/1b46b8965a9d52f6e3ec5355c1f054a7
- the timer led trigger with delay_on = 1 and delay_off = 1 has been
enabled, so there is concurrent access.

Tainted because nvidia drivers, external module: v4l2loopback.
Gentoo Kernel, running on desktop. Its on 6.18, but as far as I know,
this logic hasn't changed (and fixed would be backported).

[  273.080793] Reported by Kernel Concurrency Sanitizer on:
[  273.080805] CPU: 1 UID: 0 PID: 4070 Comm: write_intensity Tainted: P
O        6.18.54 #1 PREEMPT(lazy)
[  273.080826] Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE
[  273.080836] Hardware name: Micro-Star International Co., Ltd. MS-
7C56/MPG B550 GAMING PLUS (MS-7C56), BIOS 1.K0 09/02/2025
[  273.080847]
==================================================================
[  275.913835]
==================================================================
[  275.913855] BUG: KCSAN: data-race in led_mc_calc_color_components /
multi_intensity_store

[  275.913883] write to 0xffff8a84e71d1c70 of 4 bytes by task 4067 on
cpu 8:
[  275.913896]  multi_intensity_store+0x1a4/0x2b0
[  275.913912]  dev_attr_store+0x41/0x60
[  275.913931]  sysfs_kf_write+0x192/0x1d0
[  275.913948]  kernfs_fop_write_iter+0x1cb/0x400
[  275.913964]  vfs_write+0x5ad/0x650
[  275.913977]  __x64_sys_pwrite64+0xaf/0x100
[  275.913992]  x64_sys_call+0x206c/0x24c0
[  275.914006]  do_syscall_64+0x89/0x390
[  275.914022]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

[  275.914043] read to 0xffff8a84e71d1c70 of 4 bytes by interrupt on
cpu 3:
[  275.914056]  led_mc_calc_color_components+0x87/0xf0
[  275.914072]  led_set_brightness_nopm+0x2f/0xd0
[  275.914093]  led_timer_function+0x1f5/0x2c0
[  275.914106]  call_timer_fn+0x32/0x1e0
[  275.914126]  __run_timer_base+0x7b3/0x980
[  275.914146]  run_timer_softirq+0x31/0x60
[  275.914166]  handle_softirqs+0x157/0x400
[  275.914181]  __irq_exit_rcu+0xb9/0x200
[  275.914195]  sysvec_apic_timer_interrupt+0x7a/0x90
[  275.914212]  asm_sysvec_apic_timer_interrupt+0x1a/0x20
[  275.914228]  osq_lock+0x121/0x260
[  275.914246]  __mutex_lock+0x172/0xf70
[  275.914261]  __mutex_lock_slowpath+0xf/0x20
[  275.914278]  mutex_lock+0x9f/0xb0
[  275.914293]  kernfs_fop_write_iter+0x11b/0x400
[  275.914310]  vfs_write+0x5ad/0x650
[  275.914322]  __x64_sys_pwrite64+0xaf/0x100
[  275.914337]  x64_sys_call+0x206c/0x24c0
[  275.914351]  do_syscall_64+0x89/0x390
[  275.914367]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

[  275.914389] value changed: 0x000000f8 -> 0x00000034

Thanks
- Markus Probst

> 
> > Writes to "brightness" are on the C-side handled by the driver by
> > calling `led_mc_calc_color_components`. This rust abstraction always
> > calles it in `brightness_set_callback`. So on the C-side, this at least
> > is less of an issue, as writes and reads are controlled by the C
> > driver.
> > 
> 
> Thank you for taking a look into this.
> 
> > > 
> > > The general rule is: if C side has a data race, they should fix it, if C
> > > side doesn't care ("the compiler should not data race on this code"),
> > > then the Rust side treat it as atomic operations. This is the only way
> > > to better code regarding data races.
> > It probably should use WRITE_ONCE and READ_ONCE, but it also shouldn't
> > create any issues if its not used. There is no load tearing on a 32-bit
> > integer and memory ordering is not required. Not sure if its worth the
> 
> I think some people would disagree with you on "no load tearing"
> (because data race = UB = anything can happen), but..
> 
> > trouble changing every existing multicolor led driver.
> > 
> 
> I agree it's probably not worth doing this at the moment.
> 
> > > 
> > > > Ofc, this function won't be used, but I think given that the same
> > > > struct is also accessed by the C-side, it should also apply here.
> > > > 
> > > > Like Sashiko suggests, "core::ptr::read_volatile()" might be a better
> > > > option to prevent certain compiler optimizations.
> > > > 
> > > 
> > > No, please don't over-use read_volatile(). The reason that READ_ONCE()
> > > and WRITE_ONCE() are safe to use for synchronization is because
> > > semantics-wise they are atomic on certain types (if aligned), and the
> > > them being volatile is just an implementation detail.
> > Ok.
> > 
> > I will need to make .get_mut() const for this.
> > 
> 
> Sounds good to me.
> 
> Regards,
> Boqun
> 
> > > 
> > > Regards,
> > > Boqun
> > 
> > Thanks
> > - Markus Probst
> > 
> > > 
> > > > Thanks
> > > > - Markus Probst
> > > > 
> > > > > 
> > > > > Regards,
> > > > > Boqun
> > > > > 
> > > > > > +    /// The maximum supported intensity value.
> > > > > > +    ///
> > > > > > +    /// If None the maximum intensity equals to [`LedOps::MAX_BRIGHTNESS`].
> > > > > > +    pub max_intensity: Option<NonZero<u32>>,
> > > > > > +    /// Arbitrary data for the driver to store.
> > > > > > +    pub channel: u32,
> > > > > > +}
> > > > > > +
> > > > > [...]
> > > 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]