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

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 7876 bytes --]

On Mon, 2026-04-27 at 22:01 +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 5 potential issue(s):
> - [Critical] `MultiColorDevice::subleds` returns an immutable slice to plain data concurrently mutated by C code, causing an aliasing violation and Undefined Behavior.
> - [High] Incorrect `KVec` capacity tracking causes Undefined Behavior, and an early return causes a memory leak.
> - [High] The `fwnode` reference is permanently leaked (and may cause underflow on a different node) if a device name is explicitly set.
> - [High] Creating exclusive mutable references to `delay_on` and `delay_off` in `blink_set_callback` violates mutable aliasing rules.
> - [Low] Subsystem guidelines regarding `#[inline]` annotations and import formatting are violated.
> --
> 
> commit c95f500a2f1b623bcca663aa0589f4b833fe42ae
> 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 safe wrappers around led_classdev_mc
> and mc_subled.
> 
> > diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> > index e8a69efd3066..8ec95c4f230c 100644
> > --- a/rust/kernel/led.rs
> > +++ b/rust/kernel/led.rs
> [ ... ]
> >  #[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
> >  pub use multicolor::{MultiColor, MultiColorDevice, MultiColorSubLed};
> 
> This isn't a bug, but does this import follow the vertical import style
> mandated by the Rust subsystem coding guidelines?
It is a public export, so it may be special. I will add fix it anyways.

> 
> > diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> > new file mode 100644
> > index 000000000000..726fdaf068cb
> > --- /dev/null
> > +++ b/rust/kernel/led/multicolor.rs
> [ ... ]
> > +impl MultiColorSubLed {
> > +    /// Create a new multicolor sub led info.
> > +    pub const fn new(color: Color) -> Self {
> 
> This isn't a bug, but shouldn't small abstraction functions like new(),
> channel(), and initial_intensity() be annotated with #[inline] to comply
> with the subsystem guidelines?
Yes.

> 
> [ ... ]
> > +                    let mut subleds_vec = KVec::new();
> > +                    subleds_vec.extend_from_slice(subleds, GFP_KERNEL)?;
> > +                    let (subled_info, num_colors, capacity) = subleds_vec.into_raw_parts();
> > +                    debug_assert_eq!(num_colors, capacity);
> > +
> > +                    // 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 {
> [ ... ]
> > +                            num_colors: u32::try_from(num_colors)?,
> 
> If u32::try_from(num_colors) fails, does this early return leak the memory
> allocated for subled_info?
Very unlikely event, but yes. In the case that it is greater than
`u32::MAX`, there is a memory leak.

> 
> The inspect_err closure responsible for cleaning up the allocation is
> attached to the register_ext call further down, which wouldn't be reached
> if the try_from conversion evaluates to an error.
> 
> [ ... ]
> > +                    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()
> > +                            },
> > +                        )
> > +                    })
> > +                    .inspect_err(|_err| {
> > +                        // 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) });
> > +                    })?;
> 
> Could reconstructing the vector this way cause undefined behavior?
> 
> The capacity returned by into_raw_parts() is discarded earlier, and the
> vector is rebuilt here with a hardcoded capacity of num_colors. Since
> extend_from_slice() can over-allocate, passing an incorrect capacity back
> to the allocator violates safety contracts.
On the first allocation, length always equals capacity. But I agree it
isn't a perfect solution.

> 
> > +
> > +                    core::mem::forget(self.fwnode); // keep the reference count incremented
> 
> Does this leak the fwnode reference if self.name is explicitly set?
No, because it happens on an Option type.

> 
> When self.name.is_some(), core::ptr::null_mut() is passed to register_ext
> above, so the C device never associates with the fwnode. However,
> core::mem::forget(self.fwnode) is called here unconditionally.
> 
> During PinnedDrop, the code attempts to drop the reference by calling
> dev.fwnode(). Since the device was never given the fwnode, this might
> return None and leak the reference, or return an inherited fwnode, which
> would cause a reference count underflow on the wrong node.
> 
> [ ... ]
> > +    pub fn subleds(&self) -> &[MultiColorSubLed] {
> 
> This isn't a bug, but shouldn't this small abstraction function also be
> annotated with #[inline]?
Yes.
> 
> Additionally, can returning an immutable slice of plain data here lead to
> undefined behavior?
> 
> The C subsystem can concurrently modify the underlying brightness and
> intensity fields (for example, led_mc_calc_color_components modifies
> brightness, and multi_intensity_store modifies intensity).
> 
> Because MultiColorSubLed defines these as plain u32 fields rather than
> wrapping them in UnsafeCell or Opaque, the Rust compiler assumes they
> will never change while the immutable reference exists. This violates strict
> aliasing rules and could lead to data races.
I don't think there is any quick way to avoid this.

> 
> [ ... ]
> > +    unsafe extern "C" fn blink_set_callback(
> > +        led_cdev: *mut bindings::led_classdev,
> > +        delay_on: *mut usize,
> > +        delay_off: *mut usize,
> > +    ) -> i32 {
> > +        from_result(|| {
> [ ... ]
> > +            classdev.ops.blink_set(
> > +                parent,
> > +                classdev,
> > +                // SAFETY: The function's contract guarantees that `delay_on` points to a `usize`
> > +                // and is exclusive for the period of this function.
> > +                unsafe { &mut *delay_on },
> > +                // SAFETY: The function's contract guarantees that `delay_off` points to a `usize`
> > +                // and is exclusive for the period of this function.
> > +                unsafe { &mut *delay_off },
> > +            )?;
> 
> Is it safe to create exclusive mutable references to delay_on and delay_off
> in this context?
Yes.
> 
> These pointers point directly to the shared led_cdev->blink_delay_on and
> led_cdev->blink_delay_off fields in the C struct.
Only with the timer trigger and the timer trigger only sets it directly
on the pointer if it is happening on a sysfs store call.

> 
> If the sysfs interface reads these fields concurrently, such as through
> delay_on_show, it would violate Rust's strict mutable exclusivity guarantees
> and result in undefined behavior.
Afaik sysfs blocks any "show" function calls during this time.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]