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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Data race and Undefined Behavior in `MultiColorSubLed` field accessors due to plain non-volatile reads.
- [High] Fwnode reference leak and potential Use-After-Free due to conditional failure to pass `init_data`.
- [High] `led_mc_calc_color_components` is invoked without driver locks, causing data races on `subled_info`.
- [High] The `blink_set_callback` constructs `&mut usize` references to `delay_on` and `delay_off` while they may be concurrently read by the C subsystem, violating Rust's strict aliasing rules.
- [Low] The commit message capitalization does not follow the LED subsystem guidelines.
--

--- Patch [3]: [PATCH v22 3/3] rust: leds: add multicolor classdev abstractions ---
commit d8a204f9781a723e65e93fe2f08ed4aaa56bb20f
Author: Markus Probst <[email protected]>

rust: leds: add multicolor classdev abstractions

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

> rust: leds: add multicolor classdev abstractions

[Severity: Low]
This isn't a bug, but does the commit message capitalization follow the LED
subsystem guidelines? 

The guidelines require capitalizing the description after the subsystem prefix,
such as "rust: leds: Add multicolor classdev abstractions".

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..82fe96fd5273b
> --- /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]
Can this lead to a data race and undefined behavior? 

The C LED subsystem might mutate the intensity concurrently, for instance via
sysfs writes like multi_intensity_store().

In Rust's memory model, concurrent non-atomic reads and writes to identical
memory locations constitute a data race regardless of alignment. 

Would it be safer to use core::ptr::read_volatile() here?

> +    }
> +
> +    /// 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, could concurrent sysfs writes mutate the brightness while it's
being read here? 

Using a plain read instead of read_volatile might lead to data races and
undefined behavior for the same reasons as intensity().

[ ... ]
> +                // SAFETY:
> +                // - `parent.as_ref().as_raw()` is guaranteed to be a pointer to a valid
> +                //    `device`.
> +                // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev_mc`.
> +                to_result(unsafe {
> +                    bindings::led_classdev_multicolor_register_ext(
> +                        parent.as_ref().as_raw(),
> +                        ptr,
> +                        if self.name.is_none() {
> +                            &raw mut init_data
> +                        } else {
> +                            core::ptr::null_mut()
> +                        },
> +                    )
> +                })?;
> +
> +                subled_guard.dismiss();
> +
> +                core::mem::forget(self.fwnode); // keep the reference count incremented

[Severity: High]
Does this sequence potentially cause a memory leak or a use-after-free of the
fwnode?

When a custom name is used (self.name.is_some()), init_data is omitted and
null_mut() is passed to led_classdev_multicolor_register_ext(), preventing
the C device from inheriting the fwnode.

However, core::mem::forget(self.fwnode) is called unconditionally.

[ ... ]
> +#[pinned_drop]
> +impl<'bound, T: LedOps<Mode = MultiColor> + 'bound> PinnedDrop for MultiColorDevice<'bound, T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        let raw = self.classdev.get();
> +        // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
> +        // valid `led_classdev_mc`.
> +        let dev: &device::Device = unsafe { device::Device::from_raw((*raw).led_cdev.dev) };
> +
> +        let _fwnode = dev
> +            .fwnode()
> +            // SAFETY: the reference count of `fwnode` has previously been
> +            // incremented in `led::DeviceBuilder::build_multicolor`.
> +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

[Severity: High]
If init_data wasn't passed during registration, won't dev.fwnode() either
return null (permanently leaking the ARef) or return an inherited fwnode
from a parent device?

If it inherits a parent's fwnode, this code seems like it would drop a
reference it never owned, resulting in a use-after-free.

[ ... ]
> +    unsafe extern "C" fn brightness_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) {
> +        // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +        // `led_classdev` embedded within a `led::MultiColorDevice`.
> +        let classdev = unsafe { MultiColorDevice::<T>::from_raw(led_cdev) };
> +        // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> +        let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> +
> +        // 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]
Could calling led_mc_calc_color_components unconditionally before dispatching
to the driver's brightness_set callback lead to data races?

The LED core doesn't serialize brightness_set calls. Hardware triggers and
sysfs writes could occur concurrently.

Normally in C drivers, led_mc_calc_color_components is invoked manually
inside the driver's lock to protect concurrent writes to
subled_info[i].brightness. 

By executing it here outside the driver's callback, doesn't it make it
impossible for the driver to protect the calculation?

[ ... ]
> +    unsafe extern "C" fn blink_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        delay_on: *mut usize,
> +        delay_off: *mut usize,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +            // `led_classdev` embedded within a `led::MultiColorDevice`.
> +            let classdev = unsafe { MultiColorDevice::<T>::from_raw(led_cdev) };
> +            // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> +            let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> +
> +            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 },
> +            )?;

[Severity: High]
Does casting delay_on and delay_off to exclusive mutable references 
(&mut usize) violate strict aliasing?

These FFI pointers point to led_cdev->blink_delay_on and blink_delay_off,
which the C subsystem allows to be read locklessly via sysfs
(e.g., delay_on_show()).

Since Rust's &mut T guarantees absolutely exclusive access, a concurrent
read from C would invalidate this contract and cause undefined behavior.

Should these be passed as raw pointers or use interior mutability types?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3