Re: [PATCH v4 2/2] rust: serio: add serio bus abstractions and port Gravis Stinger driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] rust: serio: invalid irqreturn_t constructor call in
  interrupt_callback()
- [Medium] rust: serio: premature input registration before serio_open()
  in connect()
- [Low] rust: serio: redundant as_device() and public as_raw() on
  serio::Device
--

commit 98f91fdb41f94854e741d0d25d20d83c3e00ed06
Author: clicedvly768 <harepro29@gmail.com>

rust: serio: add serio bus abstractions and port Gravis Stinger driver

This commit adds Rust abstractions for the serio bus subsystem and ports the
Gravis Stinger serial gamepad driver to Rust.

> diff --git a/rust/kernel/serio.rs b/rust/kernel/serio.rs
> new file mode 100644
> index 0000000000000..1878055fb6449
> --- /dev/null
> +++ b/rust/kernel/serio.rs
[ ... ]
> +impl<Ctx: device::DeviceContext> Device<Ctx> {
> +    /// Returns the raw pointer to the underlying `struct serio`.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::serio {
> +        self.0.get()
> +    }
> +
> +    /// Reads the physical device path into a buffer.
> +    #[inline]
> +    pub fn phys(&self) -> &[u8; 32] {
> +        // SAFETY: `self.as_raw()` points to a valid `struct serio`.
> +        unsafe { &(*self.as_raw()).phys }
> +    }
> +
> +    /// Returns a reference to the base [`device::Device`].
> +    #[inline]
> +    pub fn as_device(&self) -> &device::Device {
> +        // SAFETY: `self.as_raw()` points to a valid `struct serio`.
> +        unsafe {
> +            let dev = core::ptr::addr_of_mut!((*self.as_raw()).dev);
> +            device::Device::from_raw(dev)
> +        }
> +    }
> +}
> +
> +// SAFETY: `serio::Device` is a transparent wrapper of `struct serio`.
> +// The offset is guaranteed to point to a valid device field inside `serio::Device`.
> +unsafe impl<Ctx: device::DeviceContext> device::AsBusDevice<Ctx> for Device<Ctx> {
> +    const OFFSET: usize = core::mem::offset_of!(bindings::serio, dev);
> +}
> +
> +kernel::impl_device_context_deref!(unsafe { Device });
> +
> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> +    #[inline]
> +    fn as_ref(&self) -> &device::Device<Ctx> {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct serio`.
> +        let dev = unsafe { core::ptr::addr_of_mut!((*self.as_raw()).dev) };
> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::from_raw(dev) }
> +    }
> +}

[Severity: Low]
This isn't a bug, but Device::as_device() duplicates the
AsRef<device::Device<Ctx>> implementation right below it while erasing the
DeviceContext parameter Ctx.  Other bus device abstractions in rust/kernel/
expose the base device through as_ref() (which StingerDriver::connect() can
call instead of as_device()) and keep as_raw() private or pub(crate) rather
than exposing the raw pointer outside the kernel crate.

> +
> +/// Trait implemented by serio device drivers.
> +pub trait Driver: Send + Sync + Sized {
> +    /// Context data stored per connected device instance.
> +    type Data: ForeignOwnable + Send + Sync;
> +
> +    /// Device ID match table.
> +    const ID_TABLE: IdTable;
> +
> +    /// Initializes driver data and registers child devices when a matching port is detected.
> +    fn connect(dev: &Device<device::Bound>) -> Result<Self::Data>;
[ ... ]
> +    unsafe extern "C" fn connect_callback(
> +        serio: *mut bindings::serio,
> +        drv: *mut bindings::serio_driver,
> +    ) -> c_int {
> +        if serio.is_null() {
> +            return crate::error::code::EINVAL.to_errno();
> +        }
> +
> +        // SAFETY: `serio` is valid. `Device<Bound>` is repr(transparent).
> +        let dev = unsafe { &*(serio.cast::<Device<device::Bound>>()) };
> +
> +        // 1. Initialize driver data and register input devices before the port is opened.
> +        let data = match T::connect(dev) {
> +            Ok(d) => d,
> +            Err(e) => return e.to_errno(),
> +        };
> +
> +        let raw_data = data.into_foreign();
> +
> +        // 2. Set drvdata BEFORE opening port so interrupts never encounter NULL.
> +        // SAFETY: `serio` is valid; store driver data pointer.
> +        unsafe {
> +            bindings::serio_set_drvdata(serio, raw_data.cast());
> +        }
> +
> +        // 3. Open serio port.
> +        // SAFETY: `serio` and `drv` are valid non-null pointers provided by the kernel.
> +        let ret = unsafe { bindings::serio_open(serio, drv) };
> +        if ret != 0 {
> +            // SAFETY: Clean up drvdata if open failed.
> +            unsafe {
> +                bindings::serio_set_drvdata(serio, core::ptr::null_mut());
> +                let data = <T::Data as ForeignOwnable>::from_foreign(raw_data);
> +                T::disconnect(data);
> +            }
> +            return ret;
> +        }
> +
> +        0
> +    }

[Severity: Medium]
Does calling T::connect() before bindings::serio_open() publish the input
device to userspace before the serio port is opened?

In StingerDriver::connect(), input_alloc.register() registers the input
device and emits a KOBJ_ADD uevent before KBox::new() allocates the Stinger
state and before connect_callback() calls serio_open():

drivers/input/joystick/stinger_rust.rs:StingerDriver::connect() {
    ...
        let registered = input_alloc.register()?;

        let stinger = KBox::new(
            Stinger {
                input: registered,
                idx: 0,
                data: [0; STINGER_MAX_LENGTH],
            },
            GFP_KERNEL,
        )?;
    ...
}

In the C stinger driver, input_register_device() is only called after
serio_open() succeeds.  With the single connect() callback here, userspace
can open the input device before the underlying serio port is open, and if
KBox::new() or serio_open() fails, the input device is published and
immediately unregistered (emitting spurious KOBJ_ADD and KOBJ_REMOVE
uevents) and T::disconnect() is invoked on a connection that never
succeeded.

Also, the commit message states that Driver provides separate init and start
callbacks so devices are published in T::start after serio_open(), which
does not match the Driver trait in the patch.

> +
> +    /// # Safety
> +    ///
> +    /// Must only be invoked by the serio subsystem during an interrupt on a connected port.
> +    /// `serio` must be a valid pointer.
> +    unsafe extern "C" fn interrupt_callback(
> +        serio: *mut bindings::serio,
> +        data: c_uchar,
> +        flags: c_uint,
> +    ) -> bindings::irqreturn_t {
> +        // SAFETY: `serio` is valid.
> +        let raw = unsafe { bindings::serio_get_drvdata(serio) };
> +        if raw.is_null() {
> +            return bindings::irqreturn_t(IrqReturn::None as c_uint);
> +        }
> +
> +        // SAFETY: `raw` was created via `into_foreign()` in `connect_callback`.
> +        let mut borrowed = unsafe { <T::Data as ForeignOwnable>::borrow_mut(raw) };
> +        let ret = T::interrupt(&mut borrowed, data, flags);
> +        bindings::irqreturn_t(ret as c_uint)
> +    }

[Severity: High]
Does bindings::irqreturn_t(...) fail to compile when CONFIG_SERIO=y and
CONFIG_RUST=y are enabled?

bindgen generates bindings::irqreturn_t as a type alias for c_uint rather
than a tuple struct, so calling bindings::irqreturn_t(...) as a constructor
fails with rustc error E0423.  Cast the return value with
IrqReturn::None as bindings::irqreturn_t and ret as bindings::irqreturn_t
instead.

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