Re: [PATCH v2 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 5 potential
issues to consider:
- [High] rust: serio: premature serio_open() and unlocked drvdata in
  connect_callback()
- [Medium] input: stinger_rust changes device name, phys path,
  description, and module name
- [Medium] rust: serio: invalid irqreturn_t and IRQ_NONE bindings in
  interrupt_callback()
- [Low] rust: serio: Device and Registration bypass generic driver
  abstractions
- [Low] rust: serio: non-vertical import style in serio.rs and
  stinger_rust.rs
--

commit 986d37559af3527a1a01463ffbfccd5c47d94c58
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/drivers/input/joystick/stinger_rust.rs b/drivers/input/joystick/stinger_rust.rs
> new file mode 100644
> index 0000000000000..6250d628ee3da
> --- /dev/null
> +++ b/drivers/input/joystick/stinger_rust.rs

[ ... ]

> +const DRIVER_NAME: &CStr = c"stinger";
> +const DRIVER_DESC: &CStr = c"Gravis Stinger gamepad";
> +const DRIVER_PHYS: &CStr = c"serio0/input0";

[ ... ]

> +    fn connect(dev: &mut serio::Device) -> Result<Self::Data> {
> +        let mut input_alloc = AllocatedDevice::new()?;
> +
> +        input_alloc.set_name(DRIVER_DESC);
> +        input_alloc.set_phys(DRIVER_PHYS);

[Severity: Medium]
Does replacing stinger.c with stinger_rust.rs when
CONFIG_JOYSTICK_STINGER_RUST=y change the userspace-visible device and
module attributes?

In drivers/input/joystick/stinger.c, stinger_connect() sets input_dev->name
to "Gravis Stinger" and formats input_dev->phys dynamically as "%s/input0"
from serio->phys.  Setting the name to "Gravis Stinger gamepad" breaks
userspace controller mappings and udev rules that match EVIOCGNAME, and
hardcoding "serio0/input0" (leaving serio::Device::phys() unused) reports
the wrong physical path on any port other than serio0 or when multiple
devices are attached.

In addition, drivers/input/joystick/Makefile builds stinger_rust.ko (with
module! name "stinger_rust") instead of stinger.ko as documented in the
CONFIG_JOYSTICK_STINGER Kconfig help text, and Registration::new() sets
serio_driver.description to "stinger" instead of "Gravis Stinger gamepad
driver".

> diff --git a/rust/kernel/serio.rs b/rust/kernel/serio.rs
> new file mode 100644
> index 0000000000000..b4a8ec7139a28
> --- /dev/null
> +++ b/rust/kernel/serio.rs
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Serio (Serial I/O) bus abstractions for safe Rust drivers.
> +//!
> +//! Provides a safe Rust interface for serio devices and drivers in the Linux kernel,
> +//! supporting input devices such as serial gamepads (e.g. Gravis Stinger), mice, and keyboards.
> +
> +use crate::{
> +    bindings, device,
> +    device_id::{IdTable as GenericIdTable, RawDeviceId},
> +    error::{to_result, Result},
> +    ffi::{c_int, c_uchar, c_uint},
> +    prelude::*,
> +    types::{ForeignOwnable, Opaque},
> +    ThisModule,
> +};
> +use core::{marker::PhantomData, ops::Deref, pin::Pin, ptr::NonNull};

[Severity: Low]
This isn't a bug, but the imports here and in
drivers/input/joystick/stinger_rust.rs group multiple items on single
horizontal lines instead of following the kernel vertical import style in
Documentation/rust/coding-guidelines.rst.

[ ... ]

> +/// A serio port device instance passed to driver callbacks.
> +pub struct Device {
> +    ptr: NonNull<bindings::serio>,
> +}

[ ... ]

> +/// Driver adapter that bridges the safe Rust [`Driver`] trait to the C serio bus subsystem.
> +pub struct Registration<T: Driver> {
> +    drv: Pin<KBox<Opaque<bindings::serio_driver>>>,
> +    _p: PhantomData<T>,
> +}

[Severity: Low]
Should serio::Device and serio::Registration use the generic bus device and
driver registration infrastructure in the kernel crate instead of custom
wrappers?

Other bus abstractions (such as serdev, i2c, platform, and pci) define
Device<Ctx: device::DeviceContext = device::Normal> as a repr(transparent)
wrapper around Opaque<bindings::...> implementing device::AsBusDevice<Ctx>
and AlwaysRefCounted, and implement driver::DriverLayout and
driver::RegistrationOps on an Adapter<T> type.  Because serio::Device lacks
a DeviceContext parameter, driver callbacks do not receive
&serio::Device<device::Bound> and cannot use device-managed resources such
as devres::register() or Devres::new().

[ ... ]

> +    unsafe extern "C" fn connect_callback(
> +        serio: *mut bindings::serio,
> +        drv: *mut bindings::serio_driver,
> +    ) -> c_int {
> +        let Some(serio_nn) = NonNull::new(serio) else {
> +            return crate::error::code::EINVAL.to_errno();
> +        };
> +
> +        // SAFETY: `serio` and `drv` are valid non-null pointers provided by the kernel.
> +        let ret = unsafe { bindings::serio_open(serio, drv) };
> +        if ret != 0 {
> +            return ret;
> +        }
> +
> +        let mut dev = Device { ptr: serio_nn };
> +
> +        let data = match T::connect(&mut dev) {
> +            Ok(d) => d,
> +            Err(e) => {
> +                // SAFETY: Close the opened port on probe error.
> +                unsafe { bindings::serio_close(serio) };
> +                return e.to_errno();
> +            }
> +        };
> +
> +        let raw_data = data.into_foreign();
> +        // SAFETY: `serio` is valid; store driver data pointer.
> +        unsafe {
> +            bindings::serio_set_drvdata(serio, raw_data.cast());
> +        }
> +
> +        0
> +    }

[Severity: High]
Does calling serio_open() before T::connect() and serio_set_drvdata() race
with incoming serial interrupts?

serio_open() binds serio->drv under serio->lock and immediately enables
port interrupts via serio->open(serio), while T::connect() sleeps (for
example in AllocatedDevice::new(), input_alloc.register(), and KBox::new()
in StingerDriver::connect()) before drvdata is populated:

CPU0                                  CPU1 (<hardirq>)
connect_callback()
  serio_open(serio, drv)
  T::connect(&mut dev)
                                      serio_interrupt()
                                        spin_lock_irqsave(&serio->lock)
                                        interrupt_callback()
                                          serio_get_drvdata() -> NULL
                                          return IRQ_NONE  <-- drops byte
  serio_set_drvdata(serio, raw_data)
                                        interrupt_callback()
                                          serio_get_drvdata()  <-- data race
                                          ForeignOwnable::borrow_mut(raw)

First, any bytes arriving while T::connect() runs are dropped because
drvdata is still NULL.  In StingerDriver::interrupt(), packets are framed
solely by counting 4 bytes (stinger.idx == 4), so dropping 1 to 3 bytes
during connect permanently misaligns all subsequent packet decoding.

Second, serio_set_drvdata() performs a plain unlocked store to
serio->dev.driver_data without holding serio->lock or issuing a release
barrier, racing with interrupt_callback() reading serio_get_drvdata() and
dereferencing T::Data on weakly-ordered architectures.

Third, StingerDriver::connect() calls input_alloc.register() before
KBox::new(Stinger { ... }, GFP_KERNEL) and before drvdata is set, exposing
the input device to userspace before driver initialization can no longer
fail.

In C serio drivers such as stinger_connect() in
drivers/input/joystick/stinger.c, driver state is allocated and stored via
serio_set_drvdata() before calling serio_open() (which synchronizes under
serio->lock in serio_set_drv()), and input_register_device() is called
after serio_open() succeeds.

> +
> +    /// # 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::IRQ_NONE;
> +        }
> +
> +        // 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: Medium]
Does interrupt_callback() fail to compile when CONFIG_SERIO=y and
CONFIG_RUST=y?

bindgen generates irqreturn and irqreturn_t as type aliases for ffi::c_uint
along with top-level constants (as seen in rust/kernel/irq/request.rs), not
a module or newtype struct.  bindings::irqreturn::IRQ_NONE does not exist
(it is bindings::irqreturn_IRQ_NONE), and bindings::irqreturn_t(ret as
c_uint) attempts to call a type alias as a tuple struct constructor instead
of casting with ret as bindings::irqreturn_t.

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