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

Markus Probst <[email protected]> Wed, 29 Apr 2026 14:36:25 +0000
Newsgroups	dev.linux.lists.sashiko
Message-ID	<[email protected]>
On Mon, 2026-04-27 at 21:20 +0000, [email protected] wrote:
> Sashiko AI review found 4 potential issue(s):
> - [Critical] Unsound fwnode reference count management leads to refcount underflow and Use-After-Free.
> - [Critical] Lifetime erasure in `DeviceBuilder` allows dangling string pointers in the C structure (Use-After-Free).
> - [High] Missing `Sync` bound on `LedOps` allows data races and undefined behavior.
> - [Low] Missing `#[inline]` annotations on small forwarding abstraction functions.
> --
> 
> commit 919a1118f74d7259424462187e0643f2925bb6d6
> Author: Markus Probst <[email protected]>
> 
> rust: leds: add basic led classdev abstractions
> 
> Implement the core abstractions needed for led class devices. This includes
> the LedOps trait for handling LEDs, the DeviceBuilder to construct the
> device, and a safe wrapper around led_classdev.
> 
> > diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> > new file mode 100644
> > index 000000000000..f371ba48ef12
> > --- /dev/null
> > +++ b/rust/kernel/led.rs
> 
> [ ... ]
> 
> > +#[vtable]
> > +pub trait LedOps: Send + 'static + Sized {
> > +    /// The bus device required by the implementation.
> 
> The LED subsystem frequently invokes led_classdev callbacks concurrently from
> multiple threads without locks. For example, led_trigger_event() calls
> led_set_brightness() locklessly within an RCU read-side critical section.
> 
> If LedOps only requires Send and lacks a Sync bound, could a driver
> implement this trait using types with non-thread-safe interior mutability
> (like core::cell::Cell), resulting in data races?
If BLOCKING is false, concurrent access to `LedOps` is indeed possible.

> 
> [ ... ]
> 
> > +impl Color {
> > +    /// Name of the color.
> > +    pub fn as_c_str(self) -> &'static CStr {
> > +        // SAFETY:
> 
> This isn't a bug, but the Rust subsystem coding guidelines require small
> forwarding abstraction functions to be annotated with #[inline]. Could this
> attribute be added here?
I suppose yes.

> 
> > diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> > new file mode 100644
> > index 000000000000..bd239f186c64
> > --- /dev/null
> > +++ b/rust/kernel/led/normal.rs
> 
> [ ... ]
> 
> > +/// The led class device representation.
> > +///
> > +/// This structure represents the Rust abstraction for a led class device.
> > +#[pin_data(PinnedDrop)]
> > +pub struct Device<T: LedOps> {
> > +    #[pin]
> > +    ops: T,
> > +    #[pin]
> > +    classdev: Opaque<bindings::led_classdev>,
> > +}
> > +
> > +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> > +    /// Registers a new [`Device`].
> > +    pub fn build<T: LedOps>(
> > +        self,
> > +        parent: &'a T::Bus,
> > +        ops: impl PinInit<T, Error> + 'a,
> > +    ) -> impl PinInit<Devres<Device<T>>, Error> + 'a {
> 
> Since the Device struct does not carry a lifetime parameter, does the return
> type impl PinInit<Devres<Device<T>>, Error> + 'a fail to prevent the caller
> from dropping the strings provided for the name and default_trigger fields?
> 
> The C subsystem retains these pointers indefinitely. If a caller passes a
> dynamically allocated string (like a CString), registers the LED, and
> drops the string, couldn't this lead to a use-after-free when the kernel
> later accesses led_cdev->name or led_cdev->default_trigger?
This is true for `default_trigger`, but not for `name`.

led_cdev->name will be copied on register and only has to be valid
until the registration finishes.

I will remove `default_trigger` as I don't see a use for it at the
current moment.

> 
> [ ... ]
> 
> > +impl<T: LedOps> Device<T> {
> > +    /// # Safety
> > +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> > +    /// `led::Device`.
> > +    unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self {
> 
> This isn't a bug, but the Rust subsystem coding guidelines require small
> forwarding abstraction functions to be annotated with #[inline]. Could this
> attribute be added here?
> 
> [ ... ]
> 
> > +    fn parent(&self) -> &device::Device<Bound> {
> > +        // SAFETY: `self.classdev.get()` is guaranteed to be a valid pointer to `led_classdev`.
> 
> This isn't a bug, but could the #[inline] attribute be added here as well?
> 
> [ ... ]
> 
> > +#[pinned_drop]
> > +impl<T: LedOps> PinnedDrop for Device<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::Device::new`.
> > +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });
> 
> In DeviceBuilder::build(), core::mem::forget(self.fwnode) is called, which
> transfers ownership of the reference count if a node was explicitly
> provided.
> 
> However, the core kernel can automatically assign a fwnode to the device
> during registration (for example, device_platform_notify() may assign an
> ACPI companion node).
> 
> If self.fwnode was None but the kernel assigned a node automatically, won't
> this unconditional ARef::from_raw() incorrectly decrement a reference count
> the driver never incremented, resulting in an underflow and use-after-free?
false positive.

`device_platform_notify` calls 2 functions:
  - `acpi_device_notify`:
    only assigns fwnode to *bus devices*. This is a led class device.
  - `software_node_notify`:
    does not change the fwnode
signature.asc (application/pgp-signature, 870 B)