Re: [PATCH v2 1/2] rust: input: add input device abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] rust: input: unreferenced parent pointer in
  AllocatedDevice::set_parent()
- [Low] rust: input: missing #[inline] on C wrapper methods in input.rs
- [Low] rust: input: non-vertical import style and redundant imports in
  input.rs
- [Low] rust: input: duplicate error cleanup in
  AllocatedDevice::register()
--

commit e772f543b269b5d6668847be715161384d0d5953
Author: clicedvly768 <harepro29@gmail.com>

rust: input: add input device abstractions

This commit adds Rust abstractions for allocating, configuring, and
registering Linux input devices (struct input_dev).

> diff --git a/rust/kernel/input.rs b/rust/kernel/input.rs
> new file mode 100644
> index 0000000000000..bef0c3a07da06
> --- /dev/null
> +++ b/rust/kernel/input.rs
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Input subsystem abstractions for safe Rust drivers.
> +//!
> +//! Provides a safe Rust object-oriented interface around Linux `struct input_dev`,
> +//! ensuring safe allocation, capability configuration, registration, and RAII cleanup.
> +
> +use crate::{
> +    bindings, device,
> +    error::{
> +        code::{EINVAL, ENOMEM},
> +        Result,
> +    },
> +    prelude::*,
> +    str::CStr,
> +};

[Severity: Low]
This isn't a bug, but the imports here group multiple items on a single line
without trailing comments instead of using the kernel vertical import style.
Also, EINVAL, ENOMEM, Error, Result, and CStr are already exported by
crate::prelude::*, so importing them explicitly is redundant.

[ ... ]

> +    /// Sets the parent device for this input device.
> +    #[inline]
> +    pub fn set_parent(&mut self, parent: &device::Device) {
> +        // SAFETY: `self.ptr` is valid, and `parent.as_raw()` returns a valid `struct device`.
> +        unsafe {
> +            (*self.ptr.as_ptr()).dev.parent = parent.as_raw();
> +        }
> +    }

[Severity: Medium]
Can AllocatedDevice::set_parent() leave dev.parent as a dangling pointer?

set_parent() takes a short-lived &device::Device borrow and stores
parent.as_raw() in dev.parent without holding a reference count or tying the
lifetime of AllocatedDevice to parent.  The C driver core does not call
get_device(dev->parent) until input_register_device() -> device_add(), and
input_alloc_absinfo() also dereferences dev->dev.parent before registration on
allocation failure.  If a caller drops the parent device after set_parent()
returns, a subsequent call to set_capability(), set_abs_params(), or
register() from safe Rust will trigger a use-after-free.

> +    /// Enables support for a capability (e.g. key or axis).
> +    #[inline]
> +    pub fn set_capability(&mut self, type_: u32, code: u32) {
> +        // SAFETY: `self.ptr` is valid and type/code are valid capability values.
> +        unsafe {
> +            bindings::input_set_capability(self.ptr.as_ptr(), type_, code);
> +        }
> +    }
> +
> +    /// Sets parameter limits for an absolute axis.
> +    ///
> +    /// Bounds-checks `axis < ABS_CNT` (0x40) to prevent out-of-bounds writes
> +    /// to the `absinfo` array in `struct input_dev`.
> +    pub fn set_abs_params(
> +        &mut self,
> +        axis: u32,
> +        min: i32,
> +        max: i32,
> +        fuzz: i32,
> +        flat: i32,
> +    ) -> Result {
> +        if axis >= bindings::ABS_CNT {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: `self.ptr` is valid and `axis` has been verified within bounds.
> +        unsafe {
> +            bindings::input_set_abs_params(self.ptr.as_ptr(), axis, min, max, fuzz, flat);
> +        }
> +        Ok(())
> +    }
> +
> +    /// Registers the input device with the kernel input core.
> +    ///
> +    /// Consumes the `AllocatedDevice` and returns a [`RegisteredDevice`].
> +    pub fn register(self) -> Result<RegisteredDevice> {
> +        let ptr = self.ptr;
> +        // Don't run the AllocatedDevice drop destructor, registration takes ownership.
> +        core::mem::forget(self);
> +
> +        // SAFETY: `ptr` is valid and was allocated by `input_allocate_device`.
> +        let ret = unsafe { bindings::input_register_device(ptr.as_ptr()) };
> +        if ret != 0 {
> +            // SAFETY: Free the allocated device if registration failed.
> +            unsafe {
> +                bindings::input_free_device(ptr.as_ptr());
> +            }
> +            return Err(crate::error::Error::from_errno(ret));
> +        }
> +
> +        Ok(RegisteredDevice { ptr })
> +    }
> +}

[Severity: Low]
This isn't a bug, but calling core::mem::forget(self) before
input_register_device() forces the error path in register() to manually call
input_free_device() and Error::from_errno(), duplicating Drop::drop() for
AllocatedDevice and error::to_result().  Checking
to_result(unsafe { bindings::input_register_device(self.ptr.as_ptr()) })?
before forgetting self would let Drop handle cleanup on failure.

> +impl Drop for AllocatedDevice {
> +    fn drop(&mut self) {
> +        // SAFETY: `self.ptr` was allocated by `input_allocate_device` and has not been registered.
> +        unsafe {
> +            bindings::input_free_device(self.ptr.as_ptr());
> +        }
> +    }
> +}

[ ... ]

> +impl Drop for RegisteredDevice {
> +    fn drop(&mut self) {
> +        // SAFETY: `self.ptr` was successfully registered with `input_register_device`.
> +        // Unregistering automatically unbinds and frees the device.
> +        unsafe {
> +            bindings::input_unregister_device(self.ptr.as_ptr());
> +        }
> +    }
> +}

[Severity: Low]
Small functions in kernel crate abstractions that forward to C bindings should
be marked #[inline].  AllocatedDevice::new(),
AllocatedDevice::set_abs_params(), AllocatedDevice::register(),
<AllocatedDevice as Drop>::drop(), and <RegisteredDevice as Drop>::drop() are
missing #[inline].

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106201429.376997-1-harepro29@gmail.com?part=1