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: Add dma_fence abstractions
Date: Thu, 04 Jun 2026 15:39:36 +1000
Message-ID: <review-patch3-20260530143541.229628-5-phasta@kernel.org>
In-Reply-To: <20260530143541.229628-5-phasta@kernel.org>
References: <20260530143541.229628-2-phasta@kernel.org> <20260530143541.229628-5-phasta@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Author: Philipp Stanner**

This is the core patch and contains the most substance. The design is ambit=
ious and well-motivated. Several issues:

#### Bug: Positive errno passed to `dma_fence_set_error`

In `DriverFence::drop()`:
```rust
bindings::dma_fence_set_error(fence, ECANCELED as i32);
```

`bindings::ECANCELED` is the positive value `125` (from `include/uapi/asm-g=
eneric/errno.h`). The C `dma_fence_set_error` requires a **negative** errno=
 and will trigger `WARN_ON(error >=3D 0)`:

```c
static inline void dma_fence_set_error(struct dma_fence *fence, int error)
{
    WARN_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
    WARN_ON(error >=3D 0 || error < -MAX_ERRNO);
    fence->error =3D error;
}
```

Furthermore, `fence->error` is set to the positive value `125`, which code =
checking `fence->error < 0` will not recognize as an error. This should be:
```rust
bindings::dma_fence_set_error(fence, -(ECANCELED as i32));
```
Or better, use the kernel error infrastructure consistently (e.g., `kernel:=
:error::code::ECANCELED.to_errno()`).

Note: the `signal()` method correctly uses `err.to_errno()` (which returns =
a negative value), so only the drop path is affected.

#### Potential soundness concern: FenceCtx lifetime after DriverFence drop

When `DriverFence::drop()` runs, it:
1. Signals the fence (setting `DMA_FENCE_FLAG_SIGNALED_BIT`)
2. Calls `drop_in_place(self.data.as_ptr())` =E2=80=94 drops the `Arc<Fence=
Ctx>`, the user data, etc.
3. Calls `dma_fence_put(fence)`

The C backend's `dma_fence_driver_name()` and `dma_fence_timeline_name()` c=
heck the signaled flag:
```c
if (!dma_fence_test_signaled_flag(fence))
    return (const char __rcu *)ops->get_driver_name(fence);
else
    return (const char __rcu *)"detached-driver";
```

This prevents NEW callers from invoking the callback after signaling. Howev=
er, there's a TOCTOU race: a caller that reads `signaled_flag =3D=3D false`=
 before step 1, but executes the callback after step 2, will access the dro=
pped `Arc<FenceCtx>`. The `get_driver_name` callback (`FenceCtx::get_driver=
_name`) does:
```rust
extern "C" fn get_driver_name(ptr: *mut bindings::dma_fence) -> *const c_ch=
ar {
    let fctx =3D unsafe { Self::from_raw_fence(ptr) };
    fctx.driver_name.as_char_ptr()
}
```

Where `from_raw_fence` accesses `fence_data.fctx` =E2=80=94 a field that wa=
s logically dropped by `drop_in_place`. If this was the last `Arc<FenceCtx>=
`, the `FenceCtx` memory (including `driver_name` and `timeline_name`) is f=
reed. The `rcu_barrier()` in `FenceCtx::drop` waits for pending `call_rcu` =
callbacks, but does **not** wait for in-flight RCU read sections (that woul=
d require `synchronize_rcu()`).

The C code has the same race pattern but is safe because `get_driver_name` =
returns static strings. The Rust abstraction uses dynamically allocated `CS=
tring`s, making this a potential UAF.

Possible fixes:
- Add `synchronize_rcu()` between signaling and `drop_in_place` to ensure a=
ll in-flight RCU readers have completed.
- Store the name strings in the `DriverFenceData` directly instead of going=
 through `FenceCtx`.
- Use `RcuBox` (or similar deferred-free wrapper) for the `Arc<FenceCtx>` s=
tored in `DriverFenceData`.

#### `__GFP_ZERO` is ineffective in `DriverFenceAllocation::new`

```rust
let data =3D KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
```

`KBox::new` allocates memory (zeroed with `__GFP_ZERO`), then **moves** the=
 `fence_data` struct on top of it, overwriting the zeros. The `Opaque::unin=
it()` field in the struct carries indeterminate bytes from the stack, so th=
e dma_fence part of the allocation is NOT actually zeroed despite the flag.=
 The `__GFP_ZERO` gives a false sense of security =E2=80=94 it should eithe=
r be removed (since `dma_fence_init` fully initializes the struct) or repla=
ced with an approach that actually zeroes the `Opaque` field.

#### `FenceCtxOps` trait is unused

```rust
pub trait FenceCtxOps {}
```

Declared but never referenced. It should either be implemented as part of t=
his series or removed to avoid dead code in the public API.

#### Missing `#[repr(transparent)]` on `Fence`

`Fence::from_raw` casts `*mut bindings::dma_fence` directly to `*const Fenc=
e`:
```rust
pub unsafe fn from_raw<'a>(ptr: *mut bindings::dma_fence) -> &'a Self {
    unsafe { &*ptr.cast() }
}
```

And the `DriverFenceData` `#[repr(C)]` layout assumes `Fence` starts at off=
set 0 and has the same address as its inner `Opaque<bindings::dma_fence>`. =
Without `#[repr(transparent)]` on `Fence`, this layout is not guaranteed by=
 Rust's ABI (though it holds in practice for single-field structs). Adding =
`#[repr(transparent)]` would make the safety argument explicit.

#### `DriverFence::as_raw` creates `&mut` from shared state

```rust
fn as_raw(&self) -> *mut bindings::dma_fence {
    let fence_data =3D unsafe { &mut *self.data.as_ptr() };
    fence_data.inner.inner.get()
}
```

This creates a `&mut DriverFenceData` from `&self`, which is unsound if any=
 other reference exists. Since `Opaque::get()` only requires `&self` (it re=
turns `*mut T` through `UnsafeCell`), this should be `&*self.data.as_ptr()`=
 instead of `&mut *self.data.as_ptr()`. The same issue exists in `DriverFen=
ceBorrow::as_raw`.

#### Incomplete re-exports in `dma_buf/mod.rs`

The module exports:
```rust
pub use self::dma_fence::{
    DriverFence, Fence, FenceCb, FenceCbRegistration, FenceCtx,
};
```

But users also need `DriverFenceAllocation`, `CallbackError`, and `DriverFe=
nceAllowedData` to use the API. These should be re-exported.

#### No `cfg` gate for `dma_buf` module

```rust
pub mod dma_buf;
```

Unlike `pub mod drm` (which is gated on `CONFIG_DRM`), `dma_buf` is uncondi=
tionally compiled. This is probably fine since dma-fence is core infrastruc=
ture, but it should be verified that the helpers and bindings compile in mi=
nimal kernel configurations.

---

---
Generated by Claude Code Patch Reviewer