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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unsynchronized reads of concurrently modified `intensity` and `brightness` fields in `MultiColorSubLed` cause a data race and Undefined Behavior in safe Rust.
- [High] Flawed `fwnode` reference counting ownership in `MultiColorDevice` leads to potential Use-After-Free or memory leak.
--

commit 53cb84fc2ecc8ded772da6376e83ccc60f89c50f
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add multicolor classdev abstractions

Implement the abstractions needed for multicolor led class devices.

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..2c55140dd37ed
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
> @@ -0,0 +1,460 @@
[ ... ]
> +    /// 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() }

[Severity: High]
Does this unsynchronized raw pointer read risk causing a data race and
Undefined Behavior in safe Rust?

The C multicolor LED core modifies subled_info[i].intensity and brightness
concurrently when a user writes to sysfs attributes. Meanwhile, this safe Rust
getter method performs an unsynchronized raw pointer read. Since
MultiColorDevice implements Sync, multiple threads can share
MultiColorSubLed and read these fields at any time.

Should this use core::ptr::read_volatile to ensure soundness?

> +    }
> +
> +    /// 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]
Similarly, does this unsynchronized read of brightness risk causing a data
race when the C side modifies it concurrently?

Should core::ptr::read_volatile be used here as well?

[ ... ]
> +                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 leaking the fwnode reference count to the C subsystem and then trying
to reclaim it in drop() by reading dev.fwnode() risk a Use-After-Free?

If the kernel (e.g. ACPI) assigns a different fwnode to the device after
registration, dev.fwnode() will return this new fwnode. The drop()
implementation below would then incorrectly take ownership of the new
fwnode's reference count and drop it, while permanently leaking the
originally passed fwnode.

Should MultiColorDevice store Option<ARef<FwNode>> locally to ensure it
only manages the exact reference it uniquely owns?

> +
> +                Ok::<_, Error>(())
> +            }),
> +            _p: PhantomData,
> +        })
[ ... ]
> +#[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]
Is it safe to unconditionally reconstruct an ARef from dev.fwnode() here?

As mentioned above, if dev.fwnode() returns a different fwnode than the one
originally passed to build_multicolor, this will drop a reference it does not
own, potentially leading to a Use-After-Free, while the originally leaked
reference remains un-dropped.

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