Currently, the design of MiscDeviceRegistration is focused on creating
and registering a misc device directly from a module and hence does not
support using misc device from a device driver.
However, it is important for the design of the misc device abstraction to
take the driver model into account.
Hence, consider the use-case of using misc device from a device driver;
let's go through the design motivation bottom-up:
Ideally, we want to be able to access (bus) device resources (such as I/O
memory) from misc device callbacks (such as open() or ioctl()) without
additional overhead, i.e. without having to go through
Devres::try_access(), which implies an atomic check and an RCU read side
critical section. Instead, we want to be able to use Devres::access(),
which does not have any overhead, which requires a &Device<Bound> to
prove that we can directly access the device resource.
Given that all misc device callbacks are synchronized against
misc_deregister(), we can prove that the misc device's parent device is
bound iff we guarantee that the misc device's registration won't
out-live the parent device's unbind.
This can easily be proven by using devres for the misc device's
registration object itself.
Since this is only applicable for the device driver use-case, abstract
the actual registration instance with a Rust enum, which is either a
"raw" registration or a "managed" registration.
In order to avoid any penalties from a managed registration, structurally
separate the registration's private data from the "raw" misc device
registration (which either stays "raw" or becomes "managed") depending
on whether a parent device is supplied.
The advantage of this solution is that it is entirely transparent to the
user -- no separate structures or functions for whether the abstraction
is used directly from a module or from a device driver; instead
MiscDeviceRegistration::register() gets an optional parent argument.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
rust/kernel/miscdevice.rs | 178 ++++++++++++++++++++++++-------
samples/rust/rust_misc_device.rs | 9 +-
2 files changed, 143 insertions(+), 44 deletions(-)
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 1b5ec13868e2..6801fe72a8a6 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -10,16 +10,17 @@
use crate::{
bindings, container_of,
- device::Device,
+ device::{Bound, Device},
+ devres::Devres,
error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
ffi::{c_int, c_long, c_uint, c_ulong},
fs::File,
prelude::*,
seq_file::SeqFile,
str::CStr,
- types::{ForeignOwnable, Opaque},
+ types::{ARef, ForeignOwnable, Opaque},
};
-use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
+use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin, ptr::NonNull};
/// Options for creating a misc device.
#[derive(Copy, Clone)]
@@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
}
}
-/// A registration of a miscdevice.
-///
/// # Invariants
///
-/// `inner` is a registered misc device.
+/// - `inner` is a registered misc device,
+/// - `data` is valid for the entire lifetime of `Self`.
#[repr(C)]
#[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T: MiscDevice> {
+struct RawDeviceRegistration<T: MiscDevice> {
#[pin]
inner: Opaque<bindings::miscdevice>,
- #[pin]
- data: Opaque<T::RegistrationData>,
+ data: NonNull<T::RegistrationData>,
_t: PhantomData<T>,
}
-// SAFETY:
-// - It is allowed to call `misc_deregister` on a different thread from where you called
-// `misc_register`.
-// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
-unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
-
-// SAFETY:
-// - All `&self` methods on this type are written to ensure that it is safe to call them in
-// parallel.
-// - `MiscDevice::RegistrationData` is always `Sync`.
-unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
-
-impl<T: MiscDevice> MiscDeviceRegistration<T> {
- /// Register a misc device.
- pub fn register(
+impl<T: MiscDevice> RawDeviceRegistration<T> {
+ fn new<'a>(
opts: MiscDeviceOptions,
- data: impl PinInit<T::RegistrationData, Error>,
- ) -> impl PinInit<Self, Error> {
+ parent: Option<&'a Device<Bound>>,
+ data: &'a T::RegistrationData,
+ ) -> impl PinInit<Self, Error> + 'a
+ where
+ T: 'a,
+ {
try_pin_init!(Self {
- data <- Opaque::pin_init(data),
+ // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
+ // is guaranteed to be valid for the entire lifetime of `Self`.
+ data: NonNull::from(data),
inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
+ let mut value = opts.into_raw::<T>();
+
+ if let Some(parent) = parent {
+ // The device core code will take care to take a reference of `parent` in
+ // `device_add()` called by `misc_register()`.
+ value.parent = parent.as_raw();
+ }
+
// SAFETY: The initializer can write to the provided `slot`.
- unsafe { slot.write(opts.into_raw::<T>()) };
+ unsafe { slot.write(value) };
// SAFETY:
// * We just wrote the misc device options to the slot. The miscdevice will
@@ -94,12 +94,12 @@ pub fn register(
}
/// Returns a raw pointer to the misc device.
- pub fn as_raw(&self) -> *mut bindings::miscdevice {
+ fn as_raw(&self) -> *mut bindings::miscdevice {
self.inner.get()
}
/// Access the `this_device` field.
- pub fn device(&self) -> &Device {
+ fn device(&self) -> &Device {
// SAFETY: This can only be called after a successful register(), which always
// initialises `this_device` with a valid device. Furthermore, the signature of this
// function tells the borrow-checker that the `&Device` reference must not outlive the
@@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
unsafe { Device::as_ref((*self.as_raw()).this_device) }
}
+ fn data(&self) -> &T::RegistrationData {
+ // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
+ // `Self`.
+ unsafe { self.data.as_ref() }
+ }
+}
+
+#[pinned_drop]
+impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY: We know that the device is registered by the type invariants.
+ unsafe { bindings::misc_deregister(self.inner.get()) };
+ }
+}
+
+#[expect(dead_code)]
+enum DeviceRegistrationInner<T: MiscDevice> {
+ Raw(Pin<KBox<RawDeviceRegistration<T>>>),
+ Managed(Devres<RawDeviceRegistration<T>>),
+}
+
+/// A registration of a miscdevice.
+#[pin_data(PinnedDrop)]
+pub struct MiscDeviceRegistration<T: MiscDevice> {
+ inner: DeviceRegistrationInner<T>,
+ #[pin]
+ data: Opaque<T::RegistrationData>,
+ this_device: ARef<Device>,
+ _t: PhantomData<T>,
+}
+
+// SAFETY:
+// - It is allowed to call `misc_deregister` on a different thread from where you called
+// `misc_register`.
+// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
+unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
+
+// SAFETY:
+// - All `&self` methods on this type are written to ensure that it is safe to call them in
+// parallel.
+// - `MiscDevice::RegistrationData` is always `Sync`.
+unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
+
+impl<T: MiscDevice> MiscDeviceRegistration<T> {
+ /// Register a misc device.
+ pub fn register<'a>(
+ opts: MiscDeviceOptions,
+ data: impl PinInit<T::RegistrationData, Error> + 'a,
+ parent: Option<&'a Device<Bound>>,
+ ) -> impl PinInit<Self, Error> + 'a
+ where
+ T: 'a,
+ {
+ let mut dev: Option<ARef<Device>> = None;
+
+ try_pin_init!(&this in Self {
+ data <- Opaque::pin_init(data),
+ // TODO: make `inner` in-place when enums get supported by pin-init.
+ //
+ // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
+ inner: {
+ // SAFETY:
+ // - `this` is a valid pointer to `Self`,
+ // - `data` was properly initialized above.
+ let data = unsafe { &*(*this.as_ptr()).data.get() };
+
+ let raw = RawDeviceRegistration::new(opts, parent, data);
+
+ // FIXME: Work around a bug in rustc, to prevent the following warning:
+ //
+ // "warning: value captured by `dev` is never read."
+ //
+ // Link: https://github.com/rust-lang/rust/issues/141615
+ let _ = dev;
+
+ if let Some(parent) = parent {
+ let devres = Devres::new(parent, raw, GFP_KERNEL)?;
+
+ dev = Some(devres.access(parent)?.device().into());
+ DeviceRegistrationInner::Managed(devres)
+ } else {
+ let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
+
+ dev = Some(boxed.device().into());
+ DeviceRegistrationInner::Raw(boxed)
+ }
+ },
+ // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
+ // case.
+ this_device: {
+ // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
+ unsafe { dev.unwrap_unchecked() }
+ },
+ _t: PhantomData,
+ })
+ }
+
+ /// Access the `this_device` field.
+ pub fn device(&self) -> &Device {
+ &self.this_device
+ }
+
/// Access the additional data stored in this registration.
pub fn data(&self) -> &T::RegistrationData {
// SAFETY:
@@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
#[pinned_drop]
impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
fn drop(self: Pin<&mut Self>) {
- // SAFETY: We know that the device is registered by the type invariants.
- unsafe { bindings::misc_deregister(self.inner.get()) };
-
// SAFETY: `self.data` is valid for dropping.
unsafe { core::ptr::drop_in_place(self.data.get()) };
}
@@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
/// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
/// If no additional data is required than the unit type `()` should be used.
///
- /// This data can be accessed in [`MiscDevice::open()`] using
- /// [`MiscDeviceRegistration::data()`].
+ /// This data can be accessed in [`MiscDevice::open()`].
type RegistrationData: Sync;
/// Called when the misc device is opened.
///
/// The returned pointer will be stored as the private data for the file.
- fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
+ fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
/// Called when the misc device is released.
fn release(device: Self::Ptr, _file: &File) {
@@ -217,17 +315,17 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
// SAFETY:
// * `misc_open()` ensures that the `struct miscdevice` can't be unregistered and freed
// during this call to `fops_open`.
- // * The `misc_ptr` always points to the `inner` field of a `MiscDeviceRegistration<T>`.
- // * The `MiscDeviceRegistration<T>` is valid until the `struct miscdevice` was
+ // * The `misc_ptr` always points to the `inner` field of a `RawDeviceRegistration<T>`.
+ // * The `RawDeviceRegistration<T>` is valid until the `struct miscdevice` was
// unregistered.
- let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<T>, inner) };
+ let registration = unsafe { &*container_of!(misc_ptr, RawDeviceRegistration<T>, inner) };
// SAFETY:
// * This underlying file is valid for (much longer than) the duration of `T::open`.
// * There is no active fdget_pos region on the file on this thread.
let file = unsafe { File::from_raw_file(raw_file) };
- let ptr = match T::open(file, registration) {
+ let ptr = match T::open(file, registration.device(), registration.data()) {
Ok(ptr) => ptr,
Err(err) => return err.to_errno(),
};
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 843442b0ea1d..b60fd063afa8 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -198,7 +198,8 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
try_pin_init!(Self {
_miscdev <- MiscDeviceRegistration::register(
options,
- Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL)
+ Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
+ None,
),
})
}
@@ -222,15 +223,15 @@ impl MiscDevice for RustMiscDevice {
type RegistrationData = Arc<Mutex<Inner>>;
- fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
- let dev = ARef::from(misc.device());
+ fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) -> Result<Pin<KBox<Self>>> {
+ let dev = ARef::from(misc);
dev_info!(dev, "Opening Rust Misc Device Sample\n");
KBox::try_pin_init(
try_pin_init! {
RustMiscDevice {
- shared: misc.data().clone(),
+ shared: data.clone(),
unique <- new_mutex!(Inner { value: 0_i32 }),
dev: dev,
}
--
2.49.0On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> }
> }
>
> -/// A registration of a miscdevice.
> -///
> /// # Invariants
> ///
> -/// `inner` is a registered misc device.
> +/// - `inner` is a registered misc device,
> +/// - `data` is valid for the entire lifetime of `Self`.
> #[repr(C)]
> #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T: MiscDevice> {
> +struct RawDeviceRegistration<T: MiscDevice> {
> #[pin]
> inner: Opaque<bindings::miscdevice>,
> - #[pin]
> - data: Opaque<T::RegistrationData>,
> + data: NonNull<T::RegistrationData>,
> _t: PhantomData<T>,
You shouldn't need the `PhantomData` here.
Also, do we need to ask for `T: MiscDevice` here? Could we instead have
just `T` and then below you write
`RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)
> }
>
> -// SAFETY:
> -// - It is allowed to call `misc_deregister` on a different thread from where you called
> -// `misc_register`.
> -// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> -unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> -
> -// SAFETY:
> -// - All `&self` methods on this type are written to ensure that it is safe to call them in
> -// parallel.
> -// - `MiscDevice::RegistrationData` is always `Sync`.
> -unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> -
> -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> - /// Register a misc device.
> - pub fn register(
> +impl<T: MiscDevice> RawDeviceRegistration<T> {
> + fn new<'a>(
> opts: MiscDeviceOptions,
> - data: impl PinInit<T::RegistrationData, Error>,
> - ) -> impl PinInit<Self, Error> {
> + parent: Option<&'a Device<Bound>>,
> + data: &'a T::RegistrationData,
> + ) -> impl PinInit<Self, Error> + 'a
> + where
> + T: 'a,
> + {
> try_pin_init!(Self {
> - data <- Opaque::pin_init(data),
> + // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> + // is guaranteed to be valid for the entire lifetime of `Self`.
> + data: NonNull::from(data),
Both the argument in the INVARIANT comment and way this works are a bit
flawed. Instead, I'd recommend directly taking the `NonNull` as a
parameter. Yes the function will need to be `unsafe`, but the lifetime
that you're creating below only lives for `'a`, but the object might
live much longer. You might still be fine, but I'd just recommend
staying in raw pointer land (or in this case `NonNull`).
> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> + let mut value = opts.into_raw::<T>();
> +
> + if let Some(parent) = parent {
> + // The device core code will take care to take a reference of `parent` in
Just a question: with "take a reference of" you mean that it will
increment the refcount?
> + // `device_add()` called by `misc_register()`.
> + value.parent = parent.as_raw();
> + }
> +
> // SAFETY: The initializer can write to the provided `slot`.
> - unsafe { slot.write(opts.into_raw::<T>()) };
> + unsafe { slot.write(value) };
>
> // SAFETY:
> // * We just wrote the misc device options to the slot. The miscdevice will
> @@ -94,12 +94,12 @@ pub fn register(
> }
>
> /// Returns a raw pointer to the misc device.
> - pub fn as_raw(&self) -> *mut bindings::miscdevice {
> + fn as_raw(&self) -> *mut bindings::miscdevice {
> self.inner.get()
> }
>
> /// Access the `this_device` field.
> - pub fn device(&self) -> &Device {
> + fn device(&self) -> &Device {
> // SAFETY: This can only be called after a successful register(), which always
> // initialises `this_device` with a valid device. Furthermore, the signature of this
> // function tells the borrow-checker that the `&Device` reference must not outlive the
> @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
> unsafe { Device::as_ref((*self.as_raw()).this_device) }
> }
>
> + fn data(&self) -> &T::RegistrationData {
> + // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> + // `Self`.
> + unsafe { self.data.as_ref() }
> + }
> +}
> +
> +#[pinned_drop]
> +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: We know that the device is registered by the type invariants.
> + unsafe { bindings::misc_deregister(self.inner.get()) };
> + }
> +}
> +
> +#[expect(dead_code)]
> +enum DeviceRegistrationInner<T: MiscDevice> {
> + Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> + Managed(Devres<RawDeviceRegistration<T>>),
These two names could be shortened (`DeviceRegistrationInner` and
`RawDeviceRegistration`) as they are only implementation details of this
file. How about `InnerRegistration` and `RawRegistration`? Or maybe
something even shorter.
> +}
> +
> +/// A registration of a miscdevice.
> +#[pin_data(PinnedDrop)]
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> + inner: DeviceRegistrationInner<T>,
> + #[pin]
> + data: Opaque<T::RegistrationData>,
Why is it necessary to store `data` inside of `Opaque`?
> + this_device: ARef<Device>,
> + _t: PhantomData<T>,
> +}
> +
> +// SAFETY:
> +// - It is allowed to call `misc_deregister` on a different thread from where you called
> +// `misc_register`.
> +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> +
> +// SAFETY:
> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
> +// parallel.
> +// - `MiscDevice::RegistrationData` is always `Sync`.
> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> +
> +impl<T: MiscDevice> MiscDeviceRegistration<T> {
> + /// Register a misc device.
> + pub fn register<'a>(
> + opts: MiscDeviceOptions,
> + data: impl PinInit<T::RegistrationData, Error> + 'a,
> + parent: Option<&'a Device<Bound>>,
> + ) -> impl PinInit<Self, Error> + 'a
> + where
> + T: 'a,
> + {
> + let mut dev: Option<ARef<Device>> = None;
> +
> + try_pin_init!(&this in Self {
> + data <- Opaque::pin_init(data),
> + // TODO: make `inner` in-place when enums get supported by pin-init.
> + //
> + // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
You might want to add that this would avoid the extra allocation in
`DeviceRegistrationInner`.
> + inner: {
> + // SAFETY:
> + // - `this` is a valid pointer to `Self`,
> + // - `data` was properly initialized above.
> + let data = unsafe { &*(*this.as_ptr()).data.get() };
As mentioned above, this creates a reference that is valid for this
*block*. So its lifetime will end after the `},` and before
`this_device` is initialized.
It *might* be ok to turn it back into a raw pointer in
`RawDeviceRegistration::new`, but I wouldn't bet on it.
> +
> + let raw = RawDeviceRegistration::new(opts, parent, data);
> +
> + // FIXME: Work around a bug in rustc, to prevent the following warning:
> + //
> + // "warning: value captured by `dev` is never read."
> + //
> + // Link: https://github.com/rust-lang/rust/issues/141615
Note that the bug is that the compiler complains about the wrong span.
The original value of `dev` is `None` and that value is never used, so
the warning is justified. So this `let _ = dev;` still needs to stay
until `pin-init` supports accessing previously initialized fields (now
I'm pretty certain that I will implement that soon).
> + let _ = dev;
> +
> + if let Some(parent) = parent {
> + let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> +
> + dev = Some(devres.access(parent)?.device().into());
> + DeviceRegistrationInner::Managed(devres)
> + } else {
> + let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> +
> + dev = Some(boxed.device().into());
> + DeviceRegistrationInner::Raw(boxed)
> + }
> + },
> + // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> + // case.
> + this_device: {
> + // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> + unsafe { dev.unwrap_unchecked() }
> + },
No need for the extra block, just do:
// Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
// case.
// SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
this_device: unsafe { dev.unwrap_unchecked() },
I'm also pretty sure that the compiler would optimize `.take().unwrap()`
and also this is only executed once per `MiscDeviceRegistration`, so
even if it isn't it wouldn't really matter. So I'd prefer if we don't
use `unsafe` here even if it is painfully obvious (if I'm fast enough
with implementing, you can rebase on top before you merge and then this
will be gone anyways :)
> + _t: PhantomData,
> + })
> + }
> +
> + /// Access the `this_device` field.
> + pub fn device(&self) -> &Device {
> + &self.this_device
> + }
> +
> /// Access the additional data stored in this registration.
> pub fn data(&self) -> &T::RegistrationData {
> // SAFETY:
> @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
> #[pinned_drop]
> impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> fn drop(self: Pin<&mut Self>) {
> - // SAFETY: We know that the device is registered by the type invariants.
> - unsafe { bindings::misc_deregister(self.inner.get()) };
> -
> // SAFETY: `self.data` is valid for dropping.
> unsafe { core::ptr::drop_in_place(self.data.get()) };
> }
> @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
> /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> /// If no additional data is required than the unit type `()` should be used.
> ///
> - /// This data can be accessed in [`MiscDevice::open()`] using
> - /// [`MiscDeviceRegistration::data()`].
> + /// This data can be accessed in [`MiscDevice::open()`].
> type RegistrationData: Sync;
>
> /// Called when the misc device is opened.
> ///
> /// The returned pointer will be stored as the private data for the file.
> - fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> + fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
What is the reason that these parameters begin with `_`? In a trait
function without a body, the compiler shouldn't war about unused
parameters.
---
Cheers,
Benno
>
> /// Called when the misc device is released.
> fn release(device: Self::Ptr, _file: &File) {
On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> > @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> > }
> > }
> >
> > -/// A registration of a miscdevice.
> > -///
> > /// # Invariants
> > ///
> > -/// `inner` is a registered misc device.
> > +/// - `inner` is a registered misc device,
> > +/// - `data` is valid for the entire lifetime of `Self`.
> > #[repr(C)]
> > #[pin_data(PinnedDrop)]
> > -pub struct MiscDeviceRegistration<T: MiscDevice> {
> > +struct RawDeviceRegistration<T: MiscDevice> {
> > #[pin]
> > inner: Opaque<bindings::miscdevice>,
> > - #[pin]
> > - data: Opaque<T::RegistrationData>,
> > + data: NonNull<T::RegistrationData>,
> > _t: PhantomData<T>,
>
> You shouldn't need the `PhantomData` here.
>
> Also, do we need to ask for `T: MiscDevice` here? Could we instead have
> just `T` and then below you write
> `RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
> needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)
Sure, is there any advantage? Your proposal seems more complicated at a first
glance.
> > }
> >
> > -// SAFETY:
> > -// - It is allowed to call `misc_deregister` on a different thread from where you called
> > -// `misc_register`.
> > -// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> > -unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> > -
> > -// SAFETY:
> > -// - All `&self` methods on this type are written to ensure that it is safe to call them in
> > -// parallel.
> > -// - `MiscDevice::RegistrationData` is always `Sync`.
> > -unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> > -
> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> > - /// Register a misc device.
> > - pub fn register(
> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
> > + fn new<'a>(
> > opts: MiscDeviceOptions,
> > - data: impl PinInit<T::RegistrationData, Error>,
> > - ) -> impl PinInit<Self, Error> {
> > + parent: Option<&'a Device<Bound>>,
> > + data: &'a T::RegistrationData,
> > + ) -> impl PinInit<Self, Error> + 'a
> > + where
> > + T: 'a,
> > + {
> > try_pin_init!(Self {
> > - data <- Opaque::pin_init(data),
> > + // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> > + // is guaranteed to be valid for the entire lifetime of `Self`.
> > + data: NonNull::from(data),
>
> Both the argument in the INVARIANT comment and way this works are a bit
> flawed.
Why is the argument flawed? Let's say we go with your proposal below, what would
the safety requirement for RawDeviceRegistration::new and the invariant of
RawDeviceRegistration look like? Wouldn't it be the exact same argument?
> Instead, I'd recommend directly taking the `NonNull` as a
> parameter. Yes the function will need to be `unsafe`, but the lifetime
> that you're creating below only lives for `'a`, but the object might
> live much longer. You might still be fine, but I'd just recommend
> staying in raw pointer land (or in this case `NonNull`).
>
> > inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> > + let mut value = opts.into_raw::<T>();
> > +
> > + if let Some(parent) = parent {
> > + // The device core code will take care to take a reference of `parent` in
>
> Just a question: with "take a reference of" you mean that it will
> increment the refcount?
Exactly -- will change it to "increment the refcount" for clarity.
>
> > + // `device_add()` called by `misc_register()`.
> > + value.parent = parent.as_raw();
> > + }
> > +
> > // SAFETY: The initializer can write to the provided `slot`.
> > - unsafe { slot.write(opts.into_raw::<T>()) };
> > + unsafe { slot.write(value) };
> >
> > // SAFETY:
> > // * We just wrote the misc device options to the slot. The miscdevice will
> > @@ -94,12 +94,12 @@ pub fn register(
> > }
> >
> > /// Returns a raw pointer to the misc device.
> > - pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > + fn as_raw(&self) -> *mut bindings::miscdevice {
> > self.inner.get()
> > }
> >
> > /// Access the `this_device` field.
> > - pub fn device(&self) -> &Device {
> > + fn device(&self) -> &Device {
> > // SAFETY: This can only be called after a successful register(), which always
> > // initialises `this_device` with a valid device. Furthermore, the signature of this
> > // function tells the borrow-checker that the `&Device` reference must not outlive the
> > @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
> > unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > }
> >
> > + fn data(&self) -> &T::RegistrationData {
> > + // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> > + // `Self`.
> > + unsafe { self.data.as_ref() }
> > + }
> > +}
> > +
> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> > + fn drop(self: Pin<&mut Self>) {
> > + // SAFETY: We know that the device is registered by the type invariants.
> > + unsafe { bindings::misc_deregister(self.inner.get()) };
> > + }
> > +}
> > +
> > +#[expect(dead_code)]
> > +enum DeviceRegistrationInner<T: MiscDevice> {
> > + Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> > + Managed(Devres<RawDeviceRegistration<T>>),
>
> These two names could be shortened (`DeviceRegistrationInner` and
> `RawDeviceRegistration`) as they are only implementation details of this
> file. How about `InnerRegistration` and `RawRegistration`? Or maybe
> something even shorter.
There's a reason why I keep them something with "DeviceRegistration" everywhere,
which is to make it clear that it's both a device instance *and* a registration,
which is actually rather uncommon and caused by the fact that device creation
and registration needs to be done under the misc_mtx in misc_register().
This is also the reason for those data structures to be a bit complicated; it
would be much simpler if device creation and registration would be independent
things.
> > +}
> > +
> > +/// A registration of a miscdevice.
> > +#[pin_data(PinnedDrop)]
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> > + inner: DeviceRegistrationInner<T>,
> > + #[pin]
> > + data: Opaque<T::RegistrationData>,
>
> Why is it necessary to store `data` inside of `Opaque`?
It was UnsafePinned before, but Alice proposed to go with Opaque for the
meantime. Anyways, this is not introduced by this patch, it comes from
Christians patch adding T::RegistrationData.
>
> > + this_device: ARef<Device>,
> > + _t: PhantomData<T>,
> > +}
> > +
> > +// SAFETY:
> > +// - It is allowed to call `misc_deregister` on a different thread from where you called
> > +// `misc_register`.
> > +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> > +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> > +
> > +// SAFETY:
> > +// - All `&self` methods on this type are written to ensure that it is safe to call them in
> > +// parallel.
> > +// - `MiscDevice::RegistrationData` is always `Sync`.
> > +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> > +
> > +impl<T: MiscDevice> MiscDeviceRegistration<T> {
> > + /// Register a misc device.
> > + pub fn register<'a>(
> > + opts: MiscDeviceOptions,
> > + data: impl PinInit<T::RegistrationData, Error> + 'a,
> > + parent: Option<&'a Device<Bound>>,
> > + ) -> impl PinInit<Self, Error> + 'a
> > + where
> > + T: 'a,
> > + {
> > + let mut dev: Option<ARef<Device>> = None;
> > +
> > + try_pin_init!(&this in Self {
> > + data <- Opaque::pin_init(data),
> > + // TODO: make `inner` in-place when enums get supported by pin-init.
> > + //
> > + // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
>
> You might want to add that this would avoid the extra allocation in
> `DeviceRegistrationInner`.
Sure, will do.
> > + inner: {
> > + // SAFETY:
> > + // - `this` is a valid pointer to `Self`,
> > + // - `data` was properly initialized above.
> > + let data = unsafe { &*(*this.as_ptr()).data.get() };
>
> As mentioned above, this creates a reference that is valid for this
> *block*. So its lifetime will end after the `},` and before
> `this_device` is initialized.
>
> It *might* be ok to turn it back into a raw pointer in
> `RawDeviceRegistration::new`, but I wouldn't bet on it.
Why? The reference is still valid in RawDeviceRegistration::new, no?
> > +
> > + let raw = RawDeviceRegistration::new(opts, parent, data);
> > +
> > + // FIXME: Work around a bug in rustc, to prevent the following warning:
> > + //
> > + // "warning: value captured by `dev` is never read."
> > + //
> > + // Link: https://github.com/rust-lang/rust/issues/141615
>
> Note that the bug is that the compiler complains about the wrong span.
> The original value of `dev` is `None` and that value is never used, so
> the warning is justified. So this `let _ = dev;` still needs to stay
> until `pin-init` supports accessing previously initialized fields (now
> I'm pretty certain that I will implement that soon).
Do you want to propose an alternative comment about this?
> > + let _ = dev;
> > +
> > + if let Some(parent) = parent {
> > + let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> > +
> > + dev = Some(devres.access(parent)?.device().into());
> > + DeviceRegistrationInner::Managed(devres)
> > + } else {
> > + let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> > +
> > + dev = Some(boxed.device().into());
> > + DeviceRegistrationInner::Raw(boxed)
> > + }
> > + },
> > + // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> > + // case.
> > + this_device: {
> > + // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> > + unsafe { dev.unwrap_unchecked() }
> > + },
>
> No need for the extra block, just do:
>
> // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> // case.
> // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> this_device: unsafe { dev.unwrap_unchecked() },
Yes, I know, but I found the above a bit cleaner -- I don't mind changing it
though.
> I'm also pretty sure that the compiler would optimize `.take().unwrap()`
> and also this is only executed once per `MiscDeviceRegistration`, so
> even if it isn't it wouldn't really matter. So I'd prefer if we don't
> use `unsafe` here even if it is painfully obvious (if I'm fast enough
> with implementing, you can rebase on top before you merge and then this
> will be gone anyways :)
Sounds good! :)
But I think that unsafe is better than unwrap() in such cases; unsafe requires
us to explain why it's OK to do it, which makes it less likely to create bugs.
(Just recently I wrote some code, hit the need for unsafe and, while writing up
the safety comment, I had to explain to myself, why the way I was about to
implement this was pretty broken.)
unwrap() on the other hand, doesn't require any explanation, but panics the
kernel in the worst case.
> > + _t: PhantomData,
> > + })
> > + }
> > +
> > + /// Access the `this_device` field.
> > + pub fn device(&self) -> &Device {
> > + &self.this_device
> > + }
> > +
> > /// Access the additional data stored in this registration.
> > pub fn data(&self) -> &T::RegistrationData {
> > // SAFETY:
> > @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
> > #[pinned_drop]
> > impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> > fn drop(self: Pin<&mut Self>) {
> > - // SAFETY: We know that the device is registered by the type invariants.
> > - unsafe { bindings::misc_deregister(self.inner.get()) };
> > -
> > // SAFETY: `self.data` is valid for dropping.
> > unsafe { core::ptr::drop_in_place(self.data.get()) };
> > }
> > @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
> > /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> > /// If no additional data is required than the unit type `()` should be used.
> > ///
> > - /// This data can be accessed in [`MiscDevice::open()`] using
> > - /// [`MiscDeviceRegistration::data()`].
> > + /// This data can be accessed in [`MiscDevice::open()`].
> > type RegistrationData: Sync;
> >
> > /// Called when the misc device is opened.
> > ///
> > /// The returned pointer will be stored as the private data for the file.
> > - fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> > + fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
>
> What is the reason that these parameters begin with `_`? In a trait
> function without a body, the compiler shouldn't war about unused
> parameters.
No idea, I just tried to be complient with the existing style of the file. :)
On Sat May 31, 2025 at 12:17 AM CEST, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> > @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>> > }
>> > }
>> >
>> > -/// A registration of a miscdevice.
>> > -///
>> > /// # Invariants
>> > ///
>> > -/// `inner` is a registered misc device.
>> > +/// - `inner` is a registered misc device,
>> > +/// - `data` is valid for the entire lifetime of `Self`.
>> > #[repr(C)]
>> > #[pin_data(PinnedDrop)]
>> > -pub struct MiscDeviceRegistration<T: MiscDevice> {
>> > +struct RawDeviceRegistration<T: MiscDevice> {
>> > #[pin]
>> > inner: Opaque<bindings::miscdevice>,
>> > - #[pin]
>> > - data: Opaque<T::RegistrationData>,
>> > + data: NonNull<T::RegistrationData>,
>> > _t: PhantomData<T>,
>>
>> You shouldn't need the `PhantomData` here.
>>
>> Also, do we need to ask for `T: MiscDevice` here? Could we instead have
>> just `T` and then below you write
>> `RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
>> needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)
>
> Sure, is there any advantage? Your proposal seems more complicated at a first
> glance.
It would make `RawDeviceRegistration` simpler, but maybe it's not worth
it.
>> > }
>> >
>> > -// SAFETY:
>> > -// - It is allowed to call `misc_deregister` on a different thread from where you called
>> > -// `misc_register`.
>> > -// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
>> > -unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
>> > -
>> > -// SAFETY:
>> > -// - All `&self` methods on this type are written to ensure that it is safe to call them in
>> > -// parallel.
>> > -// - `MiscDevice::RegistrationData` is always `Sync`.
>> > -unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>> > -
>> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
>> > - /// Register a misc device.
>> > - pub fn register(
>> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
>> > + fn new<'a>(
>> > opts: MiscDeviceOptions,
>> > - data: impl PinInit<T::RegistrationData, Error>,
>> > - ) -> impl PinInit<Self, Error> {
>> > + parent: Option<&'a Device<Bound>>,
>> > + data: &'a T::RegistrationData,
>> > + ) -> impl PinInit<Self, Error> + 'a
>> > + where
>> > + T: 'a,
>> > + {
>> > try_pin_init!(Self {
>> > - data <- Opaque::pin_init(data),
>> > + // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
>> > + // is guaranteed to be valid for the entire lifetime of `Self`.
>> > + data: NonNull::from(data),
>>
>> Both the argument in the INVARIANT comment and way this works are a bit
>> flawed.
>
> Why is the argument flawed? Let's say we go with your proposal below, what would
> the safety requirement for RawDeviceRegistration::new and the invariant of
> RawDeviceRegistration look like? Wouldn't it be the exact same argument?
So what I write below it not really true. But the argument relies on the
fact that `data` is pointing to a value that will stay alive for the
duration of the existence of `self`. That should be a safety requirement
of `new` (even if we take a reference as an argument).
>> Instead, I'd recommend directly taking the `NonNull` as a
>> parameter. Yes the function will need to be `unsafe`, but the lifetime
>> that you're creating below only lives for `'a`, but the object might
>> live much longer. You might still be fine, but I'd just recommend
>> staying in raw pointer land (or in this case `NonNull`).
>> > @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
>> > unsafe { Device::as_ref((*self.as_raw()).this_device) }
>> > }
>> >
>> > + fn data(&self) -> &T::RegistrationData {
>> > + // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
>> > + // `Self`.
>> > + unsafe { self.data.as_ref() }
>> > + }
>> > +}
>> > +
>> > +#[pinned_drop]
>> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
>> > + fn drop(self: Pin<&mut Self>) {
>> > + // SAFETY: We know that the device is registered by the type invariants.
>> > + unsafe { bindings::misc_deregister(self.inner.get()) };
>> > + }
>> > +}
>> > +
>> > +#[expect(dead_code)]
>> > +enum DeviceRegistrationInner<T: MiscDevice> {
>> > + Raw(Pin<KBox<RawDeviceRegistration<T>>>),
>> > + Managed(Devres<RawDeviceRegistration<T>>),
>>
>> These two names could be shortened (`DeviceRegistrationInner` and
>> `RawDeviceRegistration`) as they are only implementation details of this
>> file. How about `InnerRegistration` and `RawRegistration`? Or maybe
>> something even shorter.
>
> There's a reason why I keep them something with "DeviceRegistration" everywhere,
> which is to make it clear that it's both a device instance *and* a registration,
> which is actually rather uncommon and caused by the fact that device creation
> and registration needs to be done under the misc_mtx in misc_register().
>
> This is also the reason for those data structures to be a bit complicated; it
> would be much simpler if device creation and registration would be independent
> things.
Then keep the longer names.
>> > +}
>> > +
>> > +/// A registration of a miscdevice.
>> > +#[pin_data(PinnedDrop)]
>> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
>> > + inner: DeviceRegistrationInner<T>,
>> > + #[pin]
>> > + data: Opaque<T::RegistrationData>,
>>
>> Why is it necessary to store `data` inside of `Opaque`?
>
> It was UnsafePinned before, but Alice proposed to go with Opaque for the
> meantime. Anyways, this is not introduced by this patch, it comes from
> Christians patch adding T::RegistrationData.
Ah then I'll re-read that discussion.
>> > + this_device: ARef<Device>,
>> > + _t: PhantomData<T>,
>> > +}
>> > + inner: {
>> > + // SAFETY:
>> > + // - `this` is a valid pointer to `Self`,
>> > + // - `data` was properly initialized above.
>> > + let data = unsafe { &*(*this.as_ptr()).data.get() };
>>
>> As mentioned above, this creates a reference that is valid for this
>> *block*. So its lifetime will end after the `},` and before
>> `this_device` is initialized.
>>
>> It *might* be ok to turn it back into a raw pointer in
>> `RawDeviceRegistration::new`, but I wouldn't bet on it.
>
> Why? The reference is still valid in RawDeviceRegistration::new, no?
Yes the reference is still valid in the `new` function call. I was under
the impression that the pointer created in `new` would get invalidated,
because the struct would get reborrowed in the meantime, but that's not
the case, since this pointer is obtained by `get`. So you should be
good.
>> > +
>> > + let raw = RawDeviceRegistration::new(opts, parent, data);
>> > +
>> > + // FIXME: Work around a bug in rustc, to prevent the following warning:
>> > + //
>> > + // "warning: value captured by `dev` is never read."
>> > + //
>> > + // Link: https://github.com/rust-lang/rust/issues/141615
>>
>> Note that the bug is that the compiler complains about the wrong span.
>> The original value of `dev` is `None` and that value is never used, so
>> the warning is justified. So this `let _ = dev;` still needs to stay
>> until `pin-init` supports accessing previously initialized fields (now
>> I'm pretty certain that I will implement that soon).
>
> Do you want to propose an alternative comment about this?
I think we don't need a comment here.
>> > + let _ = dev;
>> > +
>> > + if let Some(parent) = parent {
>> > + let devres = Devres::new(parent, raw, GFP_KERNEL)?;
>> > +
>> > + dev = Some(devres.access(parent)?.device().into());
>> > + DeviceRegistrationInner::Managed(devres)
>> > + } else {
>> > + let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
>> > +
>> > + dev = Some(boxed.device().into());
>> > + DeviceRegistrationInner::Raw(boxed)
>> > + }
>> > + },
>> > + // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>> > + // case.
>> > + this_device: {
>> > + // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>> > + unsafe { dev.unwrap_unchecked() }
>> > + },
>>
>> No need for the extra block, just do:
>>
>> // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>> // case.
>> // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>> this_device: unsafe { dev.unwrap_unchecked() },
>
> Yes, I know, but I found the above a bit cleaner -- I don't mind changing it
> though.
>
>> I'm also pretty sure that the compiler would optimize `.take().unwrap()`
>> and also this is only executed once per `MiscDeviceRegistration`, so
>> even if it isn't it wouldn't really matter. So I'd prefer if we don't
>> use `unsafe` here even if it is painfully obvious (if I'm fast enough
>> with implementing, you can rebase on top before you merge and then this
>> will be gone anyways :)
>
> Sounds good! :)
>
> But I think that unsafe is better than unwrap() in such cases; unsafe requires
> us to explain why it's OK to do it, which makes it less likely to create bugs.
That's not exactly how I would think about it, but your argument makes
sense.
> (Just recently I wrote some code, hit the need for unsafe and, while writing up
> the safety comment, I had to explain to myself, why the way I was about to
> implement this was pretty broken.)
That's great :)
> unwrap() on the other hand, doesn't require any explanation, but panics the
> kernel in the worst case.
Yeah that is true. Ultimately for this case it won't matter, since I'll
just implement the access thing in pin-init.
It'll still take a while, because it will touch several parts that other
patches also touch. Thus I prefer to first pick the patches already on
the list, but for that I'll wait until the merge window closes.
---
Cheers,
Benno
On Sat, May 31, 2025 at 10:05:28AM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:17 AM CEST, Danilo Krummrich wrote:
> > On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> >> > - /// Register a misc device.
> >> > - pub fn register(
> >> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
> >> > + fn new<'a>(
> >> > opts: MiscDeviceOptions,
> >> > - data: impl PinInit<T::RegistrationData, Error>,
> >> > - ) -> impl PinInit<Self, Error> {
> >> > + parent: Option<&'a Device<Bound>>,
> >> > + data: &'a T::RegistrationData,
> >> > + ) -> impl PinInit<Self, Error> + 'a
> >> > + where
> >> > + T: 'a,
> >> > + {
> >> > try_pin_init!(Self {
> >> > - data <- Opaque::pin_init(data),
> >> > + // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> >> > + // is guaranteed to be valid for the entire lifetime of `Self`.
> >> > + data: NonNull::from(data),
> >>
> >> Both the argument in the INVARIANT comment and way this works are a bit
> >> flawed.
> >
> > Why is the argument flawed? Let's say we go with your proposal below, what would
> > the safety requirement for RawDeviceRegistration::new and the invariant of
> > RawDeviceRegistration look like? Wouldn't it be the exact same argument?
>
> So what I write below it not really true. But the argument relies on the
> fact that `data` is pointing to a value that will stay alive for the
> duration of the existence of `self`. That should be a safety requirement
> of `new` (even if we take a reference as an argument).
Ok, that's fair -- I'll add the safety requirement.
> >> Instead, I'd recommend directly taking the `NonNull` as a
> >> parameter. Yes the function will need to be `unsafe`, but the lifetime
> >> that you're creating below only lives for `'a`, but the object might
> >> live much longer. You might still be fine, but I'd just recommend
> >> staying in raw pointer land (or in this case `NonNull`).
On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> Currently, the design of MiscDeviceRegistration is focused on creating
> and registering a misc device directly from a module and hence does not
> support using misc device from a device driver.
>
> However, it is important for the design of the misc device abstraction to
> take the driver model into account.
>
> Hence, consider the use-case of using misc device from a device driver;
> let's go through the design motivation bottom-up:
>
> Ideally, we want to be able to access (bus) device resources (such as I/O
> memory) from misc device callbacks (such as open() or ioctl()) without
> additional overhead, i.e. without having to go through
> Devres::try_access(), which implies an atomic check and an RCU read side
> critical section. Instead, we want to be able to use Devres::access(),
> which does not have any overhead, which requires a &Device<Bound> to
> prove that we can directly access the device resource.
>
> Given that all misc device callbacks are synchronized against
> misc_deregister(), we can prove that the misc device's parent device is
> bound iff we guarantee that the misc device's registration won't
> out-live the parent device's unbind.
>
> This can easily be proven by using devres for the misc device's
> registration object itself.
>
> Since this is only applicable for the device driver use-case, abstract
> the actual registration instance with a Rust enum, which is either a
> "raw" registration or a "managed" registration.
>
> In order to avoid any penalties from a managed registration, structurally
> separate the registration's private data from the "raw" misc device
> registration (which either stays "raw" or becomes "managed") depending
> on whether a parent device is supplied.
>
> The advantage of this solution is that it is entirely transparent to the
> user -- no separate structures or functions for whether the abstraction
> is used directly from a module or from a device driver; instead
> MiscDeviceRegistration::register() gets an optional parent argument.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> rust/kernel/miscdevice.rs | 178 ++++++++++++++++++++++++-------
> samples/rust/rust_misc_device.rs | 9 +-
> 2 files changed, 143 insertions(+), 44 deletions(-)
>
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 1b5ec13868e2..6801fe72a8a6 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -10,16 +10,17 @@
>
> use crate::{
> bindings, container_of,
> - device::Device,
> + device::{Bound, Device},
> + devres::Devres,
> error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> ffi::{c_int, c_long, c_uint, c_ulong},
> fs::File,
> prelude::*,
> seq_file::SeqFile,
> str::CStr,
> - types::{ForeignOwnable, Opaque},
> + types::{ARef, ForeignOwnable, Opaque},
> };
> -use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
> +use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin, ptr::NonNull};
>
> /// Options for creating a misc device.
> #[derive(Copy, Clone)]
> @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> }
> }
>
> -/// A registration of a miscdevice.
> -///
> /// # Invariants
> ///
> -/// `inner` is a registered misc device.
> +/// - `inner` is a registered misc device,
> +/// - `data` is valid for the entire lifetime of `Self`.
> #[repr(C)]
> #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T: MiscDevice> {
> +struct RawDeviceRegistration<T: MiscDevice> {
> #[pin]
> inner: Opaque<bindings::miscdevice>,
> - #[pin]
> - data: Opaque<T::RegistrationData>,
> + data: NonNull<T::RegistrationData>,
> _t: PhantomData<T>,
> }
>
> -// SAFETY:
> -// - It is allowed to call `misc_deregister` on a different thread from where you called
> -// `misc_register`.
> -// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> -unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> -
> -// SAFETY:
> -// - All `&self` methods on this type are written to ensure that it is safe to call them in
> -// parallel.
> -// - `MiscDevice::RegistrationData` is always `Sync`.
> -unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> -
> -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> - /// Register a misc device.
> - pub fn register(
> +impl<T: MiscDevice> RawDeviceRegistration<T> {
> + fn new<'a>(
> opts: MiscDeviceOptions,
> - data: impl PinInit<T::RegistrationData, Error>,
> - ) -> impl PinInit<Self, Error> {
> + parent: Option<&'a Device<Bound>>,
> + data: &'a T::RegistrationData,
> + ) -> impl PinInit<Self, Error> + 'a
> + where
> + T: 'a,
> + {
> try_pin_init!(Self {
> - data <- Opaque::pin_init(data),
> + // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> + // is guaranteed to be valid for the entire lifetime of `Self`.
> + data: NonNull::from(data),
> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> + let mut value = opts.into_raw::<T>();
> +
> + if let Some(parent) = parent {
> + // The device core code will take care to take a reference of `parent` in
> + // `device_add()` called by `misc_register()`.
> + value.parent = parent.as_raw();
> + }
> +
> // SAFETY: The initializer can write to the provided `slot`.
> - unsafe { slot.write(opts.into_raw::<T>()) };
> + unsafe { slot.write(value) };
>
> // SAFETY:
> // * We just wrote the misc device options to the slot. The miscdevice will
> @@ -94,12 +94,12 @@ pub fn register(
> }
>
> /// Returns a raw pointer to the misc device.
> - pub fn as_raw(&self) -> *mut bindings::miscdevice {
> + fn as_raw(&self) -> *mut bindings::miscdevice {
> self.inner.get()
> }
>
> /// Access the `this_device` field.
> - pub fn device(&self) -> &Device {
> + fn device(&self) -> &Device {
> // SAFETY: This can only be called after a successful register(), which always
> // initialises `this_device` with a valid device. Furthermore, the signature of this
> // function tells the borrow-checker that the `&Device` reference must not outlive the
> @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
> unsafe { Device::as_ref((*self.as_raw()).this_device) }
> }
>
> + fn data(&self) -> &T::RegistrationData {
> + // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> + // `Self`.
> + unsafe { self.data.as_ref() }
> + }
> +}
> +
> +#[pinned_drop]
> +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: We know that the device is registered by the type invariants.
> + unsafe { bindings::misc_deregister(self.inner.get()) };
> + }> +}
> +
> +#[expect(dead_code)]
> +enum DeviceRegistrationInner<T: MiscDevice> {
> + Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> + Managed(Devres<RawDeviceRegistration<T>>),
> +}
> +
> +/// A registration of a miscdevice.
> +#[pin_data(PinnedDrop)]
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> + inner: DeviceRegistrationInner<T>,
> + #[pin]
> + data: Opaque<T::RegistrationData>,
> + this_device: ARef<Device>,
> + _t: PhantomData<T>,
> +}
> +
> +// SAFETY:
> +// - It is allowed to call `misc_deregister` on a different thread from where you called
> +// `misc_register`.
> +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> +
> +// SAFETY:
> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
> +// parallel.
> +// - `MiscDevice::RegistrationData` is always `Sync`.
> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> +
> +impl<T: MiscDevice> MiscDeviceRegistration<T> {
> + /// Register a misc device.
> + pub fn register<'a>(
> + opts: MiscDeviceOptions,
> + data: impl PinInit<T::RegistrationData, Error> + 'a,
> + parent: Option<&'a Device<Bound>>,
> + ) -> impl PinInit<Self, Error> + 'a
> + where
> + T: 'a,
> + {
> + let mut dev: Option<ARef<Device>> = None;
> +
> + try_pin_init!(&this in Self {
> + data <- Opaque::pin_init(data),
> + // TODO: make `inner` in-place when enums get supported by pin-init.
> + //
> + // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
> + inner: {
> + // SAFETY:
> + // - `this` is a valid pointer to `Self`,
> + // - `data` was properly initialized above.
> + let data = unsafe { &*(*this.as_ptr()).data.get() };
> +
> + let raw = RawDeviceRegistration::new(opts, parent, data);
> +
> + // FIXME: Work around a bug in rustc, to prevent the following warning:
> + //
> + // "warning: value captured by `dev` is never read."
> + //
> + // Link: https://github.com/rust-lang/rust/issues/141615
> + let _ = dev;
> +
> + if let Some(parent) = parent {
> + let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> +
> + dev = Some(devres.access(parent)?.device().into());
> + DeviceRegistrationInner::Managed(devres)
> + } else {
> + let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> +
> + dev = Some(boxed.device().into());
> + DeviceRegistrationInner::Raw(boxed)
> + }
> + },
> + // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> + // case.
> + this_device: {
> + // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> + unsafe { dev.unwrap_unchecked() }
> + },
> + _t: PhantomData,
> + })
> + }
> +
> + /// Access the `this_device` field.
> + pub fn device(&self) -> &Device {
> + &self.this_device
> + }
> +
> /// Access the additional data stored in this registration.
> pub fn data(&self) -> &T::RegistrationData {
> // SAFETY:
> @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
> #[pinned_drop]
> impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> fn drop(self: Pin<&mut Self>) {
> - // SAFETY: We know that the device is registered by the type invariants.
> - unsafe { bindings::misc_deregister(self.inner.get()) };
> -
> // SAFETY: `self.data` is valid for dropping.
> unsafe { core::ptr::drop_in_place(self.data.get()) };
I think this can race for a use after free.
The data gets freed in this `Drop` impl but the `miscdevice_deregister` call will only
happen in the `DeviceRegistrationInner` `Drop` implementatation, since the fields
will only be dropped after the `drop` function has executed.
Either inner: DeviceRegistrationInner<T> needs to be wrapped in a ManuallyDrop and
dropped manually,
or the Data needs to be wrapped in a type that will automatically drop it (this would
be fine with `UnsafePinned`).
> }
> @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
> /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> /// If no additional data is required than the unit type `()` should be used.
> ///
> - /// This data can be accessed in [`MiscDevice::open()`] using
> - /// [`MiscDeviceRegistration::data()`].
> + /// This data can be accessed in [`MiscDevice::open()`].
> type RegistrationData: Sync;
>
> /// Called when the misc device is opened.
> ///
> /// The returned pointer will be stored as the private data for the file.
> - fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> + fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
>
> /// Called when the misc device is released.
> fn release(device: Self::Ptr, _file: &File) {
> @@ -217,17 +315,17 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
> // SAFETY:
> // * `misc_open()` ensures that the `struct miscdevice` can't be unregistered and freed
> // during this call to `fops_open`.
> - // * The `misc_ptr` always points to the `inner` field of a `MiscDeviceRegistration<T>`.
> - // * The `MiscDeviceRegistration<T>` is valid until the `struct miscdevice` was
> + // * The `misc_ptr` always points to the `inner` field of a `RawDeviceRegistration<T>`.
> + // * The `RawDeviceRegistration<T>` is valid until the `struct miscdevice` was
> // unregistered.
> - let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<T>, inner) };
> + let registration = unsafe { &*container_of!(misc_ptr, RawDeviceRegistration<T>, inner) };
>
> // SAFETY:
> // * This underlying file is valid for (much longer than) the duration of `T::open`.
> // * There is no active fdget_pos region on the file on this thread.
> let file = unsafe { File::from_raw_file(raw_file) };
>
> - let ptr = match T::open(file, registration) {
> + let ptr = match T::open(file, registration.device(), registration.data()) {
> Ok(ptr) => ptr,
> Err(err) => return err.to_errno(),
> };
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 843442b0ea1d..b60fd063afa8 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -198,7 +198,8 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> try_pin_init!(Self {
> _miscdev <- MiscDeviceRegistration::register(
> options,
> - Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL)
> + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> + None,
> ),
> })
> }
> @@ -222,15 +223,15 @@ impl MiscDevice for RustMiscDevice {
>
> type RegistrationData = Arc<Mutex<Inner>>;
>
> - fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
> - let dev = ARef::from(misc.device());
> + fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) -> Result<Pin<KBox<Self>>> {
> + let dev = ARef::from(misc);
>
> dev_info!(dev, "Opening Rust Misc Device Sample\n");
>
> KBox::try_pin_init(
> try_pin_init! {
> RustMiscDevice {
> - shared: misc.data().clone(),
> + shared: data.clone(),
> unique <- new_mutex!(Inner { value: 0_i32 }),
> dev: dev,
> }
On Fri, May 30, 2025 at 07:35:56PM +0200, Christian Schrefl wrote:
> On 30.05.25 4:24 PM, Danilo Krummrich wrote:
<snip>
> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> > + fn drop(self: Pin<&mut Self>) {
> > + // SAFETY: We know that the device is registered by the type invariants.
> > + unsafe { bindings::misc_deregister(self.inner.get()) };
> > + }> +}
<snip>
> > #[pinned_drop]
> > impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> > fn drop(self: Pin<&mut Self>) {
> > - // SAFETY: We know that the device is registered by the type invariants.
> > - unsafe { bindings::misc_deregister(self.inner.get()) };
> > -
> > // SAFETY: `self.data` is valid for dropping.
> > unsafe { core::ptr::drop_in_place(self.data.get()) };
>
> I think this can race for a use after free.
> The data gets freed in this `Drop` impl but the `miscdevice_deregister` call will only
> happen in the `DeviceRegistrationInner` `Drop` implementatation, since the fields
> will only be dropped after the `drop` function has executed.
Yes and no. :-) The fun part is that the drop order actually depends on whether
we have a parent device and use Devres or whether we have no parent device.
In the first case the drop order is correct by chance, due to Devres revoking
things when the parent device in unbound, which happens before, since the faux
device is dropped first.
But in general you're right, this needs to be fixed.
> Either inner: DeviceRegistrationInner<T> needs to be wrapped in a ManuallyDrop and
> dropped manually,
> or the Data needs to be wrapped in a type that will automatically drop it (this would
> be fine with `UnsafePinned`).
There's also option 3), which is moving the drop_in_place() for the registration
data into RawDeviceRegistration::drop(), right below misc_deregister(), until we
have `UnsafePinned`.
(This is fine, since RawDeviceRegistration already has (and requires) a pointer
to the registration data and hence is always embedded in a
MiscDeviceRegistration.)
© 2016 - 2025 Red Hat, Inc.