On Mon, Jul 13, 2026 at 11:25:43AM +0200, Christian König wrote:
> On 7/12/26 21:51, Matthew Brost wrote:
> > On Sat, Jul 11, 2026 at 03:10:58PM +0200, Danilo Krummrich wrote:
> >> On Sat Jul 11, 2026 at 3:27 AM CEST, Matthew Brost wrote:
> >>> On Fri, Jul 10, 2026 at 08:52:41PM +0200, Christian König wrote:
> >>>> This provides clearer ownership semantics and makes the code more
> >>>> maintainable by removing the embedded allocation hack.
> >>>>
> >>>
> >>> This looks a lot better to me. In particular, I agree with the last
> >>> sentence in the commit message.
> >>
> >> I have to disagree with this, it is the opposite. As long as the struct
> >
> > Daniilo is of course correct here, completely missed this when I looked.
> >
> >> dma_resv::allocated fields and the corresponding semantics exists, this
> >> does
> >> result into less clear ownership semantics.
> >>
> >> When the dma_resv is embedded in another object the reference count becomes
> >> meaningless. If the object embedding the dma_resv is freed it doesn't
> >> matter
> >> whether I have a reference count, it would a UAF regardless.
> >>
> >
> > Yes, I agree. The allocated field would need to be dropped to make this
> > viable, and we would disallow embedding a dma-resv object into other
> > objects (which I believe is the suggestion).
>
> Yeah completely agree as well. This was basically just the first hacky
> version.
>
> > This doesn't look too painful, as I can only find two instances of
> > embedding in the kernel: drm_gem_object and i915_address_space and
> > handful of stack variables.
>
> The use case in the TTM BO deletion path is the only really ugly one as far
> as I can see.
>
> It uses the drm_gem_object embedded reservation object to make sure memory
> allocation can't fail during deletion.
>
Can't you just preallocate, during gem initialization, what is currently
the embedded reservation as a separate pointer, drop the reference to
the shared reservation, copy the fences, and then perform a pointer
swap during individualization?
Maybe I'm missing some details here, but when I briefly thought about
the individualization, it seemed fairly straightforward.
Matt
> We need something like a dummy delete_resv allocated for each TTM BO during
> creation or something like that to avoid this.
>
> But that in turn means potentially means taking a look at all TTM using
> drivers if/when they use this in their deletion path.
>
> Doable but a bit more work. Probably also a good job for AI.
>
> Question is also who is taking that work? @Natalie can you pick up from here?
>
> Regards,
> Christian.
>
> >
> > Matt
> >
> >> It is misleading (and hence error prone) to have an API where one can
> >> obtain a
> >> reference count of an object where the underlying memory can be freed
> >> regardless
> >> of the obtained reference count.
> >>
> >> A refernece count represents a shared ownership model, which is undermined
> >> if
> >> the underlying memory is not owned by the reference count.
> >>
> >> That said, I don't mind the reference count, but we can't mix up exclusive
> >> ownership (embedding a structure) and shared ownership (reference count).
> >>
> >>>> +static void dma_resv_release(struct kref *kref)
> >>>> {
> >>>> - /*
> >>>> - * This object should be dead and all references must have
> >>>> - * been released to it, so no need to be protected with rcu.
> >>>> - */
> >>>> + struct dma_resv *obj = container_of(kref, struct dma_resv,
> >>>> refcount);
> >>>> +
> >>>> dma_resv_list_free(rcu_dereference_protected(obj->fences,
> >>>> true));
> >>>> ww_mutex_destroy(&obj->lock);
> >>>> + if (obj->allocated)
> >>>> + kfree(obj);
> >>>> +}
>
