Re: [PATCH v24 3/4] rust: leds: add multicolor classdev abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Plain non-atomic reads of `intensity` and `brightness` from `UnsafeCell` in `MultiColorSubLed` cause a data race and Undefined Behavior with concurrent C subsystem modifications.
- [High] Calling `led_mc_calc_color_components` outside of the driver's locking creates a data race and TOCTOU vulnerability.
- [High] Casting concurrent pointers to `&mut usize` in `blink_set_callback` creates an aliasing violation (Undefined Behavior).
- [Low] The commit message description is not capitalized, violating the LED subsystem guidelines.
--

commit 0c1eb1680fe4ed1aa0ee60e6b95decec69575365
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add multicolor classdev abstractions

This commit introduces the necessary Rust abstractions to support multicolor
LED class devices. It provides wrappers around the C structures mc_subled
and led_classdev_mc, alongside integration with the LED device builder.

> rust: leds: add multicolor classdev abstractions
>
> Implement the abstractions needed for multicolor led class devices,

[Severity: Low]
This isn't a bug, but the LED subsystem guidelines require capitalizing the
description after the subsystem prefix. Should the subject be capitalized as
"Add multicolor classdev abstractions", and similarly the body text?

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
[ ... ]
> +impl MultiColorSubLed {
[ ... ]
> +    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 plain read cause Undefined Behavior under Rust's concurrency rules?

If a trigger from timer or interrupt context calls intensity() concurrently
while sysfs (via multi_intensity_store) modifies the memory under the
led_access mutex, this creates a data race.

Since plain non-atomic reads of concurrently mutated memory are considered
Undefined Behavior in Rust, should this use an atomic read or read_volatile
instead?

> +
> +    /// 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]
Does this have the same data race and Undefined Behavior issue as the
intensity() read above?

[ ... ]
> +    unsafe extern "C" fn brightness_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) {
[ ... ]
> +        // SAFETY: classdev.classdev.get() is guaranteed to be a pointer to a valid
> +        // led_classdev_mc.
> +        unsafe { bindings::led_mc_calc_color_components(classdev.classdev.get(), brightness) };
> +
> +        let _ = classdev.ops.brightness_set(parent, classdev, brightness);
> +    }

[Severity: High]
Can calling bindings::led_mc_calc_color_components() before the driver 
acquires its lock cause data corruption of the hardware state?

Concurrent calls to brightness_set_callback, for example from sysfs and a
hardware trigger, will execute bindings::led_mc_calc_color_components()
outside any locks, racing to overwrite the subled_info array. 

The driver's lock in classdev.ops.brightness_set is acquired too late to
prevent this corruption. Should C drivers invoke
bindings::led_mc_calc_color_components() while holding their own lock
instead?

[ ... ]
> +    unsafe extern "C" fn brightness_set_blocking_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) -> i32 {
> +        from_result(|| {
[ ... ]
> +            // SAFETY: classdev.classdev.get() is guaranteed to be a pointer to a valid
> +            // led_classdev_mc.
> +            unsafe { bindings::led_mc_calc_color_components(classdev.classdev.get(), brightness) };
> +
> +            classdev.ops.brightness_set(parent, classdev, brightness)?;
> +            Ok(0)
> +        })
> +    }

[Severity: High]
Does this have the same data race vulnerability as brightness_set_callback()
above?

[ ... ]
> +    unsafe extern "C" fn blink_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        delay_on: *mut usize,
> +        delay_off: *mut usize,
> +    ) -> i32 {
> +        from_result(|| {
[ ... ]
> +            classdev.ops.blink_set(
> +                parent,
> +                classdev,
> +                // SAFETY: The function's contract guarantees that delay_on points to a usize
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_on },
> +                // SAFETY: The function's contract guarantees that delay_off points to a usize
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_off },
> +            )?;
> +            Ok(0)
> +        })
> +    }

[Severity: High]
Does casting these pointers to exclusive mutable references violate Rust's
aliasing rules and cause Undefined Behavior?

The C subsystem calls blink_set with pointers to led_cdev->blink_delay_on
and blink_delay_off. Concurrently, sysfs handlers like delay_on_store write
to these fields without taking a lock. 
    
Casting these shared, concurrently accessed pointers to exclusive mutable
references falsely asserts exclusive access.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-rust_leds-v24-0-8d9e4c04db69@posteo.de?part=3