Am 13.06.2017 um 08:54 schrieb axie:
On 2017-06-12 06:47 PM, Christian König wrote:
Am 12.06.2017 um 22:31 schrieb Alex Xie:
Make the critical section smaller. There is no
need to protect the bo_list, because there is
no other task can access the newly created BO
list yet.
NAK, a task can guess the next id number so could get the kernel to
use the structure before it is initialized.
Christian.
How did you find such an extreme corner case?
Twenty-two years of experience working with that stuff, have seen all of
that before.
I am fine with this comment. Tuesday/Wednesday I will address it with
next version of patch set.
Currently, there are 2 options:
Option 1: I may use a write_lock in the create function. And restore
the original code for the creation of BO list.
Option 2: I may move the function call of idr_alloc to the end of the
creation of BO list ioctl. This is more efficient but
the code look dirty.
Option #2 doesn't sound so dirty to me. Properly initializing the
bo_list and then adding it to the idr actually sounds rather sane to me.
The only tricky part is that the currently helper function doesn't
really match that use case.
BTW: Something I've just notices while working on this:
*result = kzalloc(sizeof(struct amdgpu_bo_list), GFP_KERNEL);
...
mutex_init(&(*result)->lock);
(*result)->num_entries = 0;
(*result)->array = NULL;
We can probably save quite some CPU cycles when we still to zero
initialize the structure twice.
Regards,
Christian.
Signed-off-by: Alex Xie <alexbin....@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
index 02c138f..c994a04 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
@@ -48,8 +48,9 @@ static int amdgpu_bo_list_create(struct
amdgpu_fpriv *fpriv,
mutex_lock(&fpriv->bo_list_lock);
r = idr_alloc(&fpriv->bo_list_handles, *result,
1, 0, GFP_KERNEL);
+ mutex_unlock(&fpriv->bo_list_lock);
+
if (r < 0) {
- mutex_unlock(&fpriv->bo_list_lock);
kfree(*result);
return r;
}
@@ -60,7 +61,6 @@ static int amdgpu_bo_list_create(struct
amdgpu_fpriv *fpriv,
(*result)->array = NULL;
mutex_lock(&(*result)->lock);
- mutex_unlock(&fpriv->bo_list_lock);
return 0;
}
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
