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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Data race and Undefined Behavior due to `&[MultiColorSubLed]` exposing concurrently mutated plain `u32` fields.
- [High] Reconstructing KVec with a hardcoded capacity violates the `KVec::from_raw_parts` safety contract.
- [High] FwNode reference count leak or Use-After-Free when `name` is explicitly set.
- [Low] Dead documentation link for `MultiColorSubLed::brightness`.
- [Low] Missing `#[inline]` attribute on the `from_raw` internal helper function.
--

commit 06862c24665d65b7a33c6a01c9dd3881267810e1
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` and `MultiColorDevice`.

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

[ ... ]

> +#[repr(C)]
> +#[derive(Copy, Clone, Debug)]
> +#[non_exhaustive]
> +pub struct MultiColorSubLed {
> +    /// the color of the sub led
> +    pub color: Color,
> +    /// the brightness of the sub led.
> +    ///
> +    /// The value will be automatically calculated.
> +    /// See `MultiColor::pre_brightness_set`.

[Severity: Low]
Is the reference to MultiColor::pre_brightness_set correct?  This function
does not appear to exist in the code, and MultiColor is a marker enum.

> +    pub brightness: u32,
> +    /// the intensity of the sub led.
> +    pub intensity: u32,
> +    /// arbitrary data for the driver to store.
> +    pub channel: u32,
> +}

[Severity: High]
Does this expose concurrently mutated fields as plain integers?

When the C kernel LED core modifies the subled values (for instance, via
led_mc_calc_color_components modifying brightness during sysfs writes),
these fields are mutated without Rust-visible synchronization.

Since MultiColorDevice::subleds() hands out a shared, immutable slice
&[MultiColorSubLed] to the driver, does using plain u32 instead of
interior mutability primitives like Opaque or UnsafeCell violate strict
aliasing invariants and risk undefined behavior?

[ ... ]

> +                let subled_guard = ScopeGuard::new(|| {
> +                    // SAFETY: `subled_info` is guaranteed to be a valid array pointer to
> +                    // `mc_subled` with the length and capacity of `num_colors`.
> +                    drop(unsafe { KVec::from_raw_parts(subled_info, num_colors, num_colors) });
> +                });

[Severity: High]
Is it safe to hardcode the capacity to num_colors when reconstructing
this KVec?

The original KVec was dynamically allocated using extend_from_slice(subleds)
which could potentially over-allocate.  Since the exact allocated capacity 
is verified but then discarded, reconstructing the KVec here (and during 
PinnedDrop) with num_colors as the capacity might violate 
KVec::from_raw_parts safety contract.

Should the exact capacity be stored, or perhaps a boxed slice (KBox<[T]>)
used instead to ensure the allocator frees the correct size?

[ ... ]

> +                // 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]
Could this lead to a fwnode reference count leak or a use-after-free if
self.name is configured?

If self.name is set, a null pointer is passed to register the device instead
of &mut init_data, meaning the C device does not inherit the provided fwnode.
However, self.fwnode is unconditionally forgotten, which leaves its reference
count incremented.

Later, during device drop, dev.fwnode() is queried to decrement the refcount.
If the device has no fwnode, it returns None, permanently leaking the original
fwnode. If the device inherited a fwnode from its parent, dev.fwnode() would
return the parent's fwnode, incorrectly decrementing it and potentially causing
a use-after-free of the parent's fwnode.

[ ... ]

> +impl<'bound, T: LedOps<Mode = MultiColor> + 'bound> MultiColorDevice<'bound, T> {
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::MultiColorDevice`.
> +    unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self {

[Severity: Low]
this isn't a bug, but should this internal helper function have an #[inline]
attribute?

According to the Rust subsystem guidelines, small internal helper functions
within abstractions should be annotated with #[inline]. Since this method
consists exclusively of unsafe pointer arithmetic (container_of!), it seems
it should be inlined.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-rust_leds-v18-0-d07102ba5170@posteo.de?part=3