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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The firmware node reference is insecurely tracked by assuming the C layer's `dev->fwnode` field remains unmodified, which can lead to reference count underflow, use-after-free, or memory leaks.
- [Medium] The LED device is registered using the unmanaged `led_classdev_register_ext()` instead of the managed `devm_led_classdev_register_ext()`, violating subsystem lifecycle guidelines and introducing devres LIFO ordering hazards.
--

--- Patch [1]: [PATCH v22 1/3] rust: leds: add basic led classdev abstractions ---
commit 7eff903565ff4a4fae9e842098053e60f2989206
Author: Markus Probst <[email protected]>

rust: leds: add basic led classdev abstractions

This commit introduces core abstractions for LED class devices in Rust. It
implements traits for handling LED operations, a device builder, and a safe
wrapper for led_classdev.

> 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

[ ... ]

> @@ -65,22 +65,22 @@ impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
>                  // 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(

[Severity: Medium]
Does calling the unmanaged bindings::led_classdev_register_ext() instead of
the managed bindings::devm_led_classdev_register_ext() violate the LED
subsystem lifecycle guidelines?

Relying on manual PinnedDrop for unregistration appears to decouple the
device lifecycle from the C driver core's devres cleanup. Could this
introduce devres teardown ordering hazards, use-after-free, or resource
leaks if user-space sysfs attributes are accessed after device removal?

>                          parent.as_ref().as_raw(),
>                          ptr,
>                          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]
If the driver was built without an fwnode (where self.fwnode is None), but
the C layer assigns a node to dev->fwnode during the device's lifetime, will
dev.fwnode() return Some(fwnode) during unregistration?

It seems this would unconditionally create an ARef from this unowned pointer
and drop it, decrementing a reference count it never acquired.

Similarly, if the C layer replaces a previously provided node, could the
original node leak and the new node be improperly decremented?

>  
>                  Ok::<_, Error>(())
>              }),
>              _p: PhantomData,
>          })
>      }
>  }

[ ... ]

> @@ -211,20 +211,20 @@ 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]
As mentioned above, if the fwnode was dynamically assigned or replaced by the
C layer, is it safe to unconditionally construct an ARef from the dev->fwnode
pointer here?

>  
>          // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
>          // successfully registered with `led_classdev_register_ext`.
>          unsafe { bindings::led_classdev_unregister(raw) };

[Severity: Medium]
As asked earlier, could relying on manual unregistration here during Drop
break standard C-level devres lifecycle ordering?

>      }
>  }

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