Re: [PATCH 04/24] dma-buf: add dma_resv_get_singleton v2

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 22.12.21 um 22:21 schrieb Daniel Vetter:
> On Tue, Dec 07, 2021 at 01:33:51PM +0100, Christian König wrote:
>> Add a function to simplify getting a single fence for all the fences in
>> the dma_resv object.
>>
>> v2: fix ref leak in error handling
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-resv.c | 52 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/dma-resv.h   |  2 ++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>> index 480c305554a1..694716a3d66d 100644
>> --- a/drivers/dma-buf/dma-resv.c
>> +++ b/drivers/dma-buf/dma-resv.c
>> @@ -34,6 +34,7 @@
>>    */
>>   
>>   #include <linux/dma-resv.h>
>> +#include <linux/dma-fence-array.h>
>>   #include <linux/export.h>
>>   #include <linux/mm.h>
>>   #include <linux/sched/mm.h>
>> @@ -657,6 +658,57 @@ int dma_resv_get_fences(struct dma_resv *obj, bool write,
>>   }
>>   EXPORT_SYMBOL_GPL(dma_resv_get_fences);
>>   
>> +/**
>> + * dma_resv_get_singleton - Get a single fence for all the fences
>> + * @obj: the reservation object
>> + * @write: true if we should return all fences
>> + * @fence: the resulting fence
>> + *
>> + * Get a single fence representing all the fences inside the resv object.
>> + * Returns either 0 for success or -ENOMEM.
>> + *
>> + * Warning: This can't be used like this when adding the fence back to the resv
>> + * object since that can lead to stack corruption when finalizing the
>> + * dma_fence_array.
> Uh I don't get this one? I thought the only problem with nested fences is
> the signalling recursion, which we work around with the irq_work?

Nope, the main problem is finalizing the dma_fence_array.

E.g. imagine that you build up a chain of dma_fence_array objects like this:
a<-b<-c<-d<-e<-f.....

With each one referencing the previous dma_fence_array and then you call 
dma_fence_put() on the last one. That in turn will cause calling 
dma_fence_put() on the previous one, which in turn will cause 
dma_fence_put() one the one before the previous one etc....

In other words you recurse because each dma_fence_array instance drops 
the last reference of it's predecessor.

What we could do is to delegate dropping the reference to the containing 
fences in a dma_fence_array as well, but that would require some changes 
to the irq_work_run_list() function to be halve way efficient.

> Also if there's really an issue with dma_fence_array fences, then that
> warning should be on the dma_resv kerneldoc, not somewhere hidden like
> this. And finally I really don't see what can go wrong, sure we'll end up
> with the same fence once in the dma_resv_list and then once more in the
> fence array. But they're all refcounted, so really shouldn't matter.
>
> The code itself looks correct, but me not understanding what even goes
> wrong here freaks me out a bit.

Yeah, IIRC we already discussed that with Jason in length as well.

Essentially what you can't do is to put a dma_fence_array into another 
dma_fence_array without causing issues.

So I think we should maybe just add a WARN_ON() into 
dma_fence_array_init() to make sure that this never happens.

Regards,
Christian.

>
> I guess something to figure out next year, I kinda hoped I could squeeze a
> review in before I disappear :-/
> -Daniel
>
>> + */
>> +int dma_resv_get_singleton(struct dma_resv *obj, bool write,
>> +			   struct dma_fence **fence)
>> +{
>> +	struct dma_fence_array *array;
>> +	struct dma_fence **fences;
>> +	unsigned count;
>> +	int r;
>> +
>> +	r = dma_resv_get_fences(obj, write, &count, &fences);
>> +        if (r)
>> +		return r;
>> +
>> +	if (count == 0) {
>> +		*fence = NULL;
>> +		return 0;
>> +	}
>> +
>> +	if (count == 1) {
>> +		*fence = fences[0];
>> +		kfree(fences);
>> +		return 0;
>> +	}
>> +
>> +	array = dma_fence_array_create(count, fences,
>> +				       dma_fence_context_alloc(1),
>> +				       1, false);
>> +	if (!array) {
>> +		while (count--)
>> +			dma_fence_put(fences[count]);
>> +		kfree(fences);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	*fence = &array->base;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_resv_get_singleton);
>> +
>>   /**
>>    * dma_resv_wait_timeout - Wait on reservation's objects
>>    * shared and/or exclusive fences.
>> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
>> index fa2002939b19..cdfbbda6f600 100644
>> --- a/include/linux/dma-resv.h
>> +++ b/include/linux/dma-resv.h
>> @@ -438,6 +438,8 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
>>   void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence);
>>   int dma_resv_get_fences(struct dma_resv *obj, bool write,
>>   			unsigned int *num_fences, struct dma_fence ***fences);
>> +int dma_resv_get_singleton(struct dma_resv *obj, bool write,
>> +			   struct dma_fence **fence);
>>   int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
>>   long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr,
>>   			   unsigned long timeout);
>> -- 
>> 2.25.1
>>