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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsound fwnode reference counting and recovery logic leads to a memory leak or use-after-free.
- [High] Unsynchronized concurrent reads of `brightness` and `intensity` create a data race, resulting in Undefined Behavior.
- [Low] Commit message description is not capitalized, violating the LED subsystem guidelines.
--

--- Patch [3]: [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions ---
commit 31ed8e2758580675a7a667a24dbbce311465d001
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add multicolor classdev abstractions

Implement the abstractions needed for multicolor led class devices,
including led::MultiColor, MultiColorSubLed, and led::MultiColorDevice.

> rust: leds: add multicolor classdev abstractions

[Severity: Low]
This isn't a bug, but the commit message description is not capitalized,
violating the LED subsystem guidelines. Should this be "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() }
> +    }
> +
> +    /// 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 code introduce undefined behavior due to unsynchronized concurrent
reads?

The C subsystem updates these fields concurrently when user-space writes to
sysfs. In Rust's memory model, plain pointer dereferences on non-atomic
variables that can be concurrently mutated constitute a data race and strict
undefined behavior.

Should this use core::ptr::read_volatile or atomics instead?

[ ... ]
> +                // 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
> +
> +                Ok::<_, Error>(())
> +            }),

[Severity: High]
Can this code leak the fwnode reference?

If self.name is present, null_mut() is passed for init_data, meaning
the fwnode is never passed to the C subsystem. However, core::mem::forget()
is called unconditionally, leaking the reference since the device will not
have the fwnode for recovery during drop.

[ ... ]
> +#[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]
Can this cause a use-after-free?

If self.fwnode was originally None, but the C subsystem matches and
assigns an fwnode from elsewhere (e.g. the parent device), this drop logic
will wrongly assume ownership of it and drop an unowned ARef.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-rust_leds-v23-0-b2fea4290ee6@posteo.de?part=3