Re: [PATCH v25 3/4] 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] Safe methods `brightness()` and `intensity()` in `MultiColorSubLed` perform non-atomic plain reads that race with C subsystem writes, causing Undefined Behavior.
- [High] The `MultiColorDevice` struct fails to capture the `'init` lifetime constraint from `DeviceBuilder`, allowing the device name `CStr` to be dropped prematurely and leaving a dangling pointer in the C framework.
- [Medium] Logic in `build_multicolor` permanently leaks `fwnode` refcounts when a custom device name is provided.
--

commit 7485bc19e8ccc2afa9099cb7107096ea73b39fe0
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 MultiColor, MultiColorSubLed, and MultiColorDevice, along with
a DeviceBuilder function to register a new multicolor led class device.

> 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() }
> +    }
> +
> +    /// 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 plain read trigger undefined behavior due to a data race?

Since MultiColorSubLed implements Sync, references can be shared safely
across threads. The underlying C subsystem mutates these exact fields
concurrently. For example, multi_intensity_store() in led-class-multicolor.c
modifies intensity from sysfs, and led_mc_calc_color_components() modifies
brightness in softirq context.

In the Rust memory model, a data race on a non-atomic memory access is strict
undefined behavior, regardless of architecture-level load tearing.

[ ... ]
> +#[pin_data(PinnedDrop)]
> +pub struct MultiColorDevice<'bound, T: 'bound = ()> {
> +    #[pin]
> +    ops: T,
> +    #[pin]
> +    classdev: Opaque<bindings::led_classdev_mc>,
> +    _p: PhantomData<&'bound ()>,
> +}
> +
> +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
> +    /// Registers a new [`MulticolorDevice`].
> +    pub fn build_multicolor<'bound: 'init, T: LedOps<Mode = MultiColor> + 'bound>(
> +        self,
> +        parent: &'bound device::Device<Bound>,
> +        ops: impl PinInit<T, Error> + 'init,
> +        subleds: &'init [MultiColorSubLed],
> +    ) -> impl PinInit<MultiColorDevice<'bound, T>, Error> + 'init {
[ ... ]
> +                unsafe {
> +                    ptr.write(bindings::led_classdev_mc {
> +                        led_cdev: bindings::led_classdev {
[ ... ]
> +                            name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),

[Severity: High]
Could this result in a dangling pointer in the C framework?

DeviceBuilder accepts an &'init CStr for the device name. In build_multicolor,
this bounded reference is converted to a raw pointer and written into the name
field of the C struct bindings::led_classdev_mc.

Because the instantiated MultiColorDevice type does not include the 'init
lifetime parameter, the lifetime is erased. Can this allow safe Rust code to
dynamically allocate a CString, pass a reference to name(), initialize the
device, and then safely drop the CString while the device remains alive and
the C struct still holds the name pointer?

[ ... ]
> +                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: Medium]
Does this logic permanently leak the fwnode reference count when a custom
device name is provided?

When self.name is Some, core::ptr::null_mut() is passed instead of init_data.
This bypasses init_data, meaning the LED subsystem never associates the fwnode
with the created device (dev->fwnode remains unset).

Despite this, core::mem::forget(self.fwnode) is called unconditionally.
During teardown in PinnedDrop, dev.fwnode() will correctly return None since
the C core never set it. As a result, the ARef is never reconstructed and the
reference count is permanently leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913-rust_leds-v25-0-1a10371d78c3@posteo.de?part=3