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

[Alice Ryhl <aliceryhl@google.com>]

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.

> > +
> > +        fence_data.inner.inner.get()
> > +    }
> 
> [ ... ]
> 
> > +impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
> > +    fn drop(&mut self) {
> > +        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);
> > +            #[allow(unused_unsafe)]
> > +            if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
> > +                bindings::dma_fence_set_error(fence, ECANCELED as i32);
> 
> [Severity: High]
> Is it intentional to pass a positive error code here?
> 
> bindings::ECANCELED evaluates to a positive integer, but
> dma_fence_set_error() expects a negative error code (and triggers a WARN_ON
> for positive values). A positive value might also be interpreted as success
> by callers checking for values < 0.
> 
> Should this be - (bindings::ECANCELED as i32)?

Please use ECANCELED.to_errno(). Do not import these from bindings.

> > +                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.

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.

Alice