Skip to content

Allow files to share state. #145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 109 additions & 8 deletions drivers/char/rust_example.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::FileOperations,
file_operations::{File, FileOpener, FileOperations},
miscdev, mutex_init, spinlock_init,
sync::{CondVar, Mutex, SpinLock},
user_ptr::{UserSlicePtrReader, UserSlicePtrWriter},
Error,
};

module! {
Expand Down Expand Up @@ -51,23 +53,120 @@ module! {
},
}

struct RustFile;
const MAX_TOKENS: usize = 3;

impl FileOperations for RustFile {
struct SharedStateInner {
token_count: usize,
}

struct SharedState {
state_changed: CondVar,
inner: Mutex<SharedStateInner>,
}

impl SharedState {
fn try_new() -> KernelResult<Arc<Self>> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex -- this is an example of the ugliness brought by Pin I mentioned in one of the meetings. A bunch of unsafe that wouldn't need to be unsafe if we had better support for pinning. Two aspects are relevant here:

  1. Initialisation: CondVar::new and Mutex::new are unsafe because they require a call to init after the condvar/mutex are in their final pinned location.
  2. Projection: the compiler can't (or won't) verify for us that we don't move out of the fields of a pinned structure, so we need unsafe. (Though admittedly I don't explicitly say that state is pinned, mostly because it doesn't give me anything.)

It would be nice if we could avoid this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. When you say "we had better support for pinning", do you think this is something that Rust-for-Linux can improve itself, or do you think this requires better language level support?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we could choose to ignore Pin and assume that everything that is heap-allocated is pinned, but my opinion is that this isn't desirable because Pin is actually useful. (We sort of discussed this in the meeting.)

So I think we need better language support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshtriplett as our resident friend upstream, what's the best way for us to share these observations/desires with the team?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung -- since you commented on something else related to this project, I thought I'd ask you if you have any thoughts about improving pinning support. As it is today, it forces us (unreasonably in my opinion) to use too much unsafe code.

Copy link
Member

@bjorn3 bjorn3 Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Projection: the compiler can't (or won't) verify for us that we don't move out of the fields of a pinned structure, so we need unsafe. (Though admittedly I don't explicitly say that state is pinned, mostly because it doesn't give me anything.)

The pin-project crate has a proc macro to help with this.

Copy link

@RalfJung RalfJung Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that writing code with pin projections currently requires more unsafe code than it should. I am not sure what the best way forward for that is; the pin-project crate mentioned by @bjorn3 is a great start showing how far one can go even without any language support. (I did not carefully audit it though.)

The "new and then init" pattern seems mostly orthogonal to pinning; that pattern inherently requires exposing the user to an unsafe pre-init state. If you want to expose that state safely, you need to give it a separate type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you commented on something else related to this project, I thought I'd ask you

Sure, feel free to. :) I don't have a ton of time these days, but I am super excited about Rust having a realistic chance of being used in the upstream Linux kernel, so if I can help in small ways I'm more than happy to do that.

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<SharedState>,
}

impl FileOpener<Arc<SharedState>> for Token {
fn open(shared: &Arc<SharedState>) -> KernelResult<Self::Wrapper> {
Ok(Box::try_new(Self {
shared: shared.clone(),
})?)
}
}

impl FileOperations for Token {
type Wrapper = Box<Self>;

kernel::declare_file_operations!();
kernel::declare_file_operations!(read, write);

fn read(&self, _: &File, data: &mut UserSlicePtrWriter, offset: u64) -> KernelResult<usize> {
// 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<usize> {
{
let mut inner = self.shared.inner.lock();

fn open() -> KernelResult<Self::Wrapper> {
// 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 {
fn open(_ctx: &()) -> KernelResult<Self::Wrapper> {
println!("rust file was opened!");
Ok(Box::try_new(Self)?)
}
}

impl FileOperations for RustFile {
type Wrapper = Box<Self>;

kernel::declare_file_operations!();
}

struct RustExample {
message: String,
_chrdev: Pin<Box<chrdev::Registration<2>>>,
_dev: Pin<Box<miscdev::Registration>>,
_dev: Pin<Box<miscdev::Registration<Arc<SharedState>>>>,
}

impl KernelModule for RustExample {
Expand Down Expand Up @@ -146,9 +245,11 @@ impl KernelModule for RustExample {
chrdev_reg.as_mut().register::<RustFile>()?;
chrdev_reg.as_mut().register::<RustFile>()?;

let state = SharedState::try_new()?;

Ok(RustExample {
message: "on the heap!".to_owned(),
_dev: miscdev::Registration::new_pinned::<RustFile>(cstr!("rust_miscdev"), None, ())?,
_dev: miscdev::Registration::new_pinned::<Token>(cstr!("rust_miscdev"), None, state)?,
_chrdev: chrdev_reg,
})
}
Expand Down
22 changes: 20 additions & 2 deletions rust/kernel/chrdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ impl<const N: usize> Registration<{ N }> {
/// Registers a character device.
///
/// You may call this once per device type, up to `N` times.
pub fn register<T: file_operations::FileOperations>(self: Pin<&mut Self>) -> KernelResult {
pub fn register<T: file_operations::FileOpener<()>>(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() {
Expand Down Expand Up @@ -112,7 +112,12 @@ impl<const N: usize> Registration<{ N }> {
// SAFETY: Calling unsafe functions and manipulating `MaybeUninit`
// pointer.
unsafe {
bindings::cdev_init(cdev, file_operations::FileOperationsVtable::<T>::build());
bindings::cdev_init(
cdev,
// SAFETY: The adapter doesn't retrieve any state yet, so it's compatible with any
// registration.
file_operations::FileOperationsVtable::<Self, T>::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 {
Expand All @@ -124,6 +129,19 @@ impl<const N: usize> Registration<{ N }> {
}
}

impl<const N: usize> 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<const N: usize> Sync for Registration<{ N }> {}
Expand Down
3 changes: 3 additions & 0 deletions rust/kernel/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
57 changes: 45 additions & 12 deletions rust/kernel/file_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,13 @@ macro_rules! from_kernel_result {
}};
}

unsafe extern "C" fn open_callback<T: FileOperations>(
_inode: *mut bindings::inode,
unsafe extern "C" fn open_callback<A: FileOpenAdapter, T: FileOpener<A::Arg>>(
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)
}
Expand Down Expand Up @@ -198,11 +199,11 @@ unsafe extern "C" fn fsync_callback<T: FileOperations>(
}
}

pub(crate) struct FileOperationsVtable<T>(marker::PhantomData<T>);
pub(crate) struct FileOperationsVtable<A, T>(marker::PhantomData<A>, marker::PhantomData<T>);

impl<T: FileOperations> FileOperationsVtable<T> {
impl<A: FileOpenAdapter, T: FileOpener<A::Arg>> FileOperationsVtable<A, T> {
const VTABLE: bindings::file_operations = bindings::file_operations {
open: Some(open_callback::<T>),
open: Some(open_callback::<A, T>),
release: Some(release_callback::<T>),
read: if T::TO_USE.read {
Some(read_callback::<T>)
Expand Down Expand Up @@ -262,7 +263,11 @@ impl<T: FileOperations> FileOperationsVtable<T> {
};

/// 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
}
}
Expand Down Expand Up @@ -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 adapter 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<T: ?Sized>: FileOperations {
/// Creates a new instance of this file.
///
/// Corresponds to the `open` function pointer in `struct file_operations`.
fn open(context: &T) -> KernelResult<Self::Wrapper>;
}

/// Corresponds to the kernel's `struct file_operations`.
///
/// You implement this trait whenever you would create a `struct file_operations`.
Expand All @@ -414,11 +452,6 @@ pub trait FileOperations: Send + Sync + Sized {
/// The pointer type that will be used to hold ourselves.
type Wrapper: PointerWrapper<Self>;

/// Creates a new instance of this file.
///
/// Corresponds to the `open` function pointer in `struct file_operations`.
fn open() -> KernelResult<Self::Wrapper>;

/// 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
Expand Down
21 changes: 16 additions & 5 deletions rust/kernel/miscdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/misc_devices.html>

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;
Expand All @@ -19,7 +19,8 @@ pub struct Registration<T: Sync = ()> {
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,
}

Expand All @@ -39,7 +40,7 @@ impl<T: Sync> Registration<T> {
/// Registers a miscellaneous device.
///
/// Returns a pinned heap-allocated representation of the registration.
pub fn new_pinned<F: FileOperations>(
pub fn new_pinned<F: FileOpener<T>>(
name: CStr<'static>,
minor: Option<i32>,
context: T,
Expand All @@ -53,7 +54,7 @@ impl<T: Sync> Registration<T> {
///
/// 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<F: FileOperations>(
pub fn register<F: FileOpener<T>>(
self: Pin<&mut Self>,
name: CStr<'static>,
minor: Option<i32>,
Expand All @@ -65,7 +66,8 @@ impl<T: Sync> Registration<T> {
return Err(Error::EINVAL);
}

this.mdev.fops = FileOperationsVtable::<F>::build();
// SAFETY: The adapter is compatible with `misc_register`.
this.mdev.fops = unsafe { FileOperationsVtable::<Self, F>::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);

Expand All @@ -78,6 +80,15 @@ impl<T: Sync> Registration<T> {
}
}

impl<T: Sync> FileOpenAdapter for Registration<T> {
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`.
Expand Down