From 4ab613e48376e08127aeb5563efe779e5babd930 Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Sun, 9 Jan 2022 16:17:52 +0000 Subject: [PATCH] rust: only require context when registering a `miscdev` device This way it's possible to create a registration instance without having the context yet, and later provide the context when actually registering. This is useful for cases when the registration is contained in the context data, for example, when registering a new `miscdev` from `probe` functions. Signed-off-by: Wedson Almeida Filho --- rust/kernel/lib.rs | 1 + rust/kernel/miscdev.rs | 58 ++++++++++++++++++++++++++++++++---------- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 301ce5647274ab..a3420273e03e76 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -20,6 +20,7 @@ const_mut_refs, doc_cfg, generic_associated_types, + maybe_uninit_extra, ptr_metadata, receiver_trait, coerce_unsized, diff --git a/rust/kernel/miscdev.rs b/rust/kernel/miscdev.rs index b93e423bfb1e7e..f38ec203969d06 100644 --- a/rust/kernel/miscdev.rs +++ b/rust/kernel/miscdev.rs @@ -12,9 +12,13 @@ use crate::file_operations::{FileOpenAdapter, FileOpener, FileOperationsVtable}; use crate::{str::CStr, KernelModule, ThisModule}; use alloc::boxed::Box; use core::marker::{PhantomData, PhantomPinned}; -use core::pin::Pin; +use core::{mem::MaybeUninit, pin::Pin}; /// A registration of a miscellaneous device. +/// +/// # Invariants +/// +/// `Context` is always initialised when `registered` is `true`, and not initialised otherwise. pub struct Registration { registered: bool, mdev: bindings::miscdevice, @@ -22,19 +26,20 @@ pub struct Registration { /// Context initialised on construction and made available to all file instances on /// [`FileOpener::open`]. - pub context: T, + open_data: MaybeUninit, } impl Registration { /// Creates a new [`Registration`] but does not register it yet. /// /// It is allowed to move. - pub fn new(context: T) -> Self { + pub fn new() -> Self { + // INVARIANT: `registered` is `false` and `open_data` is not initialised. Self { registered: false, mdev: bindings::miscdevice::default(), _pin: PhantomPinned, - context, + open_data: MaybeUninit::uninit(), } } @@ -44,10 +49,10 @@ impl Registration { pub fn new_pinned>( name: &'static CStr, minor: Option, - context: T, + open_data: T, ) -> Result>> { - let mut r = Pin::from(Box::try_new(Self::new(context))?); - r.as_mut().register::(name, minor)?; + let mut r = Pin::from(Box::try_new(Self::new())?); + r.as_mut().register::(name, minor, open_data)?; Ok(r) } @@ -59,6 +64,7 @@ impl Registration { self: Pin<&mut Self>, name: &'static CStr, minor: Option, + open_data: T, ) -> Result { // SAFETY: We must ensure that we never move out of `this`. let this = unsafe { self.get_unchecked_mut() }; @@ -72,31 +78,51 @@ impl Registration { this.mdev.name = name.as_char_ptr(); this.mdev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32); + // We write to `open_data` here because as soon as `misc_register` succeeds, the file can be + // opened, so we need `open_data` configured ahead of time. + // + // INVARIANT: `registered` is set to `true`, but `open_data` is also initialised. + this.registered = true; + this.open_data.write(open_data); + let ret = unsafe { bindings::misc_register(&mut this.mdev) }; if ret < 0 { + // INVARIANT: `registered` is set back to `false` and the `open_data` is destructued. + this.registered = false; + // SAFETY: `open_data` was initialised a few lines above. + unsafe { this.open_data.assume_init_drop() }; return Err(Error::from_kernel_errno(ret)); } - this.registered = true; + Ok(()) } } +impl Default for Registration { + fn default() -> Self { + Self::new() + } +} + impl FileOpenAdapter for Registration { type Arg = T; unsafe fn convert(_inode: *mut bindings::inode, file: *mut bindings::file) -> *const Self::Arg { // SAFETY: the caller must guarantee that `file` is valid. let reg = crate::container_of!(unsafe { (*file).private_data }, Self, mdev); - unsafe { &(*reg).context } + + // SAFETY: This function is only called while the misc device is still registered, so the + // registration must be valid. Additionally, the type invariants guarantee that while the + // miscdev is registered, `open_data` is initialised. + unsafe { (*reg).open_data.as_ptr() } } } // 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`. +// is safe to pass `&Registration` to multiple threads because it offers no interior mutability. unsafe impl Sync for Registration {} -// SAFETY: All functions work from any thread. So as long as the `Registration::context` is +// SAFETY: All functions work from any thread. So as long as the `Registration::open_data` is // `Send`, so is `Registration`. `T` needs to be `Sync` because it's a requirement of // `Registration`. unsafe impl Send for Registration {} @@ -105,7 +131,13 @@ impl Drop for Registration { /// Removes the registration from the kernel if it has completed successfully before. fn drop(&mut self) { if self.registered { - unsafe { bindings::misc_deregister(&mut self.mdev) } + // SAFETY: `registered` being `true` indicates that a previous call to `misc_register` + // succeeded. + unsafe { bindings::misc_deregister(&mut self.mdev) }; + + // SAFETY: The type invariant guarantees that `open_data` is initialised when + // `registered` is `true`. + unsafe { self.open_data.assume_init_drop() }; } } }