Re: [PATCH v3 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] Input: joystick - unintended removal of analog.o and db9.o from
  Makefile
- [High] rust: serio: concurrent borrow_mut() and data race between
  start() and interrupt()
- [Medium] rust: serio: non-existent bindings::irqreturn::IRQ_* paths in
  interrupt_callback()
- [Low] rust: serio: non-vertical import style in serio.rs and
  stinger_rust.rs
- [Low] rust: serio: missing #[inline] on Registration::drop() and
  Driver defaults
--

commit 26aa0ef3bea4ec16308e3ef9c8b5857b4bc682ad
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/Makefile b/drivers/input/joystick/Makefile
> index 9976f596a9208..31e23236ff91a 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -10,9 +10,7 @@ obj-$(CONFIG_JOYSTICK_ADC)		+= adc-joystick.o
>  obj-$(CONFIG_JOYSTICK_ADI)		+= adi.o
>  obj-$(CONFIG_JOYSTICK_AMIGA)		+= amijoy.o
>  obj-$(CONFIG_JOYSTICK_AS5011)		+= as5011.o
> -obj-$(CONFIG_JOYSTICK_ANALOG)		+= analog.o
>  obj-$(CONFIG_JOYSTICK_COBRA)		+= cobra.o
> -obj-$(CONFIG_JOYSTICK_DB9)		+= db9.o
>  obj-$(CONFIG_JOYSTICK_FSIA6B)		+= fsia6b.o

[Severity: High]
Were the build rules for analog.o and db9.o removed by mistake? Dropping
these lines causes builds with CONFIG_JOYSTICK_ANALOG or
CONFIG_JOYSTICK_DB9 enabled to stop compiling both drivers.

[ ... ]

> diff --git a/rust/kernel/serio.rs b/rust/kernel/serio.rs
> new file mode 100644
> index 0000000000000..bd3fae462bb53
> --- /dev/null
> +++ b/rust/kernel/serio.rs
> @@ -0,0 +1,301 @@
> +// 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, pin::Pin};

[Severity: Low]
This isn't a bug, but the imports here and in
drivers/input/joystick/stinger_rust.rs group multiple items on a single
line instead of using the kernel Rust vertical import style with one item
per line and a trailing // comment.

[ ... ]

> +        // 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;
> +        }
> +
> +        // 4. Start driver and publish devices now that port is open and interrupts are active.
> +        // SAFETY: `raw_data` was created via `into_foreign()` above and remains valid.
> +        let mut borrowed = unsafe { <T::Data as ForeignOwnable>::borrow_mut(raw_data) };
> +        if let Err(e) = T::start(&mut borrowed) {
> +            // SAFETY: Close port and clean up on start failure.
> +            unsafe {
> +                bindings::serio_close(serio);
> +                bindings::serio_set_drvdata(serio, core::ptr::null_mut());
> +                let data = <T::Data as ForeignOwnable>::from_foreign(raw_data);
> +                T::disconnect(data);
> +            }
> +            return e.to_errno();
> +        }
> +
> +        0
> +    }
> +
> +    /// # 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);

[Severity: High]
Can Registration::interrupt_callback() and Registration::connect_callback()
call ForeignOwnable::borrow_mut() on raw_data at the same time?

Once bindings::serio_open() returns in connect_callback(), port interrupts
are enabled while connect_callback() runs T::start(&mut borrowed) in
process context without holding serio->lock:

CPU0                                  CPU1 (<hardirq>)
Registration::connect_callback()
  bindings::serio_open(serio, drv)
  ForeignOwnable::borrow_mut(raw_data)
  StingerDriver::start(&mut borrowed)
    alloc.register()
                                      Registration::interrupt_callback()
                                        ForeignOwnable::borrow_mut(raw)
                                        StingerDriver::interrupt()
                                          stinger.process_packet()
                                            self.input.as_ref()
    stinger.input = Some(...)  <-- aliasing &mut / data race

This creates two concurrent mutable references to Stinger (undefined
behavior in Rust) and races the unsynchronized write to stinger.input in
StingerDriver::start() against reads of self.input in
Stinger::process_packet(), allowing a torn read of RegisteredDevice.

In addition, on the T::start() error path,
ForeignOwnable::from_foreign(raw_data) is called while borrowed is still
in scope, violating the ForeignOwnable::borrow_mut() safety requirement
that all borrows end before from_foreign() is called.

> +        match ret {
> +            IrqReturn::None => bindings::irqreturn::IRQ_NONE,
> +            IrqReturn::Handled => bindings::irqreturn::IRQ_HANDLED,
> +        }
> +    }

[Severity: Medium]
Does bindings::irqreturn::IRQ_NONE compile when CONFIG_SERIO=y is enabled?
Bindgen generates bindings::irqreturn as a type alias for c_uint and
emits top-level constants bindings::irqreturn_IRQ_NONE and
bindings::irqreturn_IRQ_HANDLED (as used in rust/kernel/irq/request.rs),
so referencing associated constants on bindings::irqreturn fails to
build.

[ ... ]

> +impl<T: Driver> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: Unregister driver with kernel. `self.drv` is valid and registered.
> +        unsafe {
> +            bindings::serio_unregister_driver(self.drv.get());
> +        }
> +    }
> +}

[Severity: Low]
Should Registration::drop() and the default methods Driver::start() and
Driver::disconnect() be marked #[inline]? In the kernel crate, small
functions and functions that forward directly to a C binding call are
annotated with #[inline].

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