From 8b0c0cd1c513f12d5e64a2feb744c561fbfc1b98 Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Thu, 25 Mar 2021 15:58:10 +0000 Subject: [PATCH 1/3] Share context with file instances based on registration. --- drivers/char/rust_example.rs | 14 +++++---- rust/kernel/chrdev.rs | 22 +++++++++++-- rust/kernel/file_operations.rs | 57 +++++++++++++++++++++++++++------- rust/kernel/miscdev.rs | 21 ++++++++++--- 4 files changed, 89 insertions(+), 25 deletions(-) diff --git a/drivers/char/rust_example.rs b/drivers/char/rust_example.rs index 20b537336a06fe..6e3eaaf9a0fc4f 100644 --- a/drivers/char/rust_example.rs +++ b/drivers/char/rust_example.rs @@ -11,7 +11,7 @@ use core::pin::Pin; use kernel::prelude::*; use kernel::{ chrdev, condvar_init, cstr, - file_operations::FileOperations, + file_operations::{FileOpener, FileOperations}, miscdev, mutex_init, spinlock_init, sync::{CondVar, Mutex, SpinLock}, }; @@ -53,15 +53,17 @@ module! { struct RustFile; +impl FileOpener<()> for RustFile { + fn open(_ctx: &()) -> KernelResult { + println!("rust file was opened!"); + Ok(Box::try_new(Self)?) + } +} + impl FileOperations for RustFile { type Wrapper = Box; kernel::declare_file_operations!(); - - fn open() -> KernelResult { - println!("rust file was opened!"); - Ok(Box::try_new(Self)?) - } } struct RustExample { diff --git a/rust/kernel/chrdev.rs b/rust/kernel/chrdev.rs index c565123a640471..6772a3a925ccf6 100644 --- a/rust/kernel/chrdev.rs +++ b/rust/kernel/chrdev.rs @@ -78,7 +78,7 @@ impl Registration<{ N }> { /// Registers a character device. /// /// You may call this once per device type, up to `N` times. - pub fn register(self: Pin<&mut Self>) -> KernelResult { + pub fn register>(self: Pin<&mut Self>) -> KernelResult { // SAFETY: We must ensure that we never move out of `this`. let this = unsafe { self.get_unchecked_mut() }; if this.inner.is_none() { @@ -112,7 +112,12 @@ impl Registration<{ N }> { // SAFETY: Calling unsafe functions and manipulating `MaybeUninit` // pointer. unsafe { - bindings::cdev_init(cdev, file_operations::FileOperationsVtable::::build()); + bindings::cdev_init( + cdev, + // SAFETY: The adapter doesn't retrieve any state yet, so it's compatible with any + // registration. + file_operations::FileOperationsVtable::::build(), + ); (*cdev).owner = this.this_module.0; let rc = bindings::cdev_add(cdev, inner.dev + inner.used as bindings::dev_t, 1); if rc != 0 { @@ -124,6 +129,19 @@ impl Registration<{ N }> { } } +impl file_operations::FileOpenAdapter for Registration<{ N }> { + type Arg = (); + + unsafe fn convert( + _inode: *mut bindings::inode, + _file: *mut bindings::file, + ) -> *const Self::Arg { + // TODO: Update the SAFETY comment on the call to `FileOperationsVTable::build` above once + // this is updated to retrieve state. + &() + } +} + // SAFETY: `Registration` does not expose any of its state across threads // (it is fine for multiple threads to have a shared reference to it). unsafe impl Sync for Registration<{ N }> {} diff --git a/rust/kernel/file_operations.rs b/rust/kernel/file_operations.rs index 3668bb29020f4a..7d18ee93d72782 100644 --- a/rust/kernel/file_operations.rs +++ b/rust/kernel/file_operations.rs @@ -82,12 +82,13 @@ macro_rules! from_kernel_result { }}; } -unsafe extern "C" fn open_callback( - _inode: *mut bindings::inode, +unsafe extern "C" fn open_callback>( + inode: *mut bindings::inode, file: *mut bindings::file, ) -> c_types::c_int { from_kernel_result! { - let ptr = T::open()?.into_pointer(); + let arg = A::convert(inode, file); + let ptr = T::open(&*arg)?.into_pointer(); (*file).private_data = ptr as *mut c_types::c_void; Ok(0) } @@ -198,11 +199,11 @@ unsafe extern "C" fn fsync_callback( } } -pub(crate) struct FileOperationsVtable(marker::PhantomData); +pub(crate) struct FileOperationsVtable(marker::PhantomData, marker::PhantomData); -impl FileOperationsVtable { +impl> FileOperationsVtable { const VTABLE: bindings::file_operations = bindings::file_operations { - open: Some(open_callback::), + open: Some(open_callback::), release: Some(release_callback::), read: if T::TO_USE.read { Some(read_callback::) @@ -262,7 +263,11 @@ impl FileOperationsVtable { }; /// Builds an instance of [`struct file_operations`]. - pub(crate) const fn build() -> &'static bindings::file_operations { + /// + /// # Safety + /// + /// The caller must ensure that the adapter is compatible with the way the device is registered. + pub(crate) const unsafe fn build() -> &'static bindings::file_operations { &Self::VTABLE } } @@ -400,6 +405,39 @@ impl IoctlCommand { } } +/// Trait for extracting file open arguments from kernel data structures. +/// +/// This is meant to be implemented by registration managers. +pub trait FileOpenAdapter { + /// The type of argument this adpter extracts. + type Arg; + + /// Converts untyped data stored in [`struct inode`] and [`struct file`] (when [`struct + /// file_operations::open`] is called) into the given type. For example, for `miscdev` + /// devices, a pointer to the registered [`struct miscdev`] is stored in [`struct + /// file::private_data`]. + /// + /// # Safety + /// + /// This function must be called only when [`struct file_operations::open`] is being called for + /// a file that was registered by the implementer. + unsafe fn convert(_inode: *mut bindings::inode, _file: *mut bindings::file) + -> *const Self::Arg; +} + +/// Trait for implementers of kernel files. +/// +/// In addition to the methods in [`FileOperations`], implementers must also provide +/// [`FileOpener::open`] with a customised argument. This allows a single implementation of +/// [`FileOperations`] to be used for different types of registrations, for example, `miscdev` and +/// `chrdev`. +pub trait FileOpener: FileOperations { + /// Creates a new instance of this file. + /// + /// Corresponds to the `open` function pointer in `struct file_operations`. + fn open(context: &T) -> KernelResult; +} + /// Corresponds to the kernel's `struct file_operations`. /// /// You implement this trait whenever you would create a `struct file_operations`. @@ -414,11 +452,6 @@ pub trait FileOperations: Send + Sync + Sized { /// The pointer type that will be used to hold ourselves. type Wrapper: PointerWrapper; - /// Creates a new instance of this file. - /// - /// Corresponds to the `open` function pointer in `struct file_operations`. - fn open() -> KernelResult; - /// Cleans up after the last reference to the file goes away. /// /// Note that the object is moved, so it will be freed automatically unless the implementation diff --git a/rust/kernel/miscdev.rs b/rust/kernel/miscdev.rs index bdd07e6194b607..92c2181f305364 100644 --- a/rust/kernel/miscdev.rs +++ b/rust/kernel/miscdev.rs @@ -7,7 +7,7 @@ //! Reference: use crate::error::{Error, KernelResult}; -use crate::file_operations::{FileOperations, FileOperationsVtable}; +use crate::file_operations::{FileOpenAdapter, FileOpener, FileOperationsVtable}; use crate::{bindings, c_types, CStr}; use alloc::boxed::Box; use core::marker::PhantomPinned; @@ -19,7 +19,8 @@ pub struct Registration { mdev: bindings::miscdevice, _pin: PhantomPinned, - /// Context initialised on construction. + /// Context initialised on construction and made available to all file instances on + /// [`FileOpener::open`]. pub context: T, } @@ -39,7 +40,7 @@ impl Registration { /// Registers a miscellaneous device. /// /// Returns a pinned heap-allocated representation of the registration. - pub fn new_pinned( + pub fn new_pinned>( name: CStr<'static>, minor: Option, context: T, @@ -53,7 +54,7 @@ impl Registration { /// /// It must be pinned because the memory block that represents the registration is /// self-referential. If a minor is not given, the kernel allocates a new one if possible. - pub fn register( + pub fn register>( self: Pin<&mut Self>, name: CStr<'static>, minor: Option, @@ -65,7 +66,8 @@ impl Registration { return Err(Error::EINVAL); } - this.mdev.fops = FileOperationsVtable::::build(); + // SAFETY: The adapter is compatible with `misc_register`. + this.mdev.fops = unsafe { FileOperationsVtable::::build() }; this.mdev.name = name.as_ptr() as *const c_types::c_char; this.mdev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32); @@ -78,6 +80,15 @@ impl Registration { } } +impl FileOpenAdapter for Registration { + type Arg = T; + + unsafe fn convert(_inode: *mut bindings::inode, file: *mut bindings::file) -> *const Self::Arg { + let reg = crate::container_of!((*file).private_data, Self, mdev); + &(*reg).context + } +} + // SAFETY: The only method is `register()`, which requires a (pinned) mutable `Registration`, so it // is safe to pass `&Registration` to multiple threads because it offers no interior mutability, // except maybe through `Registration::context`, but it is itself `Sync`. From d5032322deb9322331547fbfd7e233bb49a388c9 Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Thu, 25 Mar 2021 22:14:53 +0000 Subject: [PATCH 2/3] Add shared state example. --- drivers/char/rust_example.rs | 107 +++++++++++++++++++++++++++++++++-- rust/kernel/error.rs | 3 + 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/drivers/char/rust_example.rs b/drivers/char/rust_example.rs index 6e3eaaf9a0fc4f..2c23cb4060a876 100644 --- a/drivers/char/rust_example.rs +++ b/drivers/char/rust_example.rs @@ -6,14 +6,16 @@ #![feature(allocator_api, global_asm)] #![feature(test)] -use alloc::boxed::Box; +use alloc::{boxed::Box, sync::Arc}; use core::pin::Pin; use kernel::prelude::*; use kernel::{ chrdev, condvar_init, cstr, - file_operations::{FileOpener, FileOperations}, + file_operations::{File, FileOpener, FileOperations}, miscdev, mutex_init, spinlock_init, sync::{CondVar, Mutex, SpinLock}, + user_ptr::{UserSlicePtrReader, UserSlicePtrWriter}, + Error, }; module! { @@ -51,6 +53,101 @@ module! { }, } +const MAX_TOKENS: usize = 3; + +struct SharedStateInner { + token_count: usize, +} + +struct SharedState { + state_changed: CondVar, + inner: Mutex, +} + +impl SharedState { + fn try_new() -> KernelResult> { + let state = Arc::try_new(Self { + // SAFETY: `condvar_init!` is called below. + state_changed: unsafe { CondVar::new() }, + // SAFETY: `mutex_init!` is called below. + inner: unsafe { Mutex::new(SharedStateInner { token_count: 0 }) }, + })?; + // SAFETY: `state_changed` is pinned behind `Arc`. + let state_changed = unsafe { Pin::new_unchecked(&state.state_changed) }; + kernel::condvar_init!(state_changed, "SharedState::state_changed"); + // SAFETY: `inner` is pinned behind `Arc`. + let inner = unsafe { Pin::new_unchecked(&state.inner) }; + kernel::mutex_init!(inner, "SharedState::inner"); + Ok(state) + } +} + +struct Token { + shared: Arc, +} + +impl FileOpener> for Token { + fn open(shared: &Arc) -> KernelResult { + Ok(Box::try_new(Self { + shared: shared.clone(), + })?) + } +} + +impl FileOperations for Token { + type Wrapper = Box; + + kernel::declare_file_operations!(read, write); + + fn read(&self, _: &File, data: &mut UserSlicePtrWriter, offset: u64) -> KernelResult { + // Succeed if the caller doesn't provide a buffer or if not at the start. + if data.is_empty() || offset != 0 { + return Ok(0); + } + + { + let mut inner = self.shared.inner.lock(); + + // Wait until we are allowed to decrement the token count or a signal arrives. + while inner.token_count == 0 { + if self.shared.state_changed.wait(&mut inner) { + return Err(Error::EINTR); + } + } + + // Consume a token. + inner.token_count -= 1; + } + + // Notify a possible writer waiting. + self.shared.state_changed.notify_all(); + + // Write a one-byte 1 to the reader. + data.write_slice(&[1u8; 1])?; + Ok(1) + } + + fn write(&self, data: &mut UserSlicePtrReader, _offset: u64) -> KernelResult { + { + let mut inner = self.shared.inner.lock(); + + // Wait until we are allowed to increment the token count or a signal arrives. + while inner.token_count == MAX_TOKENS { + if self.shared.state_changed.wait(&mut inner) { + return Err(Error::EINTR); + } + } + + // Increment the number of token so that a reader can be released. + inner.token_count += 1; + } + + // Notify a possible reader waiting. + self.shared.state_changed.notify_all(); + Ok(data.len()) + } +} + struct RustFile; impl FileOpener<()> for RustFile { @@ -69,7 +166,7 @@ impl FileOperations for RustFile { struct RustExample { message: String, _chrdev: Pin>>, - _dev: Pin>, + _dev: Pin>>>, } impl KernelModule for RustExample { @@ -148,9 +245,11 @@ impl KernelModule for RustExample { chrdev_reg.as_mut().register::()?; chrdev_reg.as_mut().register::()?; + let state = SharedState::try_new()?; + Ok(RustExample { message: "on the heap!".to_owned(), - _dev: miscdev::Registration::new_pinned::(cstr!("rust_miscdev"), None, ())?, + _dev: miscdev::Registration::new_pinned::(cstr!("rust_miscdev"), None, state)?, _chrdev: chrdev_reg, }) } diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 65b96a82ffdbba..432d866232c13c 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -45,6 +45,9 @@ impl Error { /// No such file or directory. pub const ENOENT: Self = Error(-(bindings::ENOENT as i32)); + /// Interrupted system call. + pub const EINTR: Self = Error(-(bindings::EINTR as i32)); + /// Creates an [`Error`] from a kernel error code. pub fn from_kernel_errno(errno: c_types::c_int) -> Error { Error(errno) From 69ff160647051a61b9996f0b5452747c943b091d Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Fri, 2 Apr 2021 02:41:34 +0100 Subject: [PATCH 3/3] Fix comment in rust/kernel/file_operations.rs Co-authored-by: Miguel Ojeda --- rust/kernel/file_operations.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/kernel/file_operations.rs b/rust/kernel/file_operations.rs index 7d18ee93d72782..01505e9fc8f1a9 100644 --- a/rust/kernel/file_operations.rs +++ b/rust/kernel/file_operations.rs @@ -409,7 +409,7 @@ impl IoctlCommand { /// /// This is meant to be implemented by registration managers. pub trait FileOpenAdapter { - /// The type of argument this adpter extracts. + /// The type of argument this adapter extracts. type Arg; /// Converts untyped data stored in [`struct inode`] and [`struct file`] (when [`struct