* [PATCH 1/3] rust: char_dev: add character device abstraction
2024-10-11 18:55 [PATCH 0/3] Character device abstractions for Rust Josef Zoller
@ 2024-10-11 18:55 ` Josef Zoller
2024-10-12 7:36 ` Greg Kroah-Hartman
2024-10-17 11:38 ` kernel test robot
2024-10-11 18:55 ` [PATCH 2/3] rust: macros: add IoctlCommand derive macro Josef Zoller
` (2 subsequent siblings)
3 siblings, 2 replies; 11+ messages in thread
From: Josef Zoller @ 2024-10-11 18:55 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
rust-for-linux, Josef Zoller
Provide the `CharDevice` and `OpenCharDevice` traits that let you
specify the file operations for a character device. Furthermore, provide
the `IoctlCommand` trait that works together with the character device
traits to provide a way to parse ioctl commands into a Rust type. For
now, only these file operations can be defined for a char device: open,
release, read, write, ioctl, and llseek.
The abstractions do not provide a way to choose the major and minor
numbers for the devices. Instead, registered devices will always be
assigned a unique major number and consecutive minor numbers starting
from 0 and running up to the number of devices registered.
The division into `CharDevice` and `OpenCharDevice` traits is done to
allow greater flexibility when implementing devices that can be opened
multiple times, by holding permanent state in the type implementing
`CharDevice`, and state that is only valid for a single open in the type
implementing `OpenCharDevice`. For simple devices however, that do not
need such complex behavior, it is easy to just implement both traits for
the same type and just clone the state when opening the device.
All file operations except open are optional to implement. If they're
not implemented, they're not registered at all with the kernel. Most of
the operations return a Result type, allowing positive values to be
returned as is, while returning errors using the kernel::error::Error
type. Optionally, the device traits allow specifying a custom error type
that has to be convertible to kernel::error::Error.
The ioctl operation is a bit special, as it first parses the command and
argument into a Rust type using the `IoctlCommand` trait, and then calls
the ioctl method with the parsed command.
The llseek operation gets a mutable reference to the file position as an
extra pos argument, which is copied from f_pos before the operation is
called. This allows the operation to safely modify the file position,
which is then written back to f_pos. Also, the whence argument is
converted into the `Whence` enum, before being passed to the operation.
This patch does not implement the more advanced file operations that
character devices can implement, as the Rust abstractions for the types
required by them are largely still missing. As all of the operations are
optional anyway, the missing operations can be easily added later.
Signed-off-by: Josef Zoller <josef@walterzollerpiano.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/fs.c | 16 +
rust/kernel/char_dev.rs | 976 ++++++++++++++++++++++++++++++++++++++++
rust/kernel/init/macros.rs | 10 +-
rust/kernel/ioctl.rs | 46 +-
rust/kernel/lib.rs | 1 +
6 files changed, 1046 insertions(+), 4 deletions(-)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 4a400a95497915c0fe0da4adfc5dd42a328399e0..fc26db0842212b2ae691c2af748134aee6fb657f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
#include <linux/blk-mq.h>
#include <linux/blk_types.h>
#include <linux/blkdev.h>
+#include <linux/cdev.h>
#include <linux/errname.h>
#include <linux/ethtool.h>
#include <linux/file.h>
diff --git a/rust/helpers/fs.c b/rust/helpers/fs.c
index a75c9676337246ce532ef694e64ba9a7d7ad5842..44904f461dee01293aee9a21b569fc0f50062a62 100644
--- a/rust/helpers/fs.c
+++ b/rust/helpers/fs.c
@@ -5,8 +5,24 @@
*/
#include <linux/fs.h>
+#include <linux/cdev.h>
struct file *rust_helper_get_file(struct file *f)
{
return get_file(f);
}
+
+unsigned int rust_helper_MAJOR(dev_t dev)
+{
+ return MAJOR(dev);
+}
+
+unsigned int rust_helper_MINOR(dev_t dev)
+{
+ return MINOR(dev);
+}
+
+dev_t rust_helper_MKDEV(unsigned int major, unsigned int minor)
+{
+ return MKDEV(major, minor);
+}
diff --git a/rust/kernel/char_dev.rs b/rust/kernel/char_dev.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b81c0d55ab60f18dc82a99991318a5ae0a26e560
--- /dev/null
+++ b/rust/kernel/char_dev.rs
@@ -0,0 +1,976 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Character device support.
+//!
+//! C headers: [`include/linux/cdev.h`](srctree/include/linux/cdev.h) and
+//! [`include/linux/fs.h`](srctree/include/linux/fs.h)
+
+use crate::{
+ bindings, container_of,
+ error::{to_result, VTABLE_DEFAULT_ERROR},
+ fs::{File, LocalFile},
+ ioctl::IoctlCommand,
+ prelude::*,
+ types::{ForeignOwnable, Opaque},
+ uaccess::{UserPtr, UserSlice, UserSliceReader, UserSliceWriter},
+};
+use core::{ffi, mem, ops::Deref};
+
+/// Character device ID.
+///
+/// This is a wrapper around the kernel's `dev_t` type and identifies a
+/// character device by its major and minor numbers.
+#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[repr(transparent)]
+pub struct CharDeviceID(bindings::dev_t); // u32
+
+impl CharDeviceID {
+ /// Creates a new device ID from the given major and minor numbers.
+ ///
+ /// This corresponds to the kernel's `MKDEV` macro.
+ pub fn new(major: u32, minor: u32) -> Self {
+ // SAFETY: The kernel's `MKDEV` macro is safe to call with any values.
+ Self(unsafe { bindings::MKDEV(major, minor) })
+ }
+
+ /// Returns the major number of the device ID.
+ ///
+ /// This corresponds to the kernel's `MAJOR` macro.
+ pub fn major(&self) -> u32 {
+ // SAFETY: The kernel's `MAJOR` macro is safe to call with any value.
+ unsafe { bindings::MAJOR(self.0) }
+ }
+
+ /// Returns the minor number of the device ID.
+ ///
+ /// This corresponds to the kernel's `MINOR` macro.
+ pub fn minor(&self) -> u32 {
+ // SAFETY: The kernel's `MINOR` macro is safe to call with any value.
+ unsafe { bindings::MINOR(self.0) }
+ }
+}
+
+/// Seek mode for the `llseek` method.
+///
+/// This enum corresponds to the `SEEK_*` constants in the kernel.
+#[repr(u32)]
+pub enum Whence {
+ /// Set the file position to `offset`.
+ Set = bindings::SEEK_SET,
+ /// Set the file position to the current position plus `offset`.
+ Cur = bindings::SEEK_CUR,
+ /// Set the file position to the end of the file plus `offset`.
+ End = bindings::SEEK_END,
+ /// Set the file position to the next location in the file greater than or
+ /// equal to `offset` containing data.
+ Data = bindings::SEEK_DATA,
+ /// Set the file position to the next hole in the file greater than or
+ /// equal to `offset`.
+ Hole = bindings::SEEK_HOLE,
+}
+
+// Make sure at compile time that the `Whence` enum can be safely converted
+// from integers up to `SEEK_MAX`.
+const _: () = assert!(Whence::Hole as u32 == bindings::SEEK_MAX);
+
+/// Trait implemented by a registered character device.
+///
+/// A registered character device just handles the `open` operation on the
+/// device file and returns an open device type (which implements the
+/// [`OpenCharDevice`] trait) that handles the actual I/O operations on the
+/// device file. Optionally, the `release` operation can be implemented to
+/// handle the final close of the device file, but simple cleanup can also be
+/// done in the `Drop` implementation of the open device type.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::{
+/// # char_dev::{CharDevice, CharDeviceID, OpenCharDevice},
+/// # fs::{File, LocalFile},
+/// # prelude::*,
+/// # uaccess::UserSliceWriter,
+/// # };
+/// #
+/// struct MyCharDev;
+/// struct MyOpenCharDev;
+///
+/// #[vtable]
+/// impl CharDevice for MyCharDev {
+/// type OpenPtr = Box<MyOpenCharDev>;
+/// type Err = Error;
+///
+/// fn new(_dev_id: CharDeviceID) -> Result<Self, Self::Err> {
+/// Ok(Self)
+/// }
+///
+/// fn open(&self, _file: &File) -> Result<Self::OpenPtr, Self::Err> {
+/// pr_info!("Opened device!\n");
+/// Box::new(MyOpenCharDev, GFP_KERNEL).map_err(Into::into)
+/// }
+/// }
+///
+/// #[vtable]
+/// impl OpenCharDevice for MyOpenCharDev {
+/// type IoctlCmd = ();
+/// type Err = Error;
+///
+/// fn read(
+/// &self,
+/// _file: &LocalFile,
+/// _buf: UserSliceWriter,
+/// _offset: &mut i64,
+/// ) -> Result<usize, Self::Err> {
+/// pr_info!("Read from device!\n");
+/// Ok(0)
+/// }
+/// }
+/// ```
+#[vtable]
+pub trait CharDevice: Sized + Send + Sync + 'static {
+ /// The type of a (smart) pointer to the associated open device type.
+ ///
+ /// This type must implement the [`ForeignOwnable`] trait with the
+ /// associated borrowed type being a reference to the open device type.
+ ///
+ /// Most likely, this will be a smart pointer type like [`Box`] or [`Arc`],
+ /// just wrapping the open device type.
+ ///
+ /// [`Arc`]: crate::sync::Arc
+ type OpenPtr: for<'a> ForeignOwnable<Borrowed<'a>: Deref<Target: OpenCharDevice>>;
+
+ /// The error type returned by the device operations.
+ ///
+ /// This type must be convertible into the kernel [`Error`] type.
+ type Err: Into<Error>;
+
+ /// Creates a new instance of the character device.
+ ///
+ /// This is called when the device is registered with the kernel. The
+ /// registered device ID is passed as the `dev_id` parameter.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the device creation fails.
+ fn new(dev_id: CharDeviceID) -> Result<Self, Self::Err>;
+
+ /// Handles the `open` operation on the device file.
+ ///
+ /// This is called when the device file is opened. The `file` parameter
+ /// contains a reference to the file object representing the opened file.
+ ///
+ /// The function should return a pointer to an open device type that
+ /// implements the [`OpenCharDevice`] trait. This type will handle the
+ /// actual I/O operations on the opened device file. The returned pointer
+ /// will internally be written to the `private_data` field of the file
+ /// object. If the release operation is implemented, you are guaranteed to
+ /// receive this exact pointer as an argument when the final close of the
+ /// device file happens.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the operation fails. In this case,
+ /// the error will be propagated back to the user space as an error code.
+ fn open(&self, _file: &File) -> Result<Self::OpenPtr, Self::Err>;
+
+ /// Handles the `release` operation on the device file.
+ ///
+ /// This is called when the device file is closed. The `file` parameter
+ /// contains a reference to the file object representing the closed file.
+ /// The `open_dev` parameter contains the pointer to the open device type
+ /// that was returned by the corresponding `open` operation.
+ ///
+ /// This function is optional. If not implemented, the kernel will just
+ /// drop the open device type pointer and return success to the user space.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the operation fails. In this case,
+ /// the error will be propagated back to the user space as an error code.
+ fn release(&self, _file: &File, _open_dev: Self::OpenPtr) -> Result<(), Self::Err> {
+ kernel::build_error(VTABLE_DEFAULT_ERROR)
+ }
+}
+
+/// Trait implemented by an open character device.
+///
+/// An open character device handles the actual I/O operations on the device
+/// file. It is returned by the `open` operation of the associated character
+/// device type that implements the [`CharDevice`] trait.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::{
+/// # char_dev::{CharDevice, CharDeviceID, OpenCharDevice},
+/// # fs::{File, LocalFile},
+/// # prelude::*,
+/// # uaccess::UserSliceWriter,
+/// # };
+/// #
+/// struct MyCharDev;
+/// struct MyOpenCharDev;
+///
+/// #[vtable]
+/// impl CharDevice for MyCharDev {
+/// type OpenPtr = Box<MyOpenCharDev>;
+/// type Err = Error;
+///
+/// fn new(_dev_id: CharDeviceID) -> Result<Self, Self::Err> {
+/// Ok(Self)
+/// }
+///
+/// fn open(&self, _file: &File) -> Result<Self::OpenPtr, Self::Err> {
+/// pr_info!("Opened device!\n");
+/// Box::new(MyOpenCharDev, GFP_KERNEL).map_err(Into::into)
+/// }
+/// }
+///
+/// #[vtable]
+/// impl OpenCharDevice for MyOpenCharDev {
+/// type IoctlCmd = ();
+/// type Err = Error;
+///
+/// fn read(
+/// &self,
+/// _file: &LocalFile,
+/// _buf: UserSliceWriter,
+/// _offset: &mut i64,
+/// ) -> Result<usize, Self::Err> {
+/// pr_info!("Read from device!\n");
+/// Ok(0)
+/// }
+/// }
+/// ```
+#[vtable]
+pub trait OpenCharDevice: Send + Sync {
+ /// The type of the ioctl command that can be passed to the device.
+ ///
+ /// This type must implement the [`IoctlCommand`] trait, which is
+ /// normally done by deriving the trait using the eponymous macro.
+ ///
+ /// If the device does not support any ioctl commands, this type can be
+ /// set to `()`.
+ type IoctlCmd: IoctlCommand;
+
+ /// The error type returned by the device operations.
+ ///
+ /// This type must be convertible into the kernel [`Error`] type.
+ type Err: Into<Error>;
+
+ /// Handles the `read` operation on the device file.
+ ///
+ /// This is called when the device file is read from. The `file` parameter
+ /// contains a reference to the file object representing the opened file.
+ /// The `user_writer` parameter contains a writer that can be used to write
+ /// the requested data to the user space buffer.
+ /// The `offset` parameter contains the current offset in the file, and
+ /// should be updated to the new offset after the read operation.
+ ///
+ /// The function should return the number of bytes read from the device.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the operation fails. In this case,
+ /// the error will be propagated back to the user space as an error code.
+ fn read(
+ &self,
+ _file: &LocalFile,
+ _user_writer: UserSliceWriter,
+ _offset: &mut i64,
+ ) -> Result<usize, Self::Err> {
+ kernel::build_error(VTABLE_DEFAULT_ERROR)
+ }
+
+ /// Handles the `write` operation on the device file.
+ ///
+ /// This is called when the device file is written to. The `file` parameter
+ /// contains a reference to the file object representing the opened file.
+ /// The `user_reader` parameter contains a reader that can be used to read
+ /// the provided data from the user space buffer.
+ /// The `offset` parameter contains the current offset in the file, and
+ /// should be updated to the new offset after the write operation.
+ ///
+ /// The function should return the number of bytes written to the device.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the operation fails. In this case,
+ /// the error will be propagated back to the user space as an error code.
+ fn write(
+ &self,
+ _file: &LocalFile,
+ _user_reader: UserSliceReader,
+ _offset: &mut i64,
+ ) -> Result<usize, Self::Err> {
+ kernel::build_error(VTABLE_DEFAULT_ERROR)
+ }
+
+ /// Handles the `ioctl` operation on the device file.
+ ///
+ /// This is called when the user space application performs an ioctl
+ /// operation on the device file. The `file` parameter contains a reference
+ /// to the file object representing the opened file.
+ /// The `cmd` parameter contains the ioctl command to execute.
+ /// The `compat` parameter indicates whether the ioctl operation is
+ /// performed in compatibility mode, meaning that the user space application
+ /// is running in 32-bit mode on a 64-bit kernel.
+ ///
+ /// The function should return the result of the ioctl operation, which
+ /// could include writing data to a user space buffer, depending on the
+ /// command in question.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the operation fails. In this case,
+ /// the error will be propagated back to the user space as an error code.
+ fn ioctl(
+ &self,
+ _file: &File,
+ _cmd: Self::IoctlCmd,
+ #[cfg(CONFIG_COMPAT)] _compat: bool,
+ ) -> Result<u64, Self::Err> {
+ kernel::build_error(VTABLE_DEFAULT_ERROR)
+ }
+
+ /// Handles the `llseek` operation on the device file.
+ ///
+ /// This is called when the device file is seeking. The `file` parameter
+ /// contains a reference to the file object representing the opened file.
+ /// The `pos` parameter contains the current position in the file, and should
+ /// be updated to the new position after the seek operation. The `offset`
+ /// parameter contains the new offset to seek to, and the `whence` parameter
+ /// contains the seek mode.
+ ///
+ /// The function should return the new offset after the seek operation.
+ ///
+ /// # Errors
+ ///
+ /// This function can return an error if the operation fails. In this case,
+ /// the error will be propagated back to the user space as an error code.
+ fn llseek(
+ &self,
+ _file: &LocalFile,
+ _pos: &mut i64,
+ _offset: i64,
+ _whence: Whence,
+ ) -> Result<u64, Self::Err> {
+ kernel::build_error(VTABLE_DEFAULT_ERROR)
+ }
+}
+
+/// This type alias saves a lot of long types in the fops functions.
+#[rustfmt::skip]
+type OpenDev<'a, T> = <
+ <<T as CharDevice>::OpenPtr as ForeignOwnable>::Borrowed<'a> as Deref
+>::Target;
+
+/// This struct wraps a [`CharDevice`] together with a kernel [`cdev`] object.
+///
+/// This allows us to retrieve the device type from the `inode` object in the
+/// `open` function using the `container_of` macro.
+#[pin_data]
+struct CharDeviceContainer<T: CharDevice> {
+ #[pin]
+ cdev: Opaque<bindings::cdev>,
+ inner: T,
+}
+
+impl<T: CharDevice> CharDeviceContainer<T> {
+ /// Creates a new `CharDeviceContainer` from a device type and a base device ID.
+ ///
+ /// This function initializes a new `cdev` object and registers it with the
+ /// kernel using the device ID calculated from the base device ID and the
+ /// index `i`.
+ fn register(
+ i: usize,
+ base_dev_id: CharDeviceID,
+ fops: *const bindings::file_operations,
+ ) -> impl PinInit<Self, Error> {
+ let dev_id = CharDeviceID(base_dev_id.0 + i as u32);
+
+ try_pin_init!(Self {
+ cdev <- Opaque::try_ffi_init(|slot: *mut bindings::cdev| {
+ // SAFETY: The `slot` pointer is valid but uninitialized and
+ // `cdev_init` only writes to it. Also, the `fops` pointer is
+ // never dereferenced without checking for null first.
+ unsafe { bindings::cdev_init(slot, fops) };
+
+ // SAFETY: Since `cdev_init` was called, `slot` is valid and
+ // initialized, which means that `cdev_add` is safe to call.
+ to_result(unsafe {
+ bindings::cdev_add(slot, dev_id.0, 1)
+ })
+ }),
+ inner: T::new(dev_id).map_err(Into::into)?,
+ }? Error)
+ }
+}
+
+/// This struct represents a character device registration.
+///
+/// It manages the registration of `NUM_MINORS` character devices of type `T`
+/// with consecutive device IDs starting from `base_dev_id`.
+///
+/// The devices are automatically unregistered when the registration object is
+/// dropped.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::{
+/// # c_str,
+/// # char_dev::{CharDevice, CharDeviceID, DeviceRegistration, OpenCharDevice},
+/// # fs::File,
+/// # prelude::*,
+/// # };
+/// #
+/// struct MyCharDev;
+/// struct MyOpenCharDev;
+///
+/// #[vtable]
+/// impl CharDevice for MyCharDev {
+/// // --snip--
+/// # type OpenPtr = Box<MyOpenCharDev>;
+/// # type Err = Error;
+/// #
+/// # fn new(_dev_id: CharDeviceID) -> Result<Self, Self::Err> {
+/// # Ok(Self)
+/// # }
+/// #
+/// # fn open(&self, _file: &File) -> Result<Self::OpenPtr, Self::Err> {
+/// # Box::new(MyOpenCharDev, GFP_KERNEL).map_err(Into::into)
+/// # }
+/// }
+///
+/// #[vtable]
+/// impl OpenCharDevice for MyOpenCharDev {
+/// // --snip--
+/// # type IoctlCmd = ();
+/// # type Err = Error;
+/// }
+///
+/// const DEV_NAME: &CStr = c_str!("my_char_dev");
+/// const NUM_MINORS: usize = 5;
+///
+/// struct MyModule {
+/// reg: Pin<Box<DeviceRegistration<MyCharDev, NUM_MINORS>>>,
+/// }
+///
+/// impl kernel::Module for MyModule {
+/// fn init(module: &'static ThisModule) -> Result<Self> {
+/// let reg = Box::pin_init(DeviceRegistration::register(module, DEV_NAME), GFP_KERNEL)?;
+///
+/// let dev_id = reg.get_base_dev_id();
+/// pr_info!(
+/// "Registered device {DEV_NAME} with major {} and minors {} through {} \n",
+/// dev_id.major(),
+/// dev_id.minor(),
+/// dev_id.minor() + NUM_MINORS as u32 - 1
+/// );
+///
+/// Ok(Self { reg })
+/// }
+/// }
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct DeviceRegistration<T: CharDevice, const NUM_MINORS: usize> {
+ base_dev_id: CharDeviceID,
+ #[pin]
+ fops: Opaque<bindings::file_operations>,
+ #[pin]
+ devices: [CharDeviceContainer<T>; NUM_MINORS],
+}
+
+impl<T: CharDevice, const NUM_MINORS: usize> DeviceRegistration<T, NUM_MINORS> {
+ /// Registers `NUM_MINORS` character devices of type `T` with the kernel.
+ ///
+ /// The devices are registered with the name `name` and the module `module`.
+ ///
+ /// The function returns a `PinInit` that resolves to a `Registration`
+ /// object if the registration was successful.
+ pub fn register(module: &'static ThisModule, name: &'static CStr) -> impl PinInit<Self, Error> {
+ try_pin_init!(&this in Self {
+ base_dev_id: Self::alloc_region(name)?,
+ fops <- fops::create_vtable::<T>(module),
+ devices <- init::pin_init_array_from_fn(|i| {
+ // SAFETY: `this` is a non-null pointer to a partially
+ // initialized `Registration` object, where `base_dev_id`
+ // and `fops` are already initialized, so it is safe to
+ // dereference it and access these fields.
+ unsafe {
+ CharDeviceContainer::register(
+ i,
+ (*this.as_ptr()).base_dev_id,
+ (*this.as_ptr()).fops.get(),
+ )
+ }
+ }),
+ })
+ }
+
+ /// Returns the base device ID of the registration.
+ ///
+ /// The base device ID is the device ID of the first device registered by
+ /// this registration. The device IDs of the other devices are consecutive
+ /// numbers starting from this ID.
+ pub fn get_base_dev_id(&self) -> CharDeviceID {
+ self.base_dev_id
+ }
+
+ fn alloc_region(name: &'static CStr) -> Result<CharDeviceID> {
+ let mut dev_id = CharDeviceID::default();
+
+ // SAFETY: The `dev_id` pointer is valid and thus safe to write to,
+ // which means that `alloc_chrdev_region` is safe to call.
+ to_result(unsafe {
+ bindings::alloc_chrdev_region(
+ &mut dev_id as *mut CharDeviceID as *mut bindings::dev_t,
+ 0,
+ NUM_MINORS as u32,
+ name.as_char_ptr(),
+ )
+ })?;
+
+ Ok(dev_id)
+ }
+}
+
+// SAFETY: Registration with and unregistration of character devices can happen
+// from any thread or CPU, so `Registration` can be `Send`.
+unsafe impl<T: CharDevice, const NUM_MINORS: usize> Send for DeviceRegistration<T, NUM_MINORS> {}
+
+// SAFETY: `Registration` doesn't offer any methods or access to fields when
+// shared between threads or CPUs, so it is safe to share it.
+unsafe impl<T: CharDevice, const NUM_MINORS: usize> Sync for DeviceRegistration<T, NUM_MINORS> {}
+
+#[pinned_drop]
+impl<T: CharDevice, const NUM_MINORS: usize> PinnedDrop for DeviceRegistration<T, NUM_MINORS> {
+ /// Unregisters the character devices.
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY: We never move out of `this`.
+ let this = unsafe { self.get_unchecked_mut() };
+
+ for i in 0..NUM_MINORS {
+ // SAFETY: All `cdev`s in the device containers have been
+ // initialized and added to the system, so it is safe to call
+ // `cdev_del` on them.
+ unsafe {
+ bindings::cdev_del(this.devices[i].cdev.get());
+ }
+ }
+
+ // SAFETY: The device region has been allocated and registered, so it is
+ // safe to call `unregister_chrdev_region` on it.
+ unsafe {
+ bindings::unregister_chrdev_region(this.base_dev_id.0, NUM_MINORS as u32);
+ }
+ }
+}
+
+mod fops {
+ use super::*;
+
+ /// Creates a file operations table for a character device of type `T`.
+ pub(super) fn create_vtable<T: CharDevice>(
+ module: &'static ThisModule,
+ ) -> impl PinInit<Opaque<bindings::file_operations>> {
+ Opaque::ffi_init(|slot: *mut bindings::file_operations| {
+ let fops = bindings::file_operations {
+ owner: module.as_ptr(),
+ open: Some(open::<T>),
+ release: Some(close::<T>),
+ read: <OpenDev<'_, T>>::HAS_READ.then_some(read::<T>),
+ write: <OpenDev<'_, T>>::HAS_WRITE.then_some(write::<T>),
+ unlocked_ioctl: <OpenDev<'_, T>>::HAS_IOCTL.then_some(unlocked_ioctl::<T>),
+ #[cfg(CONFIG_COMPAT)]
+ compat_ioctl: <OpenDev<'_, T>>::HAS_IOCTL.then_some(compat_ioctl::<T>),
+ llseek: <OpenDev<'_, T>>::HAS_LLSEEK.then_some(llseek::<T>),
+ ..bindings::file_operations::default()
+ };
+
+ // SAFETY: `slot` is a valid but uninitialized pointer to a
+ // `file_operations` object, so it is safe to write to it.
+ unsafe {
+ slot.write(fops);
+ }
+ })
+ }
+
+ /// Handles the `open` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `inode` points at a valid inode and that
+ /// its `i_cdev` field points at a valid `cdev`, contained by a
+ /// `CharDeviceContainer<T>`.
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there are active `fdget_pos` calls on
+ /// this file, then they took the `f_pos_lock` mutex.
+ unsafe extern "C" fn open<T: CharDevice>(
+ inode: *mut bindings::inode,
+ file: *mut bindings::file,
+ ) -> i32 {
+ // Handle non O_LARGEFILE open on 32-bit systems
+ //
+ // SAFETY: The caller guarantees that `inode` points at a valid inode
+ // and that `file` points at a valid file, so it is safe to call
+ // `generic_file_open` on them.
+ let ret = unsafe { bindings::generic_file_open(inode, file) };
+ if ret != 0 {
+ return ret;
+ }
+
+ // SAFETY: The caller guarantees that `inode` points at a valid inode
+ // and that its `i_cdev` field points at a valid `cdev`, contained by a
+ // `CharDeviceContainer<T>`, so it is safe to access this container.
+ let container = unsafe {
+ &*container_of!(
+ (*inode).__bindgen_anon_4.i_cdev,
+ CharDeviceContainer<T>,
+ cdev
+ )
+ };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that any active
+ // `fdget_pos` calls on this file took the `f_pos_lock` mutex, so it is
+ // safe to call `File::from_raw_file` on it.
+ let file = unsafe { File::from_raw_file(file) };
+
+ let open_dev = match container.inner.open(file).map_err(Into::into) {
+ Ok(open_dev) => open_dev,
+ Err(e) => {
+ return e.to_errno();
+ }
+ };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file, so
+ // it is safe to access and modify the `private_data` field on it.
+ unsafe {
+ (*file.as_ptr()).private_data = open_dev.into_foreign().cast_mut();
+ }
+
+ 0
+ }
+
+ /// Handles the `close` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `inode` points at a valid inode and that
+ /// its `i_cdev` field points at a valid `cdev`, contained by a
+ /// `CharDeviceContainer<T>`.
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there are active `fdget_pos` calls on
+ /// this file, then they took the `f_pos_lock` mutex.
+ /// * The caller must ensure that the `private_data` field of `file` is a
+ /// pointer returned by [`ForeignOwnable::into_foreign`] for which a
+ /// previous matching [`ForeignOwnable::from_foreign`] hasn't been called
+ /// yet. Additionally, all instances (if any) of values returned by
+ /// [`ForeignOwnable::borrow`] for this object must have been dropped.
+ unsafe extern "C" fn close<T: CharDevice>(
+ inode: *mut bindings::inode,
+ file: *mut bindings::file,
+ ) -> i32 {
+ // SAFETY: The caller guarantees that `inode` points at a valid inode
+ // and that its `i_cdev` field points at a valid `cdev`, contained by a
+ // `CharDeviceContainer<T>`, so it is safe to access this container.
+ let container = unsafe {
+ &*container_of!(
+ (*inode).__bindgen_anon_4.i_cdev,
+ CharDeviceContainer<T>,
+ cdev
+ )
+ };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file,
+ // that its `private_data` field is a pointer returned by
+ // `ForeignOwnable::into_foreign` for which a previous matching
+ // `ForeignOwnable::from_foreign` hasn't been called yet, and that all
+ // instances (if any) of values returned by `ForeignOwnable::borrow` for
+ // this object have been dropped already, so it is safe to call
+ // `ForeignOwnable::from_foreign` on it.
+ let open_dev =
+ unsafe { <T::OpenPtr as ForeignOwnable>::from_foreign((*file).private_data) };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that any active
+ // `fdget_pos` calls on this file took the `f_pos_lock` mutex, so it is
+ // safe to call `File::from_raw_file` on it.
+ let file = unsafe { File::from_raw_file(file) };
+
+ if T::HAS_RELEASE {
+ if let Err(e) = container.inner.release(file, open_dev).map_err(Into::into) {
+ return e.to_errno();
+ }
+ }
+
+ 0
+ }
+
+ /// Handles the `read` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there is an active `fdget_pos` call on
+ /// this file that didn't take the `f_pos_lock` mutex, then that call is
+ /// on the current thread.
+ /// * The caller must ensure that the `private_data` field of `file` is a
+ /// pointer returned by [`ForeignOwnable::into_foreign`] for which a
+ /// previous matching [`ForeignOwnable::from_foreign`] hasn't been called
+ /// yet.
+ /// * The caller must ensure that the `offset` pointer is valid and
+ /// initialized, and that we have exclusive access to it for the duration
+ /// of the function call.
+ unsafe extern "C" fn read<T: CharDevice>(
+ file: *mut bindings::file,
+ user_buffer: *mut ffi::c_char,
+ count: usize,
+ offset: *mut bindings::loff_t,
+ ) -> isize {
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that its `private_data` field is a pointer returned by
+ // `ForeignOwnable::into_foreign` for which a previous matching
+ // `ForeignOwnable::from_foreign` hasn't been called yet, so it is safe
+ // to call `ForeignOwnable::borrow` on it.
+ let open_dev = unsafe { <T::OpenPtr as ForeignOwnable>::borrow((*file).private_data) };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that if there is an
+ // active `fdget_pos` call on this file that didn't take the
+ // `f_pos_lock` mutex, then that call is on the current thread, so it is
+ // safe to call `LocalFile::from_raw_file` on it.
+ let file = unsafe { LocalFile::from_raw_file(file) };
+
+ let user_writer = UserSlice::new(user_buffer as UserPtr, count).writer();
+
+ // SAFETY: The caller guarantees that `offset` is a valid and
+ // initialized pointer, and that we have exclusive access to it for the
+ // duration of the function call, so it is safe to dereference and form
+ // a mutable reference to it.
+ let offset = unsafe { &mut *offset };
+
+ match open_dev.read(file, user_writer, offset).map_err(Into::into) {
+ Ok(n) => n as isize,
+ Err(e) => e.to_errno() as isize,
+ }
+ }
+
+ /// Handles the `write` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there is an active `fdget_pos` call on
+ /// this file that didn't take the `f_pos_lock` mutex, then that call is
+ /// on the current thread.
+ /// * The caller must ensure that the `private_data` field of `file` is a
+ /// pointer returned by [`ForeignOwnable::into_foreign`] for which a
+ /// previous matching [`ForeignOwnable::from_foreign`] hasn't been called
+ /// yet.
+ /// * The caller must ensure that the `offset` pointer is valid and
+ /// initialized, and that we have exclusive access to it for the duration
+ /// of the function call.
+ unsafe extern "C" fn write<T: CharDevice>(
+ file: *mut bindings::file,
+ user_buffer: *const ffi::c_char,
+ count: usize,
+ offset: *mut bindings::loff_t,
+ ) -> isize {
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that its `private_data` field is a pointer returned by
+ // `ForeignOwnable::into_foreign` for which a previous matching
+ // `ForeignOwnable::from_foreign` hasn't been called yet, so it is safe
+ // to call `ForeignOwnable::borrow` on it.
+ let open_dev = unsafe { <T::OpenPtr as ForeignOwnable>::borrow((*file).private_data) };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that if there is an
+ // active `fdget_pos` call on this file that didn't take the
+ // `f_pos_lock` mutex, then that call is on the current thread, so it is
+ // safe to call `LocalFile::from_raw_file` on it.
+ let file = unsafe { LocalFile::from_raw_file(file) };
+
+ let user_reader = UserSlice::new(user_buffer as UserPtr, count).reader();
+
+ // SAFETY: The caller guarantees that `offset` is a valid and
+ // initialized pointer, and that we have exclusive access to it for the
+ // duration of the function call, so it is safe to dereference and form
+ // a mutable reference to it.
+ let offset = unsafe { &mut *offset };
+
+ match open_dev
+ .write(file, user_reader, offset)
+ .map_err(Into::into)
+ {
+ Ok(n) => n as isize,
+ Err(e) => e.to_errno() as isize,
+ }
+ }
+
+ /// Handles the `unlocked_ioctl` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there are active `fdget_pos` calls on
+ /// this file, then they took the `f_pos_lock` mutex.
+ /// * The caller must ensure that the `private_data` field of `file` is a
+ /// pointer returned by [`ForeignOwnable::into_foreign`] for which a
+ /// previous matching [`ForeignOwnable::from_foreign`] hasn't been called
+ /// yet.
+ unsafe extern "C" fn unlocked_ioctl<T: CharDevice>(
+ file: *mut bindings::file,
+ cmd: ffi::c_uint,
+ arg: ffi::c_ulong,
+ ) -> ffi::c_long {
+ type Cmd<'a, T> = <OpenDev<'a, T> as OpenCharDevice>::IoctlCmd;
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that its `private_data` field is a pointer returned by
+ // `ForeignOwnable::into_foreign` for which a previous matching
+ // `ForeignOwnable::from_foreign` hasn't been called yet, so it is safe
+ // to call `ForeignOwnable::borrow` on it.
+ let open_dev = unsafe { <T::OpenPtr as ForeignOwnable>::borrow((*file).private_data) };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that any active
+ // `fdget_pos` calls on this file took the `f_pos_lock` mutex, so it is
+ // safe to call `File::from_raw_file` on it.
+ let file = unsafe { File::from_raw_file(file) };
+
+ let cmd = match <Cmd<'_, T> as IoctlCommand>::parse(cmd, arg).map_err(Into::into) {
+ Ok(cmd) => cmd,
+ Err(e) => return e.to_errno() as ffi::c_long,
+ };
+
+ match open_dev
+ .ioctl(
+ file,
+ cmd,
+ #[cfg(CONFIG_COMPAT)]
+ false,
+ )
+ .map_err(Into::into)
+ {
+ Ok(ret) => ret as ffi::c_long,
+ Err(e) => e.to_errno() as ffi::c_long,
+ }
+ }
+
+ /// Handles the `compat_ioctl` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there are active `fdget_pos` calls on
+ /// this file, then they took the `f_pos_lock` mutex.
+ /// * The caller must ensure that the `private_data` field of `file` is a
+ /// pointer returned by [`ForeignOwnable::into_foreign`] for which a
+ /// previous matching [`ForeignOwnable::from_foreign`] hasn't been called
+ /// yet.
+ #[cfg(CONFIG_COMPAT)]
+ unsafe extern "C" fn compat_ioctl<T: CharDevice>(
+ file: *mut bindings::file,
+ cmd: ffi::c_uint,
+ arg: ffi::c_ulong,
+ ) -> ffi::c_long {
+ type Cmd<'a, T> = <OpenDev<'a, T> as OpenCharDevice>::IoctlCmd;
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that its `private_data` field is a pointer returned by
+ // `ForeignOwnable::into_foreign` for which a previous matching
+ // `ForeignOwnable::from_foreign` hasn't been called yet, so it is safe
+ // to call `ForeignOwnable::borrow` on it.
+ let open_dev = unsafe { <T::OpenPtr as ForeignOwnable>::borrow((*file).private_data) };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that any active
+ // `fdget_pos` calls on this file took the `f_pos_lock` mutex, so it is
+ // safe to call `File::from_raw_file` on it.
+ let file = unsafe { File::from_raw_file(file) };
+
+ let parse_fn = if <Cmd<'_, T> as IoctlCommand>::HAS_COMPAT_PARSE {
+ <Cmd<'_, T> as IoctlCommand>::compat_parse
+ } else {
+ <Cmd<'_, T> as IoctlCommand>::parse
+ };
+
+ let cmd = match parse_fn(cmd, arg).map_err(Into::into) {
+ Ok(cmd) => cmd,
+ Err(e) => return e.to_errno() as ffi::c_long,
+ };
+
+ match open_dev.ioctl(file, cmd, true).map_err(Into::into) {
+ Ok(ret) => ret as ffi::c_long,
+ Err(e) => e.to_errno() as ffi::c_long,
+ }
+ }
+
+ /// Handles the `llseek` operation for a character device.
+ ///
+ /// # Safety
+ ///
+ /// * The caller must ensure that `file` points at a valid file and that the
+ /// file's refcount is positive for the duration of the function call.
+ /// * The caller must ensure that if there is an active `fdget_pos` call on
+ /// this file that didn't take the `f_pos_lock` mutex, then that call is
+ /// on the current thread.
+ /// * The caller must ensure that the `private_data` field of `file` is a
+ /// pointer returned by [`ForeignOwnable::into_foreign`] for which a
+ /// previous matching [`ForeignOwnable::from_foreign`] hasn't been called
+ /// yet.
+ /// * The caller must ensure that `whence` is less than or equal to `SEEK_MAX`.
+ unsafe extern "C" fn llseek<T: CharDevice>(
+ file: *mut bindings::file,
+ offset: bindings::loff_t,
+ whence: ffi::c_int,
+ ) -> bindings::loff_t {
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that its `private_data` field is a pointer returned by
+ // `ForeignOwnable::into_foreign` for which a previous matching
+ // `ForeignOwnable::from_foreign` hasn't been called yet, so it is safe
+ // to call `ForeignOwnable::borrow` on it.
+ let open_dev = unsafe { <T::OpenPtr as ForeignOwnable>::borrow((*file).private_data) };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file, so
+ // it is safe to access its `f_pos` field.
+ let mut pos = unsafe { (*file).f_pos };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file and
+ // that the file's refcount is positive, as well as that if there is an
+ // active `fdget_pos` call on this file that didn't take the
+ // `f_pos_lock` mutex, then that call is on the current thread, so it is
+ // safe to call `LocalFile::from_raw_file` on it.
+ let file = unsafe { LocalFile::from_raw_file(file) };
+
+ // SAFETY: The caller guarantees that `whence` is less than or equal to
+ // `SEEK_MAX`, so it is safe to transmute it to a `Whence` by the
+ // constant assertion above.
+ let whence = unsafe { mem::transmute::<u32, Whence>(whence as _) };
+
+ let res = match open_dev
+ .llseek(file, &mut pos, offset, whence)
+ .map_err(Into::into)
+ {
+ Ok(pos) => pos as bindings::loff_t,
+ Err(e) => e.to_errno() as bindings::loff_t,
+ };
+
+ // SAFETY: The caller guarantees that `file` points at a valid file, so
+ // it is safe to access its `f_pos` field.
+ unsafe {
+ (*file.as_ptr()).f_pos = pos;
+ }
+
+ res
+ }
+}
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 1fd146a832416514a2bdcb269615509d75e3a559..d75376bb5ba8e85d19a106917b5d5ce3febc7533 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -1154,9 +1154,13 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
unsafe { ::core::ptr::write_bytes(slot, 0, 1) };
$init_zeroed // This will be `()` if set.
})?
- // Create the `this` so it can be referenced by the user inside of the
- // expressions creating the individual fields.
- $(let $this = unsafe { ::core::ptr::NonNull::new_unchecked(slot) };)?
+ $(
+ // Create the `this` so it can be referenced by the user inside of the
+ // expressions creating the individual fields.
+ //
+ // SAFETY: `slot` is valid, because we are inside of an initializer closure.
+ let $this = unsafe { ::core::ptr::NonNull::new_unchecked(slot) };
+ )?
// Initialize every field.
$crate::__init_internal!(init_slot($($use_data)?):
@data(data),
diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
index 2fc7662339e54b20153a46da06cc5c2e450024da..03359ab28495b94d98d53db2115bbbcc520c18a3 100644
--- a/rust/kernel/ioctl.rs
+++ b/rust/kernel/ioctl.rs
@@ -6,7 +6,8 @@
#![expect(non_snake_case)]
-use crate::build_assert;
+use crate::{build_assert, error::VTABLE_DEFAULT_ERROR, prelude::*};
+use core::ffi;
/// Build an ioctl number, analogous to the C macro of the same name.
#[inline(always)]
@@ -70,3 +71,46 @@ pub const fn _IOC_NR(nr: u32) -> u32 {
pub const fn _IOC_SIZE(nr: u32) -> usize {
((nr >> uapi::_IOC_SIZESHIFT) & uapi::_IOC_SIZEMASK) as usize
}
+
+/// Types implementing this trait can be used to parse ioctl commands.
+#[vtable]
+pub trait IoctlCommand: Sized + Send + Sync + 'static {
+ /// The error type returned by the parse functions.
+ ///
+ /// This type must be convertible into the kernel [`Error`] type.
+ type Err: Into<Error>;
+
+ /// Parse an ioctl command.
+ ///
+ /// This function parses the `cmd` argument as an ioctl command number
+ /// and returns a command that interprets the `arg` argument as needed.
+ ///
+ /// # Errors
+ ///
+ /// This function may return an error if the command is invalid.
+ fn parse(cmd: ffi::c_uint, arg: ffi::c_ulong) -> Result<Self, Self::Err>;
+
+ /// Parse an ioctl command for compatibility mode.
+ ///
+ /// If the compatibility mode is enabled, this function parses the `cmd`
+ /// argument as an ioctl command number and returns a command that
+ /// interprets the `arg` argument as needed. The values come from a 32-bit
+ /// user-space application and may need to be parsed differently.
+ ///
+ /// # Errors
+ ///
+ /// This function may return an error if the command is invalid.
+ #[cfg(CONFIG_COMPAT)]
+ fn compat_parse(_cmd: ffi::c_uint, _arg: ffi::c_ulong) -> Result<Self, Self::Err> {
+ kernel::build_error(VTABLE_DEFAULT_ERROR)
+ }
+}
+
+#[vtable]
+impl IoctlCommand for () {
+ type Err = Error;
+
+ fn parse(_cmd: ffi::c_uint, _arg: ffi::c_ulong) -> Result<Self> {
+ Err(ENOTTY)
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6bcbe9bbb46294e29c8d78cb4cae3cbe13062104..86969b1ac599cf00683eef1182afb39811ba88b7 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -31,6 +31,7 @@
#[cfg(CONFIG_BLOCK)]
pub mod block;
mod build_assert;
+pub mod char_dev;
pub mod device;
pub mod error;
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
--
2.47.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] rust: char_dev: add character device abstraction
2024-10-11 18:55 ` [PATCH 1/3] rust: char_dev: add character device abstraction Josef Zoller
@ 2024-10-12 7:36 ` Greg Kroah-Hartman
2024-10-12 21:48 ` Josef Zoller
2024-10-17 11:38 ` kernel test robot
1 sibling, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2024-10-12 7:36 UTC (permalink / raw)
To: Josef Zoller
Cc: Arnd Bergmann, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, linux-kernel, rust-for-linux
On Fri, Oct 11, 2024 at 08:55:42PM +0200, Josef Zoller wrote:
> +unsigned int rust_helper_MAJOR(dev_t dev)
> +{
> + return MAJOR(dev);
> +}
I don't think you use this function in the code anywhere.
> +unsigned int rust_helper_MINOR(dev_t dev)
> +{
> + return MINOR(dev);
> +}
Is this really needed? No driver should care about their minor number,
except to possibly set it, not read it.
> +dev_t rust_helper_MKDEV(unsigned int major, unsigned int minor)
> +{
> + return MKDEV(major, minor);
> +}
If you are only doing dynamic creation, as your initial text said, I
don't think you need this either as the kernel should create it for you.
> diff --git a/rust/kernel/char_dev.rs b/rust/kernel/char_dev.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b81c0d55ab60f18dc82a99991318a5ae0a26e560
> --- /dev/null
> +++ b/rust/kernel/char_dev.rs
> @@ -0,0 +1,976 @@
> +// SPDX-License-Identifier: GPL-2.0
Minor nit, you forgot a copyright line :)
> +
> +//! Character device support.
> +//!
> +//! C headers: [`include/linux/cdev.h`](srctree/include/linux/cdev.h) and
> +//! [`include/linux/fs.h`](srctree/include/linux/fs.h)
> +
> +use crate::{
> + bindings, container_of,
> + error::{to_result, VTABLE_DEFAULT_ERROR},
> + fs::{File, LocalFile},
> + ioctl::IoctlCommand,
> + prelude::*,
> + types::{ForeignOwnable, Opaque},
> + uaccess::{UserPtr, UserSlice, UserSliceReader, UserSliceWriter},
> +};
> +use core::{ffi, mem, ops::Deref};
> +
> +/// Character device ID.
> +///
> +/// This is a wrapper around the kernel's `dev_t` type and identifies a
> +/// character device by its major and minor numbers.
> +#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
> +#[repr(transparent)]
> +pub struct CharDeviceID(bindings::dev_t); // u32
> +
> +impl CharDeviceID {
> + /// Creates a new device ID from the given major and minor numbers.
> + ///
> + /// This corresponds to the kernel's `MKDEV` macro.
> + pub fn new(major: u32, minor: u32) -> Self {
> + // SAFETY: The kernel's `MKDEV` macro is safe to call with any values.
> + Self(unsafe { bindings::MKDEV(major, minor) })
> + }
> +
> + /// Returns the major number of the device ID.
> + ///
> + /// This corresponds to the kernel's `MAJOR` macro.
> + pub fn major(&self) -> u32 {
> + // SAFETY: The kernel's `MAJOR` macro is safe to call with any value.
> + unsafe { bindings::MAJOR(self.0) }
> + }
> +
> + /// Returns the minor number of the device ID.
> + ///
> + /// This corresponds to the kernel's `MINOR` macro.
> + pub fn minor(&self) -> u32 {
> + // SAFETY: The kernel's `MINOR` macro is safe to call with any value.
> + unsafe { bindings::MINOR(self.0) }
> + }
> +}
> +
> +/// Seek mode for the `llseek` method.
> +///
> +/// This enum corresponds to the `SEEK_*` constants in the kernel.
> +#[repr(u32)]
> +pub enum Whence {
> + /// Set the file position to `offset`.
> + Set = bindings::SEEK_SET,
> + /// Set the file position to the current position plus `offset`.
> + Cur = bindings::SEEK_CUR,
> + /// Set the file position to the end of the file plus `offset`.
> + End = bindings::SEEK_END,
> + /// Set the file position to the next location in the file greater than or
> + /// equal to `offset` containing data.
> + Data = bindings::SEEK_DATA,
> + /// Set the file position to the next hole in the file greater than or
> + /// equal to `offset`.
> + Hole = bindings::SEEK_HOLE,
> +}
> +
> +// Make sure at compile time that the `Whence` enum can be safely converted
> +// from integers up to `SEEK_MAX`.
> +const _: () = assert!(Whence::Hole as u32 == bindings::SEEK_MAX);
> +
> +/// Trait implemented by a registered character device.
> +///
> +/// A registered character device just handles the `open` operation on the
> +/// device file and returns an open device type (which implements the
> +/// [`OpenCharDevice`] trait) that handles the actual I/O operations on the
> +/// device file. Optionally, the `release` operation can be implemented to
> +/// handle the final close of the device file, but simple cleanup can also be
> +/// done in the `Drop` implementation of the open device type.
release is traditionally where you handle cleaning up what was allocated
for this "open", and then drop can handle any "global" state for the
device associated with this specific instance. So "simple cleanup"
might not be possible in both places, as they are different parts of the
lifecycle of a device.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] rust: char_dev: add character device abstraction
2024-10-12 7:36 ` Greg Kroah-Hartman
@ 2024-10-12 21:48 ` Josef Zoller
0 siblings, 0 replies; 11+ messages in thread
From: Josef Zoller @ 2024-10-12 21:48 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, linux-kernel, rust-for-linux
On 12.10.24 09:36, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2024 at 08:55:42PM +0200, Josef Zoller wrote:
>> +unsigned int rust_helper_MAJOR(dev_t dev)
>> +{
>> + return MAJOR(dev);
>> +}
>
> I don't think you use this function in the code anywhere.
Yes, I do. I use it at rust/kernel/char_dev.rs:41.
>> +unsigned int rust_helper_MINOR(dev_t dev)
>> +{
>> + return MINOR(dev);
>> +}
>
> Is this really needed? No driver should care about their minor number,
> except to possibly set it, not read it.
It's not really needed, but I use it in my scull example to just print
the major and minor numbers of the devices, which makes it easy to
quickly create a device file for testing using mknod.
>> +dev_t rust_helper_MKDEV(unsigned int major, unsigned int minor)
>> +{
>> + return MKDEV(major, minor);
>> +}
>
> If you are only doing dynamic creation, as your initial text said, I
> don't think you need this either as the kernel should create it for you.
Yeah, you're right. I forgot to remove this one after I changed to only
allowing dynamic device number creation.
>> diff --git a/rust/kernel/char_dev.rs b/rust/kernel/char_dev.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..b81c0d55ab60f18dc82a99991318a5ae0a26e560
>> --- /dev/null
>> +++ b/rust/kernel/char_dev.rs
>> @@ -0,0 +1,976 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> Minor nit, you forgot a copyright line :)
Oh, I just looked at some other rust files in rust/kernel and saw that
most of them don't have a copyright line. I'll add one to this file.
>> +
>> +//! Character device support.
>> +//!
>> +//! C headers: [`include/linux/cdev.h`](srctree/include/linux/cdev.h) and
>> +//! [`include/linux/fs.h`](srctree/include/linux/fs.h)
>> +
>> +use crate::{
>> + bindings, container_of,
>> + error::{to_result, VTABLE_DEFAULT_ERROR},
>> + fs::{File, LocalFile},
>> + ioctl::IoctlCommand,
>> + prelude::*,
>> + types::{ForeignOwnable, Opaque},
>> + uaccess::{UserPtr, UserSlice, UserSliceReader, UserSliceWriter},
>> +};
>> +use core::{ffi, mem, ops::Deref};
>> +
>> +/// Character device ID.
>> +///
>> +/// This is a wrapper around the kernel's `dev_t` type and identifies a
>> +/// character device by its major and minor numbers.
>> +#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
>> +#[repr(transparent)]
>> +pub struct CharDeviceID(bindings::dev_t); // u32
>> +
>> +impl CharDeviceID {
>> + /// Creates a new device ID from the given major and minor numbers.
>> + ///
>> + /// This corresponds to the kernel's `MKDEV` macro.
>> + pub fn new(major: u32, minor: u32) -> Self {
>> + // SAFETY: The kernel's `MKDEV` macro is safe to call with any values.
>> + Self(unsafe { bindings::MKDEV(major, minor) })
>> + }
>> +
>> + /// Returns the major number of the device ID.
>> + ///
>> + /// This corresponds to the kernel's `MAJOR` macro.
>> + pub fn major(&self) -> u32 {
>> + // SAFETY: The kernel's `MAJOR` macro is safe to call with any value.
>> + unsafe { bindings::MAJOR(self.0) }
>> + }
>> +
>> + /// Returns the minor number of the device ID.
>> + ///
>> + /// This corresponds to the kernel's `MINOR` macro.
>> + pub fn minor(&self) -> u32 {
>> + // SAFETY: The kernel's `MINOR` macro is safe to call with any value.
>> + unsafe { bindings::MINOR(self.0) }
>> + }
>> +}
>> +
>> +/// Seek mode for the `llseek` method.
>> +///
>> +/// This enum corresponds to the `SEEK_*` constants in the kernel.
>> +#[repr(u32)]
>> +pub enum Whence {
>> + /// Set the file position to `offset`.
>> + Set = bindings::SEEK_SET,
>> + /// Set the file position to the current position plus `offset`.
>> + Cur = bindings::SEEK_CUR,
>> + /// Set the file position to the end of the file plus `offset`.
>> + End = bindings::SEEK_END,
>> + /// Set the file position to the next location in the file greater than or
>> + /// equal to `offset` containing data.
>> + Data = bindings::SEEK_DATA,
>> + /// Set the file position to the next hole in the file greater than or
>> + /// equal to `offset`.
>> + Hole = bindings::SEEK_HOLE,
>> +}
>> +
>> +// Make sure at compile time that the `Whence` enum can be safely converted
>> +// from integers up to `SEEK_MAX`.
>> +const _: () = assert!(Whence::Hole as u32 == bindings::SEEK_MAX);
>> +
>> +/// Trait implemented by a registered character device.
>> +///
>> +/// A registered character device just handles the `open` operation on the
>> +/// device file and returns an open device type (which implements the
>> +/// [`OpenCharDevice`] trait) that handles the actual I/O operations on the
>> +/// device file. Optionally, the `release` operation can be implemented to
>> +/// handle the final close of the device file, but simple cleanup can also be
>> +/// done in the `Drop` implementation of the open device type.
>
> release is traditionally where you handle cleaning up what was allocated
> for this "open", and then drop can handle any "global" state for the
> device associated with this specific instance. So "simple cleanup"
> might not be possible in both places, as they are different parts of the
> lifecycle of a device.
I guess, I haven't explained this clearly enough in the doc comment.
We have two different types here that correspond to two different parts
of the lifecycle of a device file:
We have a type implementing the `CharDevice` trait. An instance of this
type is created on device registration and lives until the device is
deregistered. And we have a type implementing the `OpenCharDevice`
trait. An instance of this type is created by the CharDevice type in the
open implementation and lives until the corresponding release is called.
That means that the OpenCharDevice type instance is dropped in
f_ops->release, even when the CharDevice type doesn't implement release,
so simple cleanup can happen in the drop implementation of the type
implementing the OpenCharDevice trait and will correctly be called on
release.
Cheers,
Josef
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] rust: char_dev: add character device abstraction
2024-10-11 18:55 ` [PATCH 1/3] rust: char_dev: add character device abstraction Josef Zoller
2024-10-12 7:36 ` Greg Kroah-Hartman
@ 2024-10-17 11:38 ` kernel test robot
1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2024-10-17 11:38 UTC (permalink / raw)
To: Josef Zoller, Arnd Bergmann, Greg Kroah-Hartman, Miguel Ojeda,
Alex Gaynor
Cc: llvm, oe-kbuild-all, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linux-kernel, rust-for-linux, Josef Zoller
Hi Josef,
kernel test robot noticed the following build errors:
[auto build test ERROR on ce1c54fdff7c4556b08f5b875a331d8952e8b6b7]
url: https://github.com/intel-lab-lkp/linux/commits/Josef-Zoller/rust-char_dev-add-character-device-abstraction/20241012-030853
base: ce1c54fdff7c4556b08f5b875a331d8952e8b6b7
patch link: https://lore.kernel.org/r/20241011-rust-char-dev-v1-1-350225ae228b%40walterzollerpiano.com
patch subject: [PATCH 1/3] rust: char_dev: add character device abstraction
config: x86_64-randconfig-016-20241017 (https://download.01.org/0day-ci/archive/20241017/202410171902.1Oyg06At-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410171902.1Oyg06At-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410171902.1Oyg06At-lkp@intel.com/
All errors (new ones prefixed by >>):
>> error[E0658]: associated type bounds are unstable
--> rust/kernel/char_dev.rs:140:42
|
140 | type OpenPtr: for<'a> ForeignOwnable<Borrowed<'a>: Deref<Target: OpenCharDevice>>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information
= help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable
= note: this compiler was built on 2024-04-29; consider upgrading it if it is out of date
--
>> error[E0658]: associated type bounds are unstable
--> rust/kernel/char_dev.rs:140:62
|
140 | type OpenPtr: for<'a> ForeignOwnable<Borrowed<'a>: Deref<Target: OpenCharDevice>>;
| ^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information
= help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable
= note: this compiler was built on 2024-04-29; consider upgrading it if it is out of date
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] rust: macros: add IoctlCommand derive macro
2024-10-11 18:55 [PATCH 0/3] Character device abstractions for Rust Josef Zoller
2024-10-11 18:55 ` [PATCH 1/3] rust: char_dev: add character device abstraction Josef Zoller
@ 2024-10-11 18:55 ` Josef Zoller
2024-10-11 18:55 ` [PATCH 3/3] samples: rust: add character device sample Josef Zoller
2024-10-12 7:29 ` [PATCH 0/3] Character device abstractions for Rust Greg Kroah-Hartman
3 siblings, 0 replies; 11+ messages in thread
From: Josef Zoller @ 2024-10-11 18:55 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
rust-for-linux, Josef Zoller
Provide a macro that derives the `IoctlCommand` trait for simple enums
by converting every variant into a unique command.
The macro can be instructed to use a specific letter or integer as the
code. Each variant is then assigned a consecutive number starting from
0 or a given value. The type of the command, i.e. if it is a read or
write command, is inferred from the variant's associated data: if it
has no data or only an integer, it is neither read nor write, if it has
a UserSliceReader it is a write command, if it has a UserSliceWriter it
is a read command, and if it just has a UserSlice it is a read-write
command. The code and the variant's number and type are then combined
to parse the command from the user-provided cmd and arg values.
Signed-off-by: Josef Zoller <josef@walterzollerpiano.com>
---
rust/kernel/ioctl.rs | 190 ++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/prelude.rs | 2 +-
rust/macros/ioctl_cmd.rs | 202 +++++++++++++++++++++++++++++++++++++++++++++++
rust/macros/lib.rs | 21 +++++
4 files changed, 414 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
index 03359ab28495b94d98d53db2115bbbcc520c18a3..f6af9c10c0b244b8d8183cf70b4ef5ce9233c935 100644
--- a/rust/kernel/ioctl.rs
+++ b/rust/kernel/ioctl.rs
@@ -73,6 +73,22 @@ pub const fn _IOC_SIZE(nr: u32) -> usize {
}
/// Types implementing this trait can be used to parse ioctl commands.
+///
+/// Normally, this trait is derived for a command enum.
+///
+/// # Example
+///
+/// ```
+/// #[derive(IoctlCommand)]
+/// #[ioctl(code = 0x18, start_num = 0)]
+/// enum Command {
+/// NoReadWrite, // No read or write access.
+/// NoReadWriteButTakesArg(u64), // No read or write access, but takes an argument.
+/// ReadOnly(UserSliceWriter), // We write data for the user to read.
+/// WriteOnly(UserSliceReader), // We read data that the user wrote.
+/// WriteAndRead(UserSlice), // We read data from the user and then write data to the user.
+/// }
+/// ```
#[vtable]
pub trait IoctlCommand: Sized + Send + Sync + 'static {
/// The error type returned by the parse functions.
@@ -114,3 +130,177 @@ fn parse(_cmd: ffi::c_uint, _arg: ffi::c_ulong) -> Result<Self> {
Err(ENOTTY)
}
}
+
+/// Support macro for deriving the `IoctlCommand` trait.
+#[doc(hidden)]
+#[macro_export]
+macro_rules! __derive_ioctl_cmd {
+ (parse_input:
+ @enum_name($enum_name:ident),
+ @code($code:literal),
+ @variants(
+ $(
+ @variant($i:literal, $variant:ident, $arg_type:tt),
+ )*
+ )
+ ) => {
+ #[automatically_derived]
+ impl $crate::ioctl::IoctlCommand for $enum_name {
+ type Err = $crate::error::Error;
+
+ const USE_VTABLE_ATTR: () = ();
+
+ const HAS_PARSE: bool = true;
+
+ fn parse(
+ cmd: ::core::ffi::c_uint,
+ arg: ::core::ffi::c_ulong,
+ ) -> ::core::result::Result<Self, Self::Err> {
+ let ty = $crate::ioctl::_IOC_TYPE(cmd) as u8;
+
+ if ty != $code {
+ return Err($crate::error::code::ENOTTY);
+ }
+
+ let nr = $crate::ioctl::_IOC_NR(cmd) as u8;
+ let dir = $crate::ioctl::_IOC_DIR(cmd);
+ let size = $crate::ioctl::_IOC_SIZE(cmd);
+
+ // Make sure we don't get unused parameter warnings
+ let _ = arg;
+
+ match (nr, dir, size) {
+ $(
+ ::kernel::__derive_ioctl_cmd!(
+ match_pattern:
+ @variant($i, $arg_type)
+ ) => ::kernel::__derive_ioctl_cmd!(
+ match_body:
+ @dir(dir),
+ @size(size),
+ @arg(arg),
+ @variant($variant, $arg_type)
+ ),
+ )*
+ _ => Err($crate::error::code::ENOTTY),
+ }
+ }
+ }
+ };
+ (match_pattern:
+ @variant($i:literal, None)
+ ) => {
+ ($i, $crate::uapi::_IOC_NONE, 0)
+ };
+ (match_body:
+ @dir($dir:ident),
+ @size($size:ident),
+ @arg($arg:ident),
+ @variant($variant:ident, None)
+ ) => {
+ Ok(Self::$variant)
+ };
+ (match_pattern:
+ @variant($i:literal, u64)
+ ) => {
+ ($i, $crate::uapi::_IOC_NONE, 0)
+ };
+ (match_body:
+ @dir($dir:ident),
+ @size($size:ident),
+ @arg($arg:ident),
+ @variant($variant:ident, u64)
+ ) => {
+ Ok(Self::$variant($arg))
+ };
+ (match_pattern:
+ @variant($i:literal, UserSliceWriter)
+ ) => {
+ ($i, $crate::uapi::_IOC_READ, _)
+ };
+ (match_body:
+ @dir($dir:ident),
+ @size($size:ident),
+ @arg($arg:ident),
+ @variant($variant:ident, UserSliceWriter)
+ ) => {
+ {
+ let user_writer = $crate::uaccess::UserSlice::new(
+ $arg as $crate::uaccess::UserPtr,
+ $size
+ )
+ .writer();
+
+ Ok(Self::$variant(user_writer))
+ }
+ };
+ (match_pattern:
+ @variant($i:literal, UserSliceReader)
+ ) => {
+ ($i, $crate::uapi::_IOC_WRITE, _)
+ };
+ (match_body:
+ @dir($dir:ident),
+ @size($size:ident),
+ @arg($arg:ident),
+ @variant($variant:ident, UserSliceReader)
+ ) => {
+ {
+ let user_reader = $crate::uaccess::UserSlice::new(
+ $arg as $crate::uaccess::UserPtr,
+ $size
+ )
+ .reader();
+
+ Ok(Self::$variant(user_reader))
+ }
+ };
+ (match_pattern:
+ @variant($i:literal, UserSlice)
+ ) => {
+ ($i, _, _)
+ };
+ (match_body:
+ @dir($dir:ident),
+ @size($size:ident),
+ @arg($arg:ident),
+ @variant($variant:ident, UserSlice)
+ ) => {
+ // Unfortunately, we cannot just do a match guard
+ if $dir != $crate::uapi::_IOC_READ | $crate::uapi::_IOC_WRITE {
+ Err($crate::error::code::ENOTTY)
+ } else {
+ let user_slice = $crate::uaccess::UserSlice::new(
+ $arg as $crate::uaccess::UserPtr,
+ $size
+ );
+
+ Ok(Self::$variant(user_slice))
+ }
+ };
+ (match_pattern:
+ @variant($i:literal, $arg_type:tt)
+ ) => {
+ ($i, _, _)
+ };
+ (match_body:
+ @dir($dir:ident),
+ @size($size:ident),
+ @arg($arg:ident),
+ @variant($variant:ident, $arg_type:tt)
+ ) => {
+ {
+ // We have an unsupported argument type
+ const _: () = ::core::assert!(
+ false,
+ ::core::concat!(
+ "Invalid argument type for ioctl command ",
+ stringify!($variant),
+ ": ",
+ stringify!($arg_type),
+ )
+ );
+ ::core::unreachable!()
+ }
+ };
+}
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index 4571daec0961bb34fb6956a4e9eda8445954b719..1277d1ec5a476d3e115f6b2ba432b0fbe28941a2 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -20,7 +20,7 @@
pub use alloc::{boxed::Box, vec::Vec};
#[doc(no_inline)]
-pub use macros::{module, pin_data, pinned_drop, vtable, Zeroable};
+pub use macros::{module, pin_data, pinned_drop, vtable, IoctlCommand, Zeroable};
pub use super::build_assert;
diff --git a/rust/macros/ioctl_cmd.rs b/rust/macros/ioctl_cmd.rs
new file mode 100644
index 0000000000000000000000000000000000000000..366a9b1f7ba70ba764b0d78cb32d82125bc7b854
--- /dev/null
+++ b/rust/macros/ioctl_cmd.rs
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use proc_macro::{token_stream, Delimiter, Literal, TokenStream, TokenTree};
+
+fn expect_punct(input: &mut impl Iterator<Item = TokenTree>, expected: char, reason: &str) {
+ let Some(TokenTree::Punct(punct)) = input.next() else {
+ panic!("expected '{expected}' {reason}");
+ };
+
+ if punct.as_char() != expected {
+ panic!("expected '{expected}' {reason}");
+ }
+}
+
+fn expect_ident(input: &mut impl Iterator<Item = TokenTree>, expected: &str, reason: &str) {
+ let Some(TokenTree::Ident(ident)) = input.next() else {
+ panic!("expected '{expected}' {reason}");
+ };
+
+ if ident.to_string() != expected {
+ panic!("expected '{expected}' {reason}");
+ }
+}
+
+fn expect_group(
+ input: &mut impl Iterator<Item = TokenTree>,
+ expected: Delimiter,
+ reason: &str,
+) -> token_stream::IntoIter {
+ let Some(TokenTree::Group(group)) = input.next() else {
+ panic!("expected group {reason}");
+ };
+
+ if group.delimiter() != expected {
+ panic!("expected group {reason}");
+ }
+
+ group.stream().into_iter()
+}
+
+fn parse_attribute(input: &mut impl Iterator<Item = TokenTree>) -> (u8, u8) {
+ expect_punct(input, '#', "to start attribute");
+
+ let mut stream = expect_group(input, Delimiter::Bracket, "as attribute body");
+
+ expect_ident(&mut stream, "ioctl", "as attribute name");
+
+ let mut inner_stream = expect_group(
+ &mut stream,
+ Delimiter::Parenthesis,
+ "as attribute arguments",
+ );
+
+ expect_ident(&mut inner_stream, "code", "as ioctl attribute field");
+ expect_punct(&mut inner_stream, '=', "in ioctl attribute field");
+
+ let Some(TokenTree::Literal(lit)) = inner_stream.next() else {
+ panic!("expected ioctl attribute code value");
+ };
+
+ let lit_str = lit.to_string();
+ let code = if lit_str.starts_with("b'") {
+ lit_str
+ .chars()
+ .nth(2)
+ .expect("expected ioctl attribute code value") as u8
+ } else if let Some(hex) = lit_str.strip_prefix("0x") {
+ u8::from_str_radix(hex, 16).expect("expected ioctl attribute code value")
+ } else {
+ lit_str
+ .parse()
+ .expect("expected ioctl attribute code value")
+ };
+
+ let start_num = if let Some(tree) = inner_stream.next() {
+ if !matches!(tree, TokenTree::Punct(punct) if punct.as_char() == ',') {
+ panic!("expected ioctl attribute comma");
+ }
+
+ expect_ident(&mut inner_stream, "start_num", "as ioctl attribute field");
+ expect_punct(&mut inner_stream, '=', "in ioctl attribute field");
+
+ let Some(TokenTree::Literal(lit)) = inner_stream.next() else {
+ panic!("expected ioctl attribute start number value");
+ };
+
+ lit.to_string()
+ .parse()
+ .expect("expected ioctl attribute start number value")
+ } else {
+ 0
+ };
+
+ assert!(
+ inner_stream.next().is_none(),
+ "unexpected token in ioctl attribute"
+ );
+ assert!(
+ stream.next().is_none(),
+ "unexpected token in ioctl attribute"
+ );
+
+ (code, start_num)
+}
+
+fn parse_enum_def(input: &mut impl Iterator<Item = TokenTree>) -> TokenTree {
+ expect_ident(input, "enum", "to start enum definition");
+
+ let Some(ident @ TokenTree::Ident(_)) = input.next() else {
+ panic!("expected enum name");
+ };
+
+ ident
+}
+
+fn parse_enum_body(
+ input: &mut impl Iterator<Item = TokenTree>,
+) -> Vec<(TokenTree, Option<TokenTree>)> {
+ let mut stream = expect_group(input, Delimiter::Brace, "as enum body").peekable();
+
+ let mut variants = Vec::new();
+
+ while let Some(variant) = stream.next_if(|t| matches!(t, TokenTree::Ident(_))) {
+ let arg_type = if let Some(TokenTree::Group(group)) =
+ stream.next_if(|t| matches!(t, TokenTree::Group(_)))
+ {
+ if group.delimiter() != Delimiter::Parenthesis {
+ panic!("expected group");
+ }
+
+ let mut inner_stream = group.stream().into_iter();
+
+ let arg_type = if let Some(ident @ TokenTree::Ident(_)) = inner_stream.next() {
+ ident
+ } else {
+ panic!("expected argument type")
+ };
+
+ assert!(
+ inner_stream.next().is_none(),
+ "unexpected token in enum variant"
+ );
+
+ Some(arg_type)
+ } else {
+ None
+ };
+
+ variants.push((variant, arg_type));
+
+ if stream
+ .next_if(|t| matches!(t, TokenTree::Punct(punct) if punct.as_char() == ','))
+ .is_none()
+ {
+ break;
+ }
+ }
+
+ assert!(stream.next().is_none(), "unexpected token in enum body");
+
+ variants
+}
+
+pub(crate) fn derive(input: TokenStream) -> TokenStream {
+ let mut input = input.into_iter();
+
+ let (code, start_num) = parse_attribute(&mut input);
+ let enum_name = parse_enum_def(&mut input);
+ let variants = parse_enum_body(&mut input);
+
+ assert!(input.next().is_none(), "unexpected token in ioctl_cmd");
+
+ let code = TokenTree::from(Literal::u8_suffixed(code));
+
+ let variants = variants
+ .into_iter()
+ .enumerate()
+ .map(|(i, (variant, arg_type))| {
+ let i = i as u8 + start_num;
+
+ let i = TokenTree::from(Literal::u8_suffixed(i));
+
+ if let Some(arg_type) = arg_type {
+ quote! {
+ @variant(#i, #variant, #arg_type),
+ }
+ } else {
+ quote! {
+ @variant(#i, #variant, None),
+ }
+ }
+ });
+
+ quote! {
+ ::kernel::__derive_ioctl_cmd!(
+ parse_input:
+ @enum_name(#enum_name),
+ @code(#code),
+ @variants(#(#variants)*)
+ );
+ }
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index a626b1145e5c4ff00692e9d4e11fdb93500db1a8..5a33ed69b5b0b64f6720fb54e18056af9b2f7a00 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -10,6 +10,7 @@
mod quote;
mod concat_idents;
mod helpers;
+mod ioctl_cmd;
mod module;
mod paste;
mod pin_data;
@@ -412,6 +413,26 @@ pub fn paste(input: TokenStream) -> TokenStream {
tokens.into_iter().collect()
}
+/// Derives the [`IoctlCommand`] trait for the given enum.
+///
+/// # Example
+///
+/// ```
+/// #[derive(IoctlCommand)]
+/// #[ioctl(code = 0x18, start_num = 0)]
+/// enum Command {
+/// NoReadWrite, // No read or write access.
+/// NoReadWriteButTakesArg(u64), // No read or write access, but takes an argument.
+/// ReadOnly(UserSliceWriter), // We write data for the user to read.
+/// WriteOnly(UserSliceReader), // We read data that the user wrote.
+/// WriteAndRead(UserSlice), // We read data from the user and then write data to the user.
+/// }
+/// ```
+#[proc_macro_derive(IoctlCommand, attributes(ioctl))]
+pub fn derive_ioctl_cmd(input: TokenStream) -> TokenStream {
+ ioctl_cmd::derive(input)
+}
+
/// Derives the [`Zeroable`] trait for the given struct.
///
/// This can only be used for structs where every field implements the [`Zeroable`] trait.
--
2.47.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/3] samples: rust: add character device sample
2024-10-11 18:55 [PATCH 0/3] Character device abstractions for Rust Josef Zoller
2024-10-11 18:55 ` [PATCH 1/3] rust: char_dev: add character device abstraction Josef Zoller
2024-10-11 18:55 ` [PATCH 2/3] rust: macros: add IoctlCommand derive macro Josef Zoller
@ 2024-10-11 18:55 ` Josef Zoller
2024-10-12 12:37 ` Greg Kroah-Hartman
2024-10-12 7:29 ` [PATCH 0/3] Character device abstractions for Rust Greg Kroah-Hartman
3 siblings, 1 reply; 11+ messages in thread
From: Josef Zoller @ 2024-10-11 18:55 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
rust-for-linux, Josef Zoller
Provide a sample implementation of a character device in Rust, following
the scull device from the well-known book "Linux Device Drivers".
This sample uses the abstractions from the previous patches in this
series to implement the scull device in mostly safe Rust. All of the
unsafe code comes from the fact that the data structure used in the C
implementation is inherently unsafe and cannot easily be represented in
safe Rust without a lot of memory overhead.
This sample should be a good starting point for people who want to start
writing kernel code in Rust, as a character device is relatively simple
and does not require a lot of kernel knowledge to understand.
Signed-off-by: Josef Zoller <josef@walterzollerpiano.com>
---
samples/rust/Kconfig | 10 +
samples/rust/Makefile | 1 +
samples/rust/rust_char_dev.rs | 506 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 517 insertions(+)
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9ad24c9dc1ca057f83531156084aa..a85e0c2e84c07ee9c0b965eb83c07a90011e7d0d 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,16 @@ config SAMPLE_RUST_PRINT
If unsure, say N.
+config SAMPLE_RUST_CHAR_DEV
+ tristate "Character device"
+ help
+ This option builds the Rust character device module sample.
+
+ To compile this as a module, choose M here:
+ the module will be called rust_char_dev.
+
+ If unsure, say N.
+
config SAMPLE_RUST_HOSTPROGS
bool "Host programs"
help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 03086dabbea44f4aa87f4a67ac24b8ea4e5a8f2a..cea054d71a7121a2ad991bd51b0d72caafe1d86e 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,5 +2,6 @@
obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o
obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_CHAR_DEV) += rust_char_dev.o
subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs
diff --git a/samples/rust/rust_char_dev.rs b/samples/rust/rust_char_dev.rs
new file mode 100644
index 0000000000000000000000000000000000000000..8df83fb31bced0870b43416d9a8dbd1d4cd118d1
--- /dev/null
+++ b/samples/rust/rust_char_dev.rs
@@ -0,0 +1,506 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust character device sample.
+//!
+//! This sample demonstrates how to create a simple character device in Rust,
+//! by reimplementing the `scull` device from the Linux Device Drivers book.
+
+use core::{mem, ptr::NonNull};
+
+use kernel::{
+ c_str,
+ char_dev::{CharDevice, CharDeviceID, DeviceRegistration, OpenCharDevice, Whence},
+ fs::{file::flags, File, LocalFile},
+ new_mutex,
+ prelude::*,
+ sync::{Arc, Mutex},
+ uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
+};
+
+module! {
+ type: RustCharDevModule,
+ name: "rust_char_dev",
+ author: "Rust for Linux Contributors",
+ description: "Rust character device sample",
+ license: "GPL",
+}
+
+const DEVICE_NAME: &CStr = c_str!("rust_scull");
+const DEFAULT_QSET_SIZE: usize = 1000;
+const DEFAULT_QUANTUM_SIZE: usize = 4000;
+const NUM_DEVS: usize = 4;
+
+// This is probably too specific a function to be in the Rust standard library...
+trait OptionExt<T> {
+ fn get_or_try_insert_with<F, E>(&mut self, f: F) -> Result<&mut T, E>
+ where
+ F: FnOnce() -> Result<T, E>;
+}
+
+impl<T> OptionExt<T> for Option<T> {
+ fn get_or_try_insert_with<F, E>(&mut self, f: F) -> Result<&mut T, E>
+ where
+ F: FnOnce() -> Result<T, E>,
+ {
+ if self.is_none() {
+ *self = Some(f()?);
+ }
+
+ Ok(self.as_mut().unwrap())
+ }
+}
+
+#[derive(IoctlCommand)]
+#[ioctl(code = b'k')]
+enum Command {
+ Reset, // 0
+ SetQuantum(UserSliceReader), // 1
+ SetQset(UserSliceReader), // 2
+ TellQuantum(u64), // 3
+ TellQset(u64), // 4
+ GetQuantum(UserSliceWriter), // 5
+ GetQset(UserSliceWriter), // 6
+ QueryQuantum, // 7
+ QueryQset, // 8
+ ExchangeQuantum(UserSlice), // 9
+ ExchangeQset(UserSlice), // 10
+ ShiftQuantum(u64), // 11
+ ShiftQset(u64), // 12
+}
+
+// We implement `ScullQset` as close as possible to the `struct scull_qset` implementation from
+// the book. This means that we have to use raw pointers and some unsafe code for the data.
+// Otherwise, we'd have massive memory overhead by storing sizes/capacities unnecessarily.
+// E.g. every `ScullQset` would be 8 * qset_size bytes larger if we used `Box<[_]>` and
+// 16 * qset_size bytes larger if we used `Vec<_>`.
+// However, the knowledge that all data arrays are of the same size isn't possible to express
+// in safe Rust without this memory overhead.
+struct ScullQset {
+ data: Option<NonNull<Option<NonNull<u8>>>>,
+ quantum_size: usize,
+ qset_size: usize,
+ next: Option<Box<ScullQset>>,
+}
+
+impl ScullQset {
+ fn new(quantum_size: usize, qset_size: usize) -> Self {
+ Self {
+ data: None,
+ quantum_size,
+ qset_size,
+ next: None,
+ }
+ }
+
+ /// Returns a reference to the quantum at index `i` if it exists.
+ fn get_quantum(&self, i: usize) -> Option<&[u8]> {
+ let data_slice = NonNull::slice_from_raw_parts(self.data?, self.qset_size);
+ // SAFETY: `data_slice` points to a valid slice of `Option<NonNull<u8>>`.
+ let quantum = unsafe { data_slice.as_ref()[i] };
+ let quantum_slice = NonNull::slice_from_raw_parts(quantum?, self.quantum_size);
+ // SAFETY: `quantum_slice` points to a valid slice of `u8`.
+ Some(unsafe { quantum_slice.as_ref() })
+ }
+
+ /// Returns a mutable reference to the quantum at index `i`, allocating it first if necessary.
+ fn get_quantum_mut(&mut self, i: usize) -> Option<&mut [u8]> {
+ let data = self
+ .data
+ .get_or_try_insert_with(|| {
+ let mut data =
+ mem::ManuallyDrop::new(Vec::with_capacity(self.qset_size, GFP_KERNEL)?);
+ for _ in 0..self.qset_size {
+ data.push(None, GFP_KERNEL)?;
+ }
+
+ assert!(data.len() == data.capacity());
+
+ // SAFETY: `data.as_mut_ptr()` is non-null.
+ Ok::<_, Error>(unsafe { NonNull::new_unchecked(data.as_mut_ptr()) })
+ })
+ .ok()?;
+
+ let mut data_slice = NonNull::slice_from_raw_parts(*data, self.qset_size);
+
+ // SAFETY: `data_slice` points to a valid slice of `Option<NonNull<u8>>`.
+ let maybe_quantum = unsafe { &mut data_slice.as_mut()[i] };
+ let quantum = maybe_quantum
+ .get_or_try_insert_with(|| {
+ let mut quantum =
+ mem::ManuallyDrop::new(Vec::with_capacity(self.quantum_size, GFP_KERNEL)?);
+ for _ in 0..self.quantum_size {
+ quantum.push(0, GFP_KERNEL)?;
+ }
+
+ assert!(quantum.len() == quantum.capacity());
+
+ // SAFETY: `quantum.as_mut_ptr()` is non-null.
+ Ok::<_, Error>(unsafe { NonNull::new_unchecked(quantum.as_mut_ptr()) })
+ })
+ .ok()?;
+
+ let mut quantum_slice = NonNull::slice_from_raw_parts(*quantum, self.quantum_size);
+ // SAFETY: `quantum_slice` points to a valid slice of `u8`.
+ Some(unsafe { quantum_slice.as_mut() })
+ }
+}
+
+impl Drop for ScullQset {
+ fn drop(&mut self) {
+ if let Some(data) = self.data.take() {
+ // SAFETY: `data` was created by `Vec::with_capacity` with a capacity of `qset_size`.
+ let data_vec =
+ unsafe { Vec::from_raw_parts(data.as_ptr(), self.qset_size, self.qset_size) };
+
+ for quantum in data_vec {
+ let Some(quantum) = quantum else { continue };
+
+ // SAFETY: `quantum` was created by `Vec::with_capacity` with a capacity of
+ // `quantum_size`.
+ let _ = unsafe {
+ Vec::from_raw_parts(quantum.as_ptr(), self.quantum_size, self.quantum_size)
+ };
+ }
+ }
+ }
+}
+
+// SAFETY: The raw pointers are uniquely owned by `ScullQset` and not shared, so it's safe to send
+// it to another thread.
+unsafe impl Send for ScullQset {}
+
+struct ScullDevInner {
+ data: Option<Box<ScullQset>>,
+ quantum_size: usize,
+ qset_size: usize,
+ size: usize,
+}
+
+impl Default for ScullDevInner {
+ fn default() -> Self {
+ Self {
+ data: None,
+ quantum_size: DEFAULT_QUANTUM_SIZE,
+ qset_size: DEFAULT_QSET_SIZE,
+ size: 0,
+ }
+ }
+}
+
+impl ScullDevInner {
+ fn trim(&mut self) {
+ mem::take(&mut self.data);
+ self.size = 0;
+ }
+
+ fn follow(&mut self, n: usize) -> Option<&mut ScullQset> {
+ let mut qs = self
+ .data
+ .get_or_try_insert_with(|| {
+ Box::new(
+ ScullQset::new(self.quantum_size, self.qset_size),
+ GFP_KERNEL,
+ )
+ })
+ .ok()?;
+
+ for _ in 0..n {
+ qs = qs
+ .next
+ .get_or_try_insert_with(|| {
+ // We use `qs.quantum_size` and `qs.qset_size` here to avoid subtly
+ // different behavior from the original C implementation.
+ // If we used the sizes from `self`, we could end up with differently
+ // sized qsets in the linked list (which would not be a safety problem).
+ // Like this, we only use an updated size after `trim` has been called,
+ // which is the same behavior as in the book.
+ Box::new(ScullQset::new(qs.quantum_size, qs.qset_size), GFP_KERNEL)
+ })
+ .ok()?;
+ }
+
+ Some(qs)
+ }
+}
+
+#[derive(Clone)]
+struct ScullDev {
+ inner: Arc<Mutex<ScullDevInner>>,
+}
+
+#[vtable]
+impl CharDevice for ScullDev {
+ type OpenPtr = Box<Self>;
+ type Err = Error;
+
+ fn new(_dev_id: CharDeviceID) -> Result<Self> {
+ Ok(Self {
+ inner: Arc::pin_init(new_mutex!(ScullDevInner::default()), GFP_KERNEL)?,
+ })
+ }
+
+ fn open(&self, file: &File) -> Result<Self::OpenPtr> {
+ if file.flags() & flags::O_ACCMODE == flags::O_WRONLY {
+ // TODO: this should be lock_interruptible, but that's not in the Rust API yet
+ self.inner.lock().trim();
+ }
+
+ Ok(Box::new(self.clone(), GFP_KERNEL)?)
+ }
+}
+
+#[vtable]
+impl OpenCharDevice for ScullDev {
+ type IoctlCmd = Command;
+ type Err = Error;
+
+ fn read(&self, _file: &LocalFile, mut buf: UserSliceWriter, offset: &mut i64) -> Result<usize> {
+ let pos = usize::try_from(*offset).map_err(|_| EINVAL)?;
+
+ // TODO: this should be lock_interruptible, but that's not in the Rust API yet
+ let mut inner = self.inner.lock();
+
+ // To keep the behavior of the original C implementation, namely that the quantum and qset
+ // sizes are only updated after a trim, we use the sizes from the inner data if it exists.
+ let (quantum_size, qset_size) = inner
+ .data
+ .as_ref()
+ .map_or((inner.quantum_size, inner.qset_size), |qs| {
+ (qs.quantum_size, qs.qset_size)
+ });
+ let item_size = quantum_size * qset_size;
+
+ if pos >= inner.size {
+ return Ok(0);
+ }
+
+ let mut count = buf.len().min(inner.size - pos);
+ let item = pos / item_size;
+ let rest = pos % item_size;
+ let s_pos = rest / quantum_size;
+ let q_pos = rest % quantum_size;
+
+ let Some(q) = inner.follow(item).and_then(|qs| qs.get_quantum(s_pos)) else {
+ return Ok(0);
+ };
+
+ count = count.min(quantum_size - q_pos);
+
+ buf.write_slice(&q[q_pos..q_pos + count])?;
+
+ *offset += count as i64;
+
+ Ok(count)
+ }
+
+ fn write(
+ &self,
+ _file: &LocalFile,
+ mut buf: UserSliceReader,
+ offset: &mut i64,
+ ) -> Result<usize> {
+ let pos = usize::try_from(*offset).map_err(|_| EINVAL)?;
+
+ // TODO: this should be lock_interruptible, but that's not in the Rust API yet
+ let mut inner = self.inner.lock();
+
+ // To keep the behavior of the original C implementation, namely that the quantum and qset
+ // sizes are only updated after a trim, we use the sizes from the inner data if it exists.
+ let (quantum_size, qset_size) = inner
+ .data
+ .as_ref()
+ .map_or((inner.quantum_size, inner.qset_size), |qs| {
+ (qs.quantum_size, qs.qset_size)
+ });
+ let item_size = quantum_size * qset_size;
+
+ let item = pos / item_size;
+ let rest = pos % item_size;
+ let s_pos = rest / quantum_size;
+ let q_pos = rest % quantum_size;
+
+ let Some(q) = inner.follow(item).and_then(|qs| qs.get_quantum_mut(s_pos)) else {
+ return Err(ENOMEM);
+ };
+
+ let count = buf.len().min(quantum_size - q_pos);
+
+ buf.read_slice(&mut q[q_pos..q_pos + count])?;
+
+ let new_pos = pos + count;
+ *offset = new_pos as i64;
+
+ if new_pos > inner.size {
+ inner.size = new_pos;
+ }
+
+ Ok(count)
+ }
+
+ fn ioctl(
+ &self,
+ _file: &File,
+ cmd: Self::IoctlCmd,
+ #[cfg(CONFIG_COMPAT)] _compat: bool,
+ ) -> Result<u64> {
+ // The original implementation from the book actually doesn't consider the lock here at all,
+ // but Rust forces us to do so :)
+ let mut inner = self.inner.lock();
+
+ // We should definitely check if the user is trying to set a size to 0, or we'll
+ // end up with panics in the read/write functions due to division by zero.
+ // However, the original implementation doesn't account for this, so we won't either.
+ match cmd {
+ Command::Reset => {
+ inner.quantum_size = DEFAULT_QUANTUM_SIZE;
+ inner.qset_size = DEFAULT_QSET_SIZE;
+ }
+ Command::SetQuantum(mut reader) => {
+ // TODO: guard this command (and all others where a size can be set by the user)
+ // with `capability(CAP_SYS_ADMIN)`, which is not yet possible in the Rust API.
+ let quantum_size = reader.read()?;
+
+ if !reader.is_empty() {
+ return Err(EINVAL);
+ }
+
+ inner.quantum_size = quantum_size;
+ }
+ Command::TellQuantum(quantum) => {
+ inner.quantum_size = quantum as usize;
+ }
+ Command::GetQuantum(mut writer) => {
+ writer.write(&inner.quantum_size)?;
+
+ if !writer.is_empty() {
+ return Err(EINVAL);
+ }
+ }
+ Command::QueryQuantum => {
+ return Ok(inner.quantum_size as u64);
+ }
+ Command::ExchangeQuantum(slice) => {
+ let (mut reader, mut writer) = slice.reader_writer();
+ let quantum_size = reader.read()?;
+
+ if !reader.is_empty() {
+ return Err(EINVAL);
+ }
+
+ writer.write(&inner.quantum_size)?;
+
+ inner.quantum_size = quantum_size;
+ }
+ Command::ShiftQuantum(quantum) => {
+ let old_quantum = inner.quantum_size;
+ inner.quantum_size = quantum as usize;
+ return Ok(old_quantum as u64);
+ }
+ Command::SetQset(mut reader) => {
+ let qset_size = reader.read()?;
+
+ if !reader.is_empty() {
+ return Err(EINVAL);
+ }
+
+ inner.qset_size = qset_size;
+ }
+ Command::TellQset(qset) => {
+ inner.qset_size = qset as usize;
+ }
+ Command::GetQset(mut writer) => {
+ writer.write(&inner.qset_size)?;
+
+ if !writer.is_empty() {
+ return Err(EINVAL);
+ }
+ }
+ Command::QueryQset => {
+ return Ok(inner.qset_size as u64);
+ }
+ Command::ExchangeQset(slice) => {
+ let (mut reader, mut writer) = slice.reader_writer();
+ let qset_size = reader.read()?;
+
+ if !reader.is_empty() {
+ return Err(EINVAL);
+ }
+
+ writer.write(&inner.qset_size)?;
+
+ inner.qset_size = qset_size;
+ }
+ Command::ShiftQset(qset) => {
+ let old_qset = inner.qset_size;
+ inner.qset_size = qset as usize;
+ return Ok(old_qset as u64);
+ }
+ }
+
+ Ok(0)
+ }
+
+ fn llseek(
+ &self,
+ _file: &LocalFile,
+ pos: &mut i64,
+ offset: i64,
+ whence: Whence,
+ ) -> Result<u64, Self::Err> {
+ let size = self.inner.lock().size as i64;
+
+ let new_offset = match whence {
+ Whence::Set => offset,
+ Whence::Cur => *pos + offset,
+ Whence::End => size + offset,
+ _ => return Err(EINVAL),
+ };
+
+ if new_offset < 0 {
+ return Err(EINVAL);
+ }
+
+ *pos = new_offset;
+
+ Ok(new_offset as u64)
+ }
+}
+
+struct RustCharDevModule {
+ reg: Pin<Box<DeviceRegistration<ScullDev, NUM_DEVS>>>,
+}
+
+impl kernel::Module for RustCharDevModule {
+ fn init(module: &'static ThisModule) -> Result<Self> {
+ pr_info!("Rust character device sample (init)\n");
+
+ let reg = Box::pin_init(
+ DeviceRegistration::register(module, DEVICE_NAME),
+ GFP_KERNEL,
+ )?;
+
+ let base_dev_id = reg.get_base_dev_id();
+ pr_info!(
+ "Registered device {DEVICE_NAME} with major {} and minors {} through {}\n",
+ base_dev_id.major(),
+ base_dev_id.minor(),
+ base_dev_id.minor() + NUM_DEVS as u32 - 1
+ );
+
+ Ok(RustCharDevModule { reg })
+ }
+}
+
+impl Drop for RustCharDevModule {
+ fn drop(&mut self) {
+ let dev_id = self.reg.get_base_dev_id();
+ pr_info!(
+ "Device {DEVICE_NAME} had major {} and minors {} through {}\n",
+ dev_id.major(),
+ dev_id.minor(),
+ dev_id.minor() + NUM_DEVS as u32 - 1
+ );
+
+ pr_info!("Rust character device sample (exit)\n");
+ }
+}
--
2.47.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] samples: rust: add character device sample
2024-10-11 18:55 ` [PATCH 3/3] samples: rust: add character device sample Josef Zoller
@ 2024-10-12 12:37 ` Greg Kroah-Hartman
2024-10-12 23:38 ` Josef Zoller
0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2024-10-12 12:37 UTC (permalink / raw)
To: Josef Zoller
Cc: Arnd Bergmann, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, linux-kernel, rust-for-linux
On Fri, Oct 11, 2024 at 08:55:44PM +0200, Josef Zoller wrote:
> Provide a sample implementation of a character device in Rust, following
> the scull device from the well-known book "Linux Device Drivers".
>
> This sample uses the abstractions from the previous patches in this
> series to implement the scull device in mostly safe Rust. All of the
> unsafe code comes from the fact that the data structure used in the C
> implementation is inherently unsafe and cannot easily be represented in
> safe Rust without a lot of memory overhead.
>
> This sample should be a good starting point for people who want to start
> writing kernel code in Rust, as a character device is relatively simple
> and does not require a lot of kernel knowledge to understand.
>
> Signed-off-by: Josef Zoller <josef@walterzollerpiano.com>
> ---
> samples/rust/Kconfig | 10 +
> samples/rust/Makefile | 1 +
> samples/rust/rust_char_dev.rs | 506 ++++++++++++++++++++++++++++++++++++++++++
This feels like it could be a lot smaller, 500 lines for a sample char
driver seems excessive.
> --- /dev/null
> +++ b/samples/rust/rust_char_dev.rs
> @@ -0,0 +1,506 @@
> +// SPDX-License-Identifier: GPL-2.0
Again, copyright notice please.
> +
> +//! Rust character device sample.
> +//!
> +//! This sample demonstrates how to create a simple character device in Rust,
> +//! by reimplementing the `scull` device from the Linux Device Drivers book.
> +
> +use core::{mem, ptr::NonNull};
> +
> +use kernel::{
> + c_str,
> + char_dev::{CharDevice, CharDeviceID, DeviceRegistration, OpenCharDevice, Whence},
> + fs::{file::flags, File, LocalFile},
> + new_mutex,
> + prelude::*,
> + sync::{Arc, Mutex},
> + uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> +};
> +
> +module! {
> + type: RustCharDevModule,
> + name: "rust_char_dev",
> + author: "Rust for Linux Contributors",
> + description: "Rust character device sample",
> + license: "GPL",
> +}
> +
> +const DEVICE_NAME: &CStr = c_str!("rust_scull");
> +const DEFAULT_QSET_SIZE: usize = 1000;
> +const DEFAULT_QUANTUM_SIZE: usize = 4000;
> +const NUM_DEVS: usize = 4;
Comments for what these const are for?
> +// This is probably too specific a function to be in the Rust standard library...
> +trait OptionExt<T> {
> + fn get_or_try_insert_with<F, E>(&mut self, f: F) -> Result<&mut T, E>
> + where
> + F: FnOnce() -> Result<T, E>;
> +}
> +
> +impl<T> OptionExt<T> for Option<T> {
> + fn get_or_try_insert_with<F, E>(&mut self, f: F) -> Result<&mut T, E>
> + where
> + F: FnOnce() -> Result<T, E>,
> + {
> + if self.is_none() {
> + *self = Some(f()?);
> + }
> +
> + Ok(self.as_mut().unwrap())
> + }
> +}
What are these for? No comments makes it confusing to me :)
> +
> +#[derive(IoctlCommand)]
> +#[ioctl(code = b'k')]
> +enum Command {
> + Reset, // 0
> + SetQuantum(UserSliceReader), // 1
> + SetQset(UserSliceReader), // 2
> + TellQuantum(u64), // 3
> + TellQset(u64), // 4
> + GetQuantum(UserSliceWriter), // 5
> + GetQset(UserSliceWriter), // 6
> + QueryQuantum, // 7
> + QueryQset, // 8
> + ExchangeQuantum(UserSlice), // 9
> + ExchangeQset(UserSlice), // 10
> + ShiftQuantum(u64), // 11
> + ShiftQset(u64), // 12
> +}
Trying to keep ioctl numbers identical from kernel/userspace means they
should go in a .h file, putting them here is not going to work well.
> +// We implement `ScullQset` as close as possible to the `struct scull_qset` implementation from
> +// the book. This means that we have to use raw pointers and some unsafe code for the data.
That's not a good idea overall, why not look at the "untrusted data"
patch on the list and tie that into here so that things are not unsafe?
That is ideally what we want to do for ALL rust user/kernel interfaces.
> +// Otherwise, we'd have massive memory overhead by storing sizes/capacities unnecessarily.
> +// E.g. every `ScullQset` would be 8 * qset_size bytes larger if we used `Box<[_]>` and
> +// 16 * qset_size bytes larger if we used `Vec<_>`.
> +// However, the knowledge that all data arrays are of the same size isn't possible to express
> +// in safe Rust without this memory overhead.
> +struct ScullQset {
> + data: Option<NonNull<Option<NonNull<u8>>>>,
> + quantum_size: usize,
> + qset_size: usize,
> + next: Option<Box<ScullQset>>,
> +}
I'm sorry, I don't understand why this is so odd and has to be "unsafe".
It's just chunks of memory read from userspace. And we do have a list
structure somewhere (or something like that), that probably works better
than the next pointer from what I recall.
> +impl ScullQset {
> + fn new(quantum_size: usize, qset_size: usize) -> Self {
> + Self {
> + data: None,
> + quantum_size,
> + qset_size,
> + next: None,
> + }
> + }
> +
> + /// Returns a reference to the quantum at index `i` if it exists.
> + fn get_quantum(&self, i: usize) -> Option<&[u8]> {
> + let data_slice = NonNull::slice_from_raw_parts(self.data?, self.qset_size);
> + // SAFETY: `data_slice` points to a valid slice of `Option<NonNull<u8>>`.
> + let quantum = unsafe { data_slice.as_ref()[i] };
> + let quantum_slice = NonNull::slice_from_raw_parts(quantum?, self.quantum_size);
> + // SAFETY: `quantum_slice` points to a valid slice of `u8`.
> + Some(unsafe { quantum_slice.as_ref() })
> + }
> +
> + /// Returns a mutable reference to the quantum at index `i`, allocating it first if necessary.
> + fn get_quantum_mut(&mut self, i: usize) -> Option<&mut [u8]> {
> + let data = self
> + .data
> + .get_or_try_insert_with(|| {
> + let mut data =
> + mem::ManuallyDrop::new(Vec::with_capacity(self.qset_size, GFP_KERNEL)?);
> + for _ in 0..self.qset_size {
> + data.push(None, GFP_KERNEL)?;
> + }
> +
> + assert!(data.len() == data.capacity());
> +
> + // SAFETY: `data.as_mut_ptr()` is non-null.
> + Ok::<_, Error>(unsafe { NonNull::new_unchecked(data.as_mut_ptr()) })
> + })
> + .ok()?;
> +
> + let mut data_slice = NonNull::slice_from_raw_parts(*data, self.qset_size);
> +
> + // SAFETY: `data_slice` points to a valid slice of `Option<NonNull<u8>>`.
> + let maybe_quantum = unsafe { &mut data_slice.as_mut()[i] };
> + let quantum = maybe_quantum
> + .get_or_try_insert_with(|| {
> + let mut quantum =
> + mem::ManuallyDrop::new(Vec::with_capacity(self.quantum_size, GFP_KERNEL)?);
> + for _ in 0..self.quantum_size {
> + quantum.push(0, GFP_KERNEL)?;
> + }
> +
> + assert!(quantum.len() == quantum.capacity());
> +
> + // SAFETY: `quantum.as_mut_ptr()` is non-null.
> + Ok::<_, Error>(unsafe { NonNull::new_unchecked(quantum.as_mut_ptr()) })
> + })
> + .ok()?;
> +
> + let mut quantum_slice = NonNull::slice_from_raw_parts(*quantum, self.quantum_size);
> + // SAFETY: `quantum_slice` points to a valid slice of `u8`.
> + Some(unsafe { quantum_slice.as_mut() })
> + }
> +}
> +
> +impl Drop for ScullQset {
> + fn drop(&mut self) {
> + if let Some(data) = self.data.take() {
> + // SAFETY: `data` was created by `Vec::with_capacity` with a capacity of `qset_size`.
> + let data_vec =
> + unsafe { Vec::from_raw_parts(data.as_ptr(), self.qset_size, self.qset_size) };
> +
> + for quantum in data_vec {
> + let Some(quantum) = quantum else { continue };
> +
> + // SAFETY: `quantum` was created by `Vec::with_capacity` with a capacity of
> + // `quantum_size`.
> + let _ = unsafe {
> + Vec::from_raw_parts(quantum.as_ptr(), self.quantum_size, self.quantum_size)
> + };
> + }
> + }
> + }
> +}
> +
> +// SAFETY: The raw pointers are uniquely owned by `ScullQset` and not shared, so it's safe to send
> +// it to another thread.
> +unsafe impl Send for ScullQset {}
What other thread are you sending this to?
I don't see any "threads" here in the driver, do you mean "can be shared
by different userspace processes"?
> +struct ScullDevInner {
> + data: Option<Box<ScullQset>>,
> + quantum_size: usize,
> + qset_size: usize,
> + size: usize,
> +}
> +
> +impl Default for ScullDevInner {
> + fn default() -> Self {
> + Self {
> + data: None,
> + quantum_size: DEFAULT_QUANTUM_SIZE,
> + qset_size: DEFAULT_QSET_SIZE,
> + size: 0,
> + }
> + }
> +}
> +
> +impl ScullDevInner {
> + fn trim(&mut self) {
> + mem::take(&mut self.data);
> + self.size = 0;
> + }
> +
> + fn follow(&mut self, n: usize) -> Option<&mut ScullQset> {
> + let mut qs = self
> + .data
> + .get_or_try_insert_with(|| {
> + Box::new(
> + ScullQset::new(self.quantum_size, self.qset_size),
> + GFP_KERNEL,
> + )
> + })
> + .ok()?;
> +
> + for _ in 0..n {
> + qs = qs
> + .next
> + .get_or_try_insert_with(|| {
> + // We use `qs.quantum_size` and `qs.qset_size` here to avoid subtly
> + // different behavior from the original C implementation.
> + // If we used the sizes from `self`, we could end up with differently
> + // sized qsets in the linked list (which would not be a safety problem).
> + // Like this, we only use an updated size after `trim` has been called,
> + // which is the same behavior as in the book.
> + Box::new(ScullQset::new(qs.quantum_size, qs.qset_size), GFP_KERNEL)
As one of the authors of the book in reference here, I really don't
understand this at all :)
What am I missing? Why is this so complex?
You don't have to follow the implementation identically, just because
something is simple to do in C doesn't mean you have to do the same
thing in rust at all. We just implemented stuff to make the concepts
simple to follow, not to implement something specific that was trying to
solve a real problem here. So try to keep it simple, I think there are
other examples of rust char drivers in other trees that might be better
to use as an example, as this complexity probably isn't needed and will
just confuse new people (i.e. me!) more than is needed.
> + })
> + .ok()?;
> + }
> +
> + Some(qs)
> + }
> +}
> +
> +#[derive(Clone)]
> +struct ScullDev {
> + inner: Arc<Mutex<ScullDevInner>>,
> +}
> +
> +#[vtable]
> +impl CharDevice for ScullDev {
> + type OpenPtr = Box<Self>;
> + type Err = Error;
> +
> + fn new(_dev_id: CharDeviceID) -> Result<Self> {
> + Ok(Self {
> + inner: Arc::pin_init(new_mutex!(ScullDevInner::default()), GFP_KERNEL)?,
> + })
> + }
> +
> + fn open(&self, file: &File) -> Result<Self::OpenPtr> {
> + if file.flags() & flags::O_ACCMODE == flags::O_WRONLY {
> + // TODO: this should be lock_interruptible, but that's not in the Rust API yet
I think patches for that are floating around, right?
> + self.inner.lock().trim();
> + }
> +
> + Ok(Box::new(self.clone(), GFP_KERNEL)?)
> + }
> +}
> +
> +#[vtable]
> +impl OpenCharDevice for ScullDev {
> + type IoctlCmd = Command;
> + type Err = Error;
> +
> + fn read(&self, _file: &LocalFile, mut buf: UserSliceWriter, offset: &mut i64) -> Result<usize> {
> + let pos = usize::try_from(*offset).map_err(|_| EINVAL)?;
> +
> + // TODO: this should be lock_interruptible, but that's not in the Rust API yet
> + let mut inner = self.inner.lock();
> +
> + // To keep the behavior of the original C implementation, namely that the quantum and qset
> + // sizes are only updated after a trim, we use the sizes from the inner data if it exists.
> + let (quantum_size, qset_size) = inner
> + .data
> + .as_ref()
> + .map_or((inner.quantum_size, inner.qset_size), |qs| {
> + (qs.quantum_size, qs.qset_size)
> + });
> + let item_size = quantum_size * qset_size;
> +
> + if pos >= inner.size {
> + return Ok(0);
> + }
> +
> + let mut count = buf.len().min(inner.size - pos);
> + let item = pos / item_size;
> + let rest = pos % item_size;
> + let s_pos = rest / quantum_size;
> + let q_pos = rest % quantum_size;
> +
> + let Some(q) = inner.follow(item).and_then(|qs| qs.get_quantum(s_pos)) else {
> + return Ok(0);
> + };
> +
> + count = count.min(quantum_size - q_pos);
> +
> + buf.write_slice(&q[q_pos..q_pos + count])?;
> +
> + *offset += count as i64;
> +
> + Ok(count)
> + }
> +
> + fn write(
> + &self,
> + _file: &LocalFile,
> + mut buf: UserSliceReader,
> + offset: &mut i64,
> + ) -> Result<usize> {
> + let pos = usize::try_from(*offset).map_err(|_| EINVAL)?;
> +
> + // TODO: this should be lock_interruptible, but that's not in the Rust API yet
> + let mut inner = self.inner.lock();
> +
> + // To keep the behavior of the original C implementation, namely that the quantum and qset
> + // sizes are only updated after a trim, we use the sizes from the inner data if it exists.
> + let (quantum_size, qset_size) = inner
> + .data
> + .as_ref()
> + .map_or((inner.quantum_size, inner.qset_size), |qs| {
> + (qs.quantum_size, qs.qset_size)
> + });
> + let item_size = quantum_size * qset_size;
> +
> + let item = pos / item_size;
> + let rest = pos % item_size;
> + let s_pos = rest / quantum_size;
> + let q_pos = rest % quantum_size;
> +
> + let Some(q) = inner.follow(item).and_then(|qs| qs.get_quantum_mut(s_pos)) else {
> + return Err(ENOMEM);
> + };
> +
> + let count = buf.len().min(quantum_size - q_pos);
> +
> + buf.read_slice(&mut q[q_pos..q_pos + count])?;
> +
> + let new_pos = pos + count;
> + *offset = new_pos as i64;
> +
> + if new_pos > inner.size {
> + inner.size = new_pos;
> + }
> +
> + Ok(count)
> + }
> +
> + fn ioctl(
> + &self,
> + _file: &File,
> + cmd: Self::IoctlCmd,
> + #[cfg(CONFIG_COMPAT)] _compat: bool,
> + ) -> Result<u64> {
> + // The original implementation from the book actually doesn't consider the lock here at all,
> + // but Rust forces us to do so :)
> + let mut inner = self.inner.lock();
Does that mean the C implementation is wrong? It could be, but what
makes this so special that rust is requiring it and C doesn't?
> +
> + // We should definitely check if the user is trying to set a size to 0, or we'll
> + // end up with panics in the read/write functions due to division by zero.
> + // However, the original implementation doesn't account for this, so we won't either.
We should :)
And invalid userspace data is the #1 security bug in the kernel, let's
not copy bad examples for others to also make the same mistake wherever
possible please.
> + match cmd {
> + Command::Reset => {
> + inner.quantum_size = DEFAULT_QUANTUM_SIZE;
> + inner.qset_size = DEFAULT_QSET_SIZE;
> + }
> + Command::SetQuantum(mut reader) => {
> + // TODO: guard this command (and all others where a size can be set by the user)
> + // with `capability(CAP_SYS_ADMIN)`, which is not yet possible in the Rust API.
> + let quantum_size = reader.read()?;
> +
> + if !reader.is_empty() {
> + return Err(EINVAL);
> + }
> +
> + inner.quantum_size = quantum_size;
> + }
> + Command::TellQuantum(quantum) => {
> + inner.quantum_size = quantum as usize;
> + }
> + Command::GetQuantum(mut writer) => {
> + writer.write(&inner.quantum_size)?;
> +
> + if !writer.is_empty() {
> + return Err(EINVAL);
> + }
> + }
> + Command::QueryQuantum => {
> + return Ok(inner.quantum_size as u64);
> + }
> + Command::ExchangeQuantum(slice) => {
> + let (mut reader, mut writer) = slice.reader_writer();
> + let quantum_size = reader.read()?;
> +
> + if !reader.is_empty() {
> + return Err(EINVAL);
> + }
> +
> + writer.write(&inner.quantum_size)?;
> +
> + inner.quantum_size = quantum_size;
> + }
> + Command::ShiftQuantum(quantum) => {
> + let old_quantum = inner.quantum_size;
> + inner.quantum_size = quantum as usize;
> + return Ok(old_quantum as u64);
> + }
> + Command::SetQset(mut reader) => {
> + let qset_size = reader.read()?;
> +
> + if !reader.is_empty() {
> + return Err(EINVAL);
> + }
> +
> + inner.qset_size = qset_size;
> + }
> + Command::TellQset(qset) => {
> + inner.qset_size = qset as usize;
> + }
> + Command::GetQset(mut writer) => {
> + writer.write(&inner.qset_size)?;
> +
> + if !writer.is_empty() {
> + return Err(EINVAL);
> + }
> + }
> + Command::QueryQset => {
> + return Ok(inner.qset_size as u64);
> + }
> + Command::ExchangeQset(slice) => {
> + let (mut reader, mut writer) = slice.reader_writer();
> + let qset_size = reader.read()?;
> +
> + if !reader.is_empty() {
> + return Err(EINVAL);
> + }
> +
> + writer.write(&inner.qset_size)?;
> +
> + inner.qset_size = qset_size;
> + }
> + Command::ShiftQset(qset) => {
> + let old_qset = inner.qset_size;
> + inner.qset_size = qset as usize;
> + return Ok(old_qset as u64);
> + }
> + }
> +
> + Ok(0)
> + }
> +
> + fn llseek(
> + &self,
> + _file: &LocalFile,
> + pos: &mut i64,
> + offset: i64,
> + whence: Whence,
> + ) -> Result<u64, Self::Err> {
> + let size = self.inner.lock().size as i64;
> +
> + let new_offset = match whence {
> + Whence::Set => offset,
> + Whence::Cur => *pos + offset,
> + Whence::End => size + offset,
> + _ => return Err(EINVAL),
> + };
> +
> + if new_offset < 0 {
> + return Err(EINVAL);
> + }
> +
> + *pos = new_offset;
> +
> + Ok(new_offset as u64)
> + }
> +}
> +
> +struct RustCharDevModule {
> + reg: Pin<Box<DeviceRegistration<ScullDev, NUM_DEVS>>>,
You really want NUM_DEVS of these statically? I haven't read the old
book example code, but that's really a bad example if so, the authors
should have known better :)
Ideally these are created dynamically, like what happens in the real
world.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] samples: rust: add character device sample
2024-10-12 12:37 ` Greg Kroah-Hartman
@ 2024-10-12 23:38 ` Josef Zoller
0 siblings, 0 replies; 11+ messages in thread
From: Josef Zoller @ 2024-10-12 23:38 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, linux-kernel, rust-for-linux
On 12.10.24 14:37, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2024 at 08:55:44PM +0200, Josef Zoller wrote:
>> Provide a sample implementation of a character device in Rust, following
>> the scull device from the well-known book "Linux Device Drivers".
>>
>> This sample uses the abstractions from the previous patches in this
>> series to implement the scull device in mostly safe Rust. All of the
>> unsafe code comes from the fact that the data structure used in the C
>> implementation is inherently unsafe and cannot easily be represented in
>> safe Rust without a lot of memory overhead.
>>
>> This sample should be a good starting point for people who want to start
>> writing kernel code in Rust, as a character device is relatively simple
>> and does not require a lot of kernel knowledge to understand.
>>
>> Signed-off-by: Josef Zoller <josef@walterzollerpiano.com>
>> ---
>> samples/rust/Kconfig | 10 +
>> samples/rust/Makefile | 1 +
>> samples/rust/rust_char_dev.rs | 506 ++++++++++++++++++++++++++++++++++++++++++
>
> This feels like it could be a lot smaller, 500 lines for a sample char
> driver seems excessive.
You're totally right and I'm not too happy with this either. It's mainly
that large because I wanted to model the storage of the device as
closely as possible to the C version, which probably just doesn't make
a lot of sense. I'll try to find a way to make this more rusty without
having the massive memory overhead that comes with a naive translation.
>> --- /dev/null
>> +++ b/samples/rust/rust_char_dev.rs
>> @@ -0,0 +1,506 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> Again, copyright notice please.
Sure, maybe it should be added to all rust files?
>> +
>> +//! Rust character device sample.
>> +//!
>> +//! This sample demonstrates how to create a simple character device in Rust,
>> +//! by reimplementing the `scull` device from the Linux Device Drivers book.
>> +
>> +use core::{mem, ptr::NonNull};
>> +
>> +use kernel::{
>> + c_str,
>> + char_dev::{CharDevice, CharDeviceID, DeviceRegistration, OpenCharDevice, Whence},
>> + fs::{file::flags, File, LocalFile},
>> + new_mutex,
>> + prelude::*,
>> + sync::{Arc, Mutex},
>> + uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
>> +};
>> +
>> +module! {
>> + type: RustCharDevModule,
>> + name: "rust_char_dev",
>> + author: "Rust for Linux Contributors",
>> + description: "Rust character device sample",
>> + license: "GPL",
>> +}
>> +
>> +const DEVICE_NAME: &CStr = c_str!("rust_scull");
>> +const DEFAULT_QSET_SIZE: usize = 1000;
>> +const DEFAULT_QUANTUM_SIZE: usize = 4000;
>> +const NUM_DEVS: usize = 4;
>
> Comments for what these const are for?
If you're familiar with the scull device data structure, these should be
pretty much self-explanatory. So probably some explanatory comment about
that data structure would be more helpful. Noted.
>> +// This is probably too specific a function to be in the Rust standard library...
>> +trait OptionExt<T> {
>> + fn get_or_try_insert_with<F, E>(&mut self, f: F) -> Result<&mut T, E>
>> + where
>> + F: FnOnce() -> Result<T, E>;
>> +}
>> +
>> +impl<T> OptionExt<T> for Option<T> {
>> + fn get_or_try_insert_with<F, E>(&mut self, f: F) -> Result<&mut T, E>
>> + where
>> + F: FnOnce() -> Result<T, E>,
>> + {
>> + if self.is_none() {
>> + *self = Some(f()?);
>> + }
>> +
>> + Ok(self.as_mut().unwrap())
>> + }
>> +}
>
> What are these for? No comments makes it confusing to me :)
Yeah, that's a blunder on my part :)
Sometimes I forget that, especially in this project, the rust code is
read by people who are not necessarily that familiar with the language.
This is a pretty common pattern in Rust, so I just didn't think about it
too much.
Basically, there is a problem with the Rust borrow checker right now,
that makes it hard to do some basic stuff.
In this case, we have the following problem:
Suppose, we want to write a function that takes an option and returns a
mutable reference to its value. If the option is None, we want to insert
a new value first and then get a mutable reference to it. A naive
approach would be to do this (insert returns a mutable reference to the
inserted value):
fn foo(opt: &mut Option<usize>) -> &mut usize {
if let Some(reference) = opt.as_mut() {
reference
} else {
opt.insert(42)
}
}
However, this does not work, because the borrow checker doesn't allow
multiple mutable references to the same value and (currently) doesn't
understand that the references in the two if branches are mutually
exclusive. To work around this, we can use the get_or_insert method on
Option, which does exactly what we want:
fn foo(opt: &mut Option<usize>) -> &mut usize {
opt.get_or_insert(42)
}
However, this eagerly evaluates the value to insert, which is not what
we want later in this file, because we want to allocate only when we
have to. The rust standard library also has a get_or_insert_with method,
which takes a closure that returns the value to insert. However, this
method doesn't allow us to fail an allocation, because the closure can't
return a Result. So I extended Option with the get_or_try_insert_with
method, which allows us to return a Result from the closure.
>> +
>> +#[derive(IoctlCommand)]
>> +#[ioctl(code = b'k')]
>> +enum Command {
>> + Reset, // 0
>> + SetQuantum(UserSliceReader), // 1
>> + SetQset(UserSliceReader), // 2
>> + TellQuantum(u64), // 3
>> + TellQset(u64), // 4
>> + GetQuantum(UserSliceWriter), // 5
>> + GetQset(UserSliceWriter), // 6
>> + QueryQuantum, // 7
>> + QueryQset, // 8
>> + ExchangeQuantum(UserSlice), // 9
>> + ExchangeQset(UserSlice), // 10
>> + ShiftQuantum(u64), // 11
>> + ShiftQset(u64), // 12
>> +}
>
> Trying to keep ioctl numbers identical from kernel/userspace means they
> should go in a .h file, putting them here is not going to work well.
Hmm, this is a tricky problem to solve. A solution like this, where the
commands are automatically parsed into a Rust enum using a derive macro,
would definitely be the rustiest way to do it, but it's obviously not
too easy to export these commands for userspace.
>> +// We implement `ScullQset` as close as possible to the `struct scull_qset` implementation from
>> +// the book. This means that we have to use raw pointers and some unsafe code for the data.
>
> That's not a good idea overall, why not look at the "untrusted data"
> patch on the list and tie that into here so that things are not unsafe?
> That is ideally what we want to do for ALL rust user/kernel interfaces.
I don't think this particular problem can be solved with the untrusted
data patch, as it's only about data storage for data that's already been
copied into kernel space.
>> +// Otherwise, we'd have massive memory overhead by storing sizes/capacities unnecessarily.
>> +// E.g. every `ScullQset` would be 8 * qset_size bytes larger if we used `Box<[_]>` and
>> +// 16 * qset_size bytes larger if we used `Vec<_>`.
>> +// However, the knowledge that all data arrays are of the same size isn't possible to express
>> +// in safe Rust without this memory overhead.
>> +struct ScullQset {
>> + data: Option<NonNull<Option<NonNull<u8>>>>,
>> + quantum_size: usize,
>> + qset_size: usize,
>> + next: Option<Box<ScullQset>>,
>> +}
>
> I'm sorry, I don't understand why this is so odd and has to be "unsafe".
> It's just chunks of memory read from userspace. And we do have a list
> structure somewhere (or something like that), that probably works better
> than the next pointer from what I recall.
So, first of all, the linked list itself doesn't require unsafe code at
all. Option<Box<Self>> is exactly the right pattern to use for linked
lists and directly corresponds to how you'd write a linked list in C,
only with the safety guarantees of Rust.
The problem is with the data field, which should be an optional
heap-allocated array of arrays, where the outer array has qset_size
elements and each of those elements is an optional heap-allocated array
of quantum_size elements. If QSET_SIZE and QUANTUM_SIZE were known
constants at compile time, we could simply use a
Option<Box<[Option<Box<[u8; QUANTUM_SIZE]>; QSET_SIZE]>>
to represent this. However, it's not so easy when the sizes can change
at runtime. Naively, we could use a Option<Vec<Option<Vec<u8>>>>, but
that has the following downside: The outer Vec has to store qset_size
times the size of Option<Vec<u8>>, which is 24 bytes on a 64-bit system,
because each Vec stores not only a pointer, but also both a capacity and
a size. This is obviously unnecessary, because we know that all the
inner Vecs will have the same size, but we cannot prove this to the
compiler. Using a Option<Box<[Option<Box<[u8]>>]>, we can avoid storing
the size, but we still store the length for each inner array and also
the rest of our code will again be much more complicated.
Thus, I settled on using raw pointers for this version of the patch.
However, I hear your concerns about this and - as I'm myself not happy
with this solution - I would come up with a better solution in a second
version of this patch.
>> +impl ScullQset {
>> + fn new(quantum_size: usize, qset_size: usize) -> Self {
>> + Self {
>> + data: None,
>> + quantum_size,
>> + qset_size,
>> + next: None,
>> + }
>> + }
>> +
>> + /// Returns a reference to the quantum at index `i` if it exists.
>> + fn get_quantum(&self, i: usize) -> Option<&[u8]> {
>> + let data_slice = NonNull::slice_from_raw_parts(self.data?, self.qset_size);
>> + // SAFETY: `data_slice` points to a valid slice of `Option<NonNull<u8>>`.
>> + let quantum = unsafe { data_slice.as_ref()[i] };
>> + let quantum_slice = NonNull::slice_from_raw_parts(quantum?, self.quantum_size);
>> + // SAFETY: `quantum_slice` points to a valid slice of `u8`.
>> + Some(unsafe { quantum_slice.as_ref() })
>> + }
>> +
>> + /// Returns a mutable reference to the quantum at index `i`, allocating it first if necessary.
>> + fn get_quantum_mut(&mut self, i: usize) -> Option<&mut [u8]> {
>> + let data = self
>> + .data
>> + .get_or_try_insert_with(|| {
>> + let mut data =
>> + mem::ManuallyDrop::new(Vec::with_capacity(self.qset_size, GFP_KERNEL)?);
>> + for _ in 0..self.qset_size {
>> + data.push(None, GFP_KERNEL)?;
>> + }
>> +
>> + assert!(data.len() == data.capacity());
>> +
>> + // SAFETY: `data.as_mut_ptr()` is non-null.
>> + Ok::<_, Error>(unsafe { NonNull::new_unchecked(data.as_mut_ptr()) })
>> + })
>> + .ok()?;
>> +
>> + let mut data_slice = NonNull::slice_from_raw_parts(*data, self.qset_size);
>> +
>> + // SAFETY: `data_slice` points to a valid slice of `Option<NonNull<u8>>`.
>> + let maybe_quantum = unsafe { &mut data_slice.as_mut()[i] };
>> + let quantum = maybe_quantum
>> + .get_or_try_insert_with(|| {
>> + let mut quantum =
>> + mem::ManuallyDrop::new(Vec::with_capacity(self.quantum_size, GFP_KERNEL)?);
>> + for _ in 0..self.quantum_size {
>> + quantum.push(0, GFP_KERNEL)?;
>> + }
>> +
>> + assert!(quantum.len() == quantum.capacity());
>> +
>> + // SAFETY: `quantum.as_mut_ptr()` is non-null.
>> + Ok::<_, Error>(unsafe { NonNull::new_unchecked(quantum.as_mut_ptr()) })
>> + })
>> + .ok()?;
>> +
>> + let mut quantum_slice = NonNull::slice_from_raw_parts(*quantum, self.quantum_size);
>> + // SAFETY: `quantum_slice` points to a valid slice of `u8`.
>> + Some(unsafe { quantum_slice.as_mut() })
>> + }
>> +}
>> +
>> +impl Drop for ScullQset {
>> + fn drop(&mut self) {
>> + if let Some(data) = self.data.take() {
>> + // SAFETY: `data` was created by `Vec::with_capacity` with a capacity of `qset_size`.
>> + let data_vec =
>> + unsafe { Vec::from_raw_parts(data.as_ptr(), self.qset_size, self.qset_size) };
>> +
>> + for quantum in data_vec {
>> + let Some(quantum) = quantum else { continue };
>> +
>> + // SAFETY: `quantum` was created by `Vec::with_capacity` with a capacity of
>> + // `quantum_size`.
>> + let _ = unsafe {
>> + Vec::from_raw_parts(quantum.as_ptr(), self.quantum_size, self.quantum_size)
>> + };
>> + }
>> + }
>> + }
>> +}
>> +
>> +// SAFETY: The raw pointers are uniquely owned by `ScullQset` and not shared, so it's safe to send
>> +// it to another thread.
>> +unsafe impl Send for ScullQset {}
>
> What other thread are you sending this to?
>
> I don't see any "threads" here in the driver, do you mean "can be shared
> by different userspace processes"?
I'm not sending this to another thread here, I'm just saying that it
would be safe to do so. This is a requirement for implementing the
CharDevice and OpenCharDevice traits, which I added because I assumed
that the methods on f_ops can probably be called by different threads
in the kernel, which would mean that we access the device data from
different threads even if we don't explicitly spawn any threads in the
device driver. Rust requires types that can be accessed from different
threads to be Send, so I added this requirement to the device traits.
>> +struct ScullDevInner {
>> + data: Option<Box<ScullQset>>,
>> + quantum_size: usize,
>> + qset_size: usize,
>> + size: usize,
>> +}
>> +
>> +impl Default for ScullDevInner {
>> + fn default() -> Self {
>> + Self {
>> + data: None,
>> + quantum_size: DEFAULT_QUANTUM_SIZE,
>> + qset_size: DEFAULT_QSET_SIZE,
>> + size: 0,
>> + }
>> + }
>> +}
>> +
>> +impl ScullDevInner {
>> + fn trim(&mut self) {
>> + mem::take(&mut self.data);
>> + self.size = 0;
>> + }
>> +
>> + fn follow(&mut self, n: usize) -> Option<&mut ScullQset> {
>> + let mut qs = self
>> + .data
>> + .get_or_try_insert_with(|| {
>> + Box::new(
>> + ScullQset::new(self.quantum_size, self.qset_size),
>> + GFP_KERNEL,
>> + )
>> + })
>> + .ok()?;
>> +
>> + for _ in 0..n {
>> + qs = qs
>> + .next
>> + .get_or_try_insert_with(|| {
>> + // We use `qs.quantum_size` and `qs.qset_size` here to avoid subtly
>> + // different behavior from the original C implementation.
>> + // If we used the sizes from `self`, we could end up with differently
>> + // sized qsets in the linked list (which would not be a safety problem).
>> + // Like this, we only use an updated size after `trim` has been called,
>> + // which is the same behavior as in the book.
>> + Box::new(ScullQset::new(qs.quantum_size, qs.qset_size), GFP_KERNEL)
>
> As one of the authors of the book in reference here, I really don't
> understand this at all :)
>
> What am I missing? Why is this so complex?
In the C implementation of scull, the quantum and qset sizes are global
variables that are copied to a device's private data structure when its
trim method is called. I'm emulating this behaviour, but without global
variables. It is not very complex at all (I'm just using qs.quantum_size
and qs.qset_size instead of self.quantum_size and self.qset_size in this
line), I just wanted to exlain this in the comment. I think, the comment
makes it sound more complex than it actually is...
> You don't have to follow the implementation identically, just because
> something is simple to do in C doesn't mean you have to do the same
> thing in rust at all. We just implemented stuff to make the concepts
> simple to follow, not to implement something specific that was trying to
> solve a real problem here. So try to keep it simple, I think there are
> other examples of rust char drivers in other trees that might be better
> to use as an example, as this complexity probably isn't needed and will
> just confuse new people (i.e. me!) more than is needed.
Yeah, I agree. I wanted to perfectly replicate the C implementation's
behaviour, but maybe that's just not very nice for sample code that is
supposed to be as simple as possible. In a second version of this patch,
I would definitely simplify this part.
By the way, as I was writing the above paragraph, I realized, that I
already hadn't quite replicated the C implementation's behaviour
regarding the global variables (because they are shared between all
devices in the C implementation) :)
>> + })
>> + .ok()?;
>> + }
>> +
>> + Some(qs)
>> + }
>> +}
>> +
>> +#[derive(Clone)]
>> +struct ScullDev {
>> + inner: Arc<Mutex<ScullDevInner>>,
>> +}
>> +
>> +#[vtable]
>> +impl CharDevice for ScullDev {
>> + type OpenPtr = Box<Self>;
>> + type Err = Error;
>> +
>> + fn new(_dev_id: CharDeviceID) -> Result<Self> {
>> + Ok(Self {
>> + inner: Arc::pin_init(new_mutex!(ScullDevInner::default()), GFP_KERNEL)?,
>> + })
>> + }
>> +
>> + fn open(&self, file: &File) -> Result<Self::OpenPtr> {
>> + if file.flags() & flags::O_ACCMODE == flags::O_WRONLY {
>> + // TODO: this should be lock_interruptible, but that's not in the Rust API yet
>
> I think patches for that are floating around, right?
I don't think so:
https://lore.kernel.org/rust-for-linux/?q=lock_interruptible
>> + self.inner.lock().trim();
>> + }
>> +
>> + Ok(Box::new(self.clone(), GFP_KERNEL)?)
>> + }
>> +}
>> +
>> +#[vtable]
>> +impl OpenCharDevice for ScullDev {
>> + type IoctlCmd = Command;
>> + type Err = Error;
>> +
>> + fn read(&self, _file: &LocalFile, mut buf: UserSliceWriter, offset: &mut i64) -> Result<usize> {
>> + let pos = usize::try_from(*offset).map_err(|_| EINVAL)?;
>> +
>> + // TODO: this should be lock_interruptible, but that's not in the Rust API yet
>> + let mut inner = self.inner.lock();
>> +
>> + // To keep the behavior of the original C implementation, namely that the quantum and qset
>> + // sizes are only updated after a trim, we use the sizes from the inner data if it exists.
>> + let (quantum_size, qset_size) = inner
>> + .data
>> + .as_ref()
>> + .map_or((inner.quantum_size, inner.qset_size), |qs| {
>> + (qs.quantum_size, qs.qset_size)
>> + });
>> + let item_size = quantum_size * qset_size;
>> +
>> + if pos >= inner.size {
>> + return Ok(0);
>> + }
>> +
>> + let mut count = buf.len().min(inner.size - pos);
>> + let item = pos / item_size;
>> + let rest = pos % item_size;
>> + let s_pos = rest / quantum_size;
>> + let q_pos = rest % quantum_size;
>> +
>> + let Some(q) = inner.follow(item).and_then(|qs| qs.get_quantum(s_pos)) else {
>> + return Ok(0);
>> + };
>> +
>> + count = count.min(quantum_size - q_pos);
>> +
>> + buf.write_slice(&q[q_pos..q_pos + count])?;
>> +
>> + *offset += count as i64;
>> +
>> + Ok(count)
>> + }
>> +
>> + fn write(
>> + &self,
>> + _file: &LocalFile,
>> + mut buf: UserSliceReader,
>> + offset: &mut i64,
>> + ) -> Result<usize> {
>> + let pos = usize::try_from(*offset).map_err(|_| EINVAL)?;
>> +
>> + // TODO: this should be lock_interruptible, but that's not in the Rust API yet
>> + let mut inner = self.inner.lock();
>> +
>> + // To keep the behavior of the original C implementation, namely that the quantum and qset
>> + // sizes are only updated after a trim, we use the sizes from the inner data if it exists.
>> + let (quantum_size, qset_size) = inner
>> + .data
>> + .as_ref()
>> + .map_or((inner.quantum_size, inner.qset_size), |qs| {
>> + (qs.quantum_size, qs.qset_size)
>> + });
>> + let item_size = quantum_size * qset_size;
>> +
>> + let item = pos / item_size;
>> + let rest = pos % item_size;
>> + let s_pos = rest / quantum_size;
>> + let q_pos = rest % quantum_size;
>> +
>> + let Some(q) = inner.follow(item).and_then(|qs| qs.get_quantum_mut(s_pos)) else {
>> + return Err(ENOMEM);
>> + };
>> +
>> + let count = buf.len().min(quantum_size - q_pos);
>> +
>> + buf.read_slice(&mut q[q_pos..q_pos + count])?;
>> +
>> + let new_pos = pos + count;
>> + *offset = new_pos as i64;
>> +
>> + if new_pos > inner.size {
>> + inner.size = new_pos;
>> + }
>> +
>> + Ok(count)
>> + }
>> +
>> + fn ioctl(
>> + &self,
>> + _file: &File,
>> + cmd: Self::IoctlCmd,
>> + #[cfg(CONFIG_COMPAT)] _compat: bool,
>> + ) -> Result<u64> {
>> + // The original implementation from the book actually doesn't consider the lock here at all,
>> + // but Rust forces us to do so :)
>> + let mut inner = self.inner.lock();
>
> Does that mean the C implementation is wrong? It could be, but what
> makes this so special that rust is requiring it and C doesn't?
Well, yes, the C implementation is probably wrong. It doesn't take a
lock here, which could at least lead to bad interleavings in the
operations that are supposed to be atomic like exchange/shift, I think.
As I have wrapped the inner data in a Mutex, Rust forces me to take the
lock here, because I cannot access the inner data in any other way. In
C, you can just access the data directly, the lock is just a field next
to the data, so you can ignore it if you want to.
>> +
>> + // We should definitely check if the user is trying to set a size to 0, or we'll
>> + // end up with panics in the read/write functions due to division by zero.
>> + // However, the original implementation doesn't account for this, so we won't either.
>
> We should :)
>
> And invalid userspace data is the #1 security bug in the kernel, let's
> not copy bad examples for others to also make the same mistake wherever
> possible please.
Fair enough :)
>> + match cmd {
>> + Command::Reset => {
>> + inner.quantum_size = DEFAULT_QUANTUM_SIZE;
>> + inner.qset_size = DEFAULT_QSET_SIZE;
>> + }
>> + Command::SetQuantum(mut reader) => {
>> + // TODO: guard this command (and all others where a size can be set by the user)
>> + // with `capability(CAP_SYS_ADMIN)`, which is not yet possible in the Rust API.
>> + let quantum_size = reader.read()?;
>> +
>> + if !reader.is_empty() {
>> + return Err(EINVAL);
>> + }
>> +
>> + inner.quantum_size = quantum_size;
>> + }
>> + Command::TellQuantum(quantum) => {
>> + inner.quantum_size = quantum as usize;
>> + }
>> + Command::GetQuantum(mut writer) => {
>> + writer.write(&inner.quantum_size)?;
>> +
>> + if !writer.is_empty() {
>> + return Err(EINVAL);
>> + }
>> + }
>> + Command::QueryQuantum => {
>> + return Ok(inner.quantum_size as u64);
>> + }
>> + Command::ExchangeQuantum(slice) => {
>> + let (mut reader, mut writer) = slice.reader_writer();
>> + let quantum_size = reader.read()?;
>> +
>> + if !reader.is_empty() {
>> + return Err(EINVAL);
>> + }
>> +
>> + writer.write(&inner.quantum_size)?;
>> +
>> + inner.quantum_size = quantum_size;
>> + }
>> + Command::ShiftQuantum(quantum) => {
>> + let old_quantum = inner.quantum_size;
>> + inner.quantum_size = quantum as usize;
>> + return Ok(old_quantum as u64);
>> + }
>> + Command::SetQset(mut reader) => {
>> + let qset_size = reader.read()?;
>> +
>> + if !reader.is_empty() {
>> + return Err(EINVAL);
>> + }
>> +
>> + inner.qset_size = qset_size;
>> + }
>> + Command::TellQset(qset) => {
>> + inner.qset_size = qset as usize;
>> + }
>> + Command::GetQset(mut writer) => {
>> + writer.write(&inner.qset_size)?;
>> +
>> + if !writer.is_empty() {
>> + return Err(EINVAL);
>> + }
>> + }
>> + Command::QueryQset => {
>> + return Ok(inner.qset_size as u64);
>> + }
>> + Command::ExchangeQset(slice) => {
>> + let (mut reader, mut writer) = slice.reader_writer();
>> + let qset_size = reader.read()?;
>> +
>> + if !reader.is_empty() {
>> + return Err(EINVAL);
>> + }
>> +
>> + writer.write(&inner.qset_size)?;
>> +
>> + inner.qset_size = qset_size;
>> + }
>> + Command::ShiftQset(qset) => {
>> + let old_qset = inner.qset_size;
>> + inner.qset_size = qset as usize;
>> + return Ok(old_qset as u64);
>> + }
>> + }
>> +
>> + Ok(0)
>> + }
>> +
>> + fn llseek(
>> + &self,
>> + _file: &LocalFile,
>> + pos: &mut i64,
>> + offset: i64,
>> + whence: Whence,
>> + ) -> Result<u64, Self::Err> {
>> + let size = self.inner.lock().size as i64;
>> +
>> + let new_offset = match whence {
>> + Whence::Set => offset,
>> + Whence::Cur => *pos + offset,
>> + Whence::End => size + offset,
>> + _ => return Err(EINVAL),
>> + };
>> +
>> + if new_offset < 0 {
>> + return Err(EINVAL);
>> + }
>> +
>> + *pos = new_offset;
>> +
>> + Ok(new_offset as u64)
>> + }
>> +}
>> +
>> +struct RustCharDevModule {
>> + reg: Pin<Box<DeviceRegistration<ScullDev, NUM_DEVS>>>,
>
> You really want NUM_DEVS of these statically? I haven't read the old
> book example code, but that's really a bad example if so, the authors
> should have known better :)
Well, the book registers scull_nr_devs devices, which is 4 by default
but can be set using a module param, so it's not completely static.
> Ideally these are created dynamically, like what happens in the real
> world.
Hmm, do you mean that we should actually register more devices (e.g.
using register_chrdev_region) when needed? How would that work?
Otherwise, you could see the const generic on the DeviceRegistration
struct more as a way to specify the maximum number of devices that are
registered, but not necessarily the number of devices that have data
allocated for them (currently, we allocate some data for every device
on registration, but I could also change it, so that it's only allocated
on the first open of the device or something like that).
Cheers,
Josef
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Character device abstractions for Rust
2024-10-11 18:55 [PATCH 0/3] Character device abstractions for Rust Josef Zoller
` (2 preceding siblings ...)
2024-10-11 18:55 ` [PATCH 3/3] samples: rust: add character device sample Josef Zoller
@ 2024-10-12 7:29 ` Greg Kroah-Hartman
2024-10-12 21:14 ` Josef Zoller
3 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2024-10-12 7:29 UTC (permalink / raw)
To: Josef Zoller
Cc: Arnd Bergmann, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, linux-kernel, rust-for-linux
On Fri, Oct 11, 2024 at 08:55:41PM +0200, Josef Zoller wrote:
> Writing character devices is a common way to start writing kernel code,
> especially because of the book "Linux Device Drivers", which is still
> one of the best resources to learn about Linux kernel programming. To
> allow an easier entry into Rust kernel programming specifically, this
> series adds abstractions for these kinds of devices to the Rust API.
I understand this, but if at all possible, I would prefer that people
stick to using the misc char device interface instead. It's much
simpler and integrates better into the overall system (handles sysfs for
you automatically, etc.)
I've already merged the misc device rust bindings into my tree, so why
not just stick with them?
> I also included a sample that demonstrates how to use these abstractions
> to create the simplest example from LDD3, the "scull" device.
This is great, but why not just provide the scull example using misc
device?
> I'm also aware of the patch series about misc devices that was sent
> recently. I think these are both valuable additions to the Rust API, and
> could even be combined in some way, in which case the file operations
> abstractions in both series should probably be separated and
> generalized. But I'm still sending this series as it is, because it is
> my first ever patch and I could use some feedback on my approach.
That's great, but I'd prefer to stick with the misc code for now until
someone really really really proves that they want a "raw" char
interface.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 0/3] Character device abstractions for Rust
2024-10-12 7:29 ` [PATCH 0/3] Character device abstractions for Rust Greg Kroah-Hartman
@ 2024-10-12 21:14 ` Josef Zoller
0 siblings, 0 replies; 11+ messages in thread
From: Josef Zoller @ 2024-10-12 21:14 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, linux-kernel, rust-for-linux
On 12.10.24 09:29, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2024 at 08:55:41PM +0200, Josef Zoller wrote:
>> Writing character devices is a common way to start writing kernel code,
>> especially because of the book "Linux Device Drivers", which is still
>> one of the best resources to learn about Linux kernel programming. To
>> allow an easier entry into Rust kernel programming specifically, this
>> series adds abstractions for these kinds of devices to the Rust API.
>
> I understand this, but if at all possible, I would prefer that people
> stick to using the misc char device interface instead. It's much
> simpler and integrates better into the overall system (handles sysfs for
> you automatically, etc.)
>
> I've already merged the misc device rust bindings into my tree, so why
> not just stick with them?
Aaaah, I should probably have done some proper 'market research' before
just blindly diving into and implementing this. So, if I understand you
correctly, you're saying that only very niche use cases would benefit
from being implemented as a raw char device, and that the misc device
interface is the way to go for most cases?
>> I also included a sample that demonstrates how to use these abstractions
>> to create the simplest example from LDD3, the "scull" device.
>
> This is great, but why not just provide the scull example using misc
> device?
I don't remember seeing a misc device implementation in the book. Are
you just saying that the scull device could be easily implemented as a
misc device instead, or am I missing something?
>> I'm also aware of the patch series about misc devices that was sent
>> recently. I think these are both valuable additions to the Rust API, and
>> could even be combined in some way, in which case the file operations
>> abstractions in both series should probably be separated and
>> generalized. But I'm still sending this series as it is, because it is
>> my first ever patch and I could use some feedback on my approach.
>
> That's great, but I'd prefer to stick with the misc code for now until
> someone really really really proves that they want a "raw" char
> interface.
Fair enough. So, it seems like I should probably just drop this series,
right? I will probably still address the other feedback you gave me in
a second version, but putting much more effort into this series seems
like a waste of time now.
Anyway, thanks for the honest and early feedback! I could have easily
spent much more time on this series without knowing that it's not what
you're looking for.
Cheers,
Josef
^ permalink raw reply [flat|nested] 11+ messages in thread