[PATCH] drm/xe/vm: Fix an error path

[PATCH] drm/xe/vm: Fix an error path

[Thomas Hellström]

If using the VM_BIND_OP_UNMAP_ALL without any bound vmas for the
vm, we will end up dereferencin an uninitialized variable and leak a
bo lock. Fix this.

Reported-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
Closes: https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
Suggested-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
Fixes: 9f232f4ae249 ("drm/xe: Port Xe to GPUVA")

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 1ca917b8315c..127842656a23 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2063,9 +2063,11 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
 		if (err)
 			return ERR_PTR(err);
 
-		vm_bo = drm_gpuvm_bo_find(&vm->gpuvm, obj);
-		if (!vm_bo)
-			break;
+		vm_bo = drm_gpuvm_bo_obtain(&vm->gpuvm, obj);
+		if (IS_ERR(vm_bo)) {
+			xe_bo_unlock(bo);
+			return ERR_CAST(vm_bo);
+		}
 
 		ops = drm_gpuvm_bo_unmap_ops_create(vm_bo);
 		drm_gpuvm_bo_put(vm_bo);
-- 
2.42.0

Re: [PATCH] drm/xe/vm: Fix an error path

[Lucas De Marchi]

On Wed, Dec 20, 2023 at 03:42:14PM +0100, Thomas Hellström wrote:
>If using the VM_BIND_OP_UNMAP_ALL without any bound vmas for the
>vm, we will end up dereferencin an uninitialized variable and leak a

dereferencing

>bo lock. Fix this.
>
>Reported-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>Closes: https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
>Suggested-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>Fixes: 9f232f4ae249 ("drm/xe: Port Xe to GPUVA")
>

^ this newline needs to be removed so `git log --format="%(trailers)"'
shows everything, not only your s-o-b.

>Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>---
> drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>index 1ca917b8315c..127842656a23 100644
>--- a/drivers/gpu/drm/xe/xe_vm.c
>+++ b/drivers/gpu/drm/xe/xe_vm.c
>@@ -2063,9 +2063,11 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
> 		if (err)
> 			return ERR_PTR(err);
>
>-		vm_bo = drm_gpuvm_bo_find(&vm->gpuvm, obj);
>-		if (!vm_bo)
>-			break;
>+		vm_bo = drm_gpuvm_bo_obtain(&vm->gpuvm, obj);

if the issue with that we don't have any bound vmas, why are we going to
create a new one just to be released?

Lucas De Marchi

>+		if (IS_ERR(vm_bo)) {
>+			xe_bo_unlock(bo);
>+			return ERR_CAST(vm_bo);
>+		}
>
> 		ops = drm_gpuvm_bo_unmap_ops_create(vm_bo);
> 		drm_gpuvm_bo_put(vm_bo);
>-- 
>2.42.0
>

Re: [PATCH] drm/xe/vm: Fix an error path

[Thomas Hellström]

On 12/20/23 17:13, Lucas De Marchi wrote:
> On Wed, Dec 20, 2023 at 03:42:14PM +0100, Thomas Hellström wrote:
>> If using the VM_BIND_OP_UNMAP_ALL without any bound vmas for the
>> vm, we will end up dereferencin an uninitialized variable and leak a
>
> dereferencing
>
>> bo lock. Fix this.
>>
>> Reported-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>> Closes: 
>> https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
>> Suggested-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>> Fixes: 9f232f4ae249 ("drm/xe: Port Xe to GPUVA")
>>
>
> ^ this newline needs to be removed so `git log --format="%(trailers)"'
> shows everything, not only your s-o-b.
Will fix these.
>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 1ca917b8315c..127842656a23 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -2063,9 +2063,11 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, 
>> struct xe_bo *bo,
>>         if (err)
>>             return ERR_PTR(err);
>>
>> -        vm_bo = drm_gpuvm_bo_find(&vm->gpuvm, obj);
>> -        if (!vm_bo)
>> -            break;
>> +        vm_bo = drm_gpuvm_bo_obtain(&vm->gpuvm, obj);
>
> if the issue with that we don't have any bound vmas, why are we going to
> create a new one just to be released?

The expected outcome of this operation, AFAICT is, rather than to error, 
to create  an ops with an empty list of operations, so we could in 
theory kmalloc ops and just initialize its list. However that would be 
fragile and second-guessing what gpuvm does internally, so I opted for 
this solution instead.

/Thomas



>
> Lucas De Marchi
>
>> +        if (IS_ERR(vm_bo)) {
>> +            xe_bo_unlock(bo);
>> +            return ERR_CAST(vm_bo);
>> +        }
>>
>>         ops = drm_gpuvm_bo_unmap_ops_create(vm_bo);
>>         drm_gpuvm_bo_put(vm_bo);
>> -- 
>> 2.42.0
>>

Re: [PATCH] drm/xe/vm: Fix an error path

[Lucas De Marchi]

On Wed, Dec 20, 2023 at 05:17:49PM +0100, Thomas Hellström wrote:
>
>On 12/20/23 17:13, Lucas De Marchi wrote:
>>On Wed, Dec 20, 2023 at 03:42:14PM +0100, Thomas Hellström wrote:
>>>If using the VM_BIND_OP_UNMAP_ALL without any bound vmas for the
>>>vm, we will end up dereferencin an uninitialized variable and leak a
>>
>>dereferencing
>>
>>>bo lock. Fix this.
>>>
>>>Reported-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>>>Closes: https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
>>>Suggested-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>>>Fixes: 9f232f4ae249 ("drm/xe: Port Xe to GPUVA")
>>>
>>
>>^ this newline needs to be removed so `git log --format="%(trailers)"'
>>shows everything, not only your s-o-b.
>Will fix these.
>>
>>>Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>---
>>>drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
>>>1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>>>index 1ca917b8315c..127842656a23 100644
>>>--- a/drivers/gpu/drm/xe/xe_vm.c
>>>+++ b/drivers/gpu/drm/xe/xe_vm.c
>>>@@ -2063,9 +2063,11 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, 
>>>struct xe_bo *bo,
>>>        if (err)
>>>            return ERR_PTR(err);
>>>
>>>-        vm_bo = drm_gpuvm_bo_find(&vm->gpuvm, obj);
>>>-        if (!vm_bo)
>>>-            break;
>>>+        vm_bo = drm_gpuvm_bo_obtain(&vm->gpuvm, obj);
>>
>>if the issue with that we don't have any bound vmas, why are we going to
>>create a new one just to be released?
>
>The expected outcome of this operation, AFAICT is, rather than to 
>error, to create  an ops with an empty list of operations, so we could 
>in theory kmalloc ops and just initialize its list. However that would 
>be fragile and second-guessing what gpuvm does internally, so I opted 
>for this solution instead.

ok,

Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

Adding dri-devel and Danilo to Cc as it looks like we are the first
users of this api.

The NULL vs ptr-error between _find and _obtain looked suspicious when I
first looked, but they match the current implementation.

thanks
Lucas De Marchi

>
>/Thomas
>
>
>
>>
>>Lucas De Marchi
>>
>>>+        if (IS_ERR(vm_bo)) {
>>>+            xe_bo_unlock(bo);
>>>+            return ERR_CAST(vm_bo);
>>>+        }
>>>
>>>        ops = drm_gpuvm_bo_unmap_ops_create(vm_bo);
>>>        drm_gpuvm_bo_put(vm_bo);
>>>-- 
>>>2.42.0
>>>

Re: [PATCH] drm/xe/vm: Fix an error path

[Thomas Hellström]

On 12/20/23 17:29, Lucas De Marchi wrote:
> On Wed, Dec 20, 2023 at 05:17:49PM +0100, Thomas Hellström wrote:
>>
>> On 12/20/23 17:13, Lucas De Marchi wrote:
>>> On Wed, Dec 20, 2023 at 03:42:14PM +0100, Thomas Hellström wrote:
>>>> If using the VM_BIND_OP_UNMAP_ALL without any bound vmas for the
>>>> vm, we will end up dereferencin an uninitialized variable and leak a
>>>
>>> dereferencing
>>>
>>>> bo lock. Fix this.
>>>>
>>>> Reported-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>>>> Closes: 
>>>> https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
>>>> Suggested-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>>>> Fixes: 9f232f4ae249 ("drm/xe: Port Xe to GPUVA")
>>>>
>>>
>>> ^ this newline needs to be removed so `git log --format="%(trailers)"'
>>> shows everything, not only your s-o-b.
>> Will fix these.
>>>
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>>>> index 1ca917b8315c..127842656a23 100644
>>>> --- a/drivers/gpu/drm/xe/xe_vm.c
>>>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>>>> @@ -2063,9 +2063,11 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, 
>>>> struct xe_bo *bo,
>>>>         if (err)
>>>>             return ERR_PTR(err);
>>>>
>>>> -        vm_bo = drm_gpuvm_bo_find(&vm->gpuvm, obj);
>>>> -        if (!vm_bo)
>>>> -            break;
>>>> +        vm_bo = drm_gpuvm_bo_obtain(&vm->gpuvm, obj);
>>>
>>> if the issue with that we don't have any bound vmas, why are we 
>>> going to
>>> create a new one just to be released?
>>
>> The expected outcome of this operation, AFAICT is, rather than to 
>> error, to create  an ops with an empty list of operations, so we 
>> could in theory kmalloc ops and just initialize its list. However 
>> that would be fragile and second-guessing what gpuvm does internally, 
>> so I opted for this solution instead.
>
> ok,
>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
> Adding dri-devel and Danilo to Cc as it looks like we are the first
> users of this api.
>
> The NULL vs ptr-error between _find and _obtain looked suspicious when I
> first looked, but they match the current implementation.
>
> thanks
> Lucas De Marchi

Thanks for reviewing, Lucas.

/Thomas



>
>>
>> /Thomas
>>
>>
>>
>>>
>>> Lucas De Marchi
>>>
>>>> +        if (IS_ERR(vm_bo)) {
>>>> +            xe_bo_unlock(bo);
>>>> +            return ERR_CAST(vm_bo);
>>>> +        }
>>>>
>>>>         ops = drm_gpuvm_bo_unmap_ops_create(vm_bo);
>>>>         drm_gpuvm_bo_put(vm_bo);
>>>> -- 
>>>> 2.42.0
>>>>