Re: [PATCH 3/4] rust: Add dma_fence abstractions

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Jun 01, 2026 at 03:14:05PM +0200, Philipp Stanner wrote:
> +Cc Boris
> 
> On Mon, 2026-06-01 at 14:55 +0200, Alice Ryhl wrote:
> > On Mon, Jun 1, 2026 at 2:34 PM Philipp Stanner <phasta@mailbox.org> wrote:
> > > 
> > > On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> > > > On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > > > > > +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() };
> > > > > 
> > > > > [Severity: High]
> > > > > Does this create an exclusive mutable reference to actively shared memory?
> > > > > 
> > > > > DriverFenceData can be accessed concurrently by other threads holding Fence
> > > > > references (for instance, when checking if the fence is signaled). Creating
> > > > > a mutable reference (&mut) in Rust asserts exclusive access and violates
> > > > > aliasing rules, which allows the compiler to make invalid optimization
> > > > > assumptions.
> > > > > 
> > > > > Could this use an immutable reference &*self.data.as_ptr() instead?
> > > > 
> > > > Yes, please use an immutable reference here.
> > > > 
> > > > > > +
> > > > > > +        fence_data.inner.inner.get()
> > > > > > +    }
> > > > > 
> > > > > [ ... ]
> > > > > 
> > > > > > +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() };
> > > > > 
> > > > > [Severity: High]
> > > > > Similar to DriverFence::as_raw(), does this also incorrectly create a
> > > > > mutable reference to shared data?
> > > > 
> > > > Here as well.
> > > 
> > > `data` is not shared. By design there is only ever one DriverFence, and
> > > the driver's data (`data.data`) is `Sync`.
> > > 
> > > But I guess an immutable one should do the trick, too.
> > 
> > There's only one DriverFence, but I can perform shared access to that
> > one DriverFence from two threads in parallel. You made the type Sync,
> > and this is what you are allowing when you do so.
> 
> Nope, DriverFence is just Send, not Sync.
> 
> data.data is Sync, but `data` in the code above is not the actual user
> data, but the memory backing it up.

Ok, well, it probably should be Sync. I don't see any &self methods that
can't be called from multiple threads in parallel.

> > > > > > +                bindings::dma_fence_signal_locked(fence);
> > > > > > +            }
> > > > > > +            bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > > > > > +        }
> > > > > > +
> > > > > > +        // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > > > > > +        // through some dma_fence callbacks right now. Access is being revoked
> > > > > > +        // above by signalling the fence. The DriverFenceAllowedData trait
> > > > > > +        // ensures that the data either does not need drop, or if it does it
> > > > > > +        // lives in a RcuBox which will delay dropping by one grace period, hence
> > > > > > +        // ensuring that all readers have disappeared.
> > > > > > +        unsafe { drop_in_place(self.data.as_ptr()) };
> > > > > 
> > > > > [Severity: Critical]
> > > > > Could this eagerly drop FenceCtx and user data before the C dma_fence
> > > > > refcount reaches 0?
> > > > > 
> > > > > The C dma_fence object can remain alive as long as consumers (like sync_file
> > > > > or the GPU scheduler) hold references to it. If the backend later invokes
> > > > > callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> > > > > attempt to read fctx.driver_name from the already-dropped FenceCtx:
> > > > > 
> > > > > FenceCtx::get_driver_name() {
> > > > >     ...
> > > > >     fctx.driver_name.as_char_ptr()
> > > > > }
> > > > > 
> > > > > Can this result in a use-after-free?
> > > > 
> > > > Hmm, I don't know about what sahisko said, but I don't think this is the
> > > > right way to do rcu freeing. I think the type's destructor should be
> > > > reserved for cases where the value becomes immediately unusable.
> > > 
> > > We could guard the strings with RcuBox, but we could not then guard the
> > > FenceCtx code against code UAF if we don't have the rcu_barrier().
> > > 
> > > Or could we?
> > > 
> > > If a rust module unloads, module::remove() should contain an
> > > rcu_barrier() (right??). Would that be enough to guard against the
> > > FenceCtx code being unloaded?
> > > 
> > > > 
> > > > For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> > > > from dereferencing the RcuBox will stay alive for a grace period, but
> > > > IMO once you run the destructor of the box itself, the *pointer* becomes
> > > > immediately unusable.
> > > 
> > > I don't know why you're stressing the pointer?
> > > 
> > > The trick above is simply that drop / dealloc *and* code unloading is
> > > delayed by a grace period.
> > 
> > Sorry let me try to rephrase. I'm not worried about the stuff behind
> > the pointer. After all, you're using RcuBox to protect that stuff.
> > What I'm worried about is the pointer itself. You invoked
> > drop_in_place() on the pointer to the fence context,
> > 
> 
> on the pointer to DriverFenceData, which contains a refcount to the
> FenceCtx, which might then want to drop.

Let me clarify.

You invoked drop_in_place on DriverFenceData which is effectively this
method:

fn drop_in_place::<DriverFenceData<F,C>>(self: *mut DriverFenceData) {
    drop_in_place::<Fence>(&raw mut (*self).fence);
    drop_in_place::<Arc<FenceCtx<F,C>>>(&raw mut (*self).fctx);
    drop_in_place::<F>(&raw mut (*self).data);
}

So yes you did indirectly invoke `drop_in_place` on Arc<FenceCtx<F,C>>.

> >  so even though
> > the fence context may be valid for another grace period, the *pointer*
> > to the fence context is not. The pointer could have been zeroed by the
> > destructor.
> 
> That particular pointer to the DriverFenceData could have been zeroed.
> But potential other accessors have already crafted themselves a new
> pointer to the, by the power of RCU, still valid data. That new pointer
> is container-of-ed from struct dma_fence *f.

I'm not talking about the pointer to DriverFenceData, I'm talking about
the pointer to the FenceCtx, or the pointer to the data (if F is
RcuBox).

The Arc type is not a type that opts-out of &mut == exclusive, so the
second drop_in_place() above is assumed exclusive access to the
Arc<FenceCtx<F,C>> field. If another thread obtains a pointer to the
FenceCtx via reading the fctx field of the DriverFence in parallel with
this, then that's not allowed because the drop_in_place() call has
exclusive access to that field.

This is why Boqun's RcuFreeSafe has a drop_in_place_before_gp() method
that can be used instead of drop_in_place() for this kind of use-case.
You must leave the fctx and data fields in a state where they contain
data that's safe to use for at least one grace period, and drop_in_place()
fundamentally does not leave the value in a usable state.

Alice