When using the Rust miscdevice bindings, you generally embed the
`MiscDeviceRegistration` within another struct:
struct MyDriverData {
data: SomeOtherData,
misc: MiscDeviceRegistration<MyMiscFile>
}
In the `fops->open` callback of the miscdevice, you are given a
reference to the registration, which allows you to access its fields.
For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
accessor to pull out miscdevice::this_device") you can access the
internal `struct device`. However, there is still no way to access the
`data` field in the above example, because you only have a reference to
the registration.
Using `container_of` is also not possible to do safely. For example, if
the destructor of `MyDriverData` runs, then the destructor of `data`
would run before the miscdevice is deregistered, so using `container_of`
to access `data` from `fops->open` could result in a UAF. A similar
problem can happen on initialization if `misc` is not the last field to
be initialized.
To provide a safe way to access user-defined data stored next to the
`struct miscdevice`, make `MiscDeviceRegistration` into a container that
can store a user-provided piece of data. This way, `fops->open` can
access that data via the registration, since the data is stored inside
the registration.
The container enforces that the additional user data is initialized
before the miscdevice is registered, and that the miscdevice is
deregistered before the user data is destroyed. This ensures that access
to the userdata is safe.
For the same reasons as in commit 88441d5c6d17 ("rust: miscdevice:
access the `struct miscdevice` from fops->open()"), you cannot access
the user data in any other fops callback than open. This is because a
miscdevice can be deregistered while there are still open files.
A situation where this user data might be required is when a platform
driver acquires a resource in `probe` and wants to use this resource in
the `fops` implementation of a `MiscDevice`.
This solution is similar to the approach used by the initial downstream
Rust-for-Linux/Rust branch [0].
Link: https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/miscdev.rs#L108 [0]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
rust/kernel/miscdevice.rs | 79 ++++++++++++++++++++++++++++++----------
samples/rust/rust_misc_device.rs | 4 +-
2 files changed, 62 insertions(+), 21 deletions(-)
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index b4c5f74de23d6f4fbcdebfe408d6954884609e8f..ad9fc0b2383860cb976c57a398f372280c19513c 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -9,7 +9,7 @@
//! Reference: <https://www.kernel.org/doc/html/latest/driver-api/misc_devices.html>
use crate::{
- bindings,
+ bindings, container_of,
device::Device,
error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
ffi::{c_int, c_long, c_uint, c_ulong},
@@ -20,6 +20,7 @@
types::{ForeignOwnable, Opaque},
};
use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
+use pin_init::Wrapper;
/// Options for creating a misc device.
#[derive(Copy, Clone)]
@@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
/// # Invariants
///
/// `inner` is a registered misc device.
-#[repr(transparent)]
+#[repr(C)]
#[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T> {
+pub struct MiscDeviceRegistration<T: MiscDevice> {
#[pin]
inner: Opaque<bindings::miscdevice>,
+ #[pin]
+ data: Opaque<T::RegistrationData>,
_t: PhantomData<T>,
}
-// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
-// `misc_register`.
-unsafe impl<T> Send for MiscDeviceRegistration<T> {}
-// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
-// parallel.
-unsafe impl<T> Sync for MiscDeviceRegistration<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(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
+ pub fn register(
+ opts: MiscDeviceOptions,
+ data: impl PinInit<T::RegistrationData, Error>,
+ ) -> impl PinInit<Self, Error> {
try_pin_init!(Self {
+ data <- Opaque::pin_init(data),
inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
// SAFETY: The initializer can write to the provided `slot`.
unsafe { slot.write(opts.into_raw::<T>()) };
- // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
- // get unregistered before `slot` is deallocated because the memory is pinned and
- // the destructor of this type deallocates the memory.
+ // SAFETY:
+ // * We just wrote the misc device options to the slot. The miscdevice will
+ // get unregistered before `slot` is deallocated because the memory is pinned and
+ // the destructor of this type deallocates the memory.
+ // * `data` is Initialized before `misc_register` so no race with `fops->open()`
+ // is possible.
// INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
// misc device.
to_result(unsafe { bindings::misc_register(slot) })
@@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
// before the underlying `struct miscdevice` is destroyed.
unsafe { Device::as_ref((*self.as_raw()).this_device) }
}
+
+ /// Access the additional data stored in this registration.
+ pub fn data(&self) -> &T::RegistrationData {
+ // SAFETY:
+ // * No mutable reference to the value contained by `self.data` can ever be created.
+ // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
+ unsafe { &*self.data.get() }
+ }
}
#[pinned_drop]
-impl<T> PinnedDrop for MiscDeviceRegistration<T> {
+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 and nothing uses it anymore.
+ unsafe { core::ptr::drop_in_place(self.data.get()) };
}
}
@@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
/// What kind of pointer should `Self` be wrapped in.
type Ptr: ForeignOwnable + Send + Sync;
+ /// 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()`].
+ type RegistrationData: Sync;
+
/// Called when the misc device is opened.
///
/// The returned pointer will be stored as the private data for the file.
@@ -178,18 +211,24 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
// SAFETY: The open call of a file can access the private data.
let misc_ptr = unsafe { (*raw_file).private_data };
- // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
- // associated `struct miscdevice` before calling into this method. Furthermore,
- // `misc_open()` ensures that the miscdevice can't be unregistered and freed during this
- // call to `fops_open`.
- let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
+ // This is a miscdevice, so `misc_open()` sets the private data to a pointer to the
+ // associated `struct miscdevice` before calling into this method.
+ let misc_ptr = misc_ptr.cast::<bindings::miscdevice>();
+
+ // 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
+ // unregistered.
+ let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<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, misc) {
+ let ptr = match T::open(file, registration) {
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 c881fd6dbd08cf4308fe1bd37d11d28374c1f034..67a6172fbbf72dd42a1b655f5f5a782101432707 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -137,7 +137,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
};
try_pin_init!(Self {
- _miscdev <- MiscDeviceRegistration::register(options),
+ _miscdev <- MiscDeviceRegistration::register(options, ()),
})
}
}
@@ -157,6 +157,8 @@ struct RustMiscDevice {
impl MiscDevice for RustMiscDevice {
type Ptr = Pin<KBox<Self>>;
+ type RegistrationData = ();
+
fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
let dev = ARef::from(misc.device());
--
2.49.0On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> /// # Invariants
> ///
> /// `inner` is a registered misc device.
> -#[repr(transparent)]
> +#[repr(C)]
Why do we need linear layout? `container_of!` also works with the `Rust`
layout.
> #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T> {
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> #[pin]
> inner: Opaque<bindings::miscdevice>,
> + #[pin]
> + data: Opaque<T::RegistrationData>,
> _t: PhantomData<T>,
No need to keep the `PhantomData` field around, since you're using `T`
above.
> }
>
> -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
> -// `misc_register`.
> -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
> -// parallel.
> -unsafe impl<T> Sync for MiscDeviceRegistration<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> {}
I would feel better if we still add the `T::RegistrationData: Sync`
bound here even if it is vacuous today.
> impl<T: MiscDevice> MiscDeviceRegistration<T> {
> /// Register a misc device.
> - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> + pub fn register(
> + opts: MiscDeviceOptions,
> + data: impl PinInit<T::RegistrationData, Error>,
> + ) -> impl PinInit<Self, Error> {
> try_pin_init!(Self {
> + data <- Opaque::pin_init(data),
> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> // SAFETY: The initializer can write to the provided `slot`.
> unsafe { slot.write(opts.into_raw::<T>()) };
>
> - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
> - // get unregistered before `slot` is deallocated because the memory is pinned and
> - // the destructor of this type deallocates the memory.
> + // SAFETY:
> + // * We just wrote the misc device options to the slot. The miscdevice will
> + // get unregistered before `slot` is deallocated because the memory is pinned and
> + // the destructor of this type deallocates the memory.
> + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
> + // is possible.
> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
> // misc device.
> to_result(unsafe { bindings::misc_register(slot) })
> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
> // before the underlying `struct miscdevice` is destroyed.
> unsafe { Device::as_ref((*self.as_raw()).this_device) }
> }
> +
> + /// Access the additional data stored in this registration.
> + pub fn data(&self) -> &T::RegistrationData {
> + // SAFETY:
> + // * No mutable reference to the value contained by `self.data` can ever be created.
> + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
Please add type invariants for these two requirements.
> + unsafe { &*self.data.get() }
> + }
> }
>
> #[pinned_drop]
> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> +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 and nothing uses it anymore.
Ditto.
> + unsafe { core::ptr::drop_in_place(self.data.get()) };
> }
> }
>
> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> /// What kind of pointer should `Self` be wrapped in.
> type Ptr: ForeignOwnable + Send + Sync;
>
> + /// 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()`].
> + type RegistrationData: Sync;
Why do we require `Sync` here?
We might want to give this a shorter name?
---
Cheers,
Benno
> +
> /// Called when the misc device is opened.
> ///
> /// The returned pointer will be stored as the private data for the file.
On Sat, May 31, 2025 at 02:23:23PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> > @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> > /// # Invariants
> > ///
> > /// `inner` is a registered misc device.
> > -#[repr(transparent)]
> > +#[repr(C)]
>
> Why do we need linear layout? `container_of!` also works with the `Rust`
> layout.
>
> > #[pin_data(PinnedDrop)]
> > -pub struct MiscDeviceRegistration<T> {
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> > #[pin]
> > inner: Opaque<bindings::miscdevice>,
> > + #[pin]
> > + data: Opaque<T::RegistrationData>,
> > _t: PhantomData<T>,
>
> No need to keep the `PhantomData` field around, since you're using `T`
> above.
>
> > }
> >
> > -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
> > -// `misc_register`.
> > -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> > -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
> > -// parallel.
> > -unsafe impl<T> Sync for MiscDeviceRegistration<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> {}
>
> I would feel better if we still add the `T::RegistrationData: Sync`
> bound here even if it is vacuous today.
>
> > impl<T: MiscDevice> MiscDeviceRegistration<T> {
> > /// Register a misc device.
> > - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > + pub fn register(
> > + opts: MiscDeviceOptions,
> > + data: impl PinInit<T::RegistrationData, Error>,
> > + ) -> impl PinInit<Self, Error> {
> > try_pin_init!(Self {
> > + data <- Opaque::pin_init(data),
> > inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> > // SAFETY: The initializer can write to the provided `slot`.
> > unsafe { slot.write(opts.into_raw::<T>()) };
> >
> > - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
> > - // get unregistered before `slot` is deallocated because the memory is pinned and
> > - // the destructor of this type deallocates the memory.
> > + // SAFETY:
> > + // * We just wrote the misc device options to the slot. The miscdevice will
> > + // get unregistered before `slot` is deallocated because the memory is pinned and
> > + // the destructor of this type deallocates the memory.
> > + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
> > + // is possible.
> > // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
> > // misc device.
> > to_result(unsafe { bindings::misc_register(slot) })
> > @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
> > // before the underlying `struct miscdevice` is destroyed.
> > unsafe { Device::as_ref((*self.as_raw()).this_device) }
> > }
> > +
> > + /// Access the additional data stored in this registration.
> > + pub fn data(&self) -> &T::RegistrationData {
> > + // SAFETY:
> > + // * No mutable reference to the value contained by `self.data` can ever be created.
> > + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>
> Please add type invariants for these two requirements.
>
> > + unsafe { &*self.data.get() }
> > + }
> > }
> >
> > #[pinned_drop]
> > -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> > +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 and nothing uses it anymore.
>
> Ditto.
>
> > + unsafe { core::ptr::drop_in_place(self.data.get()) };
> > }
> > }
> >
> > @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> > /// What kind of pointer should `Self` be wrapped in.
> > type Ptr: ForeignOwnable + Send + Sync;
> >
> > + /// 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()`].
> > + type RegistrationData: Sync;
>
> Why do we require `Sync` here?
>
> We might want to give this a shorter name?
On Wed, Jun 4, 2025 at 11:37 AM Alice Ryhl <aliceryhl@google.com> wrote: > Please disregard this email. It was empty by mistake. Alice
On 31.05.25 2:23 PM, Benno Lossin wrote:
> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>> @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>> /// # Invariants
>> ///
>> /// `inner` is a registered misc device.
>> -#[repr(transparent)]
>> +#[repr(C)]
>
> Why do we need linear layout? `container_of!` also works with the `Rust`
> layout.
That was a leftover from a previous version, fixed.
>
>> #[pin_data(PinnedDrop)]
>> -pub struct MiscDeviceRegistration<T> {
>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>> #[pin]
>> inner: Opaque<bindings::miscdevice>,
>> + #[pin]
>> + data: Opaque<T::RegistrationData>,
>> _t: PhantomData<T>,
>
> No need to keep the `PhantomData` field around, since you're using `T`
> above.
>
Fixed.
>> }
>>
>> -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
>> -// `misc_register`.
>> -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
>> -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
>> -// parallel.
>> -unsafe impl<T> Sync for MiscDeviceRegistration<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> {}
>
> I would feel better if we still add the `T::RegistrationData: Sync`
> bound here even if it is vacuous today.
Since a reference the `MiscDeviceRegistration` struct is an
argument to the open function this struct must always be Sync,
so adding bounds here doesn't make much sense.
I'll add this a safety comment in `MiscdeviceVTable::open`
about this.
Is there a good way to assert this at build to avoid regessions?
>
>> impl<T: MiscDevice> MiscDeviceRegistration<T> {
>> /// Register a misc device.
>> - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>> + pub fn register(
>> + opts: MiscDeviceOptions,
>> + data: impl PinInit<T::RegistrationData, Error>,
>> + ) -> impl PinInit<Self, Error> {
>> try_pin_init!(Self {
>> + data <- Opaque::pin_init(data),
>> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>> // SAFETY: The initializer can write to the provided `slot`.
>> unsafe { slot.write(opts.into_raw::<T>()) };
>>
>> - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>> - // get unregistered before `slot` is deallocated because the memory is pinned and
>> - // the destructor of this type deallocates the memory.
>> + // SAFETY:
>> + // * We just wrote the misc device options to the slot. The miscdevice will
>> + // get unregistered before `slot` is deallocated because the memory is pinned and
>> + // the destructor of this type deallocates the memory.
>> + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>> + // is possible.
>> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>> // misc device.
>> to_result(unsafe { bindings::misc_register(slot) })
>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>> // before the underlying `struct miscdevice` is destroyed.
>> unsafe { Device::as_ref((*self.as_raw()).this_device) }
>> }
>> +
>> + /// Access the additional data stored in this registration.
>> + pub fn data(&self) -> &T::RegistrationData {
>> + // SAFETY:
>> + // * No mutable reference to the value contained by `self.data` can ever be created.
>> + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>
> Please add type invariants for these two requirements.
>
>> + unsafe { &*self.data.get() }
>> + }
>> }
>>
>> #[pinned_drop]
>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>> +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 and nothing uses it anymore.
>
> Ditto.
I'm not quite sure how to formulate these, what do you think of:
/// - `inner` is a registered misc device.
/// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
/// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
/// - no mutable references to `data` may be created.
>
>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>> }
>> }
>>
>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>> /// What kind of pointer should `Self` be wrapped in.
>> type Ptr: ForeignOwnable + Send + Sync;
>>
>> + /// 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()`].
>> + type RegistrationData: Sync;
>
> Why do we require `Sync` here?
Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>
> We might want to give this a shorter name?
I think its fine, but I am open to Ideas.
Cheers
Christian
On Mon, Jun 02, 2025 at 11:16:33PM +0200, Christian Schrefl wrote:
> On 31.05.25 2:23 PM, Benno Lossin wrote:
> > On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> >> + unsafe { core::ptr::drop_in_place(self.data.get()) };
> >> }
> >> }
> >>
> >> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> >> /// What kind of pointer should `Self` be wrapped in.
> >> type Ptr: ForeignOwnable + Send + Sync;
> >>
> >> + /// 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()`].
> >> + type RegistrationData: Sync;
> >
> > Why do we require `Sync` here?
>
> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
Even if `MiscDeviceRegistration` is non-Send, the registration data must
still be Sync. The f_ops->open callback can *always* be called from any
thread no matter what we do here, so the data it gives you access must
always be Sync no matter what.
Alice
On 04.06.25 11:40 AM, Alice Ryhl wrote:
> On Mon, Jun 02, 2025 at 11:16:33PM +0200, Christian Schrefl wrote:
>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>> }
>>>> }
>>>>
>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>> /// What kind of pointer should `Self` be wrapped in.
>>>> type Ptr: ForeignOwnable + Send + Sync;
>>>>
>>>> + /// 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()`].
>>>> + type RegistrationData: Sync;
>>>
>>> Why do we require `Sync` here?
>>
>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>
> Even if `MiscDeviceRegistration` is non-Send, the registration data must
> still be Sync. The f_ops->open callback can *always* be called from any
> thread no matter what we do here, so the data it gives you access must
> always be Sync no matter what.
Right I meant to write `Sync` sorry.
Cheers
Christian
On Wed, Jun 4, 2025 at 11:42 AM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> On 04.06.25 11:40 AM, Alice Ryhl wrote:
> > On Mon, Jun 02, 2025 at 11:16:33PM +0200, Christian Schrefl wrote:
> >> On 31.05.25 2:23 PM, Benno Lossin wrote:
> >>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> >>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
> >>>> }
> >>>> }
> >>>>
> >>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> >>>> /// What kind of pointer should `Self` be wrapped in.
> >>>> type Ptr: ForeignOwnable + Send + Sync;
> >>>>
> >>>> + /// 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()`].
> >>>> + type RegistrationData: Sync;
> >>>
> >>> Why do we require `Sync` here?
> >>
> >> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
> >
> > Even if `MiscDeviceRegistration` is non-Send, the registration data must
> > still be Sync. The f_ops->open callback can *always* be called from any
> > thread no matter what we do here, so the data it gives you access must
> > always be Sync no matter what.
>
> Right I meant to write `Sync` sorry.
It is still the case that RegistrationData must always be Sync no
matter what. But I guess that also applies to MiscDeviceRegistration.
Alice
On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
> On 31.05.25 2:23 PM, Benno Lossin wrote:
>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>> +// 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> {}
>>
>> I would feel better if we still add the `T::RegistrationData: Sync`
>> bound here even if it is vacuous today.
>
> Since a reference the `MiscDeviceRegistration` struct is an
> argument to the open function this struct must always be Sync,
> so adding bounds here doesn't make much sense.
Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
even if `T::RegistrationData` is not `Sync` if that bound got removed
at some point. And this "instability" is what I'm worried about.
> I'll add this a safety comment in `MiscdeviceVTable::open`
> about this.
>
> Is there a good way to assert this at build to avoid regessions?
const _: () = {
fn assert_sync<T: ?Sized + Sync>() {}
fn ctx<T: MiscDevice>() {
assert_sync::<T::RegistrationData>();
}
};
That would also be fine with me if you insist on not adding the bound.
(the `assert_sync` function should maybe be somewhere where everyone can
use it)
>>> impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>> /// Register a misc device.
>>> - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>> + pub fn register(
>>> + opts: MiscDeviceOptions,
>>> + data: impl PinInit<T::RegistrationData, Error>,
>>> + ) -> impl PinInit<Self, Error> {
>>> try_pin_init!(Self {
>>> + data <- Opaque::pin_init(data),
>>> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>> // SAFETY: The initializer can write to the provided `slot`.
>>> unsafe { slot.write(opts.into_raw::<T>()) };
>>>
>>> - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>> - // get unregistered before `slot` is deallocated because the memory is pinned and
>>> - // the destructor of this type deallocates the memory.
>>> + // SAFETY:
>>> + // * We just wrote the misc device options to the slot. The miscdevice will
>>> + // get unregistered before `slot` is deallocated because the memory is pinned and
>>> + // the destructor of this type deallocates the memory.
>>> + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>> + // is possible.
>>> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>> // misc device.
>>> to_result(unsafe { bindings::misc_register(slot) })
>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>> // before the underlying `struct miscdevice` is destroyed.
>>> unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>> }
>>> +
>>> + /// Access the additional data stored in this registration.
>>> + pub fn data(&self) -> &T::RegistrationData {
>>> + // SAFETY:
>>> + // * No mutable reference to the value contained by `self.data` can ever be created.
>>> + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>
>> Please add type invariants for these two requirements.
>>
>>> + unsafe { &*self.data.get() }
>>> + }
>>> }
>>>
>>> #[pinned_drop]
>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>> +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 and nothing uses it anymore.
>>
>> Ditto.
>
> I'm not quite sure how to formulate these, what do you think of:
>
> /// - `inner` is a registered misc device.
This doesn't really mean something to me, maybe it's better to reference
the registering function?
> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
This sounds good. But help me understand, why do we need `Opaque` /
`UnsafePinned` again? If we're only using shared references, then we
could also just store the object by value?
> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
What does "usable" mean?
> /// - no mutable references to `data` may be created.
>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>> }
>>> }
>>>
>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>> /// What kind of pointer should `Self` be wrapped in.
>>> type Ptr: ForeignOwnable + Send + Sync;
>>>
>>> + /// 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()`].
>>> + type RegistrationData: Sync;
>>
>> Why do we require `Sync` here?
>
> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
You could also just ask the type there to be `Sync`, then users will get
an error when they try to use `MiscDevice` in a way where
`RegistrationData` is required to be `Sync`.
>> We might want to give this a shorter name?
>
> I think its fine, but I am open to Ideas.
`Data`?
---
Cheers,
Benno
On 04.06.25 1:29 AM, Benno Lossin wrote:
> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>> +// 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> {}
>>>
>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>> bound here even if it is vacuous today.
>>
>> Since a reference the `MiscDeviceRegistration` struct is an
>> argument to the open function this struct must always be Sync,
>> so adding bounds here doesn't make much sense.
>
> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
> even if `T::RegistrationData` is not `Sync` if that bound got removed
> at some point. And this "instability" is what I'm worried about.
>
>> I'll add this a safety comment in `MiscdeviceVTable::open`
>> about this.
>>
>> Is there a good way to assert this at build to avoid regessions?
>
> const _: () = {
> fn assert_sync<T: ?Sized + Sync>() {}
> fn ctx<T: MiscDevice>() {
> assert_sync::<T::RegistrationData>();
> }
> };
>
I'll add the bound and a TODO about `assert_sync`, in `open`
where `Send` is required.
I intend to write a patch for `assert_sync` later.
> That would also be fine with me if you insist on not adding the bound.
>
> (the `assert_sync` function should maybe be somewhere where everyone can
> use it)
>
>>>> impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>> /// Register a misc device.
>>>> - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>> + pub fn register(
>>>> + opts: MiscDeviceOptions,
>>>> + data: impl PinInit<T::RegistrationData, Error>,
>>>> + ) -> impl PinInit<Self, Error> {
>>>> try_pin_init!(Self {
>>>> + data <- Opaque::pin_init(data),
>>>> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>> // SAFETY: The initializer can write to the provided `slot`.
>>>> unsafe { slot.write(opts.into_raw::<T>()) };
>>>>
>>>> - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>>> - // get unregistered before `slot` is deallocated because the memory is pinned and
>>>> - // the destructor of this type deallocates the memory.
>>>> + // SAFETY:
>>>> + // * We just wrote the misc device options to the slot. The miscdevice will
>>>> + // get unregistered before `slot` is deallocated because the memory is pinned and
>>>> + // the destructor of this type deallocates the memory.
>>>> + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>>> + // is possible.
>>>> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>> // misc device.
>>>> to_result(unsafe { bindings::misc_register(slot) })
>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>> // before the underlying `struct miscdevice` is destroyed.
>>>> unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>> }
>>>> +
>>>> + /// Access the additional data stored in this registration.
>>>> + pub fn data(&self) -> &T::RegistrationData {
>>>> + // SAFETY:
>>>> + // * No mutable reference to the value contained by `self.data` can ever be created.
>>>> + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>
>>> Please add type invariants for these two requirements.
>>>
>>>> + unsafe { &*self.data.get() }
>>>> + }
>>>> }
>>>>
>>>> #[pinned_drop]
>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>> +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 and nothing uses it anymore.
>>>
>>> Ditto.
>>
>> I'm not quite sure how to formulate these, what do you think of:
>>
>> /// - `inner` is a registered misc device.
>
> This doesn't really mean something to me, maybe it's better to reference
> the registering function?
That is from previous code so this should probably not be changed
in this series.
>
>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>
> This sounds good. But help me understand, why do we need `Opaque` /
> `UnsafePinned` again? If we're only using shared references, then we
> could also just store the object by value?
Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
so from what I understand having a `& RegistrationData` reference into that is UB without
`UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>
>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
>
> What does "usable" mean?
I guess valid / alive might be better wording?
I meant to say that the `fops` functions might use the `RegistrationData` until
`misc_deregister` has returned so we must ensure that these accesses are allowed.
>
>> /// - no mutable references to `data` may be created.
>
>>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>> }
>>>> }
>>>>
>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>> /// What kind of pointer should `Self` be wrapped in.
>>>> type Ptr: ForeignOwnable + Send + Sync;
>>>>
>>>> + /// 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()`].
>>>> + type RegistrationData: Sync;
>>>
>>> Why do we require `Sync` here?
>>
>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>
> You could also just ask the type there to be `Sync`, then users will get
> an error when they try to use `MiscDevice` in a way where
> `RegistrationData` is required to be `Sync`.
I don't think there is any point to allow defining a `MiscDevice` implementation
that cant actually be used/registered.
>
>>> We might want to give this a shorter name?
>>
>> I think its fine, but I am open to Ideas.
>
> `Data`?
I feel that `Data` is just very ambiguous, especially since it is associated with
`MiscDevice` not the `MiscDeviceRegistration` in which its used.
One Idea I've had was `AssociatedData` but that's less clear and not much shorter
than `RegistrationData`.
But I'd be alright to just with `Data` if that is wanted.
Cheers
Christian
On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
> On 04.06.25 1:29 AM, Benno Lossin wrote:
>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>> +// 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> {}
>>>>
>>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>>> bound here even if it is vacuous today.
>>>
>>> Since a reference the `MiscDeviceRegistration` struct is an
>>> argument to the open function this struct must always be Sync,
>>> so adding bounds here doesn't make much sense.
>>
>> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
>> even if `T::RegistrationData` is not `Sync` if that bound got removed
>> at some point. And this "instability" is what I'm worried about.
>>
>>> I'll add this a safety comment in `MiscdeviceVTable::open`
>>> about this.
>>>
>>> Is there a good way to assert this at build to avoid regessions?
>>
>> const _: () = {
>> fn assert_sync<T: ?Sized + Sync>() {}
>> fn ctx<T: MiscDevice>() {
>> assert_sync::<T::RegistrationData>();
>> }
>> };
>>
>
> I'll add the bound and a TODO about `assert_sync`, in `open`
> where `Send` is required.
>
> I intend to write a patch for `assert_sync` later.
Great :)
>> That would also be fine with me if you insist on not adding the bound.
>>
>> (the `assert_sync` function should maybe be somewhere where everyone can
>> use it)
>>
>>>>> impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>>> /// Register a misc device.
>>>>> - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>>> + pub fn register(
>>>>> + opts: MiscDeviceOptions,
>>>>> + data: impl PinInit<T::RegistrationData, Error>,
>>>>> + ) -> impl PinInit<Self, Error> {
>>>>> try_pin_init!(Self {
>>>>> + data <- Opaque::pin_init(data),
>>>>> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>>> // SAFETY: The initializer can write to the provided `slot`.
>>>>> unsafe { slot.write(opts.into_raw::<T>()) };
>>>>>
>>>>> - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>>>> - // get unregistered before `slot` is deallocated because the memory is pinned and
>>>>> - // the destructor of this type deallocates the memory.
>>>>> + // SAFETY:
>>>>> + // * We just wrote the misc device options to the slot. The miscdevice will
>>>>> + // get unregistered before `slot` is deallocated because the memory is pinned and
>>>>> + // the destructor of this type deallocates the memory.
>>>>> + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>>>> + // is possible.
>>>>> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>>> // misc device.
>>>>> to_result(unsafe { bindings::misc_register(slot) })
>>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>>> // before the underlying `struct miscdevice` is destroyed.
>>>>> unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>>> }
>>>>> +
>>>>> + /// Access the additional data stored in this registration.
>>>>> + pub fn data(&self) -> &T::RegistrationData {
>>>>> + // SAFETY:
>>>>> + // * No mutable reference to the value contained by `self.data` can ever be created.
>>>>> + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>>
>>>> Please add type invariants for these two requirements.
>>>>
>>>>> + unsafe { &*self.data.get() }
>>>>> + }
>>>>> }
>>>>>
>>>>> #[pinned_drop]
>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>> +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 and nothing uses it anymore.
>>>>
>>>> Ditto.
>>>
>>> I'm not quite sure how to formulate these, what do you think of:
>>>
>>> /// - `inner` is a registered misc device.
>>
>> This doesn't really mean something to me, maybe it's better to reference
>> the registering function?
>
> That is from previous code so this should probably not be changed
> in this series.
I personally wouldn't mind a commit that fixes this up, but if you don't
want to do it, let me know then we can make this a good-first-issue.
>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>
>> This sounds good. But help me understand, why do we need `Opaque` /
>> `UnsafePinned` again? If we're only using shared references, then we
>> could also just store the object by value?
>
> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
> so from what I understand having a `& RegistrationData` reference into that is UB without
> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
And the stored `T::RegistrationData` is shared as read-only with the C
side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
moment `Opaque`).
>>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
>>
>> What does "usable" mean?
>
> I guess valid / alive might be better wording?
>
> I meant to say that the `fops` functions might use the `RegistrationData` until
> `misc_deregister` has returned so we must ensure that these accesses are allowed.
Then use `valid`.
>>> /// - no mutable references to `data` may be created.
>>
>>>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>> /// What kind of pointer should `Self` be wrapped in.
>>>>> type Ptr: ForeignOwnable + Send + Sync;
>>>>>
>>>>> + /// 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()`].
>>>>> + type RegistrationData: Sync;
>>>>
>>>> Why do we require `Sync` here?
>>>
>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>
>> You could also just ask the type there to be `Sync`, then users will get
>> an error when they try to use `MiscDevice` in a way where
>> `RegistrationData` is required to be `Sync`.
>
> I don't think there is any point to allow defining a `MiscDevice` implementation
> that cant actually be used/registered.
Sure, but the bound asserting that it is `Sync` doesn't need to be here,
having it just on the `impl Sync for MiscDeviceRegistration` is good
enough. (though one could argue that people would get an earlier error
if it is already asserted here. I think we should have some general
guidelines here :)
>>>> We might want to give this a shorter name?
>>>
>>> I think its fine, but I am open to Ideas.
>>
>> `Data`?
>
> I feel that `Data` is just very ambiguous, especially since it is associated with
> `MiscDevice` not the `MiscDeviceRegistration` in which its used.
But it is the data of the MiscDevice, no?
> One Idea I've had was `AssociatedData` but that's less clear and not much shorter
> than `RegistrationData`.
Of the two, I'd prefer `RegistrationData`.
> But I'd be alright to just with `Data` if that is wanted.
If you think that `RegistrationData` is more clear then go with that.
But I honestly don't derive much meaning from that over just `Data`. You
can still of course mention in the docs that this data is stored in the
registration.
But since there is no other way to associate data to a `MiscDevice`, I
think it makes sense to call it `Data`.
---
Cheers,
Benno
On 05.06.25 6:05 PM, Benno Lossin wrote:
> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>> +// 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> {}
>>>>>
>>>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>>>> bound here even if it is vacuous today.
>>>>
>>>> Since a reference the `MiscDeviceRegistration` struct is an
>>>> argument to the open function this struct must always be Sync,
>>>> so adding bounds here doesn't make much sense.
>>>
>>> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
>>> even if `T::RegistrationData` is not `Sync` if that bound got removed
>>> at some point. And this "instability" is what I'm worried about.
>>>
>>>> I'll add this a safety comment in `MiscdeviceVTable::open`
>>>> about this.
>>>>
>>>> Is there a good way to assert this at build to avoid regessions?
>>>
>>> const _: () = {
>>> fn assert_sync<T: ?Sized + Sync>() {}
>>> fn ctx<T: MiscDevice>() {
>>> assert_sync::<T::RegistrationData>();
>>> }
>>> };
>>>
>>
>> I'll add the bound and a TODO about `assert_sync`, in `open`
>> where `Send` is required.
>>
>> I intend to write a patch for `assert_sync` later.
>
> Great :)
>
>>> That would also be fine with me if you insist on not adding the bound.
>>>
>>> (the `assert_sync` function should maybe be somewhere where everyone can
>>> use it)
>>>
>>>>>> impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>>>> /// Register a misc device.
>>>>>> - pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>>>> + pub fn register(
>>>>>> + opts: MiscDeviceOptions,
>>>>>> + data: impl PinInit<T::RegistrationData, Error>,
>>>>>> + ) -> impl PinInit<Self, Error> {
>>>>>> try_pin_init!(Self {
>>>>>> + data <- Opaque::pin_init(data),
>>>>>> inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>>>> // SAFETY: The initializer can write to the provided `slot`.
>>>>>> unsafe { slot.write(opts.into_raw::<T>()) };
>>>>>>
>>>>>> - // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>>>>> - // get unregistered before `slot` is deallocated because the memory is pinned and
>>>>>> - // the destructor of this type deallocates the memory.
>>>>>> + // SAFETY:
>>>>>> + // * We just wrote the misc device options to the slot. The miscdevice will
>>>>>> + // get unregistered before `slot` is deallocated because the memory is pinned and
>>>>>> + // the destructor of this type deallocates the memory.
>>>>>> + // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>>>>> + // is possible.
>>>>>> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>>>> // misc device.
>>>>>> to_result(unsafe { bindings::misc_register(slot) })
>>>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>>>> // before the underlying `struct miscdevice` is destroyed.
>>>>>> unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>>>> }
>>>>>> +
>>>>>> + /// Access the additional data stored in this registration.
>>>>>> + pub fn data(&self) -> &T::RegistrationData {
>>>>>> + // SAFETY:
>>>>>> + // * No mutable reference to the value contained by `self.data` can ever be created.
>>>>>> + // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>>>
>>>>> Please add type invariants for these two requirements.
>>>>>
>>>>>> + unsafe { &*self.data.get() }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> #[pinned_drop]
>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>> +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 and nothing uses it anymore.
>>>>>
>>>>> Ditto.
>>>>
>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>
>>>> /// - `inner` is a registered misc device.
>>>
>>> This doesn't really mean something to me, maybe it's better to reference
>>> the registering function?
>>
>> That is from previous code so this should probably not be changed
>> in this series.
>
> I personally wouldn't mind a commit that fixes this up, but if you don't
> want to do it, let me know then we can make this a good-first-issue.
I can do it, but I think it would make a good-first-issue so lets go
with that for now.
>
>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>
>>> This sounds good. But help me understand, why do we need `Opaque` /
>>> `UnsafePinned` again? If we're only using shared references, then we
>>> could also just store the object by value?
>>
>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>> so from what I understand having a `& RegistrationData` reference into that is UB without
>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>
> And the stored `T::RegistrationData` is shared as read-only with the C
> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
> moment `Opaque`).
Not really shared with the C side, but with the `open` implementation in
`MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).
Thinking about this has made me realize that the current code already is a bit
iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).
>
>>>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
>>>
>>> What does "usable" mean?
>>
>> I guess valid / alive might be better wording?
>>
>> I meant to say that the `fops` functions might use the `RegistrationData` until
>> `misc_deregister` has returned so we must ensure that these accesses are allowed.
>
> Then use `valid`.
Alright.
>
>>>> /// - no mutable references to `data` may be created.
>>>
>>>>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>> /// What kind of pointer should `Self` be wrapped in.
>>>>>> type Ptr: ForeignOwnable + Send + Sync;
>>>>>>
>>>>>> + /// 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()`].
>>>>>> + type RegistrationData: Sync;
>>>>>
>>>>> Why do we require `Sync` here?
>>>>
>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>
>>> You could also just ask the type there to be `Sync`, then users will get
>>> an error when they try to use `MiscDevice` in a way where
>>> `RegistrationData` is required to be `Sync`.
>>
>> I don't think there is any point to allow defining a `MiscDevice` implementation
>> that cant actually be used/registered.
>
> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
> having it just on the `impl Sync for MiscDeviceRegistration` is good
> enough. (though one could argue that people would get an earlier error
> if it is already asserted here. I think we should have some general
> guidelines here :)
That would require a `Send` bound in the `register` function,
since a `MiscDevice` with `!Sync` `Data` would be valid now
(meaning that `MiscDeviceRegistration` may also be `!Sync`).
If you want I can go with that. I'm not really sure if its
really better (tough I don't feel that strongly either
way).
>
>>>>> We might want to give this a shorter name?
>>>>
>>>> I think its fine, but I am open to Ideas.
>>>
>>> `Data`?
>>
>> I feel that `Data` is just very ambiguous, especially since it is associated with
>> `MiscDevice` not the `MiscDeviceRegistration` in which its used.
>
> But it is the data of the MiscDevice, no?
>
>> One Idea I've had was `AssociatedData` but that's less clear and not much shorter
>> than `RegistrationData`.
>
> Of the two, I'd prefer `RegistrationData`.
>
>> But I'd be alright to just with `Data` if that is wanted.
>
> If you think that `RegistrationData` is more clear then go with that.
> But I honestly don't derive much meaning from that over just `Data`. You
> can still of course mention in the docs that this data is stored in the
> registration.
>
> But since there is no other way to associate data to a `MiscDevice`, I
> think it makes sense to call it `Data`.
>
Alright I'll go with `Data` then.
> ---
> Cheers,
> Benno
On Thu Jun 5, 2025 at 6:52 PM CEST, Christian Schrefl wrote:
> On 05.06.25 6:05 PM, Benno Lossin wrote:
>> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>>> #[pinned_drop]
>>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>> +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 and nothing uses it anymore.
>>>>>>
>>>>>> Ditto.
>>>>>
>>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>>
>>>>> /// - `inner` is a registered misc device.
>>>>
>>>> This doesn't really mean something to me, maybe it's better to reference
>>>> the registering function?
>>>
>>> That is from previous code so this should probably not be changed
>>> in this series.
>>
>> I personally wouldn't mind a commit that fixes this up, but if you don't
>> want to do it, let me know then we can make this a good-first-issue.
>
> I can do it, but I think it would make a good-first-issue so lets go
> with that for now.
Feel free to open the issue :)
>>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>>
>>>> This sounds good. But help me understand, why do we need `Opaque` /
>>>> `UnsafePinned` again? If we're only using shared references, then we
>>>> could also just store the object by value?
>>>
>>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>>> so from what I understand having a `& RegistrationData` reference into that is UB without
>>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>>
>> And the stored `T::RegistrationData` is shared as read-only with the C
>> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
>> moment `Opaque`).
>
> Not really shared with the C side, but with the `open` implementation in
> `MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
> needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).
Ah yes, I meant "shared with other Rust code through the C side" ie the
pointer round-trips through C (that isn't actually relevant, but that's
why I mentioned C).
> Thinking about this has made me realize that the current code already is a bit
> iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
> should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
> i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).
It's fine, since all non-ZST fields are `Opaque`. Otherwise we'd need to
wrap all fields with that.
>>>>> /// - no mutable references to `data` may be created.
>>>>
>>>>>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>>> /// What kind of pointer should `Self` be wrapped in.
>>>>>>> type Ptr: ForeignOwnable + Send + Sync;
>>>>>>>
>>>>>>> + /// 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()`].
>>>>>>> + type RegistrationData: Sync;
>>>>>>
>>>>>> Why do we require `Sync` here?
>>>>>
>>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>>
>>>> You could also just ask the type there to be `Sync`, then users will get
>>>> an error when they try to use `MiscDevice` in a way where
>>>> `RegistrationData` is required to be `Sync`.
>>>
>>> I don't think there is any point to allow defining a `MiscDevice` implementation
>>> that cant actually be used/registered.
>>
>> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
>> having it just on the `impl Sync for MiscDeviceRegistration` is good
>> enough. (though one could argue that people would get an earlier error
>> if it is already asserted here. I think we should have some general
>> guidelines here :)
>
> That would require a `Send` bound in the `register` function,
> since a `MiscDevice` with `!Sync` `Data` would be valid now
> (meaning that `MiscDeviceRegistration` may also be `!Sync`).
>
> If you want I can go with that. I'm not really sure if its
> really better (tough I don't feel that strongly either
> way).
We don't lose anything by doing this, so I think we should do it.
If in the future someone invents a way `MiscDevice` that's only in the
current thread and it can be registered (so like a "thread-local"
`MiscDevice` :), then this will be less painful to change.
---
Cheers,
Benno
On 05.06.25 7:27 PM, Benno Lossin wrote:
> On Thu Jun 5, 2025 at 6:52 PM CEST, Christian Schrefl wrote:
>> On 05.06.25 6:05 PM, Benno Lossin wrote:
>>> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>>>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>>>> #[pinned_drop]
>>>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>>> +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 and nothing uses it anymore.
>>>>>>>
>>>>>>> Ditto.
>>>>>>
>>>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>>>
>>>>>> /// - `inner` is a registered misc device.
>>>>>
>>>>> This doesn't really mean something to me, maybe it's better to reference
>>>>> the registering function?
>>>>
>>>> That is from previous code so this should probably not be changed
>>>> in this series.
>>>
>>> I personally wouldn't mind a commit that fixes this up, but if you don't
>>> want to do it, let me know then we can make this a good-first-issue.
>>
>> I can do it, but I think it would make a good-first-issue so lets go
>> with that for now.
>
> Feel free to open the issue :)
I've opened [0]. I don't have the permissions to add tags for that.
[0]: https://github.com/Rust-for-Linux/linux/issues/1168
>
>>>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>>>
>>>>> This sounds good. But help me understand, why do we need `Opaque` /
>>>>> `UnsafePinned` again? If we're only using shared references, then we
>>>>> could also just store the object by value?
>>>>
>>>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>>>> so from what I understand having a `& RegistrationData` reference into that is UB without
>>>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>>>
>>> And the stored `T::RegistrationData` is shared as read-only with the C
>>> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
>>> moment `Opaque`).
>>
>> Not really shared with the C side, but with the `open` implementation in
>> `MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
>> needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).
>
> Ah yes, I meant "shared with other Rust code through the C side" ie the
> pointer round-trips through C (that isn't actually relevant, but that's
> why I mentioned C).
>
>> Thinking about this has made me realize that the current code already is a bit
>> iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
>> should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
>> i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).
>
> It's fine, since all non-ZST fields are `Opaque`. Otherwise we'd need to
> wrap all fields with that.
Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed.
That's what I meant by "a bit iffy".
>
>>>>>> /// - no mutable references to `data` may be created.
>>>>>
>>>>>>>> + unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>>>> /// What kind of pointer should `Self` be wrapped in.
>>>>>>>> type Ptr: ForeignOwnable + Send + Sync;
>>>>>>>>
>>>>>>>> + /// 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()`].
>>>>>>>> + type RegistrationData: Sync;
>>>>>>>
>>>>>>> Why do we require `Sync` here?
>>>>>>
>>>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>>>
>>>>> You could also just ask the type there to be `Sync`, then users will get
>>>>> an error when they try to use `MiscDevice` in a way where
>>>>> `RegistrationData` is required to be `Sync`.
>>>>
>>>> I don't think there is any point to allow defining a `MiscDevice` implementation
>>>> that cant actually be used/registered.
>>>
>>> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
>>> having it just on the `impl Sync for MiscDeviceRegistration` is good
>>> enough. (though one could argue that people would get an earlier error
>>> if it is already asserted here. I think we should have some general
>>> guidelines here :)
>>
>> That would require a `Send` bound in the `register` function,
>> since a `MiscDevice` with `!Sync` `Data` would be valid now
>> (meaning that `MiscDeviceRegistration` may also be `!Sync`).
>>
>> If you want I can go with that. I'm not really sure if its
>> really better (tough I don't feel that strongly either
>> way).
>
> We don't lose anything by doing this, so I think we should do it.
> If in the future someone invents a way `MiscDevice` that's only in the
> current thread and it can be registered (so like a "thread-local"
> `MiscDevice` :), then this will be less painful to change.
Alright but I doubt that realistic, since the `Data` would always at
least be shared between the owner of `MiscDeviceRegistration` and the
`fops` implementation. Meaning its always shared with syscall context
and I don't think it makes sense to have a registration owed in
that context.
Cheers
Christian
On Sat Jun 7, 2025 at 1:34 PM CEST, Christian Schrefl wrote: > Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed. > That's what I meant by "a bit iffy". What's fragile about it? That someone could add a non-opaque field to the struct? Or that one is not allowed to take an `&`? > Alright but I doubt that realistic, since the `Data` would always at > least be shared between the owner of `MiscDeviceRegistration` and the > `fops` implementation. Meaning its always shared with syscall context > and I don't think it makes sense to have a registration owed in > that context. That might be the case, but I think we should have this as a general design guideline: avoid unnecessary trait bounds. --- Cheers, Benno
On 07.06.25 5:37 PM, Benno Lossin wrote: > On Sat Jun 7, 2025 at 1:34 PM CEST, Christian Schrefl wrote: >> Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed. >> That's what I meant by "a bit iffy". > > What's fragile about it? That someone could add a non-opaque field to > the struct? Or that one is not allowed to take an `&`? Yeah that a added field could cause UB seems bad to me. > >> Alright but I doubt that realistic, since the `Data` would always at >> least be shared between the owner of `MiscDeviceRegistration` and the >> `fops` implementation. Meaning its always shared with syscall context >> and I don't think it makes sense to have a registration owed in >> that context. > > That might be the case, but I think we should have this as a general > design guideline: avoid unnecessary trait bounds. Alright seems fine to me. Cheers Christian
On Sat Jun 7, 2025 at 5:39 PM CEST, Christian Schrefl wrote: > On 07.06.25 5:37 PM, Benno Lossin wrote: >> On Sat Jun 7, 2025 at 1:34 PM CEST, Christian Schrefl wrote: >>> Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed. >>> That's what I meant by "a bit iffy". >> >> What's fragile about it? That someone could add a non-opaque field to >> the struct? Or that one is not allowed to take an `&`? > > Yeah that a added field could cause UB seems bad to me. Actually, I don't think it would be UB, since only the `data` field would be borrowed mutably/immutably at the same time, but not the new field. --- Cheers, Benno
On Wed, Jun 4, 2025 at 1:29 AM Benno Lossin <lossin@kernel.org> wrote: > > (the `assert_sync` function should maybe be somewhere where everyone can > use it) +1, and likely part of the prelude too. (We may want to put all these into an `assert.rs` or similar too.) Cheers, Miguel
On 04.06.25 10:48 AM, Miguel Ojeda wrote: > On Wed, Jun 4, 2025 at 1:29 AM Benno Lossin <lossin@kernel.org> wrote: >> >> (the `assert_sync` function should maybe be somewhere where everyone can >> use it) > > +1, and likely part of the prelude too. > > (We may want to put all these into an `assert.rs` or similar too.) I think that we can add it to `build_assert.rs`, since this would be a build time construct. Should I do this in a separate series? Cheers Christian
On Wed, Jun 4, 2025 at 11:54 AM Christian Schrefl <chrisi.schrefl@gmail.com> wrote: > > I think that we can add it to `build_assert.rs`, since this would > be a build time construct. Hmm... it definitely makes sense, but I am not sure we want to mix them -- the `build_assert!` is fairly special / different from the others, since it is not really at compile-time like the others (which is why it is a last-resort option), and it would be nice to have its implementation self-contained in a different mod/file too. Perhaps `compile_asserts.rs` or similar if we want to split them into compiletime/buildtime/runtime? > Should I do this in a separate series? Yeah, no worries, it can be done later. Thanks! Cheers, Miguel
© 2016 - 2025 Red Hat, Inc.