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

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 [off-list ref]

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.

[ ... ]

+    /// 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