From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: rust: drm: gem: Introduce shmem::SGTable
Date: Sun, 12 Apr 2026 12:01:18 +1000
Message-ID: <review-patch4-20260409001559.622026-5-lyude@redhat.com>
In-Reply-To: <20260409001559.622026-5-lyude@redhat.com>
References: <20260409001559.622026-1-lyude@redhat.com> <20260409001559.622026-5-lyude@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the core Rust abstraction patch. The design =E2=80=94 `SGTableMap` =
as a Devres-managed resource, `SGTable` as an owned handle =E2=80=94 is wel=
l thought out. However:

**Object destruction ordering (potential use-after-free):** This is the mai=
n concern. Consider the normal buffer teardown path where a GEM object is f=
reed while the driver is still bound:

1. `free_callback` calls `drm_gem_shmem_release(shmem)` =E2=86=92 frees sgt=
 (sets `shmem->sgt =3D NULL`), calls `drm_gem_object_release()` (destroys t=
he `dma_resv`)
2. `KBox::from_raw(this)` drops the `Object<T>`, which drops `sgt_res`
3. `Devres<SGTableMap>` is dropped, deregistering the devres action
4. If the `SGTableMap` inside the `Revocable` hasn't been revoked, its `Dro=
p` runs:

```rust
impl<T: DriverObject> Drop for SGTableMap<T> {
    fn drop(&mut self) {
        let obj =3D unsafe { self.obj.as_ref() };
        unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut(=
)) };
        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem=
()) };
        unsafe { bindings::dma_resv_unlock(obj.raw_dma_resv()) };
    }
}
```

At step 4, `dma_resv` has already been destroyed by `drm_gem_object_release=
()`, so `dma_resv_lock` is UB. And `shmem->sgt` is NULL, so `__drm_gem_shme=
m_free_sgt_locked` would dereference NULL.

The driver-unbind case (Case 1) is fine: Devres revokes `SGTableMap` before=
 the object is freed. But the normal-teardown case (Case 2) needs to be add=
ressed. Possible fixes: clear/revoke `sgt_res` in `free_callback` before ca=
lling `drm_gem_shmem_release`, or add a NULL guard to `__drm_gem_shmem_free=
_sgt_locked`.

**`dma_resv_lock` return value ignored:** In `create_sg_table`:

```rust
unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
```

With `ctx=3DNULL` this effectively can't fail (it's a plain mutex lock), bu=
t the return value is silently discarded. A `// SAFETY` comment should note=
 that with ctx=3DNULL, dma_resv_lock cannot return EDEADLK and thus cannot =
fail, or use `to_result()` to be explicit.

**`NonNull<Object<T>>` in SGTableMap is a raw pointer without refcounting:**

```rust
pub struct SGTableMap<T: DriverObject> {
    obj: NonNull<Object<T>>,
}
```

The soundness of this depends entirely on the Devres lifecycle guaranteeing=
 that `SGTableMap` is revoked before the Object is freed. Per the destructi=
on-ordering issue above, this invariant can be violated.

**C-style comment syntax in safety comment:**

```rust
// SAFETY: We grabbed the lock required for calling this function above */
```

There's a trailing `*/` from a C-style comment.

---

---
Generated by Claude Code Patch Reviewer