[PATCH 1/3] dma-buf/dma-fence_array: use kvzalloc

[Christian König]

Am 07.11.24 um 12:29 schrieb Tvrtko Ursulin:
>
> On 28/10/2024 10:34, Christian König wrote:
>> Am 25.10.24 um 11:05 schrieb Tvrtko Ursulin:
>>>
>>> On 25/10/2024 09:59, Tvrtko Ursulin wrote:
>>>>
>>>> On 24/10/2024 13:41, Christian König wrote:
>>>>> Reports indicates that some userspace applications try to merge 
>>>>> more than
>>>>> 80k of fences into a single dma_fence_array leading to a warning from
>>>>> kzalloc() that the requested size becomes to big.
>>>>>
>>>>> While that is clearly an userspace bug we should probably handle 
>>>>> that case
>>>>> gracefully in the kernel.
>>>>>
>>>>> So we can either reject requests to merge more than a reasonable 
>>>>> amount of
>>>>> fences (64k maybe?) or we can start to use kvzalloc() instead of 
>>>>> kzalloc().
>>>>> This patch here does the later.
>>>>
>>>> Rejecting would potentially be safer, otherwise there is a path for 
>>>> userspace to trigger a warn in kvmalloc_node (see 0829b5bcdd3b 
>>>> ("drm/i915: 2 GiB of relocations ought to be enough for anybody*")) 
>>>> and spam dmesg at will.
>>>
>>> Actually that is a WARN_ON_*ONCE* there so maybe not so critical to 
>>> invent a limit. Up for discussion I suppose.
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>
>>>> Question is what limit to set...
>>
>> That's one of the reasons why I opted for kvzalloc() initially.
>
> I didn't get that, what was the reason? To not have to invent an 
> arbitrary limit?

Well that I couldn't come up with any arbitrary limit that I had 
confidence would work and not block real world use cases.

Switching to kvzalloc() just seemed the more defensive approach.

>
>> I mean we could use some nice round number like 65536, but that would 
>> be totally arbitrary.
>
> Yeah.. Set an arbitrary limit so a warning in __kvmalloc_node_noprof() 
> is avoided? Or pass __GFP_NOWARN?

Well are we sure that will never hit 65536 in a real world use case? 
It's still pretty low.

>
>> Any comments on the other two patches? I need to get them upstream.
>
> Will look into them shortly.

Thanks,
Christian.

>
> Regards,
>
> Tvrtko
>
>
>> Thanks,
>> Christian.
>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>> CC: stable at vger.kernel.org
>>>>> ---
>>>>>   drivers/dma-buf/dma-fence-array.c | 6 +++---
>>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-fence-array.c 
>>>>> b/drivers/dma-buf/dma-fence-array.c
>>>>> index 8a08ffde31e7..46ac42bcfac0 100644
>>>>> --- a/drivers/dma-buf/dma-fence-array.c
>>>>> +++ b/drivers/dma-buf/dma-fence-array.c
>>>>> @@ -119,8 +119,8 @@ static void dma_fence_array_release(struct 
>>>>> dma_fence *fence)
>>>>>       for (i = 0; i < array->num_fences; ++i)
>>>>>           dma_fence_put(array->fences[i]);
>>>>> -    kfree(array->fences);
>>>>> -    dma_fence_free(fence);
>>>>> +    kvfree(array->fences);
>>>>> +    kvfree_rcu(fence, rcu);
>>>>>   }
>>>>>   static void dma_fence_array_set_deadline(struct dma_fence *fence,
>>>>> @@ -153,7 +153,7 @@ struct dma_fence_array 
>>>>> *dma_fence_array_alloc(int num_fences)
>>>>>   {
>>>>>       struct dma_fence_array *array;
>>>>> -    return kzalloc(struct_size(array, callbacks, num_fences), 
>>>>> GFP_KERNEL);
>>>>> +    return kvzalloc(struct_size(array, callbacks, num_fences), 
>>>>> GFP_KERNEL);
>>>>>   }
>>>>>   EXPORT_SYMBOL(dma_fence_array_alloc);
>>