[PATCH] rust: types: add Opaque::from_raw

[PATCH] rust: types: add Opaque::from_raw

[Alice Ryhl]

Since commit b20fbbc08a36 ("rust: check type of `$ptr` in
`container_of!`") we have enforced that the field pointer passed to
container_of! must match the declared field. This caused mismatches when
using a pointer to bindings::x for fields of type Opaque<bindings::x>.

This situation encourages the user to simply pass field.cast() to the
container_of! macro, but this is not great because you might
accidentally pass a *mut bindings::y when the field type is
Opaque<bindings::x>, which would be wrong.

To help catch this kind of mistake, add a new Opaque::from_raw that
wraps a raw pointer in Opaque without changing the inner type.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/drm/device.rs  | 4 +---
 rust/kernel/drm/gem/mod.rs | 4 +---
 rust/kernel/lib.rs         | 7 +++++++
 rust/kernel/types.rs       | 5 +++++
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index 624d7a4c83ead64b93325189f481d9b37c3c6eae..763c825d53aaba4f874361b785587b2c5129d49a 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -135,11 +135,9 @@ pub(crate) fn as_raw(&self) -> *mut bindings::drm_device {
     ///
     /// `ptr` must be a valid pointer to a `struct device` embedded in `Self`.
     unsafe fn from_drm_device(ptr: *const bindings::drm_device) -> *mut Self {
-        let ptr: *const Opaque<bindings::drm_device> = ptr.cast();
-
         // SAFETY: By the safety requirements of this function `ptr` is a valid pointer to a
         // `struct drm_device` embedded in `Self`.
-        unsafe { crate::container_of!(ptr, Self, dev) }.cast_mut()
+        unsafe { crate::container_of!(Opaque::from_raw(ptr), Self, dev) }.cast_mut()
     }
 
     /// Not intended to be called externally, except via declare_drm_ioctls!()
diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index 4cd69fa84318c3ff2cec57949e9bab05559a3c2f..5b80c248761bb39914a63ad7947aa8d3779054ef 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -125,11 +125,9 @@ fn as_raw(&self) -> *mut bindings::drm_gem_object {
     }
 
     unsafe fn as_ref<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self {
-        let self_ptr: *mut Opaque<bindings::drm_gem_object> = self_ptr.cast();
-
         // SAFETY: `obj` is guaranteed to be in an `Object<T>` via the safety contract of this
         // function
-        unsafe { &*crate::container_of!(self_ptr, Object<T>, obj) }
+        unsafe { &*crate::container_of!(Opaque::from_raw(self_ptr), Object<T>, obj) }
     }
 }
 
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..d2402d42b8776c9399a7dfdbe7bd61de7ef8dba3 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -204,6 +204,13 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 
 /// Produces a pointer to an object from a pointer to one of its fields.
 ///
+/// If you encounter a type mismatch due to the [`Opaque`] type, then use [`Opaque::raw_get`] or
+/// [`Opaque::from_raw`] to resolve the mismatch.
+///
+/// [`Opaque`]: crate::types::Opaque
+/// [`Opaque::raw_get`]: crate::types::Opaque::raw_get
+/// [`Opaque::from_raw`]: crate::types::Opaque::from_raw
+///
 /// # Safety
 ///
 /// The pointer passed to this macro, and the pointer returned by this macro, must both be in
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..a79295500b3c812326cea8a9d339a8545a7f457d 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -413,6 +413,11 @@ pub const fn get(&self) -> *mut T {
     pub const fn raw_get(this: *const Self) -> *mut T {
         UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
     }
+
+    /// The opposite operation of [`Opaque::raw_get`].
+    pub const fn from_raw(this: *const T) -> *const Self {
+        this.cast()
+    }
 }
 
 /// Types that are _always_ reference counted.

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250617-opaque-from-raw-ac5b8ef6faa2

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: types: add Opaque::from_raw

[Danilo Krummrich]

On Tue, Jun 17, 2025 at 01:36:47PM +0000, Alice Ryhl wrote:
> Since commit b20fbbc08a36 ("rust: check type of `$ptr` in
> `container_of!`") we have enforced that the field pointer passed to
> container_of! must match the declared field. This caused mismatches when
> using a pointer to bindings::x for fields of type Opaque<bindings::x>.
> 
> This situation encourages the user to simply pass field.cast() to the
> container_of! macro, but this is not great because you might
> accidentally pass a *mut bindings::y when the field type is
> Opaque<bindings::x>, which would be wrong.
> 
> To help catch this kind of mistake, add a new Opaque::from_raw that
> wraps a raw pointer in Opaque without changing the inner type.

The patch does more than that, it also adds a hint to container_of!() and fixes
up two occurences. I feel like we should split it up.

> +    /// The opposite operation of [`Opaque::raw_get`].
> +    pub const fn from_raw(this: *const T) -> *const Self {

Do we want to name this from_raw()? Usually from_raw() methods return either
Self or &'a Self.

Maybe something like cast_from() and rename raw_get() to cast_into()? I think
the latter may be confusing anyways, since it sounds like it would do somthing
with reference counting.

> +        this.cast()
> +    }

Re: [PATCH] rust: types: add Opaque::from_raw

[Tamir Duberstein]

On Tue, Jun 17, 2025 at 9:38 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Since commit b20fbbc08a36 ("rust: check type of `$ptr` in
> `container_of!`") we have enforced that the field pointer passed to
> container_of! must match the declared field. This caused mismatches when
> using a pointer to bindings::x for fields of type Opaque<bindings::x>.
>
> This situation encourages the user to simply pass field.cast() to the
> container_of! macro, but this is not great because you might
> accidentally pass a *mut bindings::y when the field type is
> Opaque<bindings::x>, which would be wrong.
>
> To help catch this kind of mistake, add a new Opaque::from_raw that
> wraps a raw pointer in Opaque without changing the inner type.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

> ---
>  rust/kernel/drm/device.rs  | 4 +---
>  rust/kernel/drm/gem/mod.rs | 4 +---
>  rust/kernel/lib.rs         | 7 +++++++
>  rust/kernel/types.rs       | 5 +++++
>  4 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 624d7a4c83ead64b93325189f481d9b37c3c6eae..763c825d53aaba4f874361b785587b2c5129d49a 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -135,11 +135,9 @@ pub(crate) fn as_raw(&self) -> *mut bindings::drm_device {
>      ///
>      /// `ptr` must be a valid pointer to a `struct device` embedded in `Self`.
>      unsafe fn from_drm_device(ptr: *const bindings::drm_device) -> *mut Self {
> -        let ptr: *const Opaque<bindings::drm_device> = ptr.cast();
> -
>          // SAFETY: By the safety requirements of this function `ptr` is a valid pointer to a
>          // `struct drm_device` embedded in `Self`.
> -        unsafe { crate::container_of!(ptr, Self, dev) }.cast_mut()
> +        unsafe { crate::container_of!(Opaque::from_raw(ptr), Self, dev) }.cast_mut()
>      }
>
>      /// Not intended to be called externally, except via declare_drm_ioctls!()
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 4cd69fa84318c3ff2cec57949e9bab05559a3c2f..5b80c248761bb39914a63ad7947aa8d3779054ef 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -125,11 +125,9 @@ fn as_raw(&self) -> *mut bindings::drm_gem_object {
>      }
>
>      unsafe fn as_ref<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self {
> -        let self_ptr: *mut Opaque<bindings::drm_gem_object> = self_ptr.cast();
> -
>          // SAFETY: `obj` is guaranteed to be in an `Object<T>` via the safety contract of this
>          // function
> -        unsafe { &*crate::container_of!(self_ptr, Object<T>, obj) }
> +        unsafe { &*crate::container_of!(Opaque::from_raw(self_ptr), Object<T>, obj) }
>      }
>  }
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c37f4da1866e993be6230bc6715841..d2402d42b8776c9399a7dfdbe7bd61de7ef8dba3 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -204,6 +204,13 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>
>  /// Produces a pointer to an object from a pointer to one of its fields.
>  ///
> +/// If you encounter a type mismatch due to the [`Opaque`] type, then use [`Opaque::raw_get`] or
> +/// [`Opaque::from_raw`] to resolve the mismatch.
> +///
> +/// [`Opaque`]: crate::types::Opaque
> +/// [`Opaque::raw_get`]: crate::types::Opaque::raw_get
> +/// [`Opaque::from_raw`]: crate::types::Opaque::from_raw
> +///
>  /// # Safety
>  ///
>  /// The pointer passed to this macro, and the pointer returned by this macro, must both be in
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..a79295500b3c812326cea8a9d339a8545a7f457d 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -413,6 +413,11 @@ pub const fn get(&self) -> *mut T {
>      pub const fn raw_get(this: *const Self) -> *mut T {
>          UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
>      }
> +
> +    /// The opposite operation of [`Opaque::raw_get`].
> +    pub const fn from_raw(this: *const T) -> *const Self {
> +        this.cast()
> +    }
>  }
>
>  /// Types that are _always_ reference counted.
>
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250617-opaque-from-raw-ac5b8ef6faa2
>
> Best regards,
> --
> Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: types: add Opaque::from_raw

[Alice Ryhl]

On Tue, Jun 17, 2025 at 03:54:19PM +0200, Danilo Krummrich wrote:
> On Tue, Jun 17, 2025 at 01:36:47PM +0000, Alice Ryhl wrote:
> > Since commit b20fbbc08a36 ("rust: check type of `$ptr` in
> > `container_of!`") we have enforced that the field pointer passed to
> > container_of! must match the declared field. This caused mismatches when
> > using a pointer to bindings::x for fields of type Opaque<bindings::x>.
> > 
> > This situation encourages the user to simply pass field.cast() to the
> > container_of! macro, but this is not great because you might
> > accidentally pass a *mut bindings::y when the field type is
> > Opaque<bindings::x>, which would be wrong.
> > 
> > To help catch this kind of mistake, add a new Opaque::from_raw that
> > wraps a raw pointer in Opaque without changing the inner type.
> 
> The patch does more than that, it also adds a hint to container_of!() and fixes
> up two occurences. I feel like we should split it up.

I think they go together pretty naturally, but I can split it if you
insist.

> > +    /// The opposite operation of [`Opaque::raw_get`].
> > +    pub const fn from_raw(this: *const T) -> *const Self {
> 
> Do we want to name this from_raw()? Usually from_raw() methods return either
> Self or &'a Self.
> 
> Maybe something like cast_from() and rename raw_get() to cast_into()? I think
> the latter may be confusing anyways, since it sounds like it would do somthing
> with reference counting.

The name raw_get() mirrors the stdlib function UnsafeCell::raw_get().
The stdlib uses this naming because in Rust the word "get" normally has
nothing to do with reference counting - outside of the kernel, we use
"clone" for incrementing refcounts and nobody would ever call it "get".
That said, it may still be worth to rename the method. Thoughts?

Alice

Re: [PATCH] rust: types: add Opaque::from_raw

[Danilo Krummrich]

On Wed, Jun 18, 2025 at 07:59:20AM +0000, Alice Ryhl wrote:
> On Tue, Jun 17, 2025 at 03:54:19PM +0200, Danilo Krummrich wrote:
> > On Tue, Jun 17, 2025 at 01:36:47PM +0000, Alice Ryhl wrote:
> > > Since commit b20fbbc08a36 ("rust: check type of `$ptr` in
> > > `container_of!`") we have enforced that the field pointer passed to
> > > container_of! must match the declared field. This caused mismatches when
> > > using a pointer to bindings::x for fields of type Opaque<bindings::x>.
> > > 
> > > This situation encourages the user to simply pass field.cast() to the
> > > container_of! macro, but this is not great because you might
> > > accidentally pass a *mut bindings::y when the field type is
> > > Opaque<bindings::x>, which would be wrong.
> > > 
> > > To help catch this kind of mistake, add a new Opaque::from_raw that
> > > wraps a raw pointer in Opaque without changing the inner type.
> > 
> > The patch does more than that, it also adds a hint to container_of!() and fixes
> > up two occurences. I feel like we should split it up.
> 
> I think they go together pretty naturally, but I can split it if you
> insist.

Nah, it's also fine for me to keep it as is, but in that case we should also
mention the other changes in the commit message.

> > > +    /// The opposite operation of [`Opaque::raw_get`].
> > > +    pub const fn from_raw(this: *const T) -> *const Self {
> > 
> > Do we want to name this from_raw()? Usually from_raw() methods return either
> > Self or &'a Self.
> > 
> > Maybe something like cast_from() and rename raw_get() to cast_into()? I think
> > the latter may be confusing anyways, since it sounds like it would do somthing
> > with reference counting.
> 
> The name raw_get() mirrors the stdlib function UnsafeCell::raw_get().
> The stdlib uses this naming because in Rust the word "get" normally has
> nothing to do with reference counting - outside of the kernel, we use
> "clone" for incrementing refcounts and nobody would ever call it "get".

Yeah, I'm aware. The main reason I proposed cast_into() is that it would fit
much better with the newly introduced cast_from() (in case we go with that
name).

I'm happy with other proposals too, I just think that from_raw() would be a bit
inconsistent with how we use this name otherwise.

> That said, it may still be worth to rename the method. Thoughts?

I think it'd be good if it aligns naming wise with its counter part added in
this patch and I don't see this happening with raw_get(). :-)