[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 at habana.ai>
>>>Closes: https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
>>>Suggested-by: Dafna Hirschfeld <dhirschfeld at 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 at 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 at 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
>>>