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

[Tvrtko Ursulin]

On 07/11/2024 12:48, Christian König wrote:
> 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.

Yeah it is.

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

Ah no, I did not express myself clearly. I did not mean 64k, but a limit 
to align with INT_MAX __kvmalloc_node_noprof(). Or __GFP_NOWARN might be 
better when allocation size is userspace controlled.

Regards,

Tvrtko

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