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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect manual reference count management for `fwnode` causes memory leaks and potential Use-After-Free (UAF).
- [High] Plain, unsynchronized reads of `intensity` and `brightness` from `UnsafeCell` cause data races and Undefined Behavior.
- [Low] The commit message violates formatting conventions (capitalization) and contains a typo.
--

--- Patch [3]: [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions ---
commit 2486b1ac4294e9421b47197f76e5b346cbfb370a
Author: Markus Probst <[email protected]>

rust: leds: add multicolor classdev abstractions

This patch implements the abstractions needed for multicolor led class devices.
It introduces the MultiColor mode, MultiColorSubLed wrapper, MultiColorDevice
wrapper, and device builder functions.

> rust: leds: add multicolor classdev abstractions

[Severity: Low]
This isn't a bug, but the commit message subject violates the subsystem
capitalization convention. Could this be capitalized as "Add multicolor..."?

> * `MultiColorSubLed` - a safe wrapper arround `mc_subled`

[Severity: Low]
This isn't a bug, but there is a typo (arround) in the commit message.

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> --- /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() }
> +    }
> +
> +    /// 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]
Can these plain reads cause data races and undefined behavior?

Since MultiColorDevice implements Sync, references can be safely shared across
threads. At the same time, the C LED subsystem can concurrently mutate these
fields asynchronously, for example via sysfs writes handled by
multi_intensity_store() or background state calculations in
led_mc_calc_color_components().

Concurrent unsynchronized read/write accesses to the same memory location,
where at least one is a write, are formal data races in Rust. Could these be
wrapped with proper atomic accesses or synchronization?

[ ... ]

> +                // 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
> +
> +                Ok::<_, Error>(())

[Severity: High]
Can this sequence cause a reference leak or a use-after-free for the fwnode?

If a device name is provided, led_classdev_multicolor_register_ext() is called
with a null pointer for init_data to force the use of the provided name.
Consequently, the C LED subsystem does not copy the fwnode from init_data into
the device.

However, this code unconditionally calls core::mem::forget(self.fwnode).

If self.fwnode was Some, its reference count is leaked because
MultiColorDevice::drop() will later attempt to recover it using dev.fwnode(),
which will return None.

[ ... ]

> +#[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) };
> +
> +        let _fwnode = dev
> +            .fwnode()
> +            // SAFETY: the reference count of `fwnode` has previously been
> +            // incremented in `led::DeviceBuilder::build_multicolor`.
> +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

[Severity: High]
Following up on the previous observation, if self.fwnode was initially None,
but the C driver core later sets dev->fwnode (for example, via device_set_node
from an ACPI table or parent), could this code wrap that unowned pointer in
ARef::from_raw and improperly decrement its refcount upon destruction, causing
a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3