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

  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(+)
  

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?

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.

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
>
  

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

On Mon, Jan 03, 2022 at 12:13:41PM +0100, Christian König wrote:
> 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.o
> 
> > 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.

Yeah I think this would be much clearer instead of sprinkling half the
story as a scary&confusing warning over all kinds of users which
internally use dma fence arrays.

And then if it goes boom I guess we could fix it internally in
dma_fence_array_init by flattening fences down again. But only if actually
needed.

What confused me is why dma_resv is special, and from your reply it sounds
like it really isn't.
-Daniel


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

Am 14.01.22 um 17:31 schrieb Daniel Vetter:
> On Mon, Jan 03, 2022 at 12:13:41PM +0100, Christian König wrote:
>> 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.o
>>
>>> 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.
> Yeah I think this would be much clearer instead of sprinkling half the
> story as a scary&confusing warning over all kinds of users which
> internally use dma fence arrays.
>
> And then if it goes boom I guess we could fix it internally in
> dma_fence_array_init by flattening fences down again. But only if actually
> needed.

Ok, going to do that first then.

>
> What confused me is why dma_resv is special, and from your reply it sounds
> like it really isn't.

Well, it isn't special in any way. It's just something very obvious 
which could go wrong.

Regards,
Christian.

> -Daniel
>
>
>> 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
>>>>