Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The LED abstraction uses the unmanaged `led_classdev_register_ext` API, violating subsystem guidelines and introducing memory safety vulnerabilities.
- [High] DeviceBuilder unsafely recovers the `FwNode` reference count from the mutable C structure `dev->fwnode`, leading to use-after-free or memory leaks.
- [High] Concurrent calls to `blink_set_callback` create aliased mutable references to shared `delay_on` and `delay_off` variables, causing Undefined Behavior.
- [Medium] Eager evaluation of `.then_some()` forces instantiation of optional vtable callbacks, causing linker errors if methods are omitted.
--

commit 729f3d81e7ef8cb49db95c3c7c2412eca5d86a36
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add basic led classdev abstractions

This commit implements the core Rust abstractions for the LED subsystem. It
introduces traits and structures such as LedOps, DeviceBuilder, and Device
to safely wrap and manage led_classdev registration and operations.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 0000000000000..816db7c576894
> --- a/rust/kernel/led/normal.rs
> +++ b/rust/kernel/led/normal.rs

[ ... ]

> @@ -25,58 +25,58 @@ impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
>          self,
>          parent: &'bound T::Bus,
>          ops: impl PinInit<T, Error> + 'init,
>      ) -> impl PinInit<Device<'bound, T>, Error> + 'init {
>          const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
>  
>          try_pin_init!(Device {
>              ops <- ops,
>              classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
>                  // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
>                  // `led_classdev` gets fully initialized in-place by
>                  // `led_classdev_register_ext` including `mutex` and `list_head`.
>                  unsafe {
>                      ptr.write(bindings::led_classdev {
>                          brightness_set: (!T::BLOCKING)
>                              .then_some(Adapter::<T>::brightness_set_callback),
>                          brightness_set_blocking: T::BLOCKING
>                              .then_some(Adapter::<T>::brightness_set_blocking_callback),
>                          brightness_get: T::HAS_BRIGHTNESS_GET
>                              .then_some(Adapter::<T>::brightness_get_callback),

[Severity: Medium]
Since .then_some() is a method call, doesn't it evaluate its argument eagerly?

Passing Adapter::<T>::brightness_get_callback forces the compiler to
instantiate the function body, which in turn evaluates T::brightness_get.
If the trait method is omitted by the driver, it defaults to invoking
build_error!().

Will this forced instantiation create a reference to the external symbol
rust_build_error that resists dead-code elimination and results in a
linker error?

Would a standard if T::HAS_BRIGHTNESS_GET { Some(...) } else { None } block
prevent this eager evaluation?

>                          blink_set: T::HAS_BLINK_SET.then_some(Adapter::<T>::blink_set_callback),
>                          max_brightness: T::MAX_BRIGHTNESS,
>                          brightness: self.initial_brightness,
>                          color: self.color as u32,
>                          name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),
>                          ..bindings::led_classdev::default()
>                      })
>                  };
>  
>                  let mut init_data = bindings::led_init_data {
>                      fwnode: self
>                          .fwnode
>                          .as_ref()
>                          .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()),
>                      default_label: core::ptr::null(),
>                      devicename: self
>                          .devicename
>                          .map_or(core::ptr::null(), CStrExt::as_char_ptr),
>                      devname_mandatory: self.devname_mandatory,
>                  };
>  
>                  // 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`.
>                  to_result(unsafe {
>                      bindings::led_classdev_register_ext(
>                          parent.as_ref().as_raw(),
>                          ptr,

[Severity: High]
Does using the unmanaged led_classdev_register_ext() API here violate the LED
subsystem guidelines?

The guidelines mandate managed registration (devm_led_classdev_register_ext)
to prevent resource leaks and use-after-free bugs. If the Rust device is
safely leaked (e.g., via Box::leak or a cyclic Arc), the PinnedDrop
destructor is never called.

Won't this leave the unmanaged LED registered after the driver module
unloads, potentially causing an exploitable use-after-free kernel panic on
subsequent userspace sysfs accesses?

>                          if self.name.is_none() {
>                              &raw mut init_data
>                          } else {
>                              core::ptr::null_mut()
>                          },
>                      )
>                  })?;
>  
>                  core::mem::forget(self.fwnode); // keep the reference count incremented

[Severity: High]
Is it safe to unconditionally forget self.fwnode here?

If self.name is Some, init_data is passed as NULL to the C subsystem, so
the FwNode is ignored. Doesn't this permanently leak the ARef?

[ ... ]

> @@ -174,27 +174,27 @@ impl<T: LedOps> Adapter<T> {
>      /// # Safety
>      /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
>      /// `led::Device`.
>      /// `delay_on` and `delay_off` must be valid pointers to `usize` and have
>      /// exclusive access for the period of this function.
>      /// This function is called on enabling hardware accelerated blinking.
>      unsafe extern "C" fn blink_set_callback(
>          led_cdev: *mut bindings::led_classdev,
>          delay_on: *mut usize,
>          delay_off: *mut usize,
>      ) -> i32 {
>          from_result(|| {
>              // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
>              // `led_classdev` embedded within a `led::Device`.
>              let classdev = unsafe { Device::<T>::from_raw(led_cdev) };
>              // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
>              let parent = unsafe { T::Bus::from_device(classdev.parent()) };
>  
>              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 },

[Severity: High]
Can this create aliased mutable references to shared variables?

The C LED subsystem allows lockless concurrent calls to led_blink_set() from
sysfs handlers like led_delay_on_store(). When multiple threads write to the
sysfs attributes concurrently, they pass pointers to the exact same shared
variables.

If the Rust abstraction unconditionally casts these raw pointers into
exclusive mutable references (&mut *delay_on and &mut *delay_off), doesn't
this violate Rust's aliasing rules and result in Undefined Behavior?

[ ... ]

> @@ -208,12 +208,12 @@ impl<T: LedOps> Adapter<T> {
>  #[pinned_drop]
>  impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'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`.
>          let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
>  
>          let _fwnode = dev
>              .fwnode()
>              // SAFETY: the reference count of `fwnode` has previously been
>              // incremented in `led::DeviceBuilder::build`.
>              .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

[Severity: High]
Could this reconstruct an ARef from a globally-mutable C pointer and cause a
Use-After-Free?

If self.fwnode was None during DeviceBuilder::build, forget() does nothing.
But if the kernel later dynamically assigns an fwnode to the class device
(e.g., via device links or driver core), this drop handler unconditionally
reconstructs an ARef and drops it, decrementing a reference count it never
incremented.

Should the Rust abstraction store its own Option<ARef> internally to
securely track ownership instead of relying on the C structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-rust_leds-v24-0-8d9e4c04db69@posteo.de?part=1