[PATCH] drm/amdgpu: disallow multiple BO_HANDLES chunks in one submit

[Pierre-Eric Pelloux-Prayer]

Le 03/07/2024 à 16:10, Christian König a écrit :
> Am 03.07.24 um 14:48 schrieb Pierre-Eric Pelloux-Prayer:
>> Le 02/07/2024 à 15:35, Christian König a écrit :
>>> Am 02.07.24 um 15:23 schrieb Pierre-Eric Pelloux-Prayer:
>>>> Before this commit, only submits with both a BO_HANDLES chunk and a
>>>> 'bo_list_handle' would be rejected (by amdgpu_cs_parser_bos).
>>>>
>>>> But if UMD sent a multiple BO_HANDLES, what would happen is:
>>>> * only the last one would be really used
>>>> * all the others would leak memory as amdgpu_cs_p1_bo_handles would
>>>>    overwrite the previous p->bo_list value
>>>>
>>>> This commit rejects submissions with multiple BO_HANDLES chunks to
>>>> match the implementation of the parser.
>>>>
>>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>>> <pierre-eric.pelloux-prayer at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index c08dfffae262..69d168d6f35a 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -154,6 +154,10 @@ static int amdgpu_cs_p1_bo_handles(struct 
>>>> amdgpu_cs_parser *p,
>>>>       struct drm_amdgpu_bo_list_entry *info;
>>>>       int r;
>>>> +    /* Disallow multiple BO_HANDLES chunks. */
>>>
>>> Describe why you do something, instead of what you do since that 
>>> should be obvious from the code.
>>>
>>> E.g. something like /* Only allow a single BO list to avoid memory 
>>> leak. */
>>
>> It's not really to avoid a memleak because the code below could be fixed
>> to not leak the list.
>> This change is really about only disallowing multiple BO_HANDLES since 
>> this is the de-facto API, except it was not validated until now.
>>
>> I can rephrase the comment as /* Only a single BO list is allowed. */
>>
>> Would that work?
> 
> Mhm, we still need to give a reason why only a single BO list is allowed.
> 
> Maybe "Only a single BO list is allowed to simplify handling".

Works for me. I'll push a version with this comment instead of the 
original one.

Thanks!
Pierre-Eric


> 
> I don't really see a use case for having more than one BO list, but who 
> knows.
> 
> Regards,
> Christian.
> 
>>
>> Thanks,
>> Pierre-Eric
>>
>>>
>>> With that fixed Reviewed-by: Christian König <christian.koenig at amd.com>
>>>
>>> Regards,
>>> Christian.
>>>
>>>
>>>> +    if (p->bo_list)
>>>> +        return -EINVAL;
>>>> +
>>>>       r = amdgpu_bo_create_list_entry_array(data, &info);
>>>>       if (r)
>>>>           return r;