On Thu, May 01, 2025 at 02:33:17PM -0400, Lyude Paul wrote:
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index df8f9fdae5c22..f70531889c21f 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -45,8 +45,12 @@ pub trait IntoGEMObject: Sized + super::private::Sealed {
> #[allow(clippy::wrong_self_convention)]
> fn into_gem_obj(&self) -> &Opaque<bindings::drm_gem_object>;
>
> - /// Converts a pointer to a `struct drm_gem_object` into a pointer to
> `Self`.
> - fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self;
> + /// Converts a pointer to a `struct drm_gem_object` into a reference to
> `Self`.
> + ///
> + /// # Safety
> + ///
> + /// `self_ptr` must be a valid pointer to `Self`.
Is this really a requirement? I think this should just be "`ptr` must point to
a `struct drm_gem_object` represented through `Self`". How exactly the
implementer does the conversion depends on Self, no?
> @@ -144,11 +147,25 @@ fn lookup_handle(
> ) -> Result<ARef<Self>> {
> // SAFETY: The arguments are all valid per the type invariants.
> let ptr = unsafe {
> bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) };
> - let ptr = <Self as IntoGEMObject>::from_gem_obj(ptr);
> - let ptr = NonNull::new(ptr).ok_or(ENOENT)?;
>
> - // SAFETY: We take ownership of the reference of
> `drm_gem_object_lookup()`.
> - Ok(unsafe { ARef::from_raw(ptr) })
> + // SAFETY:
> + // - A `drm::Driver` can only have a single `File` implementation.
> + // - `file` uses the same `drm::Driver` as `Self`.
> + // - Therefore, we're guaranteed that `ptr` must be a gem object
> embedded within `Self`.
> + // - And we check if the pointer is null befoe calling as_ref(),
> ensuring that `ptr` is a
> + // valid pointer to an initialized `Self`.
> + // XXX: The expect lint here is to workaround
> + // https://github.com/rust-lang/rust-clippy/issues/13024
> + #[expect(clippy::undocumented_unsafe_blocks)]
> + let obj = (!ptr.is_null())
> + .then(|| unsafe { Self::as_ref(ptr) })
> + .ok_or(ENOENT)?;
Maybe simply go for
if ptr.is_null() {
return Err(ENOENT);
}
which should be much easier to parse for kernel developers just starting to look
at Rust code anyways.
