On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> >
[…]
> > +use pin_init::pin_init_from_closure;
> > +
> > +use core::{
> > + marker::PhantomData, //
> > + ops::Deref,
> > + ptr,
> > + ptr::{
> > + drop_in_place,
> > + NonNull, //
> > + },
> > + sync::atomic::{
> > + AtomicU64,
> > + Ordering, //
> > + },
>
> Use atomics from the kernel crate instead.
OK.
>
> > +};
> > +
> > +use bindings::ECANCELED;
> > +
> > +use kernel::str::CString;
> > +use kernel::sync::{
> > + aref::{
> > + ARef,
> > + AlwaysRefCounted, //
> > + },
> > + Arc,
> > + ArcBorrow, //
> > +};
> > +
> > +/// VTable for dma_fence backend_ops callbacks.
> > +//
> > +// Mandatory dma_fence backend_ops are implemented implicitly through
> > +// [`FenceCtx`]. Additional ones shall get implemented on this trait,
> > which then
> > +// shall be demanded for the fence context data.
> > +pub trait FenceCtxOps {}
>
> This empty trait is unused.
(discussed in the other thread with Boris)
>
> > +/// A dma-fence context. A fence context takes care of associating related
> > fences with each other,
> > +/// providing each with raising sequence numbers and a common identifier.
> > +#[pin_data(PinnedDrop)]
> > +pub struct FenceCtx<F: Send + Sync, C: Send + Sync> {
>
> No need to list any trait bounds here. You can list them on `impl`
> blocks only.
ACK.
>
> >
[…]
> > + {
> > + // Uses `pin_init_from_closure` instead of `try_pin_init!` so that
> > on
> > + // `-ENOENT` (already signaled) the callback can be read back from
> > the
> > + // partially-initialized slot and returned through the error.
> > + //
> > + // SAFETY: `pin_init_from_closure` requires:
> > + // - On `Ok(())`: the slot is fully initialized and valid for
> > `Drop`.
> > + // - On `Err(_)`: the slot is clean, i.e.: no
> > partially-initialized fields
> > + // remain, and the slot can be deallocated without dropping.
> > + //
> > + // We uphold this as follows:
> > + // - On success: all three fields are initialized. Ok(()) is
> > returned.
> > + // - On ENOENT (already signaled): `callback` and `fence` are read
> > back
> > + // from the slot via `ptr::read`, leaving the slot clean. `cb`
> > was
> > + // initialized by `dma_fence_add_callback` (it calls
> > + // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is
> > + // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping
> > it is
> > + // fine. The callback is returned through `AlreadySignaled(T)`.
> > + // - On other errors: same cleanup as ENOENT, error returned as
> > + // `Other(e)`.
> > + unsafe {
> > + pin_init_from_closure(move |slot: *mut Self| {
> > + let slot_callback = &raw mut (*slot).callback;
> > + let slot_fence = &raw mut (*slot).fence;
> > + let slot_cb = &raw mut (*slot).cb;
> > +
> > + // Write callback and fence first — must be visible before
> > + // dma_fence_add_callback makes the registration live.
> > + core::ptr::write(slot_callback, callback);
> > + core::ptr::write(slot_fence, ARef::from(fence));
>
> Here you are incrementing the fence refcount. It's better to change the
> function argument to ARef<Fence> so that the user can avoid this
> increment if they happen to own a refcount they're willing to give up.
Agreed, will do
>
> > + let ret = to_result(bindings::dma_fence_add_callback(
> > + fence.inner.get(),
> > + Opaque::cast_into(slot_cb),
> > + Some(Self::dma_fence_callback),
> > + ));
> > +
> > + match ret {
> > + Ok(()) => Ok(()),
> > + Err(e) => {
> > + // Read back what we wrote to leave the slot clean.
> > + let cb_back = core::ptr::read(slot_callback);
> > + let _fence_back = core::ptr::read(slot_fence);
>
> This can be drop_in_place().
>
> > + if e.to_errno() == ENOENT.to_errno() {
> > + Err(CallbackError::AlreadySignaled(cb_back))
> > + } else {
> > + Err(CallbackError::Other(e))
> > + }
> > + }
> > + }
> > + })
> > + }
> > + }
> > +
> > + /// Raw dma fence callback that is called by the C code.
> > + ///
> > + /// # Safety
> > + ///
> > + /// This is only called by the dma_fence subsystem with valid pointers.
> > + unsafe extern "C" fn dma_fence_callback(
> > + _fence: *mut bindings::dma_fence,
> > + cb: *mut bindings::dma_fence_cb,
> > + ) {
> > + let ptr = Opaque::cast_from(cb).cast_mut();
> > +
> > + // SAFETY: All `cb` we can receive here have been created in such
> > a way
> > + // that they are embedded into a `FenceCbRegistration`. The backend
> > + // ensures synchronisation so whoever holds the registration object
> > + // cannot drop it while this code is running. See
> > `FenceCbRegistration::drop`.
> > + unsafe {
> > + let reg: *mut Self = container_of!(ptr, Self, cb);
> > +
> > + (*reg).callback.called();
> > + }
> > + }
> > +
> > + /// Returns a reference to the fence this callback is registered on.
> > + pub fn fence(self: Pin<&Self>) -> &Fence {
>
> Can be simplified to `fn fence(&self) -> &Fence`.
>
> > + &self.get_ref().fence
> > + }
> > +}
> > +
> > +#[pinned_drop]
> > +impl<T: FenceCb> PinnedDrop for FenceCbRegistration<T> {
> > + fn drop(self: Pin<&mut Self>) {
> > + // Always call dma_fence_remove_callback, even if `callback` has
> > already
> > + // been taken by `dma_fence_callback`. This is necessary for
> > + // synchronization: `dma_fence_remove_callback` acquires
> > `fence->lock`,
> > + // which ensures that any in-flight `dma_fence_signal` (which
> > calls our
> > + // callback while holding the same lock) has completed before we
> > free
> > + // the struct.
> > + //
> > + // Without this, Drop can race with a concurrent signal:
> > + // CPU0 (signal, lock held): take() -> signaled(fence_ref) (in
> > progress)
> > + // CPU1 (drop): sees is_some()==false -> skips lock -> frees
> > struct
> > + // CPU0: accesses fence_ref -> use-after-free
> > + //
> > + // When the callback has already fired, the signal path detached
> > the
> > + // list node via INIT_LIST_HEAD, so dma_fence_remove_callback just
> > sees
> > + // an empty node and returns false — the lock acquisition is the
> > only
> > + // thing that matters.
> > + //
> > + // SAFETY: The fence pointer is valid and the cb was initialized by
> > + // dma_fence_add_callback during construction.
> > + unsafe {
> > + bindings::dma_fence_remove_callback(self.fence.as_raw(),
> > self.cb.get());
> > + }
>
> Formatting nit: Usually the ; goes outside the unsafe block.
I could have sworn that it was rustfmt who did that? Maybe because the
; was inside to begin with.
>
> > + }
> > +}
> > +
> > +// SAFETY: FenceCbRegistration can be sent between threads
> > +unsafe impl<T: FenceCb> Send for FenceCbRegistration<T> {}
> > +
> > +// SAFETY: &FenceCbRegistration can be shared between threads if &T can.
> > +unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> where T: Sync {}
>
> There's no &FenceCbRegistration<T> -> &T accessor, so I don't think this
> bound is required.
>
> unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> {}
>
> There also can't be such an accessor in the future because the closure
> takes a &mut T.
Hm, very correct. The entire design only allows serial access.
>
> > +/// The receiving counterpart of a [`DriverFence`], designed to register
> > callbacks
> > +/// on, check the signalled state etc. A [`Fence`] cannot be signalled.
> > +/// A [`Fence`] is always refcounted.
> > +pub struct Fence {
> > + /// The actual dma_fence passed to C.
> > + inner: Opaque<bindings::dma_fence>,
> > +}
> > +
> > +// SAFETY: Fences are literally designed to be shared between threads.
> > +unsafe impl Send for Fence {}
> > +// SAFETY: Fences are literally designed to be shared between threads.
> > +unsafe impl Sync for Fence {}
> > +
> > +impl Fence {
> > + /// Check whether the fence was signalled at the moment of the
> > function call.
> > + pub fn is_signaled(&self) -> bool {
> > + // SAFETY: self is by definition still valid. The backend ensures
> > proper
> > + // locking.
> > + unsafe { bindings::dma_fence_is_signaled(self.as_raw()) }
> > + }
> > +
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + self.inner.get()
> > + }
> > +
> > + /// Create a [`Fence`] from a raw C [`bindings::dma_fence`].
> > + ///
> > + /// # Safety
> > + ///
> > + /// `ptr` must point to an initialized fence that is embedded into a
> > [`Fence`].
> > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::dma_fence) -> &'a Self {
> > + // SAFETY: Safe as per the function's overall safety requirements.
> > + unsafe { &*ptr.cast() }
> > + }
> > +}
> > +
> > +// SAFETY: These implement the C backends refcounting methods which are
> > proven to work correctly.
> > +unsafe impl AlwaysRefCounted for Fence {
> > + fn inc_ref(&self) {
> > + // SAFETY: `self.as_raw()` is a pointer to a valid `struct
> > dma_fence`.
> > + unsafe { bindings::dma_fence_get(self.as_raw()) }
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `ptr`must be a valid pointer to a [`DriverFence`].
> > + unsafe fn dec_ref(ptr: NonNull<Self>) {
> > + // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is
> > called
> > + // the fence is by definition still valid.
> > + let fence = unsafe { (*ptr.as_ptr()).inner.get() };
> > +
> > + // SAFETY: Valid because `fence` was created validly above.
> > + unsafe { bindings::dma_fence_put(fence) }
> > + }
> > +}
> > +
> > +#[repr(C)] // Necessary to guarantee that `inner` always comes first so
> > that we can cast.
> > +#[pin_data]
> > +struct DriverFenceData<F: Send + Sync, C: Send + Sync> {
>
> Ditto here about trait bounds. (And everywhere else.)
>
> > + #[pin]
> > + /// The inner fence.
> > + inner: Fence,
> > + /// Pointer to access the FenceCtx. Useful for obtaining name
> > parameters.
> > + // The FenceCtx lives as long as at least all its fences, hence this
> > is safe.
> > + fctx: Arc<FenceCtx<F, C>>,
> > + /// The API user's data. As required by [`DriverFenceAllowedData`],
> > this either
> > + /// does not need drop, or must live in a [`rcu::RcuBox`]. It is
> > essential
> > + /// that the data only performs operations legal in atomic context in
> > its
> > + /// [`Drop`] implementation.
> > + #[pin]
> > + data: F,
> > +}
> > +
> > +/// A trait to enforce that all data in a [`DriverFence`] either does not
> > need
> > +/// drop, or lives in a [`RcuBox`].
> > +pub trait DriverFenceAllowedData: private::Sealed {}
> > +
> > +mod private {
> > + pub trait Sealed {}
> > +}
> > +
> > +impl<F: Copy> DriverFenceAllowedData for F {}
> > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > +
> > +impl<F: Copy> private::Sealed for F {}
> > +impl<F: Send> private::Sealed for RcuBox<F> {}
>
> Why sealed? Just make the trait unsafe and require the things you
> require from the user.
This is far better. We definitely only allow the user to pass A or B,
and only then it compiles.
The unsafe implementation could be messed up.
I thought that's what Sealed is for. Or isn't it?
>
> > +/// A synchronization primitive mainly for GPU drivers.
> > +///
> > +/// Fences are always reference counted. The typical use case is that one
> > side registers
> > +/// callbacks on the fence which will perform a certain action (such as
> > queueing work) once the
> > +/// other side signals the fence.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::dma_buf::{DriverFence, FenceCtx, FenceCb,
> > FenceCbRegistration};
> > +/// use kernel::str::CString;
> > +/// use kernel::sync::{
> > +/// aref::ARef,
> > +/// rcu::RcuBox, //
> > +/// };
> > +/// use core::ops::Deref;
> > +/// use core::fmt::Display;
>
> Use fmt traits from kernel instead. (Actually, I don't think you use
> Display at all here?)
I tried, see a few lines below:
>
> > +/// struct CallbackData { }
> > +///
> > +/// impl FenceCb for CallbackData {
> > +/// fn called(&mut self) {
> > +/// pr_info!("DmaFence callback executed.\n");
> > +/// }
> > +/// }
> > +///
> > +/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
> > +/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
> > +///
> > +/// let fctx = FenceCtx::new(driver_name, timeline_name, ())?;
> > +///
> > +/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
> > +/// // DriverFence::data must either not need drop, or live in an RcuBox.
> > +/// let fence_data = RcuBox::new(fence_data, GFP_KERNEL)?;
> > +///
> > +/// let fence_alloc =
> > fctx.as_arc_borrow().new_fence_allocation(fence_data)?;
> > +/// let mut fence = fctx.new_fence(fence_alloc);
> > +///
> > +/// let cb_data = CallbackData { };
> > +/// let waiting_fence = ARef::from(fence.as_fence());
> > +/// let cb_reg = FenceCbRegistration::new(&waiting_fence, cb_data);
> > +/// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
> > +///
> > +/// // DriverFence implements Deref.
> > +/// // FIXME: unit test claims that CString does not implement Display.
> > Why?
> > +/// // pr_info!("Fence's inner data is: {}", fence.deref().deref());
Lazily, I was hoping that someone here will tell me how that is
supposed to be done correctly 8-)
> > +///
> > +/// // TODO begin_signalling
> > +/// fence.signal(Ok(()));
> > +/// assert_eq!(waiting_fence.is_signaled(), true);
> > +///
> > +/// Ok::<(), Error>(())
> > +/// ```
> > +pub struct DriverFence<F: Send + Sync, C: Send + Sync> {
> > + /// The actual content of the fence. Lives in a raw pointer so that its
> > + /// memory can be managed independently. Valid until both the
> > [`DriverFence`]
> > + /// and all associated [`Fence`]s have disappeared.
> > + data: NonNull<DriverFenceData<F, C>>,
> > +}
> > +
> > +/// A pre-prepared DMA fence, carrying the user's data and the memory it
> > and the
> > +/// fence reside in. Only useful for creating a [`DriverFence`]. Splitting
> > +/// allocation and full initialization is necessary because fences cannot
> > be
> > +/// allocated dynamically in some circumstances (deadlock).
> > +pub struct DriverFenceAllocation<F: Send + Sync, C: Send + Sync> {
> > + /// The memory for the actual content of the fence.
> > + /// Handed over to a [`DriverFence`], or deallocated once the
> > + /// [`DriverFenceAllocation`] drops.
> > + data: KBox<DriverFenceData<F, C>>,
> > +}
> > +
> > +impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync>
> > DriverFenceAllocation<F, C> {
> > + /// Create a new allocation slot that can later be used to create a
> > fully
> > + /// initialized [`DriverFence`] without the need to allocate.
> > + pub fn new(fctx: Arc<FenceCtx<F, C>>, data: F) -> Result<Self> {
> > + let fence_data = DriverFenceData {
> > + // `inner` remains uninitialized until a [`DriverFence`] takes
> > over.
> > + inner: Fence {
> > + inner: Opaque::uninit(),
> > + },
> > + fctx,
> > + data,
> > + };
> > +
> > + // In order to support the C dma_fence callbacks, it is necessary
> > for
> > + // a `Fence` and a `DriverFence` to live in the same allocation,
> > + // because the C backend passes a dma_fence, from which the driver
> > most
> > + // likely wants to be able to access its `data` in `DriverFence`.
> > + //
> > + // Hence, we need the manage the memory manually. It will be freed
> > by the
> > + // C backend automatically once the refcount within `Fence` drops
> > to 0.
> > + let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
> > +
> > + Ok(Self { data })
> > + }
> > +
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + self.data.inner.inner.get()
> > + }
> > +}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + // SAFETY: Valid because `self` is valid.
> > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > +
> > + fence_data.inner.inner.get()
> > + }
> > +
> > + /// Create a [`DriverFence`] from a raw pointer to a
> > [`bindings::dma_fence`].
> > + ///
> > + /// # Safety
> > + ///
> > + /// `ptr` must be a valid pointer to a `dma_fence` that was obtained
> > through
> > + /// a [`DriverFence`] with matching generic data for both fence and
> > associated
> > + /// [`FenceCtx`].
> > + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
> > + let opaque_fence = Opaque::cast_from(ptr);
> > +
> > + // SAFETY: Safe due to the function's overall safety requirements.
> > + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner)
> > };
> > +
> > + // DriverFenceData is repr(C) and a Fence is its first member.
> > + let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
> > +
> > + // SAFETY: `fence_data_ptr` was created validly above.
> > + let data = unsafe { NonNull::new_unchecked(fence_data_ptr) };
> > +
> > + Self { data }
> > + }
> > +
> > + /// Return the underlying [`Fence`].
> > + pub fn as_fence(&self) -> &Fence {
> > + // SAFETY: `self` is by definition still valid, and it cannot drop
> > until
> > + // this new reference is gone.
> > + unsafe { Fence::from_raw(self.as_raw()) }
> > + }
> > +
> > + /// Signal the fence. This will invoke all registered callbacks.
> > + pub fn signal(self, res: Result) {
> > + let fence = self.as_raw();
> > + let mut fence_flags: usize = 0;
> > + let flag_ptr = &raw mut fence_flags;
> > +
> > + // SAFETY: Once a `DriverFence` is initialized, the inner `fence`
> > is
> > + // valid and initialized. It is valid until the refcount drops
> > + // to 0, which can earliest happen once the `DriverFence` has been
> > dropped.
> > + unsafe {
> > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> > + if !bindings::dma_fence_is_signaled_locked(fence) {
> > + if let Err(err) = res {
> > + bindings::dma_fence_set_error(fence, err.to_errno());
> > + }
> > + bindings::dma_fence_signal_locked(fence);
> > + }
> > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > + }
>
> This single unsafe blocks spans five different unsafe operations.
Same discussion with Danilo. I'd prefer it this way, but I guess
separate blocks also have some advantages.
>
> > + }
> > +}
> > +
> > +// SAFETY: Fences are literally designed to be shared between threads.
> > +unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> {
> > + type Target = F;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: Thanks to refcounting, `data` is always valid as long
> > as `self` is.
> > + let data = unsafe { &*self.data.as_ptr() };
> > +
> > + &data.data
> > + }
> > +}
> > +
> > +/// A borrowed [`DriverFence`]. All you can do with it is access your user
> > data
> > +/// and obtain a [`Fence`].
> > +pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
> > + /// The actual content of the fence. Lives in a raw pointer so that its
> > + /// memory can be managed independently. Valid until both the
> > [`DriverFence`]
> > + /// and all associated [`Fence`]s have disappeared.
> > + data: NonNull<DriverFenceData<F, C>>,
> > +}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFenceBorrow<F, C> {
> > + type Target = F;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: Thanks to refcounting, `data` is always valid as long
> > as `self` is.
> > + let data = unsafe { &*self.data.as_ptr() };
> > +
> > + &data.data
> > + }
> > +}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + // SAFETY: Valid because `self` is valid.
> > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > +
> > + fence_data.inner.inner.get()
> > + }
> > +
> > + /// Return the underlying [`Fence`].
> > + pub fn as_fence(&self) -> &Fence {
> > + // SAFETY: `self` is by definition still valid, and it cannot drop
> > until
> > + // this new reference is gone.
> > + unsafe { Fence::from_raw(self.as_raw()) }
> > + }
> > +
> > + /// Get a [`DriverFenceBorrow`] from a raw pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// `ptr` must point to a raw dma_fence within a [`Fence`] within a
> > [`DriverFenceData`].
> > + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
> > + let opaque_fence = Opaque::cast_from(ptr);
> > +
> > + // SAFETY: Safe due to the function's overall safety requirements.
> > + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner)
> > };
> > +
> > + // DriverFenceData is repr(C) and a Fence is its first member.
> > + let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
> > +
> > + // SAFETY: `fence_data_ptr` was created validly above.
> > + let data = unsafe { NonNull::new_unchecked(fence_data_ptr) };
> > +
> > + Self { data }
> > + }
> > +}
> > +
> > +// SAFETY: The Rust dma_fence abstractions are already designed around the
> > inner
> > +// C `dma_fence`, which can serve safely as the identification point when
> > being
> > +// owned by C. Moreover, safety is ensured by not dropping `DriverFence`
> > and by
> > +// only allowing operations without side effects on the Borrowed type.
> > +unsafe impl<F: Send + Sync + 'static, C: Send + Sync + 'static>
> > ForeignOwnable
> > + for DriverFence<F, C>
> > +{
> > + // `DriverFence` is merely a wrapper around a raw pointer. Thus, we
> > can just
> > + // use it directly.
> > + type Borrowed<'a> = DriverFenceBorrow<F, C>;
> > + type BorrowedMut<'a> = DriverFenceBorrow<F, C>;
> > +
> > + const FOREIGN_ALIGN: usize =
> > core::mem::align_of::<bindings::dma_fence>();
> > +
> > + fn into_foreign(self) -> *mut c_void {
> > + let fence = self;
> > +
> > + let ptr = fence.as_raw();
> > +
> > + // DriverFence must not drop.
> > + core::mem::forget(fence);
>
> Nit: Modern Rust uses ManuallyDrop instead of forget().
You mean still take `self` here, then stuff it into ManuallyDrop and
let it go out of scope, aye?
Thx for the review,
P.
