Re: [PATCH 01/12] dma-buf: Add reference counting to dma_resv

["Christian König" <christian.koenig@amd.com>]

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.

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);
>>>> +}