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

[Alice Ryhl <aliceryhl@google.com>]

On Tue, Jun 02, 2026 at 02:06:43PM +0200, Philipp Stanner wrote:
> On Tue, 2026-06-02 at 11:59 +0000, Alice Ryhl wrote:
> > On 
> 
> […]
> 
> > > > 
> > > > If you don't implement Sync, then DriverFence cannot be stored in an
> > > > Arc. I wouldn't take away that ability unless you have to, and I don't
> > > > see anything in the DriverFence API that would mean you can't do that.
> > > 
> > > Nope. We explicitly agreed on this design.
> > > 
> > > Just 1 DriverFence. Just 1 party that can signal it.
> > > Note that we also agreed upon the Driverfence disappearing with
> > > .signal(), which certainly prevents several from existing, unless you
> > > do an Option.take()
> > 
> > I would like to clarify that I'm not suggesting any changes to the
> > design. Implementing Sync is not the same as having multiple driver
> > fences.
> 
> I mean, I guess one can do that. But it's up to the driver then to see how it can signal its fence.

I don't believe Sync changes anything with that regard. The signal
method takes 'self', but the Sync trait only affects how '&self' methods
can be called.

> > > > > > > >  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).
> > > > > 
> > > > > Yeah, but the backing memory is still alive. And new pointers to that
> > > > > memory get crafted by the accessors. If a callback accesses the data
> > > > > through `container_of(Fence)`, it gets a new pointer.
> > > > > 
> > > > > So what's the problem?
> > > > > 
> > > > > Where is the invalid pointer that someone is accessing?
> > > > > 
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > OK, so I think I see the problem. So the invalid pointer is
> > > > > Arc<FenceCtx…>? And potentially the <F> pointer (although we don't have
> > > > > a picture yet as to how that would be accessed through other callbacks.
> > > > > 
> > > > > >  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.
> > > > > 
> > > > > I think I have been asking in several of our meetings in the past
> > > > > whether it is actually a problem to access data that has been dropped()
> > > > > IF we know that drop does not cause UAF and the answer was kind of like
> > > > > a "well if it does not actually get freed…"
> > > > 
> > > > Ok, well, IMO the simplest approach is to say you can't. There may be
> > > > roundabout ways to do it, but I would suggest that we just ... don't.
> > > 
> > > Ack.
> > > 
> > > > 
> > > > > Anyways.
> > > > > 
> > > > > It would seem the way to get this right is then
> > > > > 
> > > > > synchronize_rcu();
> > > > > drop_in_palace(data);
> > > > > 
> > > > > 
> > > > > Agreed?
> > > > > 
> > > > > This would then mean, however, that every time a fence drops, you have
> > > > > to wait a grace period.
> > > > > 
> > > > > Or maybe stuff DriverFenceData into an RcuBox, too, and defer its
> > > > > dropping.
> > > > 
> > > > That would work, but I think we can do better and avoid the
> > > > synchronize_rcu() along these lines:
> > > > 
> > > > unsafe trait RcuRevocable {
> > > >     unsafe fn rcu_revoke_in_place(ptr: *mut Self);
> > > > }
> > > > 
> > > > This trait provides a method that's like drop_in_place(), except that
> > > > when you use this destructor, the value remains usable for one grace
> > > > period. You could implement it for RcuBox, and for any Copy type, and
> > > > for ARef<T> when T is cleaned up with rcu, and probably also other
> > > > stuff.
> > > 
> > > I mean, this cannot be magic. It also boils down to executing one RCU
> > > callback per DriverFence dropping.
> > > 
> > > Is there a significant difference to stuffing DriverFenceData into an
> > > RcuBox?
> > 
> > Do you mean hard-coding that the user-data of a driver fence is always
> > stored in an RcuBox?
> 
> 
> I'm talking about this:
> 
> 
> 
> 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 = RcuBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
> 
>         Ok(Self { data })
>     }
> 
> 
> This way, the entire DriverFenceData will remain valid for an
> additional grace period. I suppose this would solve your pointer-
> invalid concern.
> 
> However, it appears like overkill to me because the refcounting + C
> backend already ensure that nothing drops too soon, and the backend
> frees with kfree_rcu(), so…

I agree that it doesn't sound like we want RcuBox here.

What kind of metadata are we actually planning to store in the
DriverFence in practice?

Alice