Re: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions

Thank you for your contribution! Sashiko AI review found 3 potential issue(=
s) to consider:
- [High] Refcount underflow and use-after-free of `fwnode` via incorrect li=
fecycle management.
- [High] Lifetime erasure in `DeviceBuilder::build` leaves a dangling point=
er to the LED name string, causing Use-After-Free.
- [High] Mutable aliasing of shared C pointers in `blink_set_callback` trig=
gers Undefined Behavior.
--

--- Patch [1]: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev ab=
stractions ---
commit 6f6f8b461cc55c0944a56c43b46dbc985385115e
Author: Markus Probst <[email protected]>

rust: leds: add basic led classdev abstractions

Implement the core abstractions needed for led class devices, including:
led::LedOps, led::DeviceBuilder, and led::Device.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 0000000000000..816db7c576894
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs
> @@ -11,9 +11,6 @@
>  /// This structure represents the Rust abstraction for a led class devic=
e.
>  #[pin_data(PinnedDrop)]
>  pub struct Device<'bound, T: LedOps + 'bound> {
>      #[pin]
>      ops: T,
>      #[pin]
>      classdev: Opaque<bindings::led_classdev>,

[Severity: High]
Does this struct need to carry the 'init lifetime from DeviceBuilder?

If DeviceBuilder::name takes a &'init CStr and stores it, and then=20
DeviceBuilder::build extracts this as a raw pointer:

rust/kernel/led/normal.rs:DeviceBuilder::build() {
    ...
    name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),
    ...
}

But the resulting Device<'bound, T> does not carry the 'init lifetime.=20
This could allow the Rust borrow checker to drop the original string while=
=20
the Device remains fully active. If a driver uses a local string for the na=
me,=20
will it be freed, leaving led_classdev.name pointing to unallocated memory=
=20
and causing a use-after-free?

[ ... ]
> @@ -180,18 +180,11 @@ impl<T: LedOps> Adapter<T> {
>              // SAFETY: `classdev.parent()` is guaranteed to be contained=
 in `T::Bus`.
>              let parent =3D unsafe { T::Bus::from_device(classdev.parent(=
)) };
> =20
>              classdev.ops.blink_set(
>                  parent,
>                  classdev,
>                  // SAFETY: The function's contract guarantees that `dela=
y_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 `dela=
y_off` points to a `usize`
>                  // and is exclusive for the period of this function.
>                  unsafe { &mut *delay_off },

[Severity: High]
Does this code create overlapping mutable references when concurrent sysfs=
=20
writes occur?

The LED core's led_blink_set is invoked directly from sysfs writes=20
(delay_on_store and delay_off_store) without locking. When sysfs writes occ=
ur=20
concurrently, multiple threads can pass pointers to the shared=20
led_cdev->blink_delay_off or led_cdev->blink_delay_on members into=20
led_blink_set simultaneously.

Casting these raw pointers into exclusive mutable references (&mut usize)=20
without synchronization could result in two active overlapping &mut=20
references existing simultaneously on different threads, which is undefined=
=20
behavior in Rust.

>              )?;
>              Ok(0)
>          })
>      }
>  }
> =20
>  #[pinned_drop]
>  impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'bound, T> {
>      fn drop(self: Pin<&mut Self>) {
>          let raw =3D self.classdev.get();
>          // SAFETY: The existence of `self` guarantees that `self.classde=
v.get()` is a pointer to a
>          // valid `led_classdev`.
>          let dev: &device::Device =3D unsafe { device::Device::from_raw((=
*raw).dev) };
> =20
>          let _fwnode =3D dev
>              .fwnode()
>              // SAFETY: the reference count of `fwnode` has previously be=
en
>              // incremented in `led::DeviceBuilder::build`.
>              .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode))=
 });

[Severity: High]
Will this lead to a reference count underflow if DeviceBuilder was created=
=20
without an fwnode (self.fwnode =3D None)?

If the kernel dynamically assigns an fwnode to the device (e.g., via ACPI o=
r=20
parent inheritance), dev.fwnode() during drop will return Some(fwnode).=20
The PinnedDrop implementation unconditionally wraps this pointer in=20
ARef::from_raw and drops it, stealing a reference count it never owned.=20

Since the C device_unregister function natively cleans up its own fwnode=20
reference, could this additional drop cause a critical underflow and=20
subsequent use-after-free?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260805-rust_leds-=
[email protected]?part=3D1