Re: [PATCH v26 3/4] rust: leds: Add multicolor classdev abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rust: leds: data race and UB via plain reads in MultiColorSubLed
--

commit 8041b99a2f954026eb99d9bedff3e1091196ee48
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: Add multicolor classdev abstractions

This commit implements the abstractions needed for multicolor led class
devices. It introduces the led mode implementation, safe wrappers around
mc_subled and led_classdev_mc, and a builder function for registration.

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..309487bdf38aa
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
[ ... ]
> +    /// The intensity of the sub led.
> +    #[inline]
> +    pub const fn intensity(&self) -> u32 {
> +        // SAFETY:
> +        // - `self.intensity.get()` is a valid pointer to `u32`.
> +        // - We don't have exclusive or immutable access to `self.intensity`,
> +        //   but the alignment should prevent "load tearing".
> +        unsafe { *self.intensity.get() }

[Severity: High]
Does this unsynchronized plain read introduce a data race and undefined
behavior?

In the Rust memory model, concurrent non-atomic reads and writes constitute
a data race, which is strictly undefined behavior. A sysfs write to
multi_intensity_store can lock led_access and modify
subled_info[i].intensity.

Concurrently, the driver's workqueue (set_brightness_delayed) or an atomic
trigger could execute brightness_set without holding led_access. When this
happens, the Rust driver reads the intensity at the exact same time it is
being written by the C core.

Could we use core::ptr::read_volatile() here to emulate READ_ONCE()
semantics and prevent the compiler from optimizing under the assumption
that the value does not change?

> +    }
> +
> +    /// The brightness of the sub led.
> +    #[inline]
> +    pub const fn brightness(&self) -> u32 {
> +        // SAFETY:
> +        // - `self.brightness.get()` is a valid pointer to `u32`.
> +        // - We don't have exclusive or immutable access to `self.brightness`,
> +        //   but the alignment should prevent "load tearing".
> +        unsafe { *self.brightness.get() }

[Severity: High]
Similarly, does this plain read risk the same data race if concurrent
trigger events modify the brightness?

Replacing this read with core::ptr::read_volatile() would emulate
READ_ONCE() and safely mitigate the undefined behavior.

> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rust_leds-v26-0-83837331020e@posteo.de?part=3