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 nirmoy.das@amd.com --- 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;
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 nirmoy.das@amd.com
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 nirmoy.das@amd.com
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;
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 nirmoy.das@amd.com
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: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;
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
dri-devel@lists.freedesktop.org
-
Koenig, Christian -
Nirmoy -
Nirmoy Das