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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Safe methods `brightness()` and `intensity()` in `MultiColorSubLed` perform non-synchronized plain reads of an `UnsafeCell`, causing Undefined Behavior due to data races with concurrent C subsystem modifications.
- [High] Fragile `fwnode` reference management in `DeviceBuilder::build_multicolor` and `MultiColorDevice::drop` causes `ARef<FwNode>` leaks or invalid refcount decrements.
--

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

rust: leds: add multicolor classdev abstractions

Implement the abstractions needed for multicolor led class devices,
including MultiColor, MultiColorSubLed, MultiColorDevice, and 
DeviceBuilder::build_multicolor to register new class devices.

> 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]
Does this unsynchronized plain load introduce a data race? If a driver calls
this safe method from an interrupt handler or background thread while the C
subsystem concurrently updates the value via sysfs (which holds a C mutex
the Rust side does not take), could this trigger undefined behavior?

> +    }
> +
> +    /// 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 reading brightness() here cause undefined behavior due to
concurrent sysfs modifications?

[ ... ]

> +                // 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 unconditionally leak the fwnode reference when self.name is set?

If self.name is Some, the C registration function is passed a NULL pointer for
init_data, meaning the device fwnode is never set. By calling forget()
unconditionally here, the reference count is incremented but the fwnode is
not stored in the C device, which might permanently leak the reference.

[ ... ]

> +#[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 or double-free on an unrelated fwnode?

If self.fwnode was None during builder initialization, but the C subsystem
later assigns an unrelated fwnode to the device, it seems this code would
incorrectly wrap it in an ARef and decrement its refcount.

Alternatively, if dev.fwnode() returns None because init_data was NULL earlier,
the previously forgotten fwnode refcount is never decremented, leaking it.
It appears this same pattern may also affect normal device builders.

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