In amdgpu_bo_list_ioctl when idr_alloc fails
don't return without freeing bo list entry.
Fixes: 964d0fbf6301d ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
Signed-off-by: Nirmoy Das <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
index 7bcf86c61999..c3e5ea544857 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
@@ -284,7 +284,7 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
mutex_unlock(&fpriv->bo_list_lock);
if (r < 0) {
amdgpu_bo_list_put(list);
- return r;
+ goto error_free;
}
handle = r;
--
2.23.0
First of all please send mails regarding amdgpu to the amd-gfx mailing
list and not lkml/dri-devel.
Am 04.10.19 um 12:17 schrieb Nirmoy Das:
> In amdgpu_bo_list_ioctl when idr_alloc fails
> don't return without freeing bo list entry.
>
> Fixes: 964d0fbf6301d ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
>
> Signed-off-by: Nirmoy Das <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index 7bcf86c61999..c3e5ea544857 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -284,7 +284,7 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
> mutex_unlock(&fpriv->bo_list_lock);
> if (r < 0) {
> amdgpu_bo_list_put(list);
> - return r;
> + goto error_free;
NAK, that is a double free. The bo list entries are freed by
amdgpu_bo_list_put().
Regards,
Christian.
> }
>
> handle = r;
On 10/4/19 12:44 PM, Koenig, Christian wrote:
> First of all please send mails regarding amdgpu to the amd-gfx mailing
> list and not lkml/dri-devel.
Okay.
> Am 04.10.19 um 12:17 schrieb Nirmoy Das:
>> In amdgpu_bo_list_ioctl when idr_alloc fails
>> don't return without freeing bo list entry.
>>
>> Fixes: 964d0fbf6301d ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
>>
>> Signed-off-by: Nirmoy Das <[email protected]>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> index 7bcf86c61999..c3e5ea544857 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> @@ -284,7 +284,7 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
>> mutex_unlock(&fpriv->bo_list_lock);
>> if (r < 0) {
>> amdgpu_bo_list_put(list);
>> - return r;
>> + goto error_free;
> NAK, that is a double free. The bo list entries are freed by
> amdgpu_bo_list_put().
Thanks, didn't realize that.
> Regards,
> Christian.
Regards,
Nirmoy
>> }
>>
>> handle = r;
On 10/4/19 1:13 PM, Koenig, Christian wrote:
>
>>> NAK, that is a double free. The bo list entries are freed by
>>> amdgpu_bo_list_put().
>> Thanks, didn't realize that.
> Wait a second, what entries are you talking about?
>
> The entries in the list object are freed when amdgpu_bo_list_put() is
> called, but the temporary info array with the handles needs to be freed
> as well.
>
> And it looks like that is indeed leaked here.
I am talking about the `info` array created by
amdgpu_bo_create_list_entry_array().
> Regards,
> Christian.
>
>>> Regards,
>>> Christian.
>> Regards,
>>
>> Nirmoy
>>
>>>> }
>>>>
>>>> handle = r;
Am 04.10.19 um 13:26 schrieb Das, Nirmoy:
> On 10/4/19 1:13 PM, Koenig, Christian wrote:
>>>> NAK, that is a double free. The bo list entries are freed by
>>>> amdgpu_bo_list_put().
>>> Thanks, didn't realize that.
>> Wait a second, what entries are you talking about?
>>
>> The entries in the list object are freed when amdgpu_bo_list_put() is
>> called, but the temporary info array with the handles needs to be freed
>> as well.
>>
>> And it looks like that is indeed leaked here.
> I am talking about the `info` array created by
> amdgpu_bo_create_list_entry_array().
Yeah, that are the handles and not the entries. Sorry that I was
confused about that.
Your patch is correct, you should just update the commit message a bit.
BTW: Could you cleanup error handling here a bit more?
E.g. add an error_put_list handle and drop the "if (info)" and instead
return directly if we fail to allocate info.
Thanks,
Christian.
>> Regards,
>> Christian.
>>
>>>> Regards,
>>>> Christian.
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>>> }
>>>>>
>>>>> handle = r;
Am 04.10.19 um 13:00 schrieb Das, Nirmoy:
> On 10/4/19 12:44 PM, Koenig, Christian wrote:
>> First of all please send mails regarding amdgpu to the amd-gfx mailing
>> list and not lkml/dri-devel.
> Okay.
>> Am 04.10.19 um 12:17 schrieb Nirmoy Das:
>>> In amdgpu_bo_list_ioctl when idr_alloc fails
>>> don't return without freeing bo list entry.
>>>
>>> Fixes: 964d0fbf6301d ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
>>>
>>> Signed-off-by: Nirmoy Das <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> index 7bcf86c61999..c3e5ea544857 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> @@ -284,7 +284,7 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
>>> mutex_unlock(&fpriv->bo_list_lock);
>>> if (r < 0) {
>>> amdgpu_bo_list_put(list);
>>> - return r;
>>> + goto error_free;
>> NAK, that is a double free. The bo list entries are freed by
>> amdgpu_bo_list_put().
> Thanks, didn't realize that.
Wait a second, what entries are you talking about?
The entries in the list object are freed when amdgpu_bo_list_put() is
called, but the temporary info array with the handles needs to be freed
as well.
And it looks like that is indeed leaked here.
Regards,
Christian.
>> Regards,
>> Christian.
> Regards,
>
> Nirmoy
>
>>> }
>>>
>>> handle = r;
On 10/4/19 1:30 PM, Koenig, Christian wrote:
> Am 04.10.19 um 13:26 schrieb Das, Nirmoy:
>> On 10/4/19 1:13 PM, Koenig, Christian wrote:
>>>>> NAK, that is a double free. The bo list entries are freed by
>>>>> amdgpu_bo_list_put().
>>>> Thanks, didn't realize that.
>>> Wait a second, what entries are you talking about?
>>>
>>> The entries in the list object are freed when amdgpu_bo_list_put() is
>>> called, but the temporary info array with the handles needs to be freed
>>> as well.
>>>
>>> And it looks like that is indeed leaked here.
>> I am talking about the `info` array created by
>> amdgpu_bo_create_list_entry_array().
> Yeah, that are the handles and not the entries. Sorry that I was
> confused about that.
>
> Your patch is correct, you should just update the commit message a bit.
>
> BTW: Could you cleanup error handling here a bit more?
>
> E.g. add an error_put_list handle and drop the "if (info)" and instead
> return directly if we fail to allocate info.
Okay I will do that in v2 of this patch.
> Thanks,
> Christian.
Regards,
Nirmoy