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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unsynchronized lockless reads in `intensity()` and `brightness()` cause Undefined Behavior.
- [High] Dangling pointer in `led_cdev.name` due to builder lifetime mismatch.
- [High] Incorrect fwnode reference counting leads to memory leaks and Use-After-Free.
- [Low] Copy-paste error in safety comment for MultiColorSubLed::brightness().
--

commit e54d5ad0bd9b464f2f25c0f6cb8f040e94fe0402
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 the MultiColor mode, MultiColorSubLed,
MultiColorDevice, and the DeviceBuilder::build_multicolor function for
registration.

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..9a341b3336489
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs

[ ... ]

> +    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]
Could this plain read cause undefined behavior?

The C LED subsystem updates the intensity and brightness fields directly,
often while holding the led_access mutex. Since this read from the
UnsafeCell is performed without atomic synchronization or read_volatile,
can it race with C-side updates if called from an unsynchronized driver
context?

> +    /// The brightness of the sub led.
> +    #[inline]
> +    pub const fn brightness(&self) -> u32 {
> +        // SAFETY:
> +        // - `self.intensity.get()` is a valid pointer to `u32`.

[Severity: Low]
This isn't a bug, but there is a typo in this safety comment. It references
self.intensity.get() instead of self.brightness.get().

> +        // - We don't have exclusive or immutable access to `self.brightness`,
> +        //   but the alignment should prevent "load tearing".
> +        unsafe { *self.brightness.get() }
> +    }

[Severity: High]
Similar to the intensity() method, does this plain unsynchronized read of
brightness risk undefined behavior if the C side modifies it concurrently?

[ ... ]

> +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> +    /// Registers a new [`MulticolorDevice`].
> +    pub fn build_multicolor<'bound: 'a, T: LedOps<Mode = MultiColor> + 'bound>(
> +        self,
> +        parent: &'bound T::Bus,
> +        ops: impl PinInit<T, Error> + 'a,
> +        subleds: &'a [MultiColorSubLed],
> +    ) -> impl PinInit<MultiColorDevice<'bound, T>, Error> + 'a {

[ ... ]

> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> +                // `led_classdev_mc` gets fully initialized in-place by
> +                // `led_classdev_multicolor_register_ext` including `mutex` and `list_head`.
> +                unsafe {
> +                    ptr.write(bindings::led_classdev_mc {
> +                        led_cdev: bindings::led_classdev {
> +                            brightness_set: (!T::BLOCKING)
> +                                .then_some(Adapter::<T>::brightness_set_callback),
> +                            brightness_set_blocking: T::BLOCKING
> +                                .then_some(Adapter::<T>::brightness_set_blocking_callback),
> +                            brightness_get: T::HAS_BRIGHTNESS_GET
> +                                .then_some(Adapter::<T>::brightness_get_callback),
> +                            blink_set: T::HAS_BLINK_SET.then_some(Adapter::<T>::blink_set_callback),
> +                            max_brightness: T::MAX_BRIGHTNESS,
> +                            brightness: self.initial_brightness,
> +                            color: self.color as u32,
> +                            name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),

[Severity: High]
Is there a risk of a dangling pointer here?

The DeviceBuilder borrows self.name for a lifetime 'a, but the resulting
MultiColorDevice only captures the 'bound lifetime. This drops the 'a
constraint, meaning the caller could safely drop the original CStr
immediately after initialization.

Since the C subsystem bypasses internal name generation and retains this
pointer if self.name is provided, could this lead to a use-after-free
when sysfs accesses the name later?

> +                            ..bindings::led_classdev::default()
> +                        },

[ ... ]

> +                let mut init_data = bindings::led_init_data {
> +                    fwnode: self
> +                        .fwnode
> +                        .as_ref()
> +                        .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()),
> +                    default_label: core::ptr::null(),
> +                    devicename: self
> +                        .devicename
> +                        .map_or(core::ptr::null(), CStrExt::as_char_ptr),
> +                    devname_mandatory: self.devname_mandatory,
> +                };
> +
> +                // 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 cause a memory leak when self.name is provided?

If self.name.is_some(), the register call is passed a null pointer instead
of the init_data. This prevents the C subsystem from attaching the fwnode
to the device.

However, core::mem::forget(self.fwnode) is still called unconditionally,
which seems like it would permanently leak the reference count for the
fwnode when the subsystem didn't actually take ownership of it.

> +
> +                Ok::<_, Error>(())
> +            }),

[ ... ]

> +#[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) };
> +
> +        if let Some(fwnode) = dev.fwnode() {
> +            // SAFETY: the reference count of `fwnode` has previously been incremented in
> +            // `led::DeviceBuilder::build_multicolor`.
> +            unsafe { FwNode::dec_ref(NonNull::from(fwnode)) };
> +        }
> +
> +        // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
> +        // successfully registered with `led_classdev_multicolor_register_ext`.
> +        unsafe { bindings::led_classdev_multicolor_unregister(raw) };

[Severity: High]
Could dropping the fwnode reference before unregistering the device lead
to a use-after-free?

Unregistering the device calls device_del(), which may access dev->fwnode
for kobject_uevent or fwnode_links_read.

Since FwNode::dec_ref() is called first, the fwnode might already be freed
by the time the unregister function accesses it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-rust_leds-v20-0-f493ab053b4f@posteo.de?part=3