| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aEAT9B669Qvi0rlR@google.com>
Date: Wed, 4 Jun 2025 09:37:56 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Christian Schrefl <chrisi.schrefl@...il.com>, Miguel Ojeda <ojeda@...nel.org>,
Danilo Krummrich <dakr@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
"Björn Roy Baron" <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>,
Trevor Gross <tmgross@...ch.edu>, Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>,
Daniel Almeida <daniel.almeida@...labora.com>,
"Gerald Wisböck" <gerald.wisboeck@...ther.ink>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration
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?
Powered by blists - more mailing lists